Message-Id: <D9E5H5B9X448.12FJT48775C50@gmail.com>
Date: Wed, 23 Apr 2025 13:14:24 -0300
From: "Kurt Borja" <kuurtb@...il.com>
To: "Hans de Goede" <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>
Cc: "Lyndon Sanche" <lsanche@...deno.ca>, "Mario Limonciello"
 <mario.limonciello@....com>, <platform-driver-x86@...r.kernel.org>, "LKML"
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] platform/x86: dell-pc: Transition to faux device

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@...il.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);
>>>
>>>
>> 

Powered by blists - more mailing lists