Re: [PATCH v2 1/6] hwmon: (dell-smm-hwmon) Use platform device

[Armin Wolf <W_Armin@gmx.de>]

On 28.05.21 20:53 Pali Rohár wrote:

> On Friday 28 May 2021 19:37:11 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> Register a platform device for usage with
>> devm_hwmon_device_register_with_groups.
>> Also the platform device is necessary for
>> future changes.
> This patch changes output in 'sensors' utility:
>
> Without this patch it prints:
>    dell_smm-virtual-0
>    Adapter: Virtual device
>
> And with patch it prints:
>    dell_smm-isa-0000
>    Adapter: ISA adapter
>
> Interesting is that in this patch there is no reference to ISA and
> neither to Virtual.
>
> I think it needs to be related to symlinks in /sys/class/hwmon hierarchy
> as this patch changes it:
>
> (prior)
> $ ll /sys/class/hwmon/hwmon1
> lrwxrwxrwx 1 root root 0 máj 28 20:42 /sys/class/hwmon/hwmon1 -> ../../devices/virtual/hwmon/hwmon1/
>
> (after)
> $ ll /sys/class/hwmon/hwmon1
> lrwxrwxrwx 1 root root 0 máj 28 20:42 /sys/class/hwmon/hwmon1 -> ../../devices/platform/dell_smm_hwmon/hwmon/hwmon1/
>
> But I'm not sure what is correct to print in 'Adapter' section. Both
> Virtual and ISA looks like bogus values for which I do not care.
>
> Guenter, I will this part up to you, what you want to have in output.
>
> Other comments are below.
>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/hwmon/dell-smm-hwmon.c | 115 ++++++++++++++++++---------------
>>   1 file changed, 63 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index f2221ca0aa7b..2038f2a50e11 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -14,7 +14,9 @@
>>
>>   #include <linux/cpu.h>
>>   #include <linux/delay.h>
>> +#include <linux/err.h>
>>   #include <linux/module.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/types.h>
>>   #include <linux/init.h>
>>   #include <linux/proc_fs.h>
>> @@ -896,7 +898,7 @@ static const struct attribute_group i8k_group = {
>>   };
>>   __ATTRIBUTE_GROUPS(i8k);
>>
>> -static int __init i8k_init_hwmon(void)
>> +static int __init dell_smm_init_hwmon(struct device *dev)
>>   {
>>   	int err;
>>
>> @@ -956,15 +958,9 @@ static int __init i8k_init_hwmon(void)
>>   	if (err >= 0)
>>   		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN3;
>>
>> -	i8k_hwmon_dev = hwmon_device_register_with_groups(NULL, "dell_smm",
>> -							  NULL, i8k_groups);
>> -	if (IS_ERR(i8k_hwmon_dev)) {
>> -		err = PTR_ERR(i8k_hwmon_dev);
>> -		i8k_hwmon_dev = NULL;
>> -		pr_err("hwmon registration failed (%d)\n", err);
>> -		return err;
>> -	}
>> -	return 0;
>> +	i8k_hwmon_dev = devm_hwmon_device_register_with_groups(dev, "dell_smm", NULL, i8k_groups);
>> +
>> +	return PTR_ERR_OR_ZERO(i8k_hwmon_dev);
>>   }
>>
>>   struct i8k_config_data {
>> @@ -1221,28 +1217,11 @@ static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {
>>   	{ }
>>   };
>>
>> -/*
>> - * Probe for the presence of a supported laptop.
>> - */
>> -static int __init i8k_probe(void)
>> +static int __init dell_smm_probe(struct platform_device *pdev)
>>   {
>>   	const struct dmi_system_id *id, *fan_control;
>>   	int fan, ret;
>>
>> -	/*
>> -	 * Get DMI information
>> -	 */
>> -	if (!dmi_check_system(i8k_dmi_table)) {
>> -		if (!ignore_dmi && !force)
>> -			return -ENODEV;
>> -
>> -		pr_info("not running on a supported Dell system.\n");
>> -		pr_info("vendor=%s, model=%s, version=%s\n",
>> -			i8k_get_dmi_data(DMI_SYS_VENDOR),
>> -			i8k_get_dmi_data(DMI_PRODUCT_NAME),
>> -			i8k_get_dmi_data(DMI_BIOS_VERSION));
>> -	}
>> -
>>   	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
>>   		pr_warn("broken Dell BIOS detected, disallow fan support\n");
>>   		if (!force)
>> @@ -1255,21 +1234,11 @@ static int __init i8k_probe(void)
>>   			disallow_fan_type_call = true;
>>   	}
>>
>> -	strlcpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
>> +	strscpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
>>   		sizeof(bios_version));
>> -	strlcpy(bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
>> +	strscpy(bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
>>   		sizeof(bios_machineid));
> It is intentional to replace strlcpy by strscpy? If yes then I think it
> should be part of other change as this replacement is not related nor
> not needed by platform device registration.

It was because of a checkpatch warning, i will add that to the commit description
in the next version.

>> -	/*
>> -	 * Get SMM Dell signature
>> -	 */
>> -	if (i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG1) &&
>> -	    i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG2)) {
>> -		pr_err("unable to get SMM Dell signature\n");
>> -		if (!force)
>> -			return -ENODEV;
>> -	}
>> -
>>   	/*
>>   	 * Set fan multiplier and maximal fan speed from dmi config
>>   	 * Values specified in module parameters override values from dmi
>> @@ -1277,13 +1246,14 @@ static int __init i8k_probe(void)
>>   	id = dmi_first_match(i8k_dmi_table);
>>   	if (id && id->driver_data) {
>>   		const struct i8k_config_data *conf = id->driver_data;
>> +
>>   		if (!fan_mult && conf->fan_mult)
>>   			fan_mult = conf->fan_mult;
>>   		if (!fan_max && conf->fan_max)
>>   			fan_max = conf->fan_max;
>>   	}
>>
>> -	i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
>> +	i8k_fan_max = fan_max ? : I8K_FAN_HIGH; /* Must not be 0 */
> This is empty change?
>
> Hm... now I see tab with 1 char size is replaced by space. Quite hard to
> detect this change as it was not mentioned in commit message and such
> tab is visible in most editors in the same way as as space.

My bad, i removed that empty change.

>>   	i8k_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
>>
>>   	fan_control = dmi_first_match(i8k_whitelist_fan_control);
>> @@ -1313,29 +1283,70 @@ static int __init i8k_probe(void)
>>   		i8k_fan_mult = fan_mult;
>>   	}
>>
>> +	ret = dell_smm_init_hwmon(&pdev->dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	i8k_init_procfs();
>> +
>>   	return 0;
>>   }
>>
>> +static int dell_smm_remove(struct platform_device *pdev)
>> +{
>> +	i8k_exit_procfs();
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver dell_smm_driver = {
>> +	.driver		= {
>> +		.name	= KBUILD_MODNAME,
>> +	},
>> +	.remove		= dell_smm_remove,
>> +};
>> +
>> +static struct platform_device *dell_smm_device;
>> +
>> +/*
>> + * Probe for the presence of a supported laptop.
>> + */
>>   static int __init i8k_init(void)
>>   {
>> -	int err;
>> +	/*
>> +	 * Get DMI information
>> +	 */
>> +	if (!dmi_check_system(i8k_dmi_table)) {
>> +		if (!ignore_dmi && !force)
>> +			return -ENODEV;
>>
>> -	/* Are we running on an supported laptop? */
>> -	if (i8k_probe())
>> -		return -ENODEV;
>> +		pr_info("not running on a supported Dell system.\n");
>> +		pr_info("vendor=%s, model=%s, version=%s\n",
>> +			i8k_get_dmi_data(DMI_SYS_VENDOR),
>> +			i8k_get_dmi_data(DMI_PRODUCT_NAME),
>> +			i8k_get_dmi_data(DMI_BIOS_VERSION));
>> +	}
>>
>> -	err = i8k_init_hwmon();
>> -	if (err)
>> -		return err;
>> +	/*
>> +	 * Get SMM Dell signature
>> +	 */
>> +	if (i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG1) &&
>> +	    i8k_get_dell_signature(I8K_SMM_GET_DELL_SIG2)) {
>> +		pr_err("unable to get SMM Dell signature\n");
>> +		if (!force)
>> +			return -ENODEV;
>> +	}
>>
>> -	i8k_init_procfs();
>> -	return 0;
>> +	dell_smm_device = platform_create_bundle(&dell_smm_driver, dell_smm_probe, NULL, 0, NULL,
>> +						 0);
>> +
>> +	return PTR_ERR_OR_ZERO(dell_smm_device);
>>   }
>>
>>   static void __exit i8k_exit(void)
>>   {
>> -	hwmon_device_unregister(i8k_hwmon_dev);
>> -	i8k_exit_procfs();
>> +	platform_device_unregister(dell_smm_device);
>> +	platform_driver_unregister(&dell_smm_driver);
>>   }
>>
>>   module_init(i8k_init);
>> --
>> 2.20.1
>>