[PATCH v2 0/6] Convert to new hwmon registration api

[PATCH v2 0/6] Convert to new hwmon registration api

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

This patch series is converting the dell-smm-hwmon driver
to the new hwmon registration API. In order to do so,
it introduces a platform device in the first patch, and
applies some optimisations in the next three patches.
The switch to the new hwmon registration API is done in
the last patch.

The caching of the fan/temp values was modified to better fit
the new hwmon API.

The patches work fine for my Dell Latitude C600, but i whould
appreciate someone testing the code on another model too.

Changes in v2:
- Fix coverletter title
- Update docs regarding pwm1_enable

Armin Wolf (6):
  hwmon: (dell-smm-hwmon) Use platform device
  hwmon: (dell-smm-hwmon) Mark functions as __init
  hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset()
  hwmon: (dell-smm-hwmon) Move variables into a driver private data
    structure
  hwmon: (dell-smm-hwmon) Convert to
    devm_hwmon_device_register_with_info()
  hwmon: (dell-smm-hwmon) Update docs

 Documentation/hwmon/dell-smm-hwmon.rst |   2 +-
 drivers/hwmon/dell-smm-hwmon.c         | 860 ++++++++++++-------------
 2 files changed, 426 insertions(+), 436 deletions(-)

--
2.20.1

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

[W_Armin]

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.

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));

-	/*
-	 * 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 */
 	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

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

[Pali Rohár]

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.

> 
> -	/*
> -	 * 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.

>  	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
> 

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

[Armin Wolf]

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
>>

[PATCH v2 2/6] hwmon: (dell-smm-hwmon) Mark functions as __init

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

i8k_get_dmi_data() and i8k_get_dell_signature() are
only called during module init and probe, which both
are marked as __init.
Also mark these function as __init to lower the runtime
memory footprint.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 2038f2a50e11..deba8819164d 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -128,7 +128,7 @@ struct smm_regs {
 	unsigned int edi __packed;
 };

-static inline const char *i8k_get_dmi_data(int field)
+static inline const char __init *i8k_get_dmi_data(int field)
 {
 	const char *dmi_data = dmi_get_system_info(field);

@@ -384,7 +384,7 @@ static int i8k_get_temp(int sensor)
 	return temp;
 }

-static int i8k_get_dell_signature(int req_fn)
+static int __init i8k_get_dell_signature(int req_fn)
 {
 	struct smm_regs regs = { .eax = req_fn, };
 	int rc;
--
2.20.1

Re: [PATCH v2 2/6] hwmon: (dell-smm-hwmon) Mark functions as __init

[Pali Rohár]

On Friday 28 May 2021 19:37:12 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> i8k_get_dmi_data() and i8k_get_dell_signature() are
> only called during module init and probe, which both
> are marked as __init.
> Also mark these function as __init to lower the runtime
> memory footprint.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Looks good!

Reviewed-by: Pali Rohár <pali@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 2038f2a50e11..deba8819164d 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -128,7 +128,7 @@ struct smm_regs {
>  	unsigned int edi __packed;
>  };
> 
> -static inline const char *i8k_get_dmi_data(int field)
> +static inline const char __init *i8k_get_dmi_data(int field)
>  {
>  	const char *dmi_data = dmi_get_system_info(field);
> 
> @@ -384,7 +384,7 @@ static int i8k_get_temp(int sensor)
>  	return temp;
>  }
> 
> -static int i8k_get_dell_signature(int req_fn)
> +static int __init i8k_get_dell_signature(int req_fn)
>  {
>  	struct smm_regs regs = { .eax = req_fn, };
>  	int rc;
> --
> 2.20.1
> 

[PATCH v2 3/6] hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset()

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Use devm_add_action_or_reset() for calling i8k_exit_procfs()
so the remove() function in dell_smm_driver can be omitted.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index deba8819164d..ed2c42badf1a 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -605,24 +605,22 @@ static const struct proc_ops i8k_proc_ops = {
 	.proc_ioctl	= i8k_ioctl,
 };

-static void __init i8k_init_procfs(void)
+static void i8k_exit_procfs(void *param)
 {
-	/* Register the proc entry */
-	proc_create("i8k", 0, NULL, &i8k_proc_ops);
+	remove_proc_entry("i8k", NULL);
 }

-static void __exit i8k_exit_procfs(void)
+static void __init i8k_init_procfs(struct device *dev)
 {
-	remove_proc_entry("i8k", NULL);
+	/* Register the proc entry */
+	proc_create("i8k", 0, NULL, &i8k_proc_ops);
+
+	devm_add_action_or_reset(dev, i8k_exit_procfs, NULL);
 }

 #else

-static inline void __init i8k_init_procfs(void)
-{
-}
-
-static inline void __exit i8k_exit_procfs(void)
+static void __init i8k_init_procfs(struct device *dev)
 {
 }

@@ -1287,14 +1285,7 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

-	i8k_init_procfs();
-
-	return 0;
-}
-
-static int dell_smm_remove(struct platform_device *pdev)
-{
-	i8k_exit_procfs();
+	i8k_init_procfs(&pdev->dev);

 	return 0;
 }
@@ -1303,7 +1294,6 @@ static struct platform_driver dell_smm_driver = {
 	.driver		= {
 		.name	= KBUILD_MODNAME,
 	},
-	.remove		= dell_smm_remove,
 };

 static struct platform_device *dell_smm_device;
--
2.20.1

Re: [PATCH v2 3/6] hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset()

[Pali Rohár]

On Friday 28 May 2021 19:37:13 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> Use devm_add_action_or_reset() for calling i8k_exit_procfs()
> so the remove() function in dell_smm_driver can be omitted.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Looks good!

Reviewed-by: Pali Rohár <pali@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>

> ---
>  drivers/hwmon/dell-smm-hwmon.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index deba8819164d..ed2c42badf1a 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -605,24 +605,22 @@ static const struct proc_ops i8k_proc_ops = {
>  	.proc_ioctl	= i8k_ioctl,
>  };
> 
> -static void __init i8k_init_procfs(void)
> +static void i8k_exit_procfs(void *param)
>  {
> -	/* Register the proc entry */
> -	proc_create("i8k", 0, NULL, &i8k_proc_ops);
> +	remove_proc_entry("i8k", NULL);
>  }
> 
> -static void __exit i8k_exit_procfs(void)
> +static void __init i8k_init_procfs(struct device *dev)
>  {
> -	remove_proc_entry("i8k", NULL);
> +	/* Register the proc entry */
> +	proc_create("i8k", 0, NULL, &i8k_proc_ops);
> +
> +	devm_add_action_or_reset(dev, i8k_exit_procfs, NULL);
>  }
> 
>  #else
> 
> -static inline void __init i8k_init_procfs(void)
> -{
> -}
> -
> -static inline void __exit i8k_exit_procfs(void)
> +static void __init i8k_init_procfs(struct device *dev)
>  {
>  }
> 
> @@ -1287,14 +1285,7 @@ static int __init dell_smm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
> 
> -	i8k_init_procfs();
> -
> -	return 0;
> -}
> -
> -static int dell_smm_remove(struct platform_device *pdev)
> -{
> -	i8k_exit_procfs();
> +	i8k_init_procfs(&pdev->dev);
> 
>  	return 0;
>  }
> @@ -1303,7 +1294,6 @@ static struct platform_driver dell_smm_driver = {
>  	.driver		= {
>  		.name	= KBUILD_MODNAME,
>  	},
> -	.remove		= dell_smm_remove,
>  };
> 
>  static struct platform_device *dell_smm_device;
> --
> 2.20.1
> 

[PATCH v2 4/6] hwmon: (dell-smm-hwmon) Move variables into a driver private data structure

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Move Variables into a driver private data structure.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 277 ++++++++++++++++++---------------
 1 file changed, 152 insertions(+), 125 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index ed2c42badf1a..420fdd2de2fb 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -60,18 +60,20 @@
 #define I8K_POWER_AC		0x05
 #define I8K_POWER_BATTERY	0x01

-static DEFINE_MUTEX(i8k_mutex);
-static char bios_version[4];
-static char bios_machineid[16];
-static struct device *i8k_hwmon_dev;
-static u32 i8k_hwmon_flags;
-static uint i8k_fan_mult = I8K_FAN_MULT;
-static uint i8k_pwm_mult;
-static uint i8k_fan_max = I8K_FAN_HIGH;
-static bool disallow_fan_type_call;
-static bool disallow_fan_support;
-static unsigned int manual_fan;
-static unsigned int auto_fan;
+struct dell_smm_data {
+	struct mutex i8k_mutex; /* lock for sensors writes */
+	char bios_version[4];
+	char bios_machineid[16];
+	u32 i8k_hwmon_flags;
+	uint i8k_fan_mult;
+	uint i8k_pwm_mult;
+	uint i8k_fan_max;
+	bool disallow_fan_type_call;
+	bool disallow_fan_support;
+	unsigned int manual_fan;
+	unsigned int auto_fan;
+	int types[3];
+};

 #define I8K_HWMON_HAVE_TEMP1	(1 << 0)
 #define I8K_HWMON_HAVE_TEMP2	(1 << 1)
@@ -240,11 +242,11 @@ static int i8k_smm(struct smm_regs *regs)
 /*
  * Read the fan status.
  */
-static int i8k_get_fan_status(int fan)
+static int i8k_get_fan_status(const struct dell_smm_data *data, int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };

-	if (disallow_fan_support)
+	if (data->disallow_fan_support)
 		return -EINVAL;

 	regs.ebx = fan & 0xff;
@@ -254,84 +256,82 @@ static int i8k_get_fan_status(int fan)
 /*
  * Read the fan speed in RPM.
  */
-static int i8k_get_fan_speed(int fan)
+static int i8k_get_fan_speed(const struct dell_smm_data *data, int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };

-	if (disallow_fan_support)
+	if (data->disallow_fan_support)
 		return -EINVAL;

 	regs.ebx = fan & 0xff;
-	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
 }

 /*
  * Read the fan type.
  */
-static int _i8k_get_fan_type(int fan)
+static int _i8k_get_fan_type(struct dell_smm_data *data, int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };

-	if (disallow_fan_support || disallow_fan_type_call)
+	if (data->disallow_fan_support || data->disallow_fan_type_call)
 		return -EINVAL;

 	regs.ebx = fan & 0xff;
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

-static int i8k_get_fan_type(int fan)
+static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
 {
 	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
-	static int types[3] = { INT_MIN, INT_MIN, INT_MIN };
+	if (data->types[fan] == INT_MIN)
+		data->types[fan] = _i8k_get_fan_type(data, fan);

-	if (types[fan] == INT_MIN)
-		types[fan] = _i8k_get_fan_type(fan);
-
-	return types[fan];
+	return data->types[fan];
 }

 /*
  * Read the fan nominal rpm for specific fan speed.
  */
-static int i8k_get_fan_nominal_speed(int fan, int speed)
+static int i8k_get_fan_nominal_speed(const struct dell_smm_data *data, int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };

-	if (disallow_fan_support)
+	if (data->disallow_fan_support)
 		return -EINVAL;

 	regs.ebx = (fan & 0xff) | (speed << 8);
-	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * i8k_fan_mult;
+	return i8k_smm(&regs) ? : (regs.eax & 0xffff) * data->i8k_fan_mult;
 }

 /*
  * Enable or disable automatic BIOS fan control support
  */
-static int i8k_enable_fan_auto_mode(bool enable)
+static int i8k_enable_fan_auto_mode(const struct dell_smm_data *data, bool enable)
 {
 	struct smm_regs regs = { };

-	if (disallow_fan_support)
+	if (data->disallow_fan_support)
 		return -EINVAL;

-	regs.eax = enable ? auto_fan : manual_fan;
+	regs.eax = enable ? data->auto_fan : data->manual_fan;
 	return i8k_smm(&regs);
 }

 /*
  * Set the fan speed (off, low, high). Returns the new fan status.
  */
-static int i8k_set_fan(int fan, int speed)
+static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };

-	if (disallow_fan_support)
+	if (data->disallow_fan_support)
 		return -EINVAL;

-	speed = (speed < 0) ? 0 : ((speed > i8k_fan_max) ? i8k_fan_max : speed);
+	speed = (speed < 0) ? 0 : ((speed > data->i8k_fan_max) ? data->i8k_fan_max : speed);
 	regs.ebx = (fan & 0xff) | (speed << 8);

-	return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
+	return i8k_smm(&regs) ? : i8k_get_fan_status(data, fan);
 }

 static int i8k_get_temp_type(int sensor)
@@ -442,7 +442,7 @@ static int i8k_get_power_status(void)
  */

 static int
-i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
+i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
 {
 	int val = 0;
 	int speed;
@@ -454,12 +454,12 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)

 	switch (cmd) {
 	case I8K_BIOS_VERSION:
-		if (!isdigit(bios_version[0]) || !isdigit(bios_version[1]) ||
-		    !isdigit(bios_version[2]))
+		if (!isdigit(data->bios_version[0]) || !isdigit(data->bios_version[1]) ||
+		    !isdigit(data->bios_version[2]))
 			return -EINVAL;

-		val = (bios_version[0] << 16) |
-				(bios_version[1] << 8) | bios_version[2];
+		val = (data->bios_version[0] << 16) |
+				(data->bios_version[1] << 8) | data->bios_version[2];
 		break;

 	case I8K_MACHINE_ID:
@@ -467,7 +467,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 			return -EPERM;

 		memset(buff, 0, sizeof(buff));
-		strlcpy(buff, bios_machineid, sizeof(buff));
+		strscpy(buff, data->bios_machineid, sizeof(buff));
 		break;

 	case I8K_FN_STATUS:
@@ -486,14 +486,14 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;

-		val = i8k_get_fan_speed(val);
+		val = i8k_get_fan_speed(data, val);
 		break;

 	case I8K_GET_FAN:
 		if (copy_from_user(&val, argp, sizeof(int)))
 			return -EFAULT;

-		val = i8k_get_fan_status(val);
+		val = i8k_get_fan_status(data, val);
 		break;

 	case I8K_SET_FAN:
@@ -506,7 +506,7 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&speed, argp + 1, sizeof(int)))
 			return -EFAULT;

-		val = i8k_set_fan(val, speed);
+		val = i8k_set_fan(data, val, speed);
 		break;

 	default:
@@ -539,11 +539,12 @@ i8k_ioctl_unlocked(struct file *fp, unsigned int cmd, unsigned long arg)

 static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
 {
+	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
 	long ret;

-	mutex_lock(&i8k_mutex);
-	ret = i8k_ioctl_unlocked(fp, cmd, arg);
-	mutex_unlock(&i8k_mutex);
+	mutex_lock(&data->i8k_mutex);
+	ret = i8k_ioctl_unlocked(fp, data, cmd, arg);
+	mutex_unlock(&data->i8k_mutex);

 	return ret;
 }
@@ -553,17 +554,18 @@ static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
  */
 static int i8k_proc_show(struct seq_file *seq, void *offset)
 {
+	struct dell_smm_data *data = seq->private;
 	int fn_key, cpu_temp, ac_power;
 	int left_fan, right_fan, left_speed, right_speed;

-	cpu_temp	= i8k_get_temp(0);			/* 11100 µs */
-	left_fan	= i8k_get_fan_status(I8K_FAN_LEFT);	/*   580 µs */
-	right_fan	= i8k_get_fan_status(I8K_FAN_RIGHT);	/*   580 µs */
-	left_speed	= i8k_get_fan_speed(I8K_FAN_LEFT);	/*   580 µs */
-	right_speed	= i8k_get_fan_speed(I8K_FAN_RIGHT);	/*   580 µs */
-	fn_key		= i8k_get_fn_status();			/*   750 µs */
+	cpu_temp	= i8k_get_temp(0);				/* 11100 µs */
+	left_fan	= i8k_get_fan_status(data, I8K_FAN_LEFT);	/*   580 µs */
+	right_fan	= i8k_get_fan_status(data, I8K_FAN_RIGHT);	/*   580 µs */
+	left_speed	= i8k_get_fan_speed(data, I8K_FAN_LEFT);	/*   580 µs */
+	right_speed	= i8k_get_fan_speed(data, I8K_FAN_RIGHT);	/*   580 µs */
+	fn_key		= i8k_get_fn_status();				/*   750 µs */
 	if (power_status)
-		ac_power = i8k_get_power_status();		/* 14700 µs */
+		ac_power = i8k_get_power_status();			/* 14700 µs */
 	else
 		ac_power = -1;

@@ -583,8 +585,8 @@ static int i8k_proc_show(struct seq_file *seq, void *offset)
 	 */
 	seq_printf(seq, "%s %s %s %d %d %d %d %d %d %d\n",
 		   I8K_PROC_FMT,
-		   bios_version,
-		   (restricted && !capable(CAP_SYS_ADMIN)) ? "-1" : bios_machineid,
+		   data->bios_version,
+		   (restricted && !capable(CAP_SYS_ADMIN)) ? "-1" : data->bios_machineid,
 		   cpu_temp,
 		   left_fan, right_fan, left_speed, right_speed,
 		   ac_power, fn_key);
@@ -594,7 +596,7 @@ static int i8k_proc_show(struct seq_file *seq, void *offset)

 static int i8k_open_fs(struct inode *inode, struct file *file)
 {
-	return single_open(file, i8k_proc_show, NULL);
+	return single_open(file, i8k_proc_show, PDE_DATA(inode));
 }

 static const struct proc_ops i8k_proc_ops = {
@@ -612,8 +614,10 @@ static void i8k_exit_procfs(void *param)

 static void __init i8k_init_procfs(struct device *dev)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
+
 	/* Register the proc entry */
-	proc_create("i8k", 0, NULL, &i8k_proc_ops);
+	proc_create_data("i8k", 0, NULL, &i8k_proc_ops, data);

 	devm_add_action_or_reset(dev, i8k_exit_procfs, NULL);
 }
@@ -670,6 +674,7 @@ static ssize_t i8k_hwmon_fan_label_show(struct device *dev,
 					struct device_attribute *devattr,
 					char *buf)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
 	static const char * const labels[] = {
 		"Processor Fan",
 		"Motherboard Fan",
@@ -682,7 +687,7 @@ static ssize_t i8k_hwmon_fan_label_show(struct device *dev,
 	bool dock = false;
 	int type;

-	type = i8k_get_fan_type(index);
+	type = i8k_get_fan_type(data, index);
 	if (type < 0)
 		return type;

@@ -700,10 +705,11 @@ static ssize_t i8k_hwmon_fan_label_show(struct device *dev,
 static ssize_t i8k_hwmon_fan_show(struct device *dev,
 				  struct device_attribute *devattr, char *buf)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
 	int fan_speed;

-	fan_speed = i8k_get_fan_speed(index);
+	fan_speed = i8k_get_fan_speed(data, index);
 	if (fan_speed < 0)
 		return fan_speed;
 	return sprintf(buf, "%d\n", fan_speed);
@@ -712,19 +718,21 @@ static ssize_t i8k_hwmon_fan_show(struct device *dev,
 static ssize_t i8k_hwmon_pwm_show(struct device *dev,
 				  struct device_attribute *devattr, char *buf)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
 	int status;

-	status = i8k_get_fan_status(index);
+	status = i8k_get_fan_status(data, index);
 	if (status < 0)
 		return -EIO;
-	return sprintf(buf, "%d\n", clamp_val(status * i8k_pwm_mult, 0, 255));
+	return sprintf(buf, "%d\n", clamp_val(status * data->i8k_pwm_mult, 0, 255));
 }

 static ssize_t i8k_hwmon_pwm_store(struct device *dev,
 				   struct device_attribute *attr,
 				   const char *buf, size_t count)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
 	unsigned long val;
 	int err;
@@ -732,11 +740,11 @@ static ssize_t i8k_hwmon_pwm_store(struct device *dev,
 	err = kstrtoul(buf, 10, &val);
 	if (err)
 		return err;
-	val = clamp_val(DIV_ROUND_CLOSEST(val, i8k_pwm_mult), 0, i8k_fan_max);
+	val = clamp_val(DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult), 0, data->i8k_fan_max);

-	mutex_lock(&i8k_mutex);
-	err = i8k_set_fan(index, val);
-	mutex_unlock(&i8k_mutex);
+	mutex_lock(&data->i8k_mutex);
+	err = i8k_set_fan(data, index, val);
+	mutex_unlock(&data->i8k_mutex);

 	return err < 0 ? -EIO : count;
 }
@@ -745,11 +753,12 @@ static ssize_t i8k_hwmon_pwm_enable_store(struct device *dev,
 					  struct device_attribute *attr,
 					  const char *buf, size_t count)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
 	int err;
 	bool enable;
 	unsigned long val;

-	if (!auto_fan)
+	if (!data->auto_fan)
 		return -ENODEV;

 	err = kstrtoul(buf, 10, &val);
@@ -763,9 +772,9 @@ static ssize_t i8k_hwmon_pwm_enable_store(struct device *dev,
 	else
 		return -EINVAL;

-	mutex_lock(&i8k_mutex);
-	err = i8k_enable_fan_auto_mode(enable);
-	mutex_unlock(&i8k_mutex);
+	mutex_lock(&data->i8k_mutex);
+	err = i8k_enable_fan_auto_mode(data, enable);
+	mutex_unlock(&data->i8k_mutex);

 	return err ? err : count;
 }
@@ -838,53 +847,56 @@ static struct attribute *i8k_attrs[] = {
 static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
 			      int index)
 {
-	if (disallow_fan_support && index >= 20)
+	struct device *dev = kobj_to_dev(kobj);
+	struct dell_smm_data *data = dev_get_drvdata(dev);
+
+	if (data->disallow_fan_support && index >= 20)
 		return 0;
-	if (disallow_fan_type_call &&
+	if (data->disallow_fan_type_call &&
 	    (index == 21 || index == 25 || index == 28))
 		return 0;
 	if (index >= 0 && index <= 1 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
 		return 0;
 	if (index >= 2 && index <= 3 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
 		return 0;
 	if (index >= 4 && index <= 5 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
 		return 0;
 	if (index >= 6 && index <= 7 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
 		return 0;
 	if (index >= 8 && index <= 9 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5))
 		return 0;
 	if (index >= 10 && index <= 11 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
 		return 0;
 	if (index >= 12 && index <= 13 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
 		return 0;
 	if (index >= 14 && index <= 15 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
 		return 0;
 	if (index >= 16 && index <= 17 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
 		return 0;
 	if (index >= 18 && index <= 19 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
 		return 0;

 	if (index >= 20 && index <= 23 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
 		return 0;
 	if (index >= 24 && index <= 26 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
 		return 0;
 	if (index >= 27 && index <= 29 &&
-	    !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
+	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
 		return 0;

-	if (index == 23 && !auto_fan)
+	if (index == 23 && !data->auto_fan)
 		return 0;

 	return attr->mode;
@@ -898,65 +910,65 @@ __ATTRIBUTE_GROUPS(i8k);

 static int __init dell_smm_init_hwmon(struct device *dev)
 {
+	struct dell_smm_data *data = dev_get_drvdata(dev);
+	struct device *i8k_hwmon_dev;
 	int err;

-	i8k_hwmon_flags = 0;
-
 	/* CPU temperature attributes, if temperature type is OK */
 	err = i8k_get_temp_type(0);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
 	/* check for additional temperature sensors */
 	err = i8k_get_temp_type(1);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
 	err = i8k_get_temp_type(2);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
 	err = i8k_get_temp_type(3);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
 	err = i8k_get_temp_type(4);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
 	err = i8k_get_temp_type(5);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
 	err = i8k_get_temp_type(6);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
 	err = i8k_get_temp_type(7);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
 	err = i8k_get_temp_type(8);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
 	err = i8k_get_temp_type(9);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;

 	/* First fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(0);
+	err = i8k_get_fan_status(data, 0);
 	if (err < 0)
-		err = i8k_get_fan_type(0);
+		err = i8k_get_fan_type(data, 0);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;

 	/* Second fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(1);
+	err = i8k_get_fan_status(data, 1);
 	if (err < 0)
-		err = i8k_get_fan_type(1);
+		err = i8k_get_fan_type(data, 1);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;

 	/* Third fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(2);
+	err = i8k_get_fan_status(data, 2);
 	if (err < 0)
-		err = i8k_get_fan_type(2);
+		err = i8k_get_fan_type(data, 2);
 	if (err >= 0)
-		i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN3;
+		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN3;

-	i8k_hwmon_dev = devm_hwmon_device_register_with_groups(dev, "dell_smm", NULL, i8k_groups);
+	i8k_hwmon_dev = devm_hwmon_device_register_with_groups(dev, "dell_smm", data, i8k_groups);

 	return PTR_ERR_OR_ZERO(i8k_hwmon_dev);
 }
@@ -1217,25 +1229,38 @@ static struct dmi_system_id i8k_whitelist_fan_control[] __initdata = {

 static int __init dell_smm_probe(struct platform_device *pdev)
 {
+	struct dell_smm_data *data;
 	const struct dmi_system_id *id, *fan_control;
 	int fan, ret;

+	data = devm_kzalloc(&pdev->dev, sizeof(struct dell_smm_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->i8k_mutex);
+	data->i8k_fan_mult = I8K_FAN_MULT;
+	data->i8k_fan_max = I8K_FAN_HIGH;
+	data->types[0] = INT_MIN;
+	data->types[1] = INT_MIN;
+	data->types[2] = INT_MIN;
+	platform_set_drvdata(pdev, data);
+
 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
-		pr_warn("broken Dell BIOS detected, disallow fan support\n");
+		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan support\n");
 		if (!force)
-			disallow_fan_support = true;
+			data->disallow_fan_support = true;
 	}

 	if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
-		pr_warn("broken Dell BIOS detected, disallow fan type call\n");
+		dev_warn(&pdev->dev, "broken Dell BIOS detected, disallow fan type call\n");
 		if (!force)
-			disallow_fan_type_call = true;
+			data->disallow_fan_type_call = true;
 	}

-	strscpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
-		sizeof(bios_version));
-	strscpy(bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
-		sizeof(bios_machineid));
+	strscpy(data->bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
+		sizeof(data->bios_version));
+	strscpy(data->bios_machineid, i8k_get_dmi_data(DMI_PRODUCT_SERIAL),
+		sizeof(data->bios_machineid));

 	/*
 	 * Set fan multiplier and maximal fan speed from dmi config
@@ -1247,20 +1272,21 @@ static int __init dell_smm_probe(struct platform_device *pdev)

 		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_pwm_mult = DIV_ROUND_UP(255, i8k_fan_max);
+	data->i8k_fan_max = fan_max ? : I8K_FAN_HIGH;	/* Must not be 0 */
+	data->i8k_pwm_mult = DIV_ROUND_UP(255, data->i8k_fan_max);

 	fan_control = dmi_first_match(i8k_whitelist_fan_control);
 	if (fan_control && fan_control->driver_data) {
-		const struct i8k_fan_control_data *data = fan_control->driver_data;
+		const struct i8k_fan_control_data *control = fan_control->driver_data;

-		manual_fan = data->manual_fan;
-		auto_fan = data->auto_fan;
-		pr_info("enabling support for setting automatic/manual fan control\n");
+		data->manual_fan = control->manual_fan;
+		data->auto_fan = control->auto_fan;
+		dev_info(&pdev->dev, "enabling support for setting automatic/manual fan control\n");
 	}

 	if (!fan_mult) {
@@ -1269,16 +1295,17 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 		 * If fan reports rpm value too high then set multiplier to 1
 		 */
 		for (fan = 0; fan < 2; ++fan) {
-			ret = i8k_get_fan_nominal_speed(fan, i8k_fan_max);
+			ret = i8k_get_fan_nominal_speed(data, fan, data->i8k_fan_max);
 			if (ret < 0)
 				continue;
+
 			if (ret > I8K_FAN_MAX_RPM)
-				i8k_fan_mult = 1;
+				data->i8k_fan_mult = 1;
 			break;
 		}
 	} else {
 		/* Fan multiplier was specified in module param or in dmi */
-		i8k_fan_mult = fan_mult;
+		data->i8k_fan_mult = fan_mult;
 	}

 	ret = dell_smm_init_hwmon(&pdev->dev);
--
2.20.1

[PATCH v2 5/6] hwmon: (dell-smm-hwmon) Convert to devm_hwmon_device_register_with_info()

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

Convert to new registration API to get rid of attribute magic
numbers and reduce module size.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/dell-smm-hwmon.c | 600 +++++++++++++++------------------
 1 file changed, 281 insertions(+), 319 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 420fdd2de2fb..504ade34833e 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -25,7 +25,6 @@
 #include <linux/capability.h>
 #include <linux/mutex.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
 #include <linux/sched.h>
@@ -60,11 +59,13 @@
 #define I8K_POWER_AC		0x05
 #define I8K_POWER_BATTERY	0x01

+#define DELL_SMM_NO_TEMP	10
+#define DELL_SMM_NO_FANS	3
+
 struct dell_smm_data {
 	struct mutex i8k_mutex; /* lock for sensors writes */
 	char bios_version[4];
 	char bios_machineid[16];
-	u32 i8k_hwmon_flags;
 	uint i8k_fan_mult;
 	uint i8k_pwm_mult;
 	uint i8k_fan_max;
@@ -72,23 +73,11 @@ struct dell_smm_data {
 	bool disallow_fan_support;
 	unsigned int manual_fan;
 	unsigned int auto_fan;
-	int types[3];
+	long auto_fan_enable;
+	const char *temp_label[DELL_SMM_NO_TEMP];
+	const char *fan_label[DELL_SMM_NO_FANS];
 };

-#define I8K_HWMON_HAVE_TEMP1	(1 << 0)
-#define I8K_HWMON_HAVE_TEMP2	(1 << 1)
-#define I8K_HWMON_HAVE_TEMP3	(1 << 2)
-#define I8K_HWMON_HAVE_TEMP4	(1 << 3)
-#define I8K_HWMON_HAVE_TEMP5	(1 << 4)
-#define I8K_HWMON_HAVE_TEMP6	(1 << 5)
-#define I8K_HWMON_HAVE_TEMP7	(1 << 6)
-#define I8K_HWMON_HAVE_TEMP8	(1 << 7)
-#define I8K_HWMON_HAVE_TEMP9	(1 << 8)
-#define I8K_HWMON_HAVE_TEMP10	(1 << 9)
-#define I8K_HWMON_HAVE_FAN1	(1 << 10)
-#define I8K_HWMON_HAVE_FAN2	(1 << 11)
-#define I8K_HWMON_HAVE_FAN3	(1 << 12)
-
 MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
 MODULE_AUTHOR("Pali Rohár <pali@kernel.org>");
 MODULE_DESCRIPTION("Dell laptop SMM BIOS hwmon driver");
@@ -130,6 +119,33 @@ struct smm_regs {
 	unsigned int edi __packed;
 };

+static const char * const temp_labels[] = {
+	"CPU",
+	"GPU",
+	"SODIMM",
+	"Other",
+	"Ambient",
+	"Other",
+};
+
+static const char * const fan_labels[] = {
+	"Processor Fan",
+	"Motherboard Fan",
+	"Video Fan",
+	"Power Supply Fan",
+	"Chipset Fan",
+	"Other Fan",
+};
+
+static const char * const docking_labels[] = {
+	"Docking Processor Fan",
+	"Docking Motherboard Fan",
+	"Docking Video Fan",
+	"Docking Power Supply Fan",
+	"Docking Chipset Fan",
+	"Docking Other Fan",
+};
+
 static inline const char __init *i8k_get_dmi_data(int field)
 {
 	const char *dmi_data = dmi_get_system_info(field);
@@ -270,7 +286,7 @@ static int i8k_get_fan_speed(const struct dell_smm_data *data, int fan)
 /*
  * Read the fan type.
  */
-static int _i8k_get_fan_type(struct dell_smm_data *data, int fan)
+static int __init i8k_get_fan_type(const struct dell_smm_data *data, int fan)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };

@@ -281,15 +297,6 @@ static int _i8k_get_fan_type(struct dell_smm_data *data, int fan)
 	return i8k_smm(&regs) ? : regs.eax & 0xff;
 }

-static int i8k_get_fan_type(struct dell_smm_data *data, int fan)
-{
-	/* I8K_SMM_GET_FAN_TYPE SMM call is expensive, so cache values */
-	if (data->types[fan] == INT_MIN)
-		data->types[fan] = _i8k_get_fan_type(data, fan);
-
-	return data->types[fan];
-}
-
 /*
  * Read the fan nominal rpm for specific fan speed.
  */
@@ -334,7 +341,7 @@ static int i8k_set_fan(const struct dell_smm_data *data, int fan, int speed)
 	return i8k_smm(&regs) ? : i8k_get_fan_status(data, fan);
 }

-static int i8k_get_temp_type(int sensor)
+static int __init i8k_get_temp_type(int sensor)
 {
 	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP_TYPE, };

@@ -634,343 +641,300 @@ static void __init i8k_init_procfs(struct device *dev)
  * Hwmon interface
  */

-static ssize_t i8k_hwmon_temp_label_show(struct device *dev,
-					 struct device_attribute *devattr,
-					 char *buf)
+static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
+				   int channel)
 {
-	static const char * const labels[] = {
-		"CPU",
-		"GPU",
-		"SODIMM",
-		"Other",
-		"Ambient",
-		"Other",
-	};
-	int index = to_sensor_dev_attr(devattr)->index;
-	int type;
+	const struct dell_smm_data *data = drvdata;

-	type = i8k_get_temp_type(index);
-	if (type < 0)
-		return type;
-	if (type >= ARRAY_SIZE(labels))
-		type = ARRAY_SIZE(labels) - 1;
-	return sprintf(buf, "%s\n", labels[type]);
-}
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_label:
+			if (!IS_ERR(data->temp_label[channel]))
+				return 0444;

-static ssize_t i8k_hwmon_temp_show(struct device *dev,
-				   struct device_attribute *devattr,
-				   char *buf)
-{
-	int index = to_sensor_dev_attr(devattr)->index;
-	int temp;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		if (data->disallow_fan_support)
+			break;

-	temp = i8k_get_temp(index);
-	if (temp < 0)
-		return temp;
-	return sprintf(buf, "%d\n", temp * 1000);
-}
+		switch (attr) {
+		case hwmon_fan_input:
+			if (i8k_get_fan_speed(data, channel) >= 0)
+				return 0444;

-static ssize_t i8k_hwmon_fan_label_show(struct device *dev,
-					struct device_attribute *devattr,
-					char *buf)
-{
-	struct dell_smm_data *data = dev_get_drvdata(dev);
-	static const char * const labels[] = {
-		"Processor Fan",
-		"Motherboard Fan",
-		"Video Fan",
-		"Power Supply Fan",
-		"Chipset Fan",
-		"Other Fan",
-	};
-	int index = to_sensor_dev_attr(devattr)->index;
-	bool dock = false;
-	int type;
+			break;
+		case hwmon_fan_label:
+			if (!IS_ERR(data->fan_label[channel]))
+				return 0444;

-	type = i8k_get_fan_type(data, index);
-	if (type < 0)
-		return type;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+		if (data->disallow_fan_support)
+			break;

-	if (type & 0x10) {
-		dock = true;
-		type &= 0x0F;
-	}
+		switch (attr) {
+		case hwmon_pwm_input:
+			if (i8k_get_fan_status(data, channel) >= 0)
+				return 0644;

-	if (type >= ARRAY_SIZE(labels))
-		type = (ARRAY_SIZE(labels) - 1);
+			break;
+		case hwmon_pwm_enable:
+			if (data->auto_fan)
+				return 0644;
+
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	return sprintf(buf, "%s%s\n", (dock ? "Docking " : ""), labels[type]);
+	return 0;
 }

-static ssize_t i8k_hwmon_fan_show(struct device *dev,
-				  struct device_attribute *devattr, char *buf)
+static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+			 long *val)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int fan_speed;
+	int ret;
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			ret = i8k_get_temp(channel);
+			if (ret < 0)
+				return ret;
+
+			*val = ret * 1000;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			ret = i8k_get_fan_speed(data, channel);
+			if (ret < 0)
+				return ret;
+
+			*val = ret;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			ret = i8k_get_fan_status(data, channel);
+			if (ret < 0)
+				return ret;
+
+			*val = clamp_val(ret * data->i8k_pwm_mult, 0, 255);
+
+			return 0;
+		case hwmon_pwm_enable:
+			*val = data->auto_fan_enable;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	fan_speed = i8k_get_fan_speed(data, index);
-	if (fan_speed < 0)
-		return fan_speed;
-	return sprintf(buf, "%d\n", fan_speed);
+	return -EOPNOTSUPP;
 }

-static ssize_t i8k_hwmon_pwm_show(struct device *dev,
-				  struct device_attribute *devattr, char *buf)
+static int dell_smm_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+				int channel, const char **str)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int status;

-	status = i8k_get_fan_status(data, index);
-	if (status < 0)
-		return -EIO;
-	return sprintf(buf, "%d\n", clamp_val(status * data->i8k_pwm_mult, 0, 255));
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			*str = data->temp_label[channel];
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_label:
+			*str = data->fan_label[channel];
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
 }

-static ssize_t i8k_hwmon_pwm_store(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t count)
+static int dell_smm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+			  long val)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	unsigned long val;
+	unsigned long pwm;
+	bool enable;
 	int err;

-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
-	val = clamp_val(DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult), 0, data->i8k_fan_max);
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			pwm = clamp_val(DIV_ROUND_CLOSEST(val, data->i8k_pwm_mult), 0,
+					data->i8k_fan_max);

-	mutex_lock(&data->i8k_mutex);
-	err = i8k_set_fan(data, index, val);
-	mutex_unlock(&data->i8k_mutex);
+			mutex_lock(&data->i8k_mutex);
+			err = i8k_set_fan(data, channel, pwm);
+			mutex_unlock(&data->i8k_mutex);

-	return err < 0 ? -EIO : count;
-}
+			if (err < 0)
+				return err;

-static ssize_t i8k_hwmon_pwm_enable_store(struct device *dev,
-					  struct device_attribute *attr,
-					  const char *buf, size_t count)
-{
-	struct dell_smm_data *data = dev_get_drvdata(dev);
-	int err;
-	bool enable;
-	unsigned long val;
+			return 0;
+		case hwmon_pwm_enable:
+			if (!val)
+				return -EINVAL;

-	if (!data->auto_fan)
-		return -ENODEV;
+			if (val == 1)
+				enable = false;
+			else
+				enable = true;

-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
+			mutex_lock(&data->i8k_mutex);
+			err = i8k_enable_fan_auto_mode(data, enable);
+			mutex_unlock(&data->i8k_mutex);

-	if (val == 1)
-		enable = false;
-	else if (val == 2)
-		enable = true;
-	else
-		return -EINVAL;
+			if (err < 0)
+				return err;

-	mutex_lock(&data->i8k_mutex);
-	err = i8k_enable_fan_auto_mode(data, enable);
-	mutex_unlock(&data->i8k_mutex);
+			data->auto_fan_enable = val;

-	return err ? err : count;
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
 }

-static SENSOR_DEVICE_ATTR_RO(temp1_input, i8k_hwmon_temp, 0);
-static SENSOR_DEVICE_ATTR_RO(temp1_label, i8k_hwmon_temp_label, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, i8k_hwmon_temp, 1);
-static SENSOR_DEVICE_ATTR_RO(temp2_label, i8k_hwmon_temp_label, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_input, i8k_hwmon_temp, 2);
-static SENSOR_DEVICE_ATTR_RO(temp3_label, i8k_hwmon_temp_label, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_input, i8k_hwmon_temp, 3);
-static SENSOR_DEVICE_ATTR_RO(temp4_label, i8k_hwmon_temp_label, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_input, i8k_hwmon_temp, 4);
-static SENSOR_DEVICE_ATTR_RO(temp5_label, i8k_hwmon_temp_label, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_input, i8k_hwmon_temp, 5);
-static SENSOR_DEVICE_ATTR_RO(temp6_label, i8k_hwmon_temp_label, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_input, i8k_hwmon_temp, 6);
-static SENSOR_DEVICE_ATTR_RO(temp7_label, i8k_hwmon_temp_label, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_input, i8k_hwmon_temp, 7);
-static SENSOR_DEVICE_ATTR_RO(temp8_label, i8k_hwmon_temp_label, 7);
-static SENSOR_DEVICE_ATTR_RO(temp9_input, i8k_hwmon_temp, 8);
-static SENSOR_DEVICE_ATTR_RO(temp9_label, i8k_hwmon_temp_label, 8);
-static SENSOR_DEVICE_ATTR_RO(temp10_input, i8k_hwmon_temp, 9);
-static SENSOR_DEVICE_ATTR_RO(temp10_label, i8k_hwmon_temp_label, 9);
-static SENSOR_DEVICE_ATTR_RO(fan1_input, i8k_hwmon_fan, 0);
-static SENSOR_DEVICE_ATTR_RO(fan1_label, i8k_hwmon_fan_label, 0);
-static SENSOR_DEVICE_ATTR_RW(pwm1, i8k_hwmon_pwm, 0);
-static SENSOR_DEVICE_ATTR_WO(pwm1_enable, i8k_hwmon_pwm_enable, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, i8k_hwmon_fan, 1);
-static SENSOR_DEVICE_ATTR_RO(fan2_label, i8k_hwmon_fan_label, 1);
-static SENSOR_DEVICE_ATTR_RW(pwm2, i8k_hwmon_pwm, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_input, i8k_hwmon_fan, 2);
-static SENSOR_DEVICE_ATTR_RO(fan3_label, i8k_hwmon_fan_label, 2);
-static SENSOR_DEVICE_ATTR_RW(pwm3, i8k_hwmon_pwm, 2);
-
-static struct attribute *i8k_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,	/* 0 */
-	&sensor_dev_attr_temp1_label.dev_attr.attr,	/* 1 */
-	&sensor_dev_attr_temp2_input.dev_attr.attr,	/* 2 */
-	&sensor_dev_attr_temp2_label.dev_attr.attr,	/* 3 */
-	&sensor_dev_attr_temp3_input.dev_attr.attr,	/* 4 */
-	&sensor_dev_attr_temp3_label.dev_attr.attr,	/* 5 */
-	&sensor_dev_attr_temp4_input.dev_attr.attr,	/* 6 */
-	&sensor_dev_attr_temp4_label.dev_attr.attr,	/* 7 */
-	&sensor_dev_attr_temp5_input.dev_attr.attr,	/* 8 */
-	&sensor_dev_attr_temp5_label.dev_attr.attr,	/* 9 */
-	&sensor_dev_attr_temp6_input.dev_attr.attr,	/* 10 */
-	&sensor_dev_attr_temp6_label.dev_attr.attr,	/* 11 */
-	&sensor_dev_attr_temp7_input.dev_attr.attr,	/* 12 */
-	&sensor_dev_attr_temp7_label.dev_attr.attr,	/* 13 */
-	&sensor_dev_attr_temp8_input.dev_attr.attr,	/* 14 */
-	&sensor_dev_attr_temp8_label.dev_attr.attr,	/* 15 */
-	&sensor_dev_attr_temp9_input.dev_attr.attr,	/* 16 */
-	&sensor_dev_attr_temp9_label.dev_attr.attr,	/* 17 */
-	&sensor_dev_attr_temp10_input.dev_attr.attr,	/* 18 */
-	&sensor_dev_attr_temp10_label.dev_attr.attr,	/* 19 */
-	&sensor_dev_attr_fan1_input.dev_attr.attr,	/* 20 */
-	&sensor_dev_attr_fan1_label.dev_attr.attr,	/* 21 */
-	&sensor_dev_attr_pwm1.dev_attr.attr,		/* 22 */
-	&sensor_dev_attr_pwm1_enable.dev_attr.attr,	/* 23 */
-	&sensor_dev_attr_fan2_input.dev_attr.attr,	/* 24 */
-	&sensor_dev_attr_fan2_label.dev_attr.attr,	/* 25 */
-	&sensor_dev_attr_pwm2.dev_attr.attr,		/* 26 */
-	&sensor_dev_attr_fan3_input.dev_attr.attr,	/* 27 */
-	&sensor_dev_attr_fan3_label.dev_attr.attr,	/* 28 */
-	&sensor_dev_attr_pwm3.dev_attr.attr,		/* 29 */
+static const struct hwmon_ops dell_smm_ops = {
+	.is_visible = dell_smm_is_visible,
+	.read = dell_smm_read,
+	.read_string = dell_smm_read_string,
+	.write = dell_smm_write,
+};
+
+static const struct hwmon_channel_info *dell_smm_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL
+			   ),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_LABEL,
+			   HWMON_F_INPUT | HWMON_F_LABEL,
+			   HWMON_F_INPUT | HWMON_F_LABEL
+			   ),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT
+			   ),
 	NULL
 };

-static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
-			      int index)
-{
-	struct device *dev = kobj_to_dev(kobj);
-	struct dell_smm_data *data = dev_get_drvdata(dev);
+static const struct hwmon_chip_info dell_smm_chip_info = {
+	.ops = &dell_smm_ops,
+	.info = dell_smm_info,
+};

-	if (data->disallow_fan_support && index >= 20)
-		return 0;
-	if (data->disallow_fan_type_call &&
-	    (index == 21 || index == 25 || index == 28))
-		return 0;
-	if (index >= 0 && index <= 1 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP1))
-		return 0;
-	if (index >= 2 && index <= 3 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP2))
-		return 0;
-	if (index >= 4 && index <= 5 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP3))
-		return 0;
-	if (index >= 6 && index <= 7 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
-		return 0;
-	if (index >= 8 && index <= 9 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5))
-		return 0;
-	if (index >= 10 && index <= 11 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
-		return 0;
-	if (index >= 12 && index <= 13 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
-		return 0;
-	if (index >= 14 && index <= 15 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
-		return 0;
-	if (index >= 16 && index <= 17 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
-		return 0;
-	if (index >= 18 && index <= 19 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
-		return 0;
+static const char __init *dell_smm_temp_label(int channel)
+{
+	int type = i8k_get_temp_type(channel);

-	if (index >= 20 && index <= 23 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
-		return 0;
-	if (index >= 24 && index <= 26 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
-		return 0;
-	if (index >= 27 && index <= 29 &&
-	    !(data->i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
-		return 0;
+	if (type < 0)
+		return ERR_PTR(type);

-	if (index == 23 && !data->auto_fan)
-		return 0;
+	if (type >= ARRAY_SIZE(temp_labels))
+		type = ARRAY_SIZE(temp_labels) - 1;

-	return attr->mode;
+	return temp_labels[type];
 }

-static const struct attribute_group i8k_group = {
-	.attrs = i8k_attrs,
-	.is_visible = i8k_is_visible,
-};
-__ATTRIBUTE_GROUPS(i8k);
+static const char __init *dell_smm_fan_label(struct dell_smm_data *data, int channel)
+{
+	bool dock = false;
+	int type = i8k_get_fan_type(data, channel);
+
+	if (type < 0)
+		return ERR_PTR(type);
+
+	if (type & 0x10) {
+		dock = true;
+		type &= 0x0F;
+	}
+
+	if (type >= ARRAY_SIZE(fan_labels))
+		type = ARRAY_SIZE(fan_labels) - 1;
+
+	return dock ? docking_labels[type] : fan_labels[type];
+}

 static int __init dell_smm_init_hwmon(struct device *dev)
 {
 	struct dell_smm_data *data = dev_get_drvdata(dev);
-	struct device *i8k_hwmon_dev;
-	int err;
+	struct device *dell_smm_hwmon_dev;
+	int i;
+
+	for (i = 0; i < DELL_SMM_NO_TEMP; i++)
+		data->temp_label[i] = dell_smm_temp_label(i);
+
+	for (i = 0; i < DELL_SMM_NO_FANS; i++)
+		data->fan_label[i] = dell_smm_fan_label(data, i);
+
+	dell_smm_hwmon_dev = devm_hwmon_device_register_with_info(dev, "dell_smm", data,
+								  &dell_smm_chip_info, NULL);

-	/* CPU temperature attributes, if temperature type is OK */
-	err = i8k_get_temp_type(0);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
-	/* check for additional temperature sensors */
-	err = i8k_get_temp_type(1);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
-	err = i8k_get_temp_type(2);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
-	err = i8k_get_temp_type(3);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
-	err = i8k_get_temp_type(4);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
-	err = i8k_get_temp_type(5);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
-	err = i8k_get_temp_type(6);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
-	err = i8k_get_temp_type(7);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
-	err = i8k_get_temp_type(8);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
-	err = i8k_get_temp_type(9);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
-
-	/* First fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(data, 0);
-	if (err < 0)
-		err = i8k_get_fan_type(data, 0);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN1;
-
-	/* Second fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(data, 1);
-	if (err < 0)
-		err = i8k_get_fan_type(data, 1);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN2;
-
-	/* Third fan attributes, if fan status or type is OK */
-	err = i8k_get_fan_status(data, 2);
-	if (err < 0)
-		err = i8k_get_fan_type(data, 2);
-	if (err >= 0)
-		data->i8k_hwmon_flags |= I8K_HWMON_HAVE_FAN3;
-
-	i8k_hwmon_dev = devm_hwmon_device_register_with_groups(dev, "dell_smm", data, i8k_groups);
-
-	return PTR_ERR_OR_ZERO(i8k_hwmon_dev);
+	return PTR_ERR_OR_ZERO(dell_smm_hwmon_dev);
 }

 struct i8k_config_data {
@@ -1240,9 +1204,7 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 	mutex_init(&data->i8k_mutex);
 	data->i8k_fan_mult = I8K_FAN_MULT;
 	data->i8k_fan_max = I8K_FAN_HIGH;
-	data->types[0] = INT_MIN;
-	data->types[1] = INT_MIN;
-	data->types[2] = INT_MIN;
+	data->auto_fan_enable = 2;
 	platform_set_drvdata(pdev, data);

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
--
2.20.1

[PATCH v2 6/6] hwmon: (dell-smm-hwmon) Update docs

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

pwm1_enable is now readable too.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/dell-smm-hwmon.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index 3bf77a5df995..d6fe9b8a2c40 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -35,7 +35,7 @@ Name				Perm	Description
 fan[1-3]_input                  RO      Fan speed in RPM.
 fan[1-3]_label                  RO      Fan label.
 pwm[1-3]                        RW      Control the fan PWM duty-cycle.
-pwm1_enable                     WO      Enable or disable automatic BIOS fan
+pwm1_enable                     RW      Enable or disable automatic BIOS fan
                                         control (not supported on all laptops,
                                         see below for details).
 temp[1-10]_input                RO      Temperature reading in milli-degrees
--
2.20.1

Re: [PATCH v2 6/6] hwmon: (dell-smm-hwmon) Update docs

[Pali Rohár]

On Friday 28 May 2021 19:37:16 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> pwm1_enable is now readable too.

Hello! pwm1_enable cannot be readable. It is write-only node. See also:
https://lore.kernel.org/linux-hwmon/201605221717.26604@pali/

> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  Documentation/hwmon/dell-smm-hwmon.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index 3bf77a5df995..d6fe9b8a2c40 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -35,7 +35,7 @@ Name				Perm	Description
>  fan[1-3]_input                  RO      Fan speed in RPM.
>  fan[1-3]_label                  RO      Fan label.
>  pwm[1-3]                        RW      Control the fan PWM duty-cycle.
> -pwm1_enable                     WO      Enable or disable automatic BIOS fan
> +pwm1_enable                     RW      Enable or disable automatic BIOS fan
>                                          control (not supported on all laptops,
>                                          see below for details).
>  temp[1-10]_input                RO      Temperature reading in milli-degrees
> --
> 2.20.1
> 

Re: [PATCH v2 6/6] hwmon: (dell-smm-hwmon) Update docs

[Armin Wolf]

On 28.05.21 19:53 Pali Rohár wrote:

> On Friday 28 May 2021 19:37:16 W_Armin@gmx.de wrote:
>> From: Armin Wolf <W_Armin@gmx.de>
>>
>> pwm1_enable is now readable too.
> Hello! pwm1_enable cannot be readable. It is write-only node. See also:
> https://lore.kernel.org/linux-hwmon/201605221717.26604@pali/

Hello,

in Documentation/hwmon/sysfs-interface, pwmX_enable is marked as RW, and the document also states that
"Read/write values may be read-only for some chips, depending on the hardware implementation", so i
thought that pwm1_enable being WO is a violation of that rule.

Also i belive the i8kutils are not calling SMM anymore, so can we just leave a note saying something like
"this value is cached and is not correct when using certain userspace tools which disable/enable BIOS fan control"?

>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   Documentation/hwmon/dell-smm-hwmon.rst | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
>> index 3bf77a5df995..d6fe9b8a2c40 100644
>> --- a/Documentation/hwmon/dell-smm-hwmon.rst
>> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
>> @@ -35,7 +35,7 @@ Name				Perm	Description
>>   fan[1-3]_input                  RO      Fan speed in RPM.
>>   fan[1-3]_label                  RO      Fan label.
>>   pwm[1-3]                        RW      Control the fan PWM duty-cycle.
>> -pwm1_enable                     WO      Enable or disable automatic BIOS fan
>> +pwm1_enable                     RW      Enable or disable automatic BIOS fan
>>                                           control (not supported on all laptops,
>>                                           see below for details).
>>   temp[1-10]_input                RO      Temperature reading in milli-degrees
>> --
>> 2.20.1
>>

Re: [PATCH v2 6/6] hwmon: (dell-smm-hwmon) Update docs

[Pali Rohár]

On Friday 28 May 2021 22:37:06 Armin Wolf wrote:
> Am 28.05.21 um 19:53 schrieb Pali Rohár:
> > On Friday 28 May 2021 19:37:16 W_Armin@gmx.de wrote:
> > > From: Armin Wolf <W_Armin@gmx.de>
> > > 
> > > pwm1_enable is now readable too.
> > Hello! pwm1_enable cannot be readable. It is write-only node. See also:
> > https://lore.kernel.org/linux-hwmon/201605221717.26604@pali/
> > 
> Hello,
> 
> in Documentation/hwmon/sysfs-interface, pwmX_enable is marked as RW, and the document also states that
> "Read/write values may be read-only for some chips, depending on the hardware implementation", so i
> thought that pwm1_enable being WO is a violation of that rule.

I know that hwmon sysfs are rw but if driver cannot provide correct read
value then it is better to do not report it.

> Also i belive the i8kutils are not calling SMM anymore, so can we just leave a note saying something like
> "this value is cached and is not correct when using certain userspace tools which disable/enable BIOS fan control"?

Well, maybe state changed in time (it is 5 years since that email) but
there are still more older versions of i8k userspace tools and some of
them used to issue SMM calls. Personally, I have used them (prior
introducing kernel support) and I can expect that other people who wrote
other scripts may have them in their inittab (or converted to systemd
equivalent). So it is better to not expect that something is not used.

Whole SMM stuff is fragile for various reasons...

Also another issue is that initial value is unknown. Because we do not
know if BIOS fan control is enabled or disabled.

I was told that in past BIOS fan control was old Dell laptops was
disabled, so if people adds old laptops into whilelist it would mean
that we even do not guess state of BIOS fan control status.

> > > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> > > ---
> > >   Documentation/hwmon/dell-smm-hwmon.rst | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> > > index 3bf77a5df995..d6fe9b8a2c40 100644
> > > --- a/Documentation/hwmon/dell-smm-hwmon.rst
> > > +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> > > @@ -35,7 +35,7 @@ Name				Perm	Description
> > >   fan[1-3]_input                  RO      Fan speed in RPM.
> > >   fan[1-3]_label                  RO      Fan label.
> > >   pwm[1-3]                        RW      Control the fan PWM duty-cycle.
> > > -pwm1_enable                     WO      Enable or disable automatic BIOS fan
> > > +pwm1_enable                     RW      Enable or disable automatic BIOS fan
> > >                                           control (not supported on all laptops,
> > >                                           see below for details).
> > >   temp[1-10]_input                RO      Temperature reading in milli-degrees
> > > --
> > > 2.20.1
> > > 
> 

Re: [PATCH v2 6/6] hwmon: (dell-smm-hwmon) Update docs

[Guenter Roeck]

On Fri, May 28, 2021 at 10:37:06PM +0200, Armin Wolf wrote:
> Am 28.05.21 um 19:53 schrieb Pali Rohár:
> > On Friday 28 May 2021 19:37:16 W_Armin@gmx.de wrote:
> > > From: Armin Wolf <W_Armin@gmx.de>
> > > 
> > > pwm1_enable is now readable too.
> > Hello! pwm1_enable cannot be readable. It is write-only node. See also:
> > https://lore.kernel.org/linux-hwmon/201605221717.26604@pali/
> > 
> Hello,
> 
> in Documentation/hwmon/sysfs-interface, pwmX_enable is marked as RW, and the document also states that
> "Read/write values may be read-only for some chips, depending on the hardware implementation", so i
> thought that pwm1_enable being WO is a violation of that rule.
> 

Reality doesn't always match expectations.

Guenter

Re: [PATCH v2 0/6] Convert to new hwmon registration api

[Pali Rohár]

On Friday 28 May 2021 19:37:10 W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
> 
> This patch series is converting the dell-smm-hwmon driver
> to the new hwmon registration API. In order to do so,
> it introduces a platform device in the first patch, and
> applies some optimisations in the next three patches.
> The switch to the new hwmon registration API is done in
> the last patch.
> 
> The caching of the fan/temp values was modified to better fit
> the new hwmon API.
> 
> The patches work fine for my Dell Latitude C600, but i whould
> appreciate someone testing the code on another model too.
> 
> Changes in v2:
> - Fix coverletter title
> - Update docs regarding pwm1_enable
> 
> Armin Wolf (6):
>   hwmon: (dell-smm-hwmon) Use platform device
>   hwmon: (dell-smm-hwmon) Mark functions as __init
>   hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset()
>   hwmon: (dell-smm-hwmon) Move variables into a driver private data
>     structure
>   hwmon: (dell-smm-hwmon) Convert to
>     devm_hwmon_device_register_with_info()
>   hwmon: (dell-smm-hwmon) Update docs
> 
>  Documentation/hwmon/dell-smm-hwmon.rst |   2 +-
>  drivers/hwmon/dell-smm-hwmon.c         | 860 ++++++++++++-------------
>  2 files changed, 426 insertions(+), 436 deletions(-)
> 
> --
> 2.20.1
> 

Hello! I tried to apply this patch series but it failed on error
malformed patch. I guess that it is because patches in emails were sent
in quoted-printable transfer encoding (instead of transparent 8-bit).
I will look later how to convert them into 8-bit, for clean apply.

Re: [PATCH v2 0/6] Convert to new hwmon registration api

[Pali Rohár]

On Friday 28 May 2021 20:32:54 Pali Rohár wrote:
> On Friday 28 May 2021 19:37:10 W_Armin@gmx.de wrote:
> > From: Armin Wolf <W_Armin@gmx.de>
> > 
> > This patch series is converting the dell-smm-hwmon driver
> > to the new hwmon registration API. In order to do so,
> > it introduces a platform device in the first patch, and
> > applies some optimisations in the next three patches.
> > The switch to the new hwmon registration API is done in
> > the last patch.
> > 
> > The caching of the fan/temp values was modified to better fit
> > the new hwmon API.
> > 
> > The patches work fine for my Dell Latitude C600, but i whould
> > appreciate someone testing the code on another model too.
> > 
> > Changes in v2:
> > - Fix coverletter title
> > - Update docs regarding pwm1_enable
> > 
> > Armin Wolf (6):
> >   hwmon: (dell-smm-hwmon) Use platform device
> >   hwmon: (dell-smm-hwmon) Mark functions as __init
> >   hwmon: (dell-smm-hwmon) Use devm_add_action_or_reset()
> >   hwmon: (dell-smm-hwmon) Move variables into a driver private data
> >     structure
> >   hwmon: (dell-smm-hwmon) Convert to
> >     devm_hwmon_device_register_with_info()
> >   hwmon: (dell-smm-hwmon) Update docs
> > 
> >  Documentation/hwmon/dell-smm-hwmon.rst |   2 +-
> >  drivers/hwmon/dell-smm-hwmon.c         | 860 ++++++++++++-------------
> >  2 files changed, 426 insertions(+), 436 deletions(-)
> > 
> > --
> > 2.20.1
> > 
> 
> Hello! I tried to apply this patch series but it failed on error
> malformed patch. I guess that it is because patches in emails were sent
> in quoted-printable transfer encoding (instead of transparent 8-bit).
> I will look later how to convert them into 8-bit, for clean apply.

Quick "hack": in mutt just open email body in "v"-view and then save
content. It is then clean 8-bit, fully decoded and patch can be applied.