Use a faux device parent for registering the platform_profile instead of
a "fake" platform device.

The faux bus is a minimalistic, single driver bus designed for this
purpose.

Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
 drivers/platform/x86/dell/dell-pc.c | 46 +++++++++++--------------------------
 1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
index 794924913be0c6f13ed4aed8b01ffd21f1d34dea..48cc7511905a62d2828e3a7b593b3d2dae893e34 100644
--- a/drivers/platform/x86/dell/dell-pc.c
+++ b/drivers/platform/x86/dell/dell-pc.c
@@ -13,18 +13,18 @@
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/bits.h>
+#include <linux/device/faux.h>
 #include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_profile.h>
-#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #include "dell-smbios.h"
 
-static struct platform_device *platform_device;
+static struct faux_device *dell_pc_fdev;
 static int supported_modes;
 
 static const struct dmi_system_id dell_device_table[] __initconst = {
@@ -246,7 +246,7 @@ static const struct platform_profile_ops dell_pc_platform_profile_ops = {
 	.profile_set = thermal_platform_profile_set,
 };
 
-static int thermal_init(void)
+static int dell_pc_faux_probe(struct faux_device *fdev)
 {
 	struct device *ppdev;
 	int ret;
@@ -258,51 +258,31 @@ static int thermal_init(void)
 	if (ret < 0)
 		return ret;
 
-	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
-	if (IS_ERR(platform_device))
-		return PTR_ERR(platform_device);
+	ppdev = devm_platform_profile_register(&fdev->dev, "dell-pc", NULL,
+					       &dell_pc_platform_profile_ops);
 
-	ppdev = devm_platform_profile_register(&platform_device->dev, "dell-pc",
-					       NULL, &dell_pc_platform_profile_ops);
-	if (IS_ERR(ppdev)) {
-		ret = PTR_ERR(ppdev);
-		goto cleanup_platform_device;
-	}
-
-	return 0;
-
-cleanup_platform_device:
-	platform_device_unregister(platform_device);
-
-	return ret;
+	return PTR_ERR_OR_ZERO(ppdev);
 }
 
-static void thermal_cleanup(void)
-{
-	platform_device_unregister(platform_device);
-}
+static const struct faux_device_ops dell_pc_faux_ops = {
+	.probe = dell_pc_faux_probe,
+};
 
 static int __init dell_init(void)
 {
-	int ret;
-
 	if (!dmi_check_system(dell_device_table))
 		return -ENODEV;
 
-	ret = thermal_init();
-	if (ret)
-		goto fail_thermal;
+	dell_pc_fdev = faux_device_create("dell-pc", NULL, &dell_pc_faux_ops);
+	if (!dell_pc_fdev)
+		return -ENODEV;
 
 	return 0;
-
-fail_thermal:
-	thermal_cleanup();
-	return ret;
 }
 
 static void __exit dell_exit(void)
 {
-	thermal_cleanup();
+	faux_device_destroy(dell_pc_fdev);
 }
 
 module_init(dell_init);

-- 
2.49.0

On Fri, 11 Apr 2025, Kurt Borja wrote:

> Use a faux device parent for registering the platform_profile instead of
> a "fake" platform device.
> 
> The faux bus is a minimalistic, single driver bus designed for this
> purpose.

Hi Kurt, Hans & Greg,

I'm not sure about this change. So dell-pc not a platform device but
a "fake".

I'm not saying this is wrong, but feel I'm a bit just lost where the 
dividing line is. Is it just because this driver only happens to call
dell_send_request(), etc., not contains that low-level access code within? 
Or is that dell-smbios "fake" too?

> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> ---
>  drivers/platform/x86/dell/dell-pc.c | 46 +++++++++++--------------------------
>  1 file changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
> index 794924913be0c6f13ed4aed8b01ffd21f1d34dea..48cc7511905a62d2828e3a7b593b3d2dae893e34 100644
> --- a/drivers/platform/x86/dell/dell-pc.c
> +++ b/drivers/platform/x86/dell/dell-pc.c
> @@ -13,18 +13,18 @@
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/bits.h>
> +#include <linux/device/faux.h>
>  #include <linux/dmi.h>
>  #include <linux/err.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/platform_profile.h>
> -#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  #include "dell-smbios.h"
>  
> -static struct platform_device *platform_device;
> +static struct faux_device *dell_pc_fdev;
>  static int supported_modes;
>  
>  static const struct dmi_system_id dell_device_table[] __initconst = {
> @@ -246,7 +246,7 @@ static const struct platform_profile_ops dell_pc_platform_profile_ops = {
>  	.profile_set = thermal_platform_profile_set,
>  };
>  
> -static int thermal_init(void)
> +static int dell_pc_faux_probe(struct faux_device *fdev)
>  {
>  	struct device *ppdev;
>  	int ret;
> @@ -258,51 +258,31 @@ static int thermal_init(void)
>  	if (ret < 0)
>  		return ret;
>  
> -	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
> -	if (IS_ERR(platform_device))
> -		return PTR_ERR(platform_device);
> +	ppdev = devm_platform_profile_register(&fdev->dev, "dell-pc", NULL,
> +					       &dell_pc_platform_profile_ops);
>  
> -	ppdev = devm_platform_profile_register(&platform_device->dev, "dell-pc",
> -					       NULL, &dell_pc_platform_profile_ops);
> -	if (IS_ERR(ppdev)) {
> -		ret = PTR_ERR(ppdev);
> -		goto cleanup_platform_device;
> -	}
> -
> -	return 0;
> -
> -cleanup_platform_device:
> -	platform_device_unregister(platform_device);
> -
> -	return ret;
> +	return PTR_ERR_OR_ZERO(ppdev);
>  }
>  
> -static void thermal_cleanup(void)
> -{
> -	platform_device_unregister(platform_device);
> -}
> +static const struct faux_device_ops dell_pc_faux_ops = {
> +	.probe = dell_pc_faux_probe,
> +};
>  
>  static int __init dell_init(void)
>  {
> -	int ret;
> -
>  	if (!dmi_check_system(dell_device_table))
>  		return -ENODEV;
>  
> -	ret = thermal_init();
> -	if (ret)
> -		goto fail_thermal;
> +	dell_pc_fdev = faux_device_create("dell-pc", NULL, &dell_pc_faux_ops);
> +	if (!dell_pc_fdev)
> +		return -ENODEV;
>  
>  	return 0;
> -
> -fail_thermal:
> -	thermal_cleanup();
> -	return ret;
>  }
>  
>  static void __exit dell_exit(void)
>  {
> -	thermal_cleanup();
> +	faux_device_destroy(dell_pc_fdev);
>  }
>  
>  module_init(dell_init);
> 
> 

-- 
 i.

Hi Ilpo,

On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
> On Fri, 11 Apr 2025, Kurt Borja wrote:
> 
>> Use a faux device parent for registering the platform_profile instead of
>> a "fake" platform device.
>>
>> The faux bus is a minimalistic, single driver bus designed for this
>> purpose.
> 
> Hi Kurt, Hans & Greg,
> 
> I'm not sure about this change. So dell-pc not a platform device but
> a "fake".

Arguably the dell-pc driver does not need a struct device at all,
since it just exports /sys/firmware/acpi/platform_profile sysfs
interface by using the relevant Dell SMBIOS interfaces for this.

As such maybe we should just completely get rid of the whole
struct device here?

If we do decide to keep the struct device, then since the struct device
seems to just be there to tie the lifetime of the platform_profile
handler to, I guess that calling it a faux device is fair.

> I'm not saying this is wrong, but feel I'm a bit just lost where the 
> dividing line is.

In this case it seems to be clear that this is a faux device,
but I do agree that sometimes the line can be a bit blurry.

Regards,

Hans




> Is it just because this driver only happens to call
> dell_send_request(), etc., not contains that low-level access code within? 
> Or is that dell-smbios "fake" too?
> 
>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>> ---
>>  drivers/platform/x86/dell/dell-pc.c | 46 +++++++++++--------------------------
>>  1 file changed, 13 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
>> index 794924913be0c6f13ed4aed8b01ffd21f1d34dea..48cc7511905a62d2828e3a7b593b3d2dae893e34 100644
>> --- a/drivers/platform/x86/dell/dell-pc.c
>> +++ b/drivers/platform/x86/dell/dell-pc.c
>> @@ -13,18 +13,18 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/bitops.h>
>>  #include <linux/bits.h>
>> +#include <linux/device/faux.h>
>>  #include <linux/dmi.h>
>>  #include <linux/err.h>
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_profile.h>
>> -#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>>  #include "dell-smbios.h"
>>  
>> -static struct platform_device *platform_device;
>> +static struct faux_device *dell_pc_fdev;
>>  static int supported_modes;
>>  
>>  static const struct dmi_system_id dell_device_table[] __initconst = {
>> @@ -246,7 +246,7 @@ static const struct platform_profile_ops dell_pc_platform_profile_ops = {
>>  	.profile_set = thermal_platform_profile_set,
>>  };
>>  
>> -static int thermal_init(void)
>> +static int dell_pc_faux_probe(struct faux_device *fdev)
>>  {
>>  	struct device *ppdev;
>>  	int ret;
>> @@ -258,51 +258,31 @@ static int thermal_init(void)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
>> -	if (IS_ERR(platform_device))
>> -		return PTR_ERR(platform_device);
>> +	ppdev = devm_platform_profile_register(&fdev->dev, "dell-pc", NULL,
>> +					       &dell_pc_platform_profile_ops);
>>  
>> -	ppdev = devm_platform_profile_register(&platform_device->dev, "dell-pc",
>> -					       NULL, &dell_pc_platform_profile_ops);
>> -	if (IS_ERR(ppdev)) {
>> -		ret = PTR_ERR(ppdev);
>> -		goto cleanup_platform_device;
>> -	}
>> -
>> -	return 0;
>> -
>> -cleanup_platform_device:
>> -	platform_device_unregister(platform_device);
>> -
>> -	return ret;
>> +	return PTR_ERR_OR_ZERO(ppdev);
>>  }
>>  
>> -static void thermal_cleanup(void)
>> -{
>> -	platform_device_unregister(platform_device);
>> -}
>> +static const struct faux_device_ops dell_pc_faux_ops = {
>> +	.probe = dell_pc_faux_probe,
>> +};
>>  
>>  static int __init dell_init(void)
>>  {
>> -	int ret;
>> -
>>  	if (!dmi_check_system(dell_device_table))
>>  		return -ENODEV;
>>  
>> -	ret = thermal_init();
>> -	if (ret)
>> -		goto fail_thermal;
>> +	dell_pc_fdev = faux_device_create("dell-pc", NULL, &dell_pc_faux_ops);
>> +	if (!dell_pc_fdev)
>> +		return -ENODEV;
>>  
>>  	return 0;
>> -
>> -fail_thermal:
>> -	thermal_cleanup();
>> -	return ret;
>>  }
>>  
>>  static void __exit dell_exit(void)
>>  {
>> -	thermal_cleanup();
>> +	faux_device_destroy(dell_pc_fdev);
>>  }
>>  
>>  module_init(dell_init);
>>
>>
> 

Hi all,

On Wed Apr 23, 2025 at 10:44 AM -03, Hans de Goede wrote:
> Hi Ilpo,
>
> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
>> On Fri, 11 Apr 2025, Kurt Borja wrote:
>> 
>>> Use a faux device parent for registering the platform_profile instead of
>>> a "fake" platform device.
>>>
>>> The faux bus is a minimalistic, single driver bus designed for this
>>> purpose.
>> 
>> Hi Kurt, Hans & Greg,
>> 
>> I'm not sure about this change. So dell-pc not a platform device but
>> a "fake".
>
> Arguably the dell-pc driver does not need a struct device at all,
> since it just exports /sys/firmware/acpi/platform_profile sysfs
> interface by using the relevant Dell SMBIOS interfaces for this.
>
> As such maybe we should just completely get rid of the whole
> struct device here?
>
> If we do decide to keep the struct device, then since the struct device
> seems to just be there to tie the lifetime of the platform_profile
> handler to, I guess that calling it a faux device is fair.

I think it's important to mention that a parent device is required to
register a platform profile, see [1].

I guess we could get away with removing the device altogether from here,
but that would require to find another suitable parent device. The
obvious choice would be the `dell-smbios` device, however that would
require exporting it in the first place.

For some reason, exporting devices doesn't seem right to me, so IMO a
faux device is a good choice here.

Another solution that would make more sense, lifetime wise, is to turn
this into an aux driver and let `dell-smbios` create the matching aux
device. I could do this, but I think it's overly complicated.

>
>> I'm not saying this is wrong, but feel I'm a bit just lost where the 
>> dividing line is.
>
> In this case it seems to be clear that this is a faux device,
> but I do agree that sometimes the line can be a bit blurry.
>
> Regards,
>
> Hans
>
>
>
>
>> Is it just because this driver only happens to call
>> dell_send_request(), etc., not contains that low-level access code within? 
>> Or is that dell-smbios "fake" too?

IMO `dell-smbios` is "fake" too? It is there only to expose either the
WMI or the SMM backend through a single sysfs interface.

I think a more natural design for `dell-smbios` would be an aux driver
that exposed it's interface through a class device. Maybe I'm wrong in
this regard though.

[1] https://elixir.bootlin.com/linux/v6.15-rc3/source/drivers/acpi/platform_profile.c#L556

-- 
 ~ Kurt

>> 
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>> ---
>>>  drivers/platform/x86/dell/dell-pc.c | 46 +++++++++++--------------------------
>>>  1 file changed, 13 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c
>>> index 794924913be0c6f13ed4aed8b01ffd21f1d34dea..48cc7511905a62d2828e3a7b593b3d2dae893e34 100644
>>> --- a/drivers/platform/x86/dell/dell-pc.c
>>> +++ b/drivers/platform/x86/dell/dell-pc.c
>>> @@ -13,18 +13,18 @@
>>>  #include <linux/bitfield.h>
>>>  #include <linux/bitops.h>
>>>  #include <linux/bits.h>
>>> +#include <linux/device/faux.h>
>>>  #include <linux/dmi.h>
>>>  #include <linux/err.h>
>>>  #include <linux/init.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_profile.h>
>>> -#include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  
>>>  #include "dell-smbios.h"
>>>  
>>> -static struct platform_device *platform_device;
>>> +static struct faux_device *dell_pc_fdev;
>>>  static int supported_modes;
>>>  
>>>  static const struct dmi_system_id dell_device_table[] __initconst = {
>>> @@ -246,7 +246,7 @@ static const struct platform_profile_ops dell_pc_platform_profile_ops = {
>>>  	.profile_set = thermal_platform_profile_set,
>>>  };
>>>  
>>> -static int thermal_init(void)
>>> +static int dell_pc_faux_probe(struct faux_device *fdev)
>>>  {
>>>  	struct device *ppdev;
>>>  	int ret;
>>> @@ -258,51 +258,31 @@ static int thermal_init(void)
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> -	platform_device = platform_device_register_simple("dell-pc", PLATFORM_DEVID_NONE, NULL, 0);
>>> -	if (IS_ERR(platform_device))
>>> -		return PTR_ERR(platform_device);
>>> +	ppdev = devm_platform_profile_register(&fdev->dev, "dell-pc", NULL,
>>> +					       &dell_pc_platform_profile_ops);
>>>  
>>> -	ppdev = devm_platform_profile_register(&platform_device->dev, "dell-pc",
>>> -					       NULL, &dell_pc_platform_profile_ops);
>>> -	if (IS_ERR(ppdev)) {
>>> -		ret = PTR_ERR(ppdev);
>>> -		goto cleanup_platform_device;
>>> -	}
>>> -
>>> -	return 0;
>>> -
>>> -cleanup_platform_device:
>>> -	platform_device_unregister(platform_device);
>>> -
>>> -	return ret;
>>> +	return PTR_ERR_OR_ZERO(ppdev);
>>>  }
>>>  
>>> -static void thermal_cleanup(void)
>>> -{
>>> -	platform_device_unregister(platform_device);
>>> -}
>>> +static const struct faux_device_ops dell_pc_faux_ops = {
>>> +	.probe = dell_pc_faux_probe,
>>> +};
>>>  
>>>  static int __init dell_init(void)
>>>  {
>>> -	int ret;
>>> -
>>>  	if (!dmi_check_system(dell_device_table))
>>>  		return -ENODEV;
>>>  
>>> -	ret = thermal_init();
>>> -	if (ret)
>>> -		goto fail_thermal;
>>> +	dell_pc_fdev = faux_device_create("dell-pc", NULL, &dell_pc_faux_ops);
>>> +	if (!dell_pc_fdev)
>>> +		return -ENODEV;
>>>  
>>>  	return 0;
>>> -
>>> -fail_thermal:
>>> -	thermal_cleanup();
>>> -	return ret;
>>>  }
>>>  
>>>  static void __exit dell_exit(void)
>>>  {
>>> -	thermal_cleanup();
>>> +	faux_device_destroy(dell_pc_fdev);
>>>  }
>>>  
>>>  module_init(dell_init);
>>>
>>>
>> 

Hi Kurt,

On 23-Apr-25 6:14 PM, Kurt Borja wrote:
> Hi all,
> 
> On Wed Apr 23, 2025 at 10:44 AM -03, Hans de Goede wrote:
>> Hi Ilpo,
>>
>> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
>>> On Fri, 11 Apr 2025, Kurt Borja wrote:
>>>
>>>> Use a faux device parent for registering the platform_profile instead of
>>>> a "fake" platform device.
>>>>
>>>> The faux bus is a minimalistic, single driver bus designed for this
>>>> purpose.
>>>
>>> Hi Kurt, Hans & Greg,
>>>
>>> I'm not sure about this change. So dell-pc not a platform device but
>>> a "fake".
>>
>> Arguably the dell-pc driver does not need a struct device at all,
>> since it just exports /sys/firmware/acpi/platform_profile sysfs
>> interface by using the relevant Dell SMBIOS interfaces for this.
>>
>> As such maybe we should just completely get rid of the whole
>> struct device here?
>>
>> If we do decide to keep the struct device, then since the struct device
>> seems to just be there to tie the lifetime of the platform_profile
>> handler to, I guess that calling it a faux device is fair.
> 
> I think it's important to mention that a parent device is required to
> register a platform profile, see [1].

Ah ok, that is new, I guess that was changed with the new support
for registering multiple platform-profile handlers.

> I guess we could get away with removing the device altogether from here,
> but that would require to find another suitable parent device. The
> obvious choice would be the `dell-smbios` device, however that would
> require exporting it in the first place.
> 
> For some reason, exporting devices doesn't seem right to me, so IMO a
> faux device is a good choice here.

Agreed.

> Another solution that would make more sense, lifetime wise, is to turn
> this into an aux driver and let `dell-smbios` create the matching aux
> device. I could do this, but I think it's overly complicated.

Yes that does seem overly complicated, lets just go with the faux
device.

Regards,

Hans




>>> Is it just because this driver only happens to call
>>> dell_send_request(), etc., not contains that low-level access code within? 
>>> Or is that dell-smbios "fake" too?
> 
> IMO `dell-smbios` is "fake" too? It is there only to expose either the
> WMI or the SMM backend through a single sysfs interface.
> 
> I think a more natural design for `dell-smbios` would be an aux driver
> that exposed it's interface through a class device. Maybe I'm wrong in
> this regard though.
> 
> [1] https://elixir.bootlin.com/linux/v6.15-rc3/source/drivers/acpi/platform_profile.c#L556
> 

On Wed, 23 Apr 2025, Hans de Goede wrote:
> On 23-Apr-25 6:14 PM, Kurt Borja wrote:
> > On Wed Apr 23, 2025 at 10:44 AM -03, Hans de Goede wrote:
> >> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
> >>> On Fri, 11 Apr 2025, Kurt Borja wrote:
> >>>
> >>>> Use a faux device parent for registering the platform_profile instead of
> >>>> a "fake" platform device.
> >>>>
> >>>> The faux bus is a minimalistic, single driver bus designed for this
> >>>> purpose.
> >>>
> >>> Hi Kurt, Hans & Greg,
> >>>
> >>> I'm not sure about this change. So dell-pc not a platform device but
> >>> a "fake".
> >>
> >> Arguably the dell-pc driver does not need a struct device at all,
> >> since it just exports /sys/firmware/acpi/platform_profile sysfs
> >> interface by using the relevant Dell SMBIOS interfaces for this.
> >>
> >> As such maybe we should just completely get rid of the whole
> >> struct device here?
> >>
> >> If we do decide to keep the struct device, then since the struct device
> >> seems to just be there to tie the lifetime of the platform_profile
> >> handler to, I guess that calling it a faux device is fair.
> > 
> > I think it's important to mention that a parent device is required to
> > register a platform profile, see [1].
> 
> Ah ok, that is new, I guess that was changed with the new support
> for registering multiple platform-profile handlers.
> 
> > I guess we could get away with removing the device altogether from here,
> > but that would require to find another suitable parent device. The
> > obvious choice would be the `dell-smbios` device, however that would
> > require exporting it in the first place.
> > 
> > For some reason, exporting devices doesn't seem right to me, so IMO a
> > faux device is a good choice here.
> 
> Agreed.
> 
> > Another solution that would make more sense, lifetime wise, is to turn
> > this into an aux driver and let `dell-smbios` create the matching aux
> > device.

Well, that was what caused part of my confusion / uncertainty here as
I could see that aux bus between these two drivers. Obviously, it's not 
there currently but conceptually this relationship looks what full-blown 
aux bus was supposed to solve.

The other part was that as per Greg's simple classification, certainly 
this driver needs to access platform resources. BUT, that access is routed 
through another driver which is a case his answer/classification did not 
cover.

> > I could do this, but I think it's overly complicated.
>
> Yes that does seem overly complicated, lets just go with the faux
> device.

Okay. In part, this was also to check whether replacing full-blown aux bus 
with faux should be considered another kind of "abuse". I've no problem 
with accepting faux for cases like this as I see these as policy / 
convention decision more than one being right and another wrong. :-)

Thanks to all who answered.

-- 
 i.

On Thu Apr 24, 2025 at 8:57 AM -03, Ilpo Järvinen wrote:
> On Wed, 23 Apr 2025, Hans de Goede wrote:
>> On 23-Apr-25 6:14 PM, Kurt Borja wrote:
>> > On Wed Apr 23, 2025 at 10:44 AM -03, Hans de Goede wrote:
>> >> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
>> >>> On Fri, 11 Apr 2025, Kurt Borja wrote:
>> >>>
>> >>>> Use a faux device parent for registering the platform_profile instead of
>> >>>> a "fake" platform device.
>> >>>>
>> >>>> The faux bus is a minimalistic, single driver bus designed for this
>> >>>> purpose.
>> >>>
>> >>> Hi Kurt, Hans & Greg,
>> >>>
>> >>> I'm not sure about this change. So dell-pc not a platform device but
>> >>> a "fake".
>> >>
>> >> Arguably the dell-pc driver does not need a struct device at all,
>> >> since it just exports /sys/firmware/acpi/platform_profile sysfs
>> >> interface by using the relevant Dell SMBIOS interfaces for this.
>> >>
>> >> As such maybe we should just completely get rid of the whole
>> >> struct device here?
>> >>
>> >> If we do decide to keep the struct device, then since the struct device
>> >> seems to just be there to tie the lifetime of the platform_profile
>> >> handler to, I guess that calling it a faux device is fair.
>> > 
>> > I think it's important to mention that a parent device is required to
>> > register a platform profile, see [1].
>> 
>> Ah ok, that is new, I guess that was changed with the new support
>> for registering multiple platform-profile handlers.
>> 
>> > I guess we could get away with removing the device altogether from here,
>> > but that would require to find another suitable parent device. The
>> > obvious choice would be the `dell-smbios` device, however that would
>> > require exporting it in the first place.
>> > 
>> > For some reason, exporting devices doesn't seem right to me, so IMO a
>> > faux device is a good choice here.
>> 
>> Agreed.
>> 
>> > Another solution that would make more sense, lifetime wise, is to turn
>> > this into an aux driver and let `dell-smbios` create the matching aux
>> > device.
>
> Well, that was what caused part of my confusion / uncertainty here as
> I could see that aux bus between these two drivers. Obviously, it's not 
> there currently but conceptually this relationship looks what full-blown 
> aux bus was supposed to solve.
>
> The other part was that as per Greg's simple classification, certainly 
> this driver needs to access platform resources. BUT, that access is routed 
> through another driver which is a case his answer/classification did not 
> cover.

Perhaps it didn't cover it because, as you mentioned, this falls under
the aux bus use cases.

>
>> > I could do this, but I think it's overly complicated.
>>
>> Yes that does seem overly complicated, lets just go with the faux
>> device.
>
> Okay. In part, this was also to check whether replacing full-blown aux bus 
> with faux should be considered another kind of "abuse". I've no problem 
> with accepting faux for cases like this as I see these as policy / 
> convention decision more than one being right and another wrong. :-)

Now that you put it that way, I guess this still is kind of "abusive",
but is still an improvement over creating a full platform device.

Nevertheless, although this driver do access platform resources, it does
it completely detached from the "dell-smbios" device. The only use of
the platform device here was `&pdev->dev` :p

>
> Thanks to all who answered.

Thanks for your review!

-- 
 ~ Kurt

On Fri, 25 Apr 2025, Kurt Borja wrote:

> On Thu Apr 24, 2025 at 8:57 AM -03, Ilpo Järvinen wrote:
> > On Wed, 23 Apr 2025, Hans de Goede wrote:
> >> On 23-Apr-25 6:14 PM, Kurt Borja wrote:
> >> > On Wed Apr 23, 2025 at 10:44 AM -03, Hans de Goede wrote:
> >> >> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
> >> >>> On Fri, 11 Apr 2025, Kurt Borja wrote:
> >> >>>
> >> >>>> Use a faux device parent for registering the platform_profile instead of
> >> >>>> a "fake" platform device.
> >> >>>>
> >> >>>> The faux bus is a minimalistic, single driver bus designed for this
> >> >>>> purpose.
> >> >>>
> >> >>> Hi Kurt, Hans & Greg,
> >> >>>
> >> >>> I'm not sure about this change. So dell-pc not a platform device but
> >> >>> a "fake".
> >> >>
> >> >> Arguably the dell-pc driver does not need a struct device at all,
> >> >> since it just exports /sys/firmware/acpi/platform_profile sysfs
> >> >> interface by using the relevant Dell SMBIOS interfaces for this.
> >> >>
> >> >> As such maybe we should just completely get rid of the whole
> >> >> struct device here?
> >> >>
> >> >> If we do decide to keep the struct device, then since the struct device
> >> >> seems to just be there to tie the lifetime of the platform_profile
> >> >> handler to, I guess that calling it a faux device is fair.
> >> > 
> >> > I think it's important to mention that a parent device is required to
> >> > register a platform profile, see [1].
> >> 
> >> Ah ok, that is new, I guess that was changed with the new support
> >> for registering multiple platform-profile handlers.
> >> 
> >> > I guess we could get away with removing the device altogether from here,
> >> > but that would require to find another suitable parent device. The
> >> > obvious choice would be the `dell-smbios` device, however that would
> >> > require exporting it in the first place.
> >> > 
> >> > For some reason, exporting devices doesn't seem right to me, so IMO a
> >> > faux device is a good choice here.
> >> 
> >> Agreed.
> >> 
> >> > Another solution that would make more sense, lifetime wise, is to turn
> >> > this into an aux driver and let `dell-smbios` create the matching aux
> >> > device.
> >
> > Well, that was what caused part of my confusion / uncertainty here as
> > I could see that aux bus between these two drivers. Obviously, it's not 
> > there currently but conceptually this relationship looks what full-blown 
> > aux bus was supposed to solve.
> >
> > The other part was that as per Greg's simple classification, certainly 
> > this driver needs to access platform resources. BUT, that access is routed 
> > through another driver which is a case his answer/classification did not 
> > cover.
> 
> Perhaps it didn't cover it because, as you mentioned, this falls under
> the aux bus use cases.
> 
> >
> >> > I could do this, but I think it's overly complicated.
> >>
> >> Yes that does seem overly complicated, lets just go with the faux
> >> device.
> >
> > Okay. In part, this was also to check whether replacing full-blown aux bus 
> > with faux should be considered another kind of "abuse". I've no problem 
> > with accepting faux for cases like this as I see these as policy / 
> > convention decision more than one being right and another wrong. :-)
> 
> Now that you put it that way, I guess this still is kind of "abusive",
> but is still an improvement over creating a full platform device.
> 
> Nevertheless, although this driver do access platform resources, it does
> it completely detached from the "dell-smbios" device. The only use of
> the platform device here was `&pdev->dev` :p

Perhaps "completely detached" is just a synonym for the common assumption 
that there can be only one and abuse of statics, which are generally 
frowned upon but also commonly used in platform drivers regardless. So 
there are these caveats in that supposed complete detatchedness.

But yeah, in some case it makes things easier.

-- 
 i.

On Wed, Apr 23, 2025 at 03:44:56PM +0200, Hans de Goede wrote:
> Hi Ilpo,
> 
> On 23-Apr-25 3:27 PM, Ilpo Järvinen wrote:
> > On Fri, 11 Apr 2025, Kurt Borja wrote:
> > 
> >> Use a faux device parent for registering the platform_profile instead of
> >> a "fake" platform device.
> >>
> >> The faux bus is a minimalistic, single driver bus designed for this
> >> purpose.
> > 
> > Hi Kurt, Hans & Greg,
> > 
> > I'm not sure about this change. So dell-pc not a platform device but
> > a "fake".
> 
> Arguably the dell-pc driver does not need a struct device at all,
> since it just exports /sys/firmware/acpi/platform_profile sysfs
> interface by using the relevant Dell SMBIOS interfaces for this.
> 
> As such maybe we should just completely get rid of the whole
> struct device here?
> 
> If we do decide to keep the struct device, then since the struct device
> seems to just be there to tie the lifetime of the platform_profile
> handler to, I guess that calling it a faux device is fair.
> 
> > I'm not saying this is wrong, but feel I'm a bit just lost where the 
> > dividing line is.
> 
> In this case it seems to be clear that this is a faux device,
> but I do agree that sometimes the line can be a bit blurry.

If a device needs access to platform resources, then it is a platform
device.  If not, then it is not.  Not too complex :)

But (you knew there was a but), many drivers want to detach their
ability to create a device, and have a driver bind to them, in a
different "place" in the kernel.  For many of those, they have (ab)used
the platform driver/device api to achieve this, despite them not being a
platform device at all.  For these, we can't convert them directly to
use faux bus, as it's not as simple of a conversion and in some places,
doesn't work well.  So let's leave those alone for now, but not take any
more of them going forward in the future.

hope this helps,

greg k-h

On Wed, Apr 23, 2025, at 7:59 AM, Greg Kroah-Hartman wrote:
> On Wed, Apr 23, 2025 at 03:44:56PM +0200, Hans de Goede wrote:
>> 
>> Arguably the dell-pc driver does not need a struct device at all,
>> since it just exports /sys/firmware/acpi/platform_profile sysfs
>> interface by using the relevant Dell SMBIOS interfaces for this.
>> 
>> As such maybe we should just completely get rid of the whole
>> struct device here?
>> 
>> If we do decide to keep the struct device, then since the struct device
>> seems to just be there to tie the lifetime of the platform_profile
>> handler to, I guess that calling it a faux device is fair.

I am curious to see what this would look like. If we can get away with not using a struct for this functionality I think that is a good way to keep it simple.

>
> If a device needs access to platform resources, then it is a platform
> device.  If not, then it is not.  Not too complex :)
>
> But (you knew there was a but), many drivers want to detach their
> ability to create a device, and have a driver bind to them, in a
> different "place" in the kernel.  For many of those, they have (ab)used
> the platform driver/device api to achieve this, despite them not being a
> platform device at all.  For these, we can't convert them directly to
> use faux bus, as it's not as simple of a conversion and in some places,
> doesn't work well.  So let's leave those alone for now, but not take any
> more of them going forward in the future.

Right now we are just using the platform device to register a platform profile handler. If we only need the faux bus for that then I think that is a good way to go. I will look into this new faux bus a bit more.

I'll see if I can find some time this evening to run this patch.

Thanks,

Lyndon