[PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[PATCH v6 0/6] hwmon: (dell-smm-hwmon) 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 next patch. The last patch is fixing a small bug.

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 v6:
- Make pwm1_enable permissions write-only
- Do not test fan speed in dell_smm_is_visible()

Changes in v5:
- Fix checkpatch warning after patch 5/6
- Hide fanX_label if fan type calls are disallowed

Changes in v4:
- Make fan detection behave like before patch 5/6
- Update coverletter title

Changes in v3:
- Update description of patch 1/6 and remove empty change
- Let pwm1_enable remain write-only
- Include a small bugfix

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) Fix fan mutliplier detection for 3rd fan

 drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++-----------------
 1 file changed, 419 insertions(+), 428 deletions(-)

--
2.20.1

[PATCH v6 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 since
the platform device is necessary for future
changes.
Also fix some checkpatch warnings.

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

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index f2221ca0aa7b..f06873413aea 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,6 +1246,7 @@ 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)
@@ -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 v6 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>
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 f06873413aea..c898d6bd6a18 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 v6 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>
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 c898d6bd6a18..da7040f2442e 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 v6 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 da7040f2442e..8841fc1e5872 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(const 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 v6 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 | 583 +++++++++++++++------------------
 1 file changed, 273 insertions(+), 310 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 8841fc1e5872..e842901d770f 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];
+	int temp_type[DELL_SMM_NO_TEMP];
+	bool fan[DELL_SMM_NO_FANS];
+	int fan_type[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);
@@ -284,10 +300,10 @@ static int _i8k_get_fan_type(const struct dell_smm_data *data, 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 */
-	if (data->types[fan] == INT_MIN)
-		data->types[fan] = _i8k_get_fan_type(data, fan);
+	if (data->fan_type[fan] == INT_MIN)
+		data->fan_type[fan] = _i8k_get_fan_type(data, fan);

-	return data->types[fan];
+	return data->fan_type[fan];
 }

 /*
@@ -334,7 +350,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 +650,293 @@ 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 (data->temp_type[channel] >= 0)
+				return 0444;
+
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		if (data->disallow_fan_support)
+			break;
+
+		switch (attr) {
+		case hwmon_fan_input:
+			if (data->fan[channel])
+				return 0444;
+
+			break;
+		case hwmon_fan_label:
+			if (data->fan[channel] && !data->disallow_fan_type_call)
+				return 0444;
+
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+		if (data->disallow_fan_support)
+			break;
+
+		switch (attr) {
+		case hwmon_pwm_input:
+			if (data->fan[channel])
+				return 0644;
+
+			break;
+		case hwmon_pwm_enable:
+			if (data->auto_fan)
+				return 0200;
+
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
 }

-static ssize_t i8k_hwmon_temp_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)
 {
-	int index = to_sensor_dev_attr(devattr)->index;
-	int temp;
+	struct dell_smm_data *data = dev_get_drvdata(dev);
+	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;

-	temp = i8k_get_temp(index);
-	if (temp < 0)
-		return temp;
-	return sprintf(buf, "%d\n", temp * 1000);
+			*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;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
 }

-static ssize_t i8k_hwmon_fan_label_show(struct device *dev,
-					struct device_attribute *devattr,
-					char *buf)
+static const char *dell_smm_fan_label(struct dell_smm_data *data, int channel)
 {
-	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;
+	int type = i8k_get_fan_type(data, channel);

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

 	if (type & 0x10) {
 		dock = true;
 		type &= 0x0F;
 	}

-	if (type >= ARRAY_SIZE(labels))
-		type = (ARRAY_SIZE(labels) - 1);
+	if (type >= ARRAY_SIZE(fan_labels))
+		type = ARRAY_SIZE(fan_labels) - 1;

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

-static ssize_t i8k_hwmon_fan_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 fan_speed;
-
-	fan_speed = i8k_get_fan_speed(data, index);
-	if (fan_speed < 0)
-		return fan_speed;
-	return sprintf(buf, "%d\n", fan_speed);
-}

-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;
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			*str = temp_labels[data->temp_type[channel]];
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_label:
+			*str = dell_smm_fan_label(data, channel);
+			return PTR_ERR_OR_ZERO(*str);
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	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));
+	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);
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	return err ? err : count;
+	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)
+static const struct hwmon_chip_info dell_smm_chip_info = {
+	.ops = &dell_smm_ops,
+	.info = dell_smm_info,
+};
+
+static int __init dell_smm_init_hwmon(struct device *dev)
 {
-	struct device *dev = kobj_to_dev(kobj);
 	struct dell_smm_data *data = dev_get_drvdata(dev);
+	struct device *dell_smm_hwmon_dev;
+	int i, err;

-	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;
-
-	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 (index == 23 && !data->auto_fan)
-		return 0;
+	for (i = 0; i < DELL_SMM_NO_TEMP; i++) {
+		data->temp_type[i] = i8k_get_temp_type(i);
+		if (data->temp_type[i] < 0)
+			continue;

-	return attr->mode;
-}
+		if (data->temp_type[i] >= ARRAY_SIZE(temp_labels))
+			data->temp_type[i] = ARRAY_SIZE(temp_labels) - 1;
+	}

-static const struct attribute_group i8k_group = {
-	.attrs = i8k_attrs,
-	.is_visible = i8k_is_visible,
-};
-__ATTRIBUTE_GROUPS(i8k);
+	for (i = 0; i < DELL_SMM_NO_FANS; i++) {
+		data->fan_type[i] = INT_MIN;
+		err = i8k_get_fan_status(data, i);
+		if (err < 0)
+			err = i8k_get_fan_type(data, i);
+		if (err >= 0)
+			data->fan[i] = true;
+	}

-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;
+	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 +1206,6 @@ 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;
 	platform_set_drvdata(pdev, data);

 	if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
--
2.20.1

[PATCH v6 6/6] hwmon: (dell-smm-hwmon) Fix fan mutliplier detection for 3rd fan

[W_Armin]

From: Armin Wolf <W_Armin@gmx.de>

There are up to three fans, but the detection omits the 3rd one.
Fix that by using DELL_SMM_NO_FANS.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Fixes: 747bc8b063ae (hwmon: (dell-smm) Detect fan with index=2)
---
 drivers/hwmon/dell-smm-hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index e842901d770f..1305e73e6741 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1257,7 +1257,7 @@ static int __init dell_smm_probe(struct platform_device *pdev)
 		 * Autodetect fan multiplier based on nominal rpm
 		 * If fan reports rpm value too high then set multiplier to 1
 		 */
-		for (fan = 0; fan < 2; ++fan) {
+		for (fan = 0; fan < DELL_SMM_NO_FANS; ++fan) {
 			ret = i8k_get_fan_nominal_speed(data, fan, data->i8k_fan_max);
 			if (ret < 0)
 				continue;
--
2.20.1

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Guenter Roeck]

On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
> 
> 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 v6:
> - Make pwm1_enable permissions write-only

Sorry, guess I am missing something. Why ?

Guenter

> - Do not test fan speed in dell_smm_is_visible()
> 
> Changes in v5:
> - Fix checkpatch warning after patch 5/6
> - Hide fanX_label if fan type calls are disallowed
> 
> Changes in v4:
> - Make fan detection behave like before patch 5/6
> - Update coverletter title
> 
> Changes in v3:
> - Update description of patch 1/6 and remove empty change
> - Let pwm1_enable remain write-only
> - Include a small bugfix
> 
> 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) Fix fan mutliplier detection for 3rd fan
> 
>   drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++-----------------
>   1 file changed, 419 insertions(+), 428 deletions(-)
> 
> --
> 2.20.1
> 

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Armin Wolf]

On 28.07.21 23:07 Guenter Roeck wrote:
> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>
>> 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 v6:
>> - Make pwm1_enable permissions write-only
>
> Sorry, guess I am missing something. Why ?
>
> Guenter
>
pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
permissions where 0200.
In the v5 patch series however, the file permission where not 0200, so i
changed that.

Armin
>> - Do not test fan speed in dell_smm_is_visible()
>>
>> Changes in v5:
>> - Fix checkpatch warning after patch 5/6
>> - Hide fanX_label if fan type calls are disallowed
>>
>> Changes in v4:
>> - Make fan detection behave like before patch 5/6
>> - Update coverletter title
>>
>> Changes in v3:
>> - Update description of patch 1/6 and remove empty change
>> - Let pwm1_enable remain write-only
>> - Include a small bugfix
>>
>> 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) Fix fan mutliplier detection for 3rd fan
>>
>>   drivers/hwmon/dell-smm-hwmon.c | 847 ++++++++++++++++-----------------
>>   1 file changed, 419 insertions(+), 428 deletions(-)
>>
>> --
>> 2.20.1
>>
>

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Guenter Roeck]

On 7/28/21 2:19 PM, Armin Wolf wrote:
> On 28.07.21 23:07 Guenter Roeck wrote:
>> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>>
>>> 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 v6:
>>> - Make pwm1_enable permissions write-only
>>
>> Sorry, guess I am missing something. Why ?
>>
>> Guenter
>>
> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
> permissions where 0200.
> In the v5 patch series however, the file permission where not 0200, so i
> changed that.
> 

Is there a _reason_ for declaring this attribute write only, other than
"it used to be that way" ?

Guenter

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Armin Wolf]

Am 28.07.21 um 23:28 schrieb Guenter Roeck:
> On 7/28/21 2:19 PM, Armin Wolf wrote:
>> On 28.07.21 23:07 Guenter Roeck wrote:
>>> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>>>
>>>> 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 v6:
>>>> - Make pwm1_enable permissions write-only
>>>
>>> Sorry, guess I am missing something. Why ?
>>>
>>> Guenter
>>>
>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
>> permissions where 0200.
>> In the v5 patch series however, the file permission where not 0200, so i
>> changed that.
>>
>
> Is there a _reason_ for declaring this attribute write only, other than
> "it used to be that way" ?
>
> Guenter

I dont think so, dell_smm_read will just return -EOPNOTSUPP if someone tries to read pwm1_enable.

Armin

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Guenter Roeck]

On 7/28/21 2:34 PM, Armin Wolf wrote:
> Am 28.07.21 um 23:28 schrieb Guenter Roeck:
>> On 7/28/21 2:19 PM, Armin Wolf wrote:
>>> On 28.07.21 23:07 Guenter Roeck wrote:
>>>> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>>>>
>>>>> 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 v6:
>>>>> - Make pwm1_enable permissions write-only
>>>>
>>>> Sorry, guess I am missing something. Why ?
>>>>
>>>> Guenter
>>>>
>>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
>>> permissions where 0200.
>>> In the v5 patch series however, the file permission where not 0200, so i
>>> changed that.
>>>
>>
>> Is there a _reason_ for declaring this attribute write only, other than
>> "it used to be that way" ?
>>
>> Guenter
> 
> I dont think so, dell_smm_read will just return -EOPNOTSUPP if someone tries to read pwm1_enable.
> 
> Armin
> 
That is not what I meant. Is there a reason to not report
the current status of pwm1_enable to the user ? In other
words, does the firmware not report its current status ?

Guenter

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Armin Wolf]

Am 28.07.21 um 23:37 schrieb Guenter Roeck:
> On 7/28/21 2:34 PM, Armin Wolf wrote:
>> Am 28.07.21 um 23:28 schrieb Guenter Roeck:
>>> On 7/28/21 2:19 PM, Armin Wolf wrote:
>>>> On 28.07.21 23:07 Guenter Roeck wrote:
>>>>> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>>>>>
>>>>>> 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 v6:
>>>>>> - Make pwm1_enable permissions write-only
>>>>>
>>>>> Sorry, guess I am missing something. Why ?
>>>>>
>>>>> Guenter
>>>>>
>>>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
>>>> permissions where 0200.
>>>> In the v5 patch series however, the file permission where not 0200,
>>>> so i
>>>> changed that.
>>>>
>>>
>>> Is there a _reason_ for declaring this attribute write only, other than
>>> "it used to be that way" ?
>>>
>>> Guenter
>>
>> I dont think so, dell_smm_read will just return -EOPNOTSUPP if
>> someone tries to read pwm1_enable.
>>
>> Armin
>>
> That is not what I meant. Is there a reason to not report
> the current status of pwm1_enable to the user ? In other
> words, does the firmware not report its current status ?
>
> Guenter

Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change
the state of BIOS fan control behind our back, we cannot simply return the last set value.

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Pali Rohár]

On Wednesday 28 July 2021 23:40:21 Armin Wolf wrote:
> Am 28.07.21 um 23:37 schrieb Guenter Roeck:
> > On 7/28/21 2:34 PM, Armin Wolf wrote:
> > > Am 28.07.21 um 23:28 schrieb Guenter Roeck:
> > > > On 7/28/21 2:19 PM, Armin Wolf wrote:
> > > > > On 28.07.21 23:07 Guenter Roeck wrote:
> > > > > > On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
> > > > > > > 
> > > > > > > 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 v6:
> > > > > > > - Make pwm1_enable permissions write-only
> > > > > > 
> > > > > > Sorry, guess I am missing something. Why ?
> > > > > > 
> > > > > > Guenter
> > > > > > 
> > > > > pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
> > > > > permissions where 0200.
> > > > > In the v5 patch series however, the file permission where not 0200,
> > > > > so i
> > > > > changed that.
> > > > > 
> > > > 
> > > > Is there a _reason_ for declaring this attribute write only, other than
> > > > "it used to be that way" ?
> > > > 
> > > > Guenter
> > > 
> > > I dont think so, dell_smm_read will just return -EOPNOTSUPP if
> > > someone tries to read pwm1_enable.
> > > 
> > > Armin
> > > 
> > That is not what I meant. Is there a reason to not report
> > the current status of pwm1_enable to the user ? In other
> > words, does the firmware not report its current status ?
> > 
> > Guenter
> 
> Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change
> the state of BIOS fan control behind our back, we cannot simply return the last set value.

We have already discussed this problem years ago, see:
https://lore.kernel.org/linux-hwmon/20160522152823.GA18331@roeck-us.net/

And also again this year:
https://lore.kernel.org/linux-hwmon/20210528211037.2tnblovgb3rkcwnq@pali/

Basically there is no known firmware command to retrieve current status
yet. And both userspace and SMM/firmware itself can change state. So
kernel has no way how to retrieve current status and caching last value
cannot be used (due to userspace and firmware can change it).

Whole SMM API is undocumented and seems that Dell does not want to
provide any documentation for it.

I know that it is pity that we have no read functionality, but we have
to deal with it...

Maybe it would make sense to add comment into driver code, why attribute
is write-only...

Re: [PATCH v6 0/6] hwmon: (dell-smm-hwmon) Convert to new hwmon registration api

[Guenter Roeck]

On 7/28/21 2:54 PM, Pali Rohár wrote:
> On Wednesday 28 July 2021 23:40:21 Armin Wolf wrote:
>> Am 28.07.21 um 23:37 schrieb Guenter Roeck:
>>> On 7/28/21 2:34 PM, Armin Wolf wrote:
>>>> Am 28.07.21 um 23:28 schrieb Guenter Roeck:
>>>>> On 7/28/21 2:19 PM, Armin Wolf wrote:
>>>>>> On 28.07.21 23:07 Guenter Roeck wrote:
>>>>>>> On 7/28/21 1:51 PM, 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 next patch. The last patch is fixing a small bug.
>>>>>>>>
>>>>>>>> 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 v6:
>>>>>>>> - Make pwm1_enable permissions write-only
>>>>>>>
>>>>>>> Sorry, guess I am missing something. Why ?
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>> pwm1_enable used SENSOR_DEVICE_ATTR_WO before the patch, so the file
>>>>>> permissions where 0200.
>>>>>> In the v5 patch series however, the file permission where not 0200,
>>>>>> so i
>>>>>> changed that.
>>>>>>
>>>>>
>>>>> Is there a _reason_ for declaring this attribute write only, other than
>>>>> "it used to be that way" ?
>>>>>
>>>>> Guenter
>>>>
>>>> I dont think so, dell_smm_read will just return -EOPNOTSUPP if
>>>> someone tries to read pwm1_enable.
>>>>
>>>> Armin
>>>>
>>> That is not what I meant. Is there a reason to not report
>>> the current status of pwm1_enable to the user ? In other
>>> words, does the firmware not report its current status ?
>>>
>>> Guenter
>>
>> Pali said the driver cannot query the state of pwm1_enable from the BIOS, and with userspace tools being able to change
>> the state of BIOS fan control behind our back, we cannot simply return the last set value.
> 
> We have already discussed this problem years ago, see:
> https://lore.kernel.org/linux-hwmon/20160522152823.GA18331@roeck-us.net/
> 
> And also again this year:
> https://lore.kernel.org/linux-hwmon/20210528211037.2tnblovgb3rkcwnq@pali/
> 
> Basically there is no known firmware command to retrieve current status
> yet. And both userspace and SMM/firmware itself can change state. So
> kernel has no way how to retrieve current status and caching last value
> cannot be used (due to userspace and firmware can change it).
> 
> Whole SMM API is undocumented and seems that Dell does not want to
> provide any documentation for it.
> 
> I know that it is pity that we have no read functionality, but we have
> to deal with it...
> 
> Maybe it would make sense to add comment into driver code, why attribute
> is write-only...
> 
Yes, please.

Thanks,
Guenter