[PATCH 0/3] hwmon: (ftsteutates) Various fixes

[PATCH 0/3] hwmon: (ftsteutates) Various fixes

[Armin Wolf]

Merry Christmas,

a user reported strange sensors values when using the ftsteutates driver
on kernel 6.1. A bit of research revealed that the drivers was omiting
any scaling of the measurements, causing strange sensor values being
reported with the sensors command. This was likely done since when the
ftsteutates driver was still out-of-tree, all scaling was done by
libsensors with an custom config file provided by Fujitsu.
The first patch fixes this issue.

The seconds patch converts the driver to use the modern *_with_info() API
and also adds support for fanX_fault and pwmX_auto_channels_temp attributes.
Since the behaviour of the pwmX_auto_channels_temp attributes is a bit
special with the teutates BMC, it is added to the drivers documentation.

The last patch is a minor improvement regarding the watchdog registration.

All changes where tested on a Fujitsu DS3401-B1 and apppear to work.

Armin Wolf (3):
  hwmon: (ftsteutates) Fix scaling of measurements
  hwmon: (ftsteutates) Convert to devm_hwmon_device_register_with_info()
  hwmon: (ftsteutates) Use devm_watchdog_register_device()

 Documentation/hwmon/ftsteutates.rst |   9 +
 drivers/hwmon/ftsteutates.c         | 551 ++++++++++------------------
 2 files changed, 213 insertions(+), 347 deletions(-)

--
2.30.2

[PATCH 1/3] hwmon: (ftsteutates) Fix scaling of measurements

[Armin Wolf]

A user complained that the ftsteutates driver was displaying
bogus values since its introduction. This happens because the
sensor measurements need to be scaled in order to produce
meaningful results:
- the fan speed needs to be multiplied by 60 since its in RPS
- the temperature is in degrees celsius and needs an offset of 64
- the voltage is in 1/256 of 3.3V

The offical datasheet says the voltage needs to be divided by 256,
but this is likely an off-by-one-error, since even the BIOS
devides by 255 (otherwise 3.3V could not be measured).

The voltage channels additionally need a board-specific multiplier,
however this can be done by the driver since its board-specific.

The reason the missing scaling of measurements is the way Fujitsu
used this driver when it was still out-of-tree. Back then, all
scaling was done in userspace by libsensors, even the generic one.

Tested on a Fujitsu DS3401-B1.

Fixes: 08426eda58e0 ("hwmon: Add driver for FTS BMC chip "Teutates"")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/ftsteutates.rst |  4 ++++
 drivers/hwmon/ftsteutates.c         | 19 +++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
index 58a2483d8d0d..198fa8e2819d 100644
--- a/Documentation/hwmon/ftsteutates.rst
+++ b/Documentation/hwmon/ftsteutates.rst
@@ -22,6 +22,10 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
 8 fans. It also contains an integrated watchdog which is currently
 implemented in this driver.

+The 4 voltages require a board-specific multiplier, since the BMC can
+only measure voltages up to 3.3V and thus relies on voltage dividers.
+Consult your motherboard manual for details.
+
 To clear a temperature or fan alarm, execute the following command with the
 correct path to the alarm file::

diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
index f5b8e724a8ca..ffa0bb364877 100644
--- a/drivers/hwmon/ftsteutates.c
+++ b/drivers/hwmon/ftsteutates.c
@@ -12,6 +12,7 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
+#include <linux/math.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
@@ -347,13 +348,15 @@ static ssize_t in_value_show(struct device *dev,
 {
 	struct fts_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
+	int value, err;

 	err = fts_update_device(data);
 	if (err < 0)
 		return err;

-	return sprintf(buf, "%u\n", data->volt[index]);
+	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
+
+	return sprintf(buf, "%d\n", value);
 }

 static ssize_t temp_value_show(struct device *dev,
@@ -361,13 +364,15 @@ static ssize_t temp_value_show(struct device *dev,
 {
 	struct fts_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
+	int value, err;

 	err = fts_update_device(data);
 	if (err < 0)
 		return err;

-	return sprintf(buf, "%u\n", data->temp_input[index]);
+	value = (data->temp_input[index] - 64) * 1000;
+
+	return sprintf(buf, "%d\n", value);
 }

 static ssize_t temp_fault_show(struct device *dev,
@@ -436,13 +441,15 @@ static ssize_t fan_value_show(struct device *dev,
 {
 	struct fts_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
+	int value, err;

 	err = fts_update_device(data);
 	if (err < 0)
 		return err;

-	return sprintf(buf, "%u\n", data->fan_input[index]);
+	value = data->fan_input[index] * 60;
+
+	return sprintf(buf, "%d\n", value);
 }

 static ssize_t fan_source_show(struct device *dev,
--
2.30.2

[PATCH 2/3] hwmon: (ftsteutates) Convert to devm_hwmon_device_register_with_info()

[Armin Wolf]

Convert driver to use devm_hwmon_device_register_with_info()
to reduce module size by 40%. The non-standard fan_source
attribute is replaced with the standard pwmX_auto_channels_temp
attribute and its special behaviour being documented.
Also export fan present status as fanX_fault.

Tested on a Fujitsu DS3401-B1.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/ftsteutates.rst |   5 +
 drivers/hwmon/ftsteutates.c         | 546 ++++++++++------------------
 2 files changed, 207 insertions(+), 344 deletions(-)

diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
index 198fa8e2819d..b3bfec36661d 100644
--- a/Documentation/hwmon/ftsteutates.rst
+++ b/Documentation/hwmon/ftsteutates.rst
@@ -22,6 +22,11 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
 8 fans. It also contains an integrated watchdog which is currently
 implemented in this driver.

+The ``pwmX_auto_channels_temp`` attributes show which temperature sensor
+is currently driving which fan channel. This value might dynamically change
+during runtime depending on the temperature sensor selected by
+the fan control circuit.
+
 The 4 voltages require a board-specific multiplier, since the BMC can
 only measure voltages up to 3.3V and thus relies on voltage dividers.
 Consult your motherboard manual for details.
diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
index ffa0bb364877..66ff8d69a0de 100644
--- a/drivers/hwmon/ftsteutates.c
+++ b/drivers/hwmon/ftsteutates.c
@@ -6,9 +6,7 @@
  *		  Thilo Cestonaro <thilo.cestonaro@ts.fujitsu.com>
  */
 #include <linux/err.h>
-#include <linux/fs.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
@@ -16,8 +14,6 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
-#include <linux/sysfs.h>
-#include <linux/uaccess.h>
 #include <linux/watchdog.h>

 #define FTS_DEVICE_ID_REG		0x0000
@@ -340,376 +336,240 @@ static int fts_watchdog_init(struct fts_data *data)
 	return watchdog_register_device(&data->wdd);
 }

-/*****************************************************************************/
-/* SysFS handler functions						     */
-/*****************************************************************************/
-static ssize_t in_value_show(struct device *dev,
-			     struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int value, err;
-
-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
-
-	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
-
-	return sprintf(buf, "%d\n", value);
-}
-
-static ssize_t temp_value_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int value, err;
-
-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
-
-	value = (data->temp_input[index] - 64) * 1000;
-
-	return sprintf(buf, "%d\n", value);
-}
-
-static ssize_t temp_fault_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
-
-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
-
-	/* 00h Temperature = Sensor Error */
-	return sprintf(buf, "%d\n", data->temp_input[index] == 0);
-}
-
-static ssize_t temp_alarm_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
+static umode_t fts_is_visible(const void *devdata, enum hwmon_sensor_types type, u32 attr,
+			      int channel)
 {
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
-
-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_fault:
+			return 0444;
+		case hwmon_temp_alarm:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_fault:
+			return 0444;
+		case hwmon_fan_alarm:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+	case hwmon_in:
+		return 0444;
+	default:
+		break;
+	}

-	return sprintf(buf, "%u\n", !!(data->temp_alarm & BIT(index)));
+	return 0;
 }

-static ssize_t
-temp_alarm_store(struct device *dev, struct device_attribute *devattr,
-		 const char *buf, size_t count)
+static int fts_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+		    long *val)
 {
 	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	long ret;
+	int ret = fts_update_device(data);

-	ret = fts_update_device(data);
 	if (ret < 0)
 		return ret;

-	if (kstrtoul(buf, 10, &ret) || ret != 0)
-		return -EINVAL;
-
-	mutex_lock(&data->update_lock);
-	ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(index));
-	if (ret < 0)
-		goto error;
-
-	ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(index),
-			     ret | 0x1);
-	if (ret < 0)
-		goto error;
-
-	data->valid = false;
-	ret = count;
-error:
-	mutex_unlock(&data->update_lock);
-	return ret;
-}
-
-static ssize_t fan_value_show(struct device *dev,
-			      struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int value, err;
-
-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			*val = (data->temp_input[channel] - 64) * 1000;

-	value = data->fan_input[index] * 60;
+			return 0;
+		case hwmon_temp_alarm:
+			*val = !!(data->temp_alarm & BIT(channel));

-	return sprintf(buf, "%d\n", value);
-}
+			return 0;
+		case hwmon_temp_fault:
+			/* 00h Temperature = Sensor Error */;
+			*val = (data->temp_input[channel] == 0);

-static ssize_t fan_source_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			*val = data->fan_input[channel] * 60;

-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
+			return 0;
+		case hwmon_fan_alarm:
+			*val = !!(data->fan_alarm & BIT(channel));

-	return sprintf(buf, "%u\n", data->fan_source[index]);
-}
+			return 0;
+		case hwmon_fan_fault:
+			*val = !(data->fan_present & BIT(channel));

-static ssize_t fan_alarm_show(struct device *dev,
-			      struct device_attribute *devattr, char *buf)
-{
-	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	int err;
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_auto_channels_temp:
+			if (data->fan_source[channel] == 0xff)
+				*val = 0;
+			else
+				*val = BIT(data->fan_source[channel]);
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			*val = DIV_ROUND_CLOSEST(data->volt[channel] * 3300, 255);

-	err = fts_update_device(data);
-	if (err < 0)
-		return err;
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	return sprintf(buf, "%d\n", !!(data->fan_alarm & BIT(index)));
+	return -EOPNOTSUPP;
 }

-static ssize_t
-fan_alarm_store(struct device *dev, struct device_attribute *devattr,
-		const char *buf, size_t count)
+static int fts_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+		     long val)
 {
 	struct fts_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	long ret;
+	int ret = fts_update_device(data);

-	ret = fts_update_device(data);
 	if (ret < 0)
 		return ret;

-	if (kstrtoul(buf, 10, &ret) || ret != 0)
-		return -EINVAL;
-
-	mutex_lock(&data->update_lock);
-	ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(index));
-	if (ret < 0)
-		goto error;
-
-	ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(index),
-			     ret | 0x1);
-	if (ret < 0)
-		goto error;
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_alarm:
+			if (val)
+				return -EINVAL;
+
+			mutex_lock(&data->update_lock);
+			ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(channel));
+			if (ret >= 0)
+				ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(channel),
+						     ret | 0x1);
+			if (ret >= 0)
+				data->valid = false;
+
+			mutex_unlock(&data->update_lock);
+			if (ret < 0)
+				return ret;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_alarm:
+			if (val)
+				return -EINVAL;
+
+			mutex_lock(&data->update_lock);
+			ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(channel));
+			if (ret >= 0)
+				ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(channel),
+						     ret | 0x1);
+			if (ret >= 0)
+				data->valid = false;
+
+			mutex_unlock(&data->update_lock);
+			if (ret < 0)
+				return ret;
+
+			return 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}

-	data->valid = false;
-	ret = count;
-error:
-	mutex_unlock(&data->update_lock);
-	return ret;
+	return -EOPNOTSUPP;
 }

-/*****************************************************************************/
-/* SysFS structs							     */
-/*****************************************************************************/
-
-/* Temperature sensors */
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_value, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp_value, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_input, temp_value, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_input, temp_value, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_input, temp_value, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_input, temp_value, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_input, temp_value, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_input, temp_value, 7);
-static SENSOR_DEVICE_ATTR_RO(temp9_input, temp_value, 8);
-static SENSOR_DEVICE_ATTR_RO(temp10_input, temp_value, 9);
-static SENSOR_DEVICE_ATTR_RO(temp11_input, temp_value, 10);
-static SENSOR_DEVICE_ATTR_RO(temp12_input, temp_value, 11);
-static SENSOR_DEVICE_ATTR_RO(temp13_input, temp_value, 12);
-static SENSOR_DEVICE_ATTR_RO(temp14_input, temp_value, 13);
-static SENSOR_DEVICE_ATTR_RO(temp15_input, temp_value, 14);
-static SENSOR_DEVICE_ATTR_RO(temp16_input, temp_value, 15);
-
-static SENSOR_DEVICE_ATTR_RO(temp1_fault, temp_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp_fault, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_fault, temp_fault, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_fault, temp_fault, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_fault, temp_fault, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_fault, temp_fault, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_fault, temp_fault, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_fault, temp_fault, 7);
-static SENSOR_DEVICE_ATTR_RO(temp9_fault, temp_fault, 8);
-static SENSOR_DEVICE_ATTR_RO(temp10_fault, temp_fault, 9);
-static SENSOR_DEVICE_ATTR_RO(temp11_fault, temp_fault, 10);
-static SENSOR_DEVICE_ATTR_RO(temp12_fault, temp_fault, 11);
-static SENSOR_DEVICE_ATTR_RO(temp13_fault, temp_fault, 12);
-static SENSOR_DEVICE_ATTR_RO(temp14_fault, temp_fault, 13);
-static SENSOR_DEVICE_ATTR_RO(temp15_fault, temp_fault, 14);
-static SENSOR_DEVICE_ATTR_RO(temp16_fault, temp_fault, 15);
-
-static SENSOR_DEVICE_ATTR_RW(temp1_alarm, temp_alarm, 0);
-static SENSOR_DEVICE_ATTR_RW(temp2_alarm, temp_alarm, 1);
-static SENSOR_DEVICE_ATTR_RW(temp3_alarm, temp_alarm, 2);
-static SENSOR_DEVICE_ATTR_RW(temp4_alarm, temp_alarm, 3);
-static SENSOR_DEVICE_ATTR_RW(temp5_alarm, temp_alarm, 4);
-static SENSOR_DEVICE_ATTR_RW(temp6_alarm, temp_alarm, 5);
-static SENSOR_DEVICE_ATTR_RW(temp7_alarm, temp_alarm, 6);
-static SENSOR_DEVICE_ATTR_RW(temp8_alarm, temp_alarm, 7);
-static SENSOR_DEVICE_ATTR_RW(temp9_alarm, temp_alarm, 8);
-static SENSOR_DEVICE_ATTR_RW(temp10_alarm, temp_alarm, 9);
-static SENSOR_DEVICE_ATTR_RW(temp11_alarm, temp_alarm, 10);
-static SENSOR_DEVICE_ATTR_RW(temp12_alarm, temp_alarm, 11);
-static SENSOR_DEVICE_ATTR_RW(temp13_alarm, temp_alarm, 12);
-static SENSOR_DEVICE_ATTR_RW(temp14_alarm, temp_alarm, 13);
-static SENSOR_DEVICE_ATTR_RW(temp15_alarm, temp_alarm, 14);
-static SENSOR_DEVICE_ATTR_RW(temp16_alarm, temp_alarm, 15);
-
-static struct attribute *fts_temp_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp3_input.dev_attr.attr,
-	&sensor_dev_attr_temp4_input.dev_attr.attr,
-	&sensor_dev_attr_temp5_input.dev_attr.attr,
-	&sensor_dev_attr_temp6_input.dev_attr.attr,
-	&sensor_dev_attr_temp7_input.dev_attr.attr,
-	&sensor_dev_attr_temp8_input.dev_attr.attr,
-	&sensor_dev_attr_temp9_input.dev_attr.attr,
-	&sensor_dev_attr_temp10_input.dev_attr.attr,
-	&sensor_dev_attr_temp11_input.dev_attr.attr,
-	&sensor_dev_attr_temp12_input.dev_attr.attr,
-	&sensor_dev_attr_temp13_input.dev_attr.attr,
-	&sensor_dev_attr_temp14_input.dev_attr.attr,
-	&sensor_dev_attr_temp15_input.dev_attr.attr,
-	&sensor_dev_attr_temp16_input.dev_attr.attr,
-
-	&sensor_dev_attr_temp1_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp3_fault.dev_attr.attr,
-	&sensor_dev_attr_temp4_fault.dev_attr.attr,
-	&sensor_dev_attr_temp5_fault.dev_attr.attr,
-	&sensor_dev_attr_temp6_fault.dev_attr.attr,
-	&sensor_dev_attr_temp7_fault.dev_attr.attr,
-	&sensor_dev_attr_temp8_fault.dev_attr.attr,
-	&sensor_dev_attr_temp9_fault.dev_attr.attr,
-	&sensor_dev_attr_temp10_fault.dev_attr.attr,
-	&sensor_dev_attr_temp11_fault.dev_attr.attr,
-	&sensor_dev_attr_temp12_fault.dev_attr.attr,
-	&sensor_dev_attr_temp13_fault.dev_attr.attr,
-	&sensor_dev_attr_temp14_fault.dev_attr.attr,
-	&sensor_dev_attr_temp15_fault.dev_attr.attr,
-	&sensor_dev_attr_temp16_fault.dev_attr.attr,
-
-	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp4_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp5_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp6_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp7_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp8_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp9_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp10_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp11_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp12_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp13_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp14_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp15_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp16_alarm.dev_attr.attr,
-	NULL
-};
-
-/* Fans */
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan_value, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, fan_value, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_input, fan_value, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_input, fan_value, 3);
-static SENSOR_DEVICE_ATTR_RO(fan5_input, fan_value, 4);
-static SENSOR_DEVICE_ATTR_RO(fan6_input, fan_value, 5);
-static SENSOR_DEVICE_ATTR_RO(fan7_input, fan_value, 6);
-static SENSOR_DEVICE_ATTR_RO(fan8_input, fan_value, 7);
-
-static SENSOR_DEVICE_ATTR_RO(fan1_source, fan_source, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_source, fan_source, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_source, fan_source, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_source, fan_source, 3);
-static SENSOR_DEVICE_ATTR_RO(fan5_source, fan_source, 4);
-static SENSOR_DEVICE_ATTR_RO(fan6_source, fan_source, 5);
-static SENSOR_DEVICE_ATTR_RO(fan7_source, fan_source, 6);
-static SENSOR_DEVICE_ATTR_RO(fan8_source, fan_source, 7);
-
-static SENSOR_DEVICE_ATTR_RW(fan1_alarm, fan_alarm, 0);
-static SENSOR_DEVICE_ATTR_RW(fan2_alarm, fan_alarm, 1);
-static SENSOR_DEVICE_ATTR_RW(fan3_alarm, fan_alarm, 2);
-static SENSOR_DEVICE_ATTR_RW(fan4_alarm, fan_alarm, 3);
-static SENSOR_DEVICE_ATTR_RW(fan5_alarm, fan_alarm, 4);
-static SENSOR_DEVICE_ATTR_RW(fan6_alarm, fan_alarm, 5);
-static SENSOR_DEVICE_ATTR_RW(fan7_alarm, fan_alarm, 6);
-static SENSOR_DEVICE_ATTR_RW(fan8_alarm, fan_alarm, 7);
-
-static struct attribute *fts_fan_attrs[] = {
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	&sensor_dev_attr_fan2_input.dev_attr.attr,
-	&sensor_dev_attr_fan3_input.dev_attr.attr,
-	&sensor_dev_attr_fan4_input.dev_attr.attr,
-	&sensor_dev_attr_fan5_input.dev_attr.attr,
-	&sensor_dev_attr_fan6_input.dev_attr.attr,
-	&sensor_dev_attr_fan7_input.dev_attr.attr,
-	&sensor_dev_attr_fan8_input.dev_attr.attr,
-
-	&sensor_dev_attr_fan1_source.dev_attr.attr,
-	&sensor_dev_attr_fan2_source.dev_attr.attr,
-	&sensor_dev_attr_fan3_source.dev_attr.attr,
-	&sensor_dev_attr_fan4_source.dev_attr.attr,
-	&sensor_dev_attr_fan5_source.dev_attr.attr,
-	&sensor_dev_attr_fan6_source.dev_attr.attr,
-	&sensor_dev_attr_fan7_source.dev_attr.attr,
-	&sensor_dev_attr_fan8_source.dev_attr.attr,
-
-	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan3_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan4_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan5_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan6_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan7_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan8_alarm.dev_attr.attr,
-	NULL
+static const struct hwmon_ops fts_ops = {
+	.is_visible = fts_is_visible,
+	.read = fts_read,
+	.write = fts_write,
 };

-/* Voltages */
-static SENSOR_DEVICE_ATTR_RO(in1_input, in_value, 0);
-static SENSOR_DEVICE_ATTR_RO(in2_input, in_value, 1);
-static SENSOR_DEVICE_ATTR_RO(in3_input, in_value, 2);
-static SENSOR_DEVICE_ATTR_RO(in4_input, in_value, 3);
-static struct attribute *fts_voltage_attrs[] = {
-	&sensor_dev_attr_in1_input.dev_attr.attr,
-	&sensor_dev_attr_in2_input.dev_attr.attr,
-	&sensor_dev_attr_in3_input.dev_attr.attr,
-	&sensor_dev_attr_in4_input.dev_attr.attr,
+static const struct hwmon_channel_info *fts_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT
+			   ),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
+			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT
+			   ),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP,
+			   HWMON_PWM_AUTO_CHANNELS_TEMP
+			   ),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT,
+			   HWMON_I_INPUT,
+			   HWMON_I_INPUT,
+			   HWMON_I_INPUT
+			   ),
 	NULL
 };

-static const struct attribute_group fts_voltage_attr_group = {
-	.attrs = fts_voltage_attrs
-};
-
-static const struct attribute_group fts_temp_attr_group = {
-	.attrs = fts_temp_attrs
-};
-
-static const struct attribute_group fts_fan_attr_group = {
-	.attrs = fts_fan_attrs
-};
-
-static const struct attribute_group *fts_attr_groups[] = {
-	&fts_voltage_attr_group,
-	&fts_temp_attr_group,
-	&fts_fan_attr_group,
-	NULL
+static const struct hwmon_chip_info fts_chip_info = {
+	.ops = &fts_ops,
+	.info = fts_info,
 };

 /*****************************************************************************/
@@ -800,10 +660,8 @@ static int fts_probe(struct i2c_client *client)
 		return err;
 	revision = err;

-	hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
-							   "ftsteutates",
-							   data,
-							   fts_attr_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, "ftsteutates", data,
+							 &fts_chip_info, NULL);
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);

--
2.30.2

[PATCH 3/3] hwmon: (ftsteutates) Use devm_watchdog_register_device()

[Armin Wolf]

Use devm_watchdog_register_device() to let devres take care
of unregistering the watchdog at device removal. Remove the
now pointless fts_remove() function.

Tested on a Fujitsu DS3401-B1.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/hwmon/ftsteutates.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
index 66ff8d69a0de..d8878b8d16bc 100644
--- a/drivers/hwmon/ftsteutates.c
+++ b/drivers/hwmon/ftsteutates.c
@@ -333,7 +333,7 @@ static int fts_watchdog_init(struct fts_data *data)
 	/* max timeout 255 minutes. */
 	data->wdd.max_hw_heartbeat_ms = 0xFF * 60 * MSEC_PER_SEC;

-	return watchdog_register_device(&data->wdd);
+	return devm_watchdog_register_device(&data->client->dev, &data->wdd);
 }

 static umode_t fts_is_visible(const void *devdata, enum hwmon_sensor_types type, u32 attr,
@@ -611,13 +611,6 @@ static int fts_detect(struct i2c_client *client,
 	return 0;
 }

-static void fts_remove(struct i2c_client *client)
-{
-	struct fts_data *data = dev_get_drvdata(&client->dev);
-
-	watchdog_unregister_device(&data->wdd);
-}
-
 static int fts_probe(struct i2c_client *client)
 {
 	u8 revision;
@@ -684,7 +677,6 @@ static struct i2c_driver fts_driver = {
 	},
 	.id_table = fts_id,
 	.probe_new = fts_probe,
-	.remove = fts_remove,
 	.detect = fts_detect,
 	.address_list = normal_i2c,
 };
--
2.30.2

Re: [PATCH 2/3] hwmon: (ftsteutates) Convert to devm_hwmon_device_register_with_info()

[Guenter Roeck]

On Sat, Dec 24, 2022 at 05:18:54AM +0100, Armin Wolf wrote:
> Convert driver to use devm_hwmon_device_register_with_info()
> to reduce module size by 40%. The non-standard fan_source
> attribute is replaced with the standard pwmX_auto_channels_temp
> attribute and its special behaviour being documented.
> Also export fan present status as fanX_fault.

There are multiple changes in this patch which are not related
to the actual conversion. Please split into separate patches for
each functional change.

While I agree with the fan_source -> pwmX_auto_channels_temp
change, it needs to be done separately after the conversion,
because it is an ABI change which we may have to undo if someone
complains.

"export fan present status as fanX_fault" also needs to be a
separate patch.

In this context, normally alarms are supposed to auto-clear
after reading an attribute, if the condition no longer exists.
Is it really necessary to explicitly clear the condition, or
would it be possible to auto-clear it after reading the attribute ?

Thanks,
Guenter

> 
> Tested on a Fujitsu DS3401-B1.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  Documentation/hwmon/ftsteutates.rst |   5 +
>  drivers/hwmon/ftsteutates.c         | 546 ++++++++++------------------
>  2 files changed, 207 insertions(+), 344 deletions(-)
> 
> diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
> index 198fa8e2819d..b3bfec36661d 100644
> --- a/Documentation/hwmon/ftsteutates.rst
> +++ b/Documentation/hwmon/ftsteutates.rst
> @@ -22,6 +22,11 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
>  8 fans. It also contains an integrated watchdog which is currently
>  implemented in this driver.
> 
> +The ``pwmX_auto_channels_temp`` attributes show which temperature sensor
> +is currently driving which fan channel. This value might dynamically change
> +during runtime depending on the temperature sensor selected by
> +the fan control circuit.
> +
>  The 4 voltages require a board-specific multiplier, since the BMC can
>  only measure voltages up to 3.3V and thus relies on voltage dividers.
>  Consult your motherboard manual for details.
> diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
> index ffa0bb364877..66ff8d69a0de 100644
> --- a/drivers/hwmon/ftsteutates.c
> +++ b/drivers/hwmon/ftsteutates.c
> @@ -6,9 +6,7 @@
>   *		  Thilo Cestonaro <thilo.cestonaro@ts.fujitsu.com>
>   */
>  #include <linux/err.h>
> -#include <linux/fs.h>
>  #include <linux/hwmon.h>
> -#include <linux/hwmon-sysfs.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/jiffies.h>
> @@ -16,8 +14,6 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> -#include <linux/sysfs.h>
> -#include <linux/uaccess.h>
>  #include <linux/watchdog.h>
> 
>  #define FTS_DEVICE_ID_REG		0x0000
> @@ -340,376 +336,240 @@ static int fts_watchdog_init(struct fts_data *data)
>  	return watchdog_register_device(&data->wdd);
>  }
> 
> -/*****************************************************************************/
> -/* SysFS handler functions						     */
> -/*****************************************************************************/
> -static ssize_t in_value_show(struct device *dev,
> -			     struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int value, err;
> -
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> -
> -	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
> -
> -	return sprintf(buf, "%d\n", value);
> -}
> -
> -static ssize_t temp_value_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int value, err;
> -
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> -
> -	value = (data->temp_input[index] - 64) * 1000;
> -
> -	return sprintf(buf, "%d\n", value);
> -}
> -
> -static ssize_t temp_fault_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> -
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> -
> -	/* 00h Temperature = Sensor Error */
> -	return sprintf(buf, "%d\n", data->temp_input[index] == 0);
> -}
> -
> -static ssize_t temp_alarm_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> +static umode_t fts_is_visible(const void *devdata, enum hwmon_sensor_types type, u32 attr,
> +			      int channel)
>  {
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> -
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_fault:
> +			return 0444;
> +		case hwmon_temp_alarm:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_fault:
> +			return 0444;
> +		case hwmon_fan_alarm:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_pwm:
> +	case hwmon_in:
> +		return 0444;
> +	default:
> +		break;
> +	}
> 
> -	return sprintf(buf, "%u\n", !!(data->temp_alarm & BIT(index)));
> +	return 0;
>  }
> 
> -static ssize_t
> -temp_alarm_store(struct device *dev, struct device_attribute *devattr,
> -		 const char *buf, size_t count)
> +static int fts_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +		    long *val)
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	long ret;
> +	int ret = fts_update_device(data);
> 
> -	ret = fts_update_device(data);
>  	if (ret < 0)
>  		return ret;
> 
> -	if (kstrtoul(buf, 10, &ret) || ret != 0)
> -		return -EINVAL;
> -
> -	mutex_lock(&data->update_lock);
> -	ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(index));
> -	if (ret < 0)
> -		goto error;
> -
> -	ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(index),
> -			     ret | 0x1);
> -	if (ret < 0)
> -		goto error;
> -
> -	data->valid = false;
> -	ret = count;
> -error:
> -	mutex_unlock(&data->update_lock);
> -	return ret;
> -}
> -
> -static ssize_t fan_value_show(struct device *dev,
> -			      struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int value, err;
> -
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			*val = (data->temp_input[channel] - 64) * 1000;
> 
> -	value = data->fan_input[index] * 60;
> +			return 0;
> +		case hwmon_temp_alarm:
> +			*val = !!(data->temp_alarm & BIT(channel));
> 
> -	return sprintf(buf, "%d\n", value);
> -}
> +			return 0;
> +		case hwmon_temp_fault:
> +			/* 00h Temperature = Sensor Error */;
> +			*val = (data->temp_input[channel] == 0);
> 
> -static ssize_t fan_source_show(struct device *dev,
> -			       struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			*val = data->fan_input[channel] * 60;
> 
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> +			return 0;
> +		case hwmon_fan_alarm:
> +			*val = !!(data->fan_alarm & BIT(channel));
> 
> -	return sprintf(buf, "%u\n", data->fan_source[index]);
> -}
> +			return 0;
> +		case hwmon_fan_fault:
> +			*val = !(data->fan_present & BIT(channel));
> 
> -static ssize_t fan_alarm_show(struct device *dev,
> -			      struct device_attribute *devattr, char *buf)
> -{
> -	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_auto_channels_temp:
> +			if (data->fan_source[channel] == 0xff)
> +				*val = 0;
> +			else
> +				*val = BIT(data->fan_source[channel]);
> +
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			*val = DIV_ROUND_CLOSEST(data->volt[channel] * 3300, 255);
> 
> -	err = fts_update_device(data);
> -	if (err < 0)
> -		return err;
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> 
> -	return sprintf(buf, "%d\n", !!(data->fan_alarm & BIT(index)));
> +	return -EOPNOTSUPP;
>  }
> 
> -static ssize_t
> -fan_alarm_store(struct device *dev, struct device_attribute *devattr,
> -		const char *buf, size_t count)
> +static int fts_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +		     long val)
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
> -	int index = to_sensor_dev_attr(devattr)->index;
> -	long ret;
> +	int ret = fts_update_device(data);
> 
> -	ret = fts_update_device(data);
>  	if (ret < 0)
>  		return ret;
> 
> -	if (kstrtoul(buf, 10, &ret) || ret != 0)
> -		return -EINVAL;
> -
> -	mutex_lock(&data->update_lock);
> -	ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(index));
> -	if (ret < 0)
> -		goto error;
> -
> -	ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(index),
> -			     ret | 0x1);
> -	if (ret < 0)
> -		goto error;
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_alarm:
> +			if (val)
> +				return -EINVAL;
> +
> +			mutex_lock(&data->update_lock);
> +			ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(channel));
> +			if (ret >= 0)
> +				ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(channel),
> +						     ret | 0x1);
> +			if (ret >= 0)
> +				data->valid = false;
> +
> +			mutex_unlock(&data->update_lock);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_alarm:
> +			if (val)
> +				return -EINVAL;
> +
> +			mutex_lock(&data->update_lock);
> +			ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(channel));
> +			if (ret >= 0)
> +				ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(channel),
> +						     ret | 0x1);
> +			if (ret >= 0)
> +				data->valid = false;
> +
> +			mutex_unlock(&data->update_lock);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> 
> -	data->valid = false;
> -	ret = count;
> -error:
> -	mutex_unlock(&data->update_lock);
> -	return ret;
> +	return -EOPNOTSUPP;
>  }
> 
> -/*****************************************************************************/
> -/* SysFS structs							     */
> -/*****************************************************************************/
> -
> -/* Temperature sensors */
> -static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_value, 0);
> -static SENSOR_DEVICE_ATTR_RO(temp2_input, temp_value, 1);
> -static SENSOR_DEVICE_ATTR_RO(temp3_input, temp_value, 2);
> -static SENSOR_DEVICE_ATTR_RO(temp4_input, temp_value, 3);
> -static SENSOR_DEVICE_ATTR_RO(temp5_input, temp_value, 4);
> -static SENSOR_DEVICE_ATTR_RO(temp6_input, temp_value, 5);
> -static SENSOR_DEVICE_ATTR_RO(temp7_input, temp_value, 6);
> -static SENSOR_DEVICE_ATTR_RO(temp8_input, temp_value, 7);
> -static SENSOR_DEVICE_ATTR_RO(temp9_input, temp_value, 8);
> -static SENSOR_DEVICE_ATTR_RO(temp10_input, temp_value, 9);
> -static SENSOR_DEVICE_ATTR_RO(temp11_input, temp_value, 10);
> -static SENSOR_DEVICE_ATTR_RO(temp12_input, temp_value, 11);
> -static SENSOR_DEVICE_ATTR_RO(temp13_input, temp_value, 12);
> -static SENSOR_DEVICE_ATTR_RO(temp14_input, temp_value, 13);
> -static SENSOR_DEVICE_ATTR_RO(temp15_input, temp_value, 14);
> -static SENSOR_DEVICE_ATTR_RO(temp16_input, temp_value, 15);
> -
> -static SENSOR_DEVICE_ATTR_RO(temp1_fault, temp_fault, 0);
> -static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp_fault, 1);
> -static SENSOR_DEVICE_ATTR_RO(temp3_fault, temp_fault, 2);
> -static SENSOR_DEVICE_ATTR_RO(temp4_fault, temp_fault, 3);
> -static SENSOR_DEVICE_ATTR_RO(temp5_fault, temp_fault, 4);
> -static SENSOR_DEVICE_ATTR_RO(temp6_fault, temp_fault, 5);
> -static SENSOR_DEVICE_ATTR_RO(temp7_fault, temp_fault, 6);
> -static SENSOR_DEVICE_ATTR_RO(temp8_fault, temp_fault, 7);
> -static SENSOR_DEVICE_ATTR_RO(temp9_fault, temp_fault, 8);
> -static SENSOR_DEVICE_ATTR_RO(temp10_fault, temp_fault, 9);
> -static SENSOR_DEVICE_ATTR_RO(temp11_fault, temp_fault, 10);
> -static SENSOR_DEVICE_ATTR_RO(temp12_fault, temp_fault, 11);
> -static SENSOR_DEVICE_ATTR_RO(temp13_fault, temp_fault, 12);
> -static SENSOR_DEVICE_ATTR_RO(temp14_fault, temp_fault, 13);
> -static SENSOR_DEVICE_ATTR_RO(temp15_fault, temp_fault, 14);
> -static SENSOR_DEVICE_ATTR_RO(temp16_fault, temp_fault, 15);
> -
> -static SENSOR_DEVICE_ATTR_RW(temp1_alarm, temp_alarm, 0);
> -static SENSOR_DEVICE_ATTR_RW(temp2_alarm, temp_alarm, 1);
> -static SENSOR_DEVICE_ATTR_RW(temp3_alarm, temp_alarm, 2);
> -static SENSOR_DEVICE_ATTR_RW(temp4_alarm, temp_alarm, 3);
> -static SENSOR_DEVICE_ATTR_RW(temp5_alarm, temp_alarm, 4);
> -static SENSOR_DEVICE_ATTR_RW(temp6_alarm, temp_alarm, 5);
> -static SENSOR_DEVICE_ATTR_RW(temp7_alarm, temp_alarm, 6);
> -static SENSOR_DEVICE_ATTR_RW(temp8_alarm, temp_alarm, 7);
> -static SENSOR_DEVICE_ATTR_RW(temp9_alarm, temp_alarm, 8);
> -static SENSOR_DEVICE_ATTR_RW(temp10_alarm, temp_alarm, 9);
> -static SENSOR_DEVICE_ATTR_RW(temp11_alarm, temp_alarm, 10);
> -static SENSOR_DEVICE_ATTR_RW(temp12_alarm, temp_alarm, 11);
> -static SENSOR_DEVICE_ATTR_RW(temp13_alarm, temp_alarm, 12);
> -static SENSOR_DEVICE_ATTR_RW(temp14_alarm, temp_alarm, 13);
> -static SENSOR_DEVICE_ATTR_RW(temp15_alarm, temp_alarm, 14);
> -static SENSOR_DEVICE_ATTR_RW(temp16_alarm, temp_alarm, 15);
> -
> -static struct attribute *fts_temp_attrs[] = {
> -	&sensor_dev_attr_temp1_input.dev_attr.attr,
> -	&sensor_dev_attr_temp2_input.dev_attr.attr,
> -	&sensor_dev_attr_temp3_input.dev_attr.attr,
> -	&sensor_dev_attr_temp4_input.dev_attr.attr,
> -	&sensor_dev_attr_temp5_input.dev_attr.attr,
> -	&sensor_dev_attr_temp6_input.dev_attr.attr,
> -	&sensor_dev_attr_temp7_input.dev_attr.attr,
> -	&sensor_dev_attr_temp8_input.dev_attr.attr,
> -	&sensor_dev_attr_temp9_input.dev_attr.attr,
> -	&sensor_dev_attr_temp10_input.dev_attr.attr,
> -	&sensor_dev_attr_temp11_input.dev_attr.attr,
> -	&sensor_dev_attr_temp12_input.dev_attr.attr,
> -	&sensor_dev_attr_temp13_input.dev_attr.attr,
> -	&sensor_dev_attr_temp14_input.dev_attr.attr,
> -	&sensor_dev_attr_temp15_input.dev_attr.attr,
> -	&sensor_dev_attr_temp16_input.dev_attr.attr,
> -
> -	&sensor_dev_attr_temp1_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp2_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp3_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp4_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp5_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp6_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp7_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp8_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp9_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp10_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp11_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp12_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp13_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp14_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp15_fault.dev_attr.attr,
> -	&sensor_dev_attr_temp16_fault.dev_attr.attr,
> -
> -	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp4_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp5_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp6_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp7_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp8_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp9_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp10_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp11_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp12_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp13_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp14_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp15_alarm.dev_attr.attr,
> -	&sensor_dev_attr_temp16_alarm.dev_attr.attr,
> -	NULL
> -};
> -
> -/* Fans */
> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan_value, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan_value, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan3_input, fan_value, 2);
> -static SENSOR_DEVICE_ATTR_RO(fan4_input, fan_value, 3);
> -static SENSOR_DEVICE_ATTR_RO(fan5_input, fan_value, 4);
> -static SENSOR_DEVICE_ATTR_RO(fan6_input, fan_value, 5);
> -static SENSOR_DEVICE_ATTR_RO(fan7_input, fan_value, 6);
> -static SENSOR_DEVICE_ATTR_RO(fan8_input, fan_value, 7);
> -
> -static SENSOR_DEVICE_ATTR_RO(fan1_source, fan_source, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_source, fan_source, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan3_source, fan_source, 2);
> -static SENSOR_DEVICE_ATTR_RO(fan4_source, fan_source, 3);
> -static SENSOR_DEVICE_ATTR_RO(fan5_source, fan_source, 4);
> -static SENSOR_DEVICE_ATTR_RO(fan6_source, fan_source, 5);
> -static SENSOR_DEVICE_ATTR_RO(fan7_source, fan_source, 6);
> -static SENSOR_DEVICE_ATTR_RO(fan8_source, fan_source, 7);
> -
> -static SENSOR_DEVICE_ATTR_RW(fan1_alarm, fan_alarm, 0);
> -static SENSOR_DEVICE_ATTR_RW(fan2_alarm, fan_alarm, 1);
> -static SENSOR_DEVICE_ATTR_RW(fan3_alarm, fan_alarm, 2);
> -static SENSOR_DEVICE_ATTR_RW(fan4_alarm, fan_alarm, 3);
> -static SENSOR_DEVICE_ATTR_RW(fan5_alarm, fan_alarm, 4);
> -static SENSOR_DEVICE_ATTR_RW(fan6_alarm, fan_alarm, 5);
> -static SENSOR_DEVICE_ATTR_RW(fan7_alarm, fan_alarm, 6);
> -static SENSOR_DEVICE_ATTR_RW(fan8_alarm, fan_alarm, 7);
> -
> -static struct attribute *fts_fan_attrs[] = {
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,
> -	&sensor_dev_attr_fan3_input.dev_attr.attr,
> -	&sensor_dev_attr_fan4_input.dev_attr.attr,
> -	&sensor_dev_attr_fan5_input.dev_attr.attr,
> -	&sensor_dev_attr_fan6_input.dev_attr.attr,
> -	&sensor_dev_attr_fan7_input.dev_attr.attr,
> -	&sensor_dev_attr_fan8_input.dev_attr.attr,
> -
> -	&sensor_dev_attr_fan1_source.dev_attr.attr,
> -	&sensor_dev_attr_fan2_source.dev_attr.attr,
> -	&sensor_dev_attr_fan3_source.dev_attr.attr,
> -	&sensor_dev_attr_fan4_source.dev_attr.attr,
> -	&sensor_dev_attr_fan5_source.dev_attr.attr,
> -	&sensor_dev_attr_fan6_source.dev_attr.attr,
> -	&sensor_dev_attr_fan7_source.dev_attr.attr,
> -	&sensor_dev_attr_fan8_source.dev_attr.attr,
> -
> -	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan3_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan4_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan5_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan6_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan7_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan8_alarm.dev_attr.attr,
> -	NULL
> +static const struct hwmon_ops fts_ops = {
> +	.is_visible = fts_is_visible,
> +	.read = fts_read,
> +	.write = fts_write,
>  };
> 
> -/* Voltages */
> -static SENSOR_DEVICE_ATTR_RO(in1_input, in_value, 0);
> -static SENSOR_DEVICE_ATTR_RO(in2_input, in_value, 1);
> -static SENSOR_DEVICE_ATTR_RO(in3_input, in_value, 2);
> -static SENSOR_DEVICE_ATTR_RO(in4_input, in_value, 3);
> -static struct attribute *fts_voltage_attrs[] = {
> -	&sensor_dev_attr_in1_input.dev_attr.attr,
> -	&sensor_dev_attr_in2_input.dev_attr.attr,
> -	&sensor_dev_attr_in3_input.dev_attr.attr,
> -	&sensor_dev_attr_in4_input.dev_attr.attr,
> +static const struct hwmon_channel_info *fts_info[] = {
> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT
> +			   ),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT
> +			   ),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP
> +			   ),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT,
> +			   HWMON_I_INPUT,
> +			   HWMON_I_INPUT,
> +			   HWMON_I_INPUT
> +			   ),
>  	NULL
>  };
> 
> -static const struct attribute_group fts_voltage_attr_group = {
> -	.attrs = fts_voltage_attrs
> -};
> -
> -static const struct attribute_group fts_temp_attr_group = {
> -	.attrs = fts_temp_attrs
> -};
> -
> -static const struct attribute_group fts_fan_attr_group = {
> -	.attrs = fts_fan_attrs
> -};
> -
> -static const struct attribute_group *fts_attr_groups[] = {
> -	&fts_voltage_attr_group,
> -	&fts_temp_attr_group,
> -	&fts_fan_attr_group,
> -	NULL
> +static const struct hwmon_chip_info fts_chip_info = {
> +	.ops = &fts_ops,
> +	.info = fts_info,
>  };
> 
>  /*****************************************************************************/
> @@ -800,10 +660,8 @@ static int fts_probe(struct i2c_client *client)
>  		return err;
>  	revision = err;
> 
> -	hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
> -							   "ftsteutates",
> -							   data,
> -							   fts_attr_groups);
> +	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, "ftsteutates", data,
> +							 &fts_chip_info, NULL);
>  	if (IS_ERR(hwmon_dev))
>  		return PTR_ERR(hwmon_dev);
> 
> --
> 2.30.2
> 

Re: [PATCH 1/3] hwmon: (ftsteutates) Fix scaling of measurements

[Guenter Roeck]

On Sat, Dec 24, 2022 at 05:18:53AM +0100, Armin Wolf wrote:
> A user complained that the ftsteutates driver was displaying
> bogus values since its introduction. This happens because the
> sensor measurements need to be scaled in order to produce
> meaningful results:
> - the fan speed needs to be multiplied by 60 since its in RPS
> - the temperature is in degrees celsius and needs an offset of 64
> - the voltage is in 1/256 of 3.3V
> 
> The offical datasheet says the voltage needs to be divided by 256,
> but this is likely an off-by-one-error, since even the BIOS
> devides by 255 (otherwise 3.3V could not be measured).
> 
> The voltage channels additionally need a board-specific multiplier,
> however this can be done by the driver since its board-specific.
> 
> The reason the missing scaling of measurements is the way Fujitsu
> used this driver when it was still out-of-tree. Back then, all
> scaling was done in userspace by libsensors, even the generic one.
> 
> Tested on a Fujitsu DS3401-B1.
> 
> Fixes: 08426eda58e0 ("hwmon: Add driver for FTS BMC chip "Teutates"")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied.

Thanks,
Guenter

> ---
>  Documentation/hwmon/ftsteutates.rst |  4 ++++
>  drivers/hwmon/ftsteutates.c         | 19 +++++++++++++------
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
> index 58a2483d8d0d..198fa8e2819d 100644
> --- a/Documentation/hwmon/ftsteutates.rst
> +++ b/Documentation/hwmon/ftsteutates.rst
> @@ -22,6 +22,10 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
>  8 fans. It also contains an integrated watchdog which is currently
>  implemented in this driver.
> 
> +The 4 voltages require a board-specific multiplier, since the BMC can
> +only measure voltages up to 3.3V and thus relies on voltage dividers.
> +Consult your motherboard manual for details.
> +
>  To clear a temperature or fan alarm, execute the following command with the
>  correct path to the alarm file::
> 
> diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
> index f5b8e724a8ca..ffa0bb364877 100644
> --- a/drivers/hwmon/ftsteutates.c
> +++ b/drivers/hwmon/ftsteutates.c
> @@ -12,6 +12,7 @@
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/jiffies.h>
> +#include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> @@ -347,13 +348,15 @@ static ssize_t in_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->volt[index]);
> +	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t temp_value_show(struct device *dev,
> @@ -361,13 +364,15 @@ static ssize_t temp_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->temp_input[index]);
> +	value = (data->temp_input[index] - 64) * 1000;
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t temp_fault_show(struct device *dev,
> @@ -436,13 +441,15 @@ static ssize_t fan_value_show(struct device *dev,
>  {
>  	struct fts_data *data = dev_get_drvdata(dev);
>  	int index = to_sensor_dev_attr(devattr)->index;
> -	int err;
> +	int value, err;
> 
>  	err = fts_update_device(data);
>  	if (err < 0)
>  		return err;
> 
> -	return sprintf(buf, "%u\n", data->fan_input[index]);
> +	value = data->fan_input[index] * 60;
> +
> +	return sprintf(buf, "%d\n", value);
>  }
> 
>  static ssize_t fan_source_show(struct device *dev,

Re: [PATCH 3/3] hwmon: (ftsteutates) Use devm_watchdog_register_device()

[Guenter Roeck]

On Sat, Dec 24, 2022 at 05:18:55AM +0100, Armin Wolf wrote:
> Use devm_watchdog_register_device() to let devres take care
> of unregistering the watchdog at device removal. Remove the
> now pointless fts_remove() function.
> 
> Tested on a Fujitsu DS3401-B1.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/ftsteutates.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> --
> 2.30.2
> 
> diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
> index 66ff8d69a0de..d8878b8d16bc 100644
> --- a/drivers/hwmon/ftsteutates.c
> +++ b/drivers/hwmon/ftsteutates.c
> @@ -333,7 +333,7 @@ static int fts_watchdog_init(struct fts_data *data)
>  	/* max timeout 255 minutes. */
>  	data->wdd.max_hw_heartbeat_ms = 0xFF * 60 * MSEC_PER_SEC;
> 
> -	return watchdog_register_device(&data->wdd);
> +	return devm_watchdog_register_device(&data->client->dev, &data->wdd);
>  }
> 
>  static umode_t fts_is_visible(const void *devdata, enum hwmon_sensor_types type, u32 attr,
> @@ -611,13 +611,6 @@ static int fts_detect(struct i2c_client *client,
>  	return 0;
>  }
> 
> -static void fts_remove(struct i2c_client *client)
> -{
> -	struct fts_data *data = dev_get_drvdata(&client->dev);
> -
> -	watchdog_unregister_device(&data->wdd);
> -}
> -
>  static int fts_probe(struct i2c_client *client)
>  {
>  	u8 revision;
> @@ -684,7 +677,6 @@ static struct i2c_driver fts_driver = {
>  	},
>  	.id_table = fts_id,
>  	.probe_new = fts_probe,
> -	.remove = fts_remove,
>  	.detect = fts_detect,
>  	.address_list = normal_i2c,
>  };

Re: [PATCH 2/3] hwmon: (ftsteutates) Convert to devm_hwmon_device_register_with_info()

[Armin Wolf]

Am 24.12.22 um 16:18 schrieb Guenter Roeck:

> On Sat, Dec 24, 2022 at 05:18:54AM +0100, Armin Wolf wrote:
>> Convert driver to use devm_hwmon_device_register_with_info()
>> to reduce module size by 40%. The non-standard fan_source
>> attribute is replaced with the standard pwmX_auto_channels_temp
>> attribute and its special behaviour being documented.
>> Also export fan present status as fanX_fault.
> There are multiple changes in this patch which are not related
> to the actual conversion. Please split into separate patches for
> each functional change.
>
> While I agree with the fan_source -> pwmX_auto_channels_temp
> change, it needs to be done separately after the conversion,
> because it is an ABI change which we may have to undo if someone
> complains.
>
> "export fan present status as fanX_fault" also needs to be a
> separate patch.
>
> In this context, normally alarms are supposed to auto-clear
> after reading an attribute, if the condition no longer exists.
> Is it really necessary to explicitly clear the condition, or
> would it be possible to auto-clear it after reading the attribute ?
>
> Thanks,
> Guenter

According to the datasheet, the alarm flags are "sticky" and need to
be reset explicitly. Unfortunately, the BMC does not immediately reassign
those flags in case of the alarm condition being present, but does this at
the next "poll cycle". In order to emulate auto-clearable alarms, the driver
would have to constantly poll the BMC, which would still glitch since we do
not know the duration of a "poll cycle".

Other than that, we could:
- introduce sticky alarm attributes ("fanX_alarm_sticky", ...)
- keep it as it is

Armin Wolf

>> Tested on a Fujitsu DS3401-B1.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   Documentation/hwmon/ftsteutates.rst |   5 +
>>   drivers/hwmon/ftsteutates.c         | 546 ++++++++++------------------
>>   2 files changed, 207 insertions(+), 344 deletions(-)
>>
>> diff --git a/Documentation/hwmon/ftsteutates.rst b/Documentation/hwmon/ftsteutates.rst
>> index 198fa8e2819d..b3bfec36661d 100644
>> --- a/Documentation/hwmon/ftsteutates.rst
>> +++ b/Documentation/hwmon/ftsteutates.rst
>> @@ -22,6 +22,11 @@ enhancements. It can monitor up to 4 voltages, 16 temperatures and
>>   8 fans. It also contains an integrated watchdog which is currently
>>   implemented in this driver.
>>
>> +The ``pwmX_auto_channels_temp`` attributes show which temperature sensor
>> +is currently driving which fan channel. This value might dynamically change
>> +during runtime depending on the temperature sensor selected by
>> +the fan control circuit.
>> +
>>   The 4 voltages require a board-specific multiplier, since the BMC can
>>   only measure voltages up to 3.3V and thus relies on voltage dividers.
>>   Consult your motherboard manual for details.
>> diff --git a/drivers/hwmon/ftsteutates.c b/drivers/hwmon/ftsteutates.c
>> index ffa0bb364877..66ff8d69a0de 100644
>> --- a/drivers/hwmon/ftsteutates.c
>> +++ b/drivers/hwmon/ftsteutates.c
>> @@ -6,9 +6,7 @@
>>    *		  Thilo Cestonaro <thilo.cestonaro@ts.fujitsu.com>
>>    */
>>   #include <linux/err.h>
>> -#include <linux/fs.h>
>>   #include <linux/hwmon.h>
>> -#include <linux/hwmon-sysfs.h>
>>   #include <linux/i2c.h>
>>   #include <linux/init.h>
>>   #include <linux/jiffies.h>
>> @@ -16,8 +14,6 @@
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>>   #include <linux/slab.h>
>> -#include <linux/sysfs.h>
>> -#include <linux/uaccess.h>
>>   #include <linux/watchdog.h>
>>
>>   #define FTS_DEVICE_ID_REG		0x0000
>> @@ -340,376 +336,240 @@ static int fts_watchdog_init(struct fts_data *data)
>>   	return watchdog_register_device(&data->wdd);
>>   }
>>
>> -/*****************************************************************************/
>> -/* SysFS handler functions						     */
>> -/*****************************************************************************/
>> -static ssize_t in_value_show(struct device *dev,
>> -			     struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int value, err;
>> -
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> -
>> -	value = DIV_ROUND_CLOSEST(data->volt[index] * 3300, 255);
>> -
>> -	return sprintf(buf, "%d\n", value);
>> -}
>> -
>> -static ssize_t temp_value_show(struct device *dev,
>> -			       struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int value, err;
>> -
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> -
>> -	value = (data->temp_input[index] - 64) * 1000;
>> -
>> -	return sprintf(buf, "%d\n", value);
>> -}
>> -
>> -static ssize_t temp_fault_show(struct device *dev,
>> -			       struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int err;
>> -
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> -
>> -	/* 00h Temperature = Sensor Error */
>> -	return sprintf(buf, "%d\n", data->temp_input[index] == 0);
>> -}
>> -
>> -static ssize_t temp_alarm_show(struct device *dev,
>> -			       struct device_attribute *devattr, char *buf)
>> +static umode_t fts_is_visible(const void *devdata, enum hwmon_sensor_types type, u32 attr,
>> +			      int channel)
>>   {
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int err;
>> -
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_input:
>> +		case hwmon_temp_fault:
>> +			return 0444;
>> +		case hwmon_temp_alarm:
>> +			return 0644;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_fan:
>> +		switch (attr) {
>> +		case hwmon_fan_input:
>> +		case hwmon_fan_fault:
>> +			return 0444;
>> +		case hwmon_fan_alarm:
>> +			return 0644;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_pwm:
>> +	case hwmon_in:
>> +		return 0444;
>> +	default:
>> +		break;
>> +	}
>>
>> -	return sprintf(buf, "%u\n", !!(data->temp_alarm & BIT(index)));
>> +	return 0;
>>   }
>>
>> -static ssize_t
>> -temp_alarm_store(struct device *dev, struct device_attribute *devattr,
>> -		 const char *buf, size_t count)
>> +static int fts_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
>> +		    long *val)
>>   {
>>   	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	long ret;
>> +	int ret = fts_update_device(data);
>>
>> -	ret = fts_update_device(data);
>>   	if (ret < 0)
>>   		return ret;
>>
>> -	if (kstrtoul(buf, 10, &ret) || ret != 0)
>> -		return -EINVAL;
>> -
>> -	mutex_lock(&data->update_lock);
>> -	ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(index));
>> -	if (ret < 0)
>> -		goto error;
>> -
>> -	ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(index),
>> -			     ret | 0x1);
>> -	if (ret < 0)
>> -		goto error;
>> -
>> -	data->valid = false;
>> -	ret = count;
>> -error:
>> -	mutex_unlock(&data->update_lock);
>> -	return ret;
>> -}
>> -
>> -static ssize_t fan_value_show(struct device *dev,
>> -			      struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int value, err;
>> -
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_input:
>> +			*val = (data->temp_input[channel] - 64) * 1000;
>>
>> -	value = data->fan_input[index] * 60;
>> +			return 0;
>> +		case hwmon_temp_alarm:
>> +			*val = !!(data->temp_alarm & BIT(channel));
>>
>> -	return sprintf(buf, "%d\n", value);
>> -}
>> +			return 0;
>> +		case hwmon_temp_fault:
>> +			/* 00h Temperature = Sensor Error */;
>> +			*val = (data->temp_input[channel] == 0);
>>
>> -static ssize_t fan_source_show(struct device *dev,
>> -			       struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int err;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_fan:
>> +		switch (attr) {
>> +		case hwmon_fan_input:
>> +			*val = data->fan_input[channel] * 60;
>>
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> +			return 0;
>> +		case hwmon_fan_alarm:
>> +			*val = !!(data->fan_alarm & BIT(channel));
>>
>> -	return sprintf(buf, "%u\n", data->fan_source[index]);
>> -}
>> +			return 0;
>> +		case hwmon_fan_fault:
>> +			*val = !(data->fan_present & BIT(channel));
>>
>> -static ssize_t fan_alarm_show(struct device *dev,
>> -			      struct device_attribute *devattr, char *buf)
>> -{
>> -	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	int err;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_pwm:
>> +		switch (attr) {
>> +		case hwmon_pwm_auto_channels_temp:
>> +			if (data->fan_source[channel] == 0xff)
>> +				*val = 0;
>> +			else
>> +				*val = BIT(data->fan_source[channel]);
>> +
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_in:
>> +		switch (attr) {
>> +		case hwmon_in_input:
>> +			*val = DIV_ROUND_CLOSEST(data->volt[channel] * 3300, 255);
>>
>> -	err = fts_update_device(data);
>> -	if (err < 0)
>> -		return err;
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>>
>> -	return sprintf(buf, "%d\n", !!(data->fan_alarm & BIT(index)));
>> +	return -EOPNOTSUPP;
>>   }
>>
>> -static ssize_t
>> -fan_alarm_store(struct device *dev, struct device_attribute *devattr,
>> -		const char *buf, size_t count)
>> +static int fts_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
>> +		     long val)
>>   {
>>   	struct fts_data *data = dev_get_drvdata(dev);
>> -	int index = to_sensor_dev_attr(devattr)->index;
>> -	long ret;
>> +	int ret = fts_update_device(data);
>>
>> -	ret = fts_update_device(data);
>>   	if (ret < 0)
>>   		return ret;
>>
>> -	if (kstrtoul(buf, 10, &ret) || ret != 0)
>> -		return -EINVAL;
>> -
>> -	mutex_lock(&data->update_lock);
>> -	ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(index));
>> -	if (ret < 0)
>> -		goto error;
>> -
>> -	ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(index),
>> -			     ret | 0x1);
>> -	if (ret < 0)
>> -		goto error;
>> +	switch (type) {
>> +	case hwmon_temp:
>> +		switch (attr) {
>> +		case hwmon_temp_alarm:
>> +			if (val)
>> +				return -EINVAL;
>> +
>> +			mutex_lock(&data->update_lock);
>> +			ret = fts_read_byte(data->client, FTS_REG_TEMP_CONTROL(channel));
>> +			if (ret >= 0)
>> +				ret = fts_write_byte(data->client, FTS_REG_TEMP_CONTROL(channel),
>> +						     ret | 0x1);
>> +			if (ret >= 0)
>> +				data->valid = false;
>> +
>> +			mutex_unlock(&data->update_lock);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	case hwmon_fan:
>> +		switch (attr) {
>> +		case hwmon_fan_alarm:
>> +			if (val)
>> +				return -EINVAL;
>> +
>> +			mutex_lock(&data->update_lock);
>> +			ret = fts_read_byte(data->client, FTS_REG_FAN_CONTROL(channel));
>> +			if (ret >= 0)
>> +				ret = fts_write_byte(data->client, FTS_REG_FAN_CONTROL(channel),
>> +						     ret | 0x1);
>> +			if (ret >= 0)
>> +				data->valid = false;
>> +
>> +			mutex_unlock(&data->update_lock);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			return 0;
>> +		default:
>> +			break;
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
>>
>> -	data->valid = false;
>> -	ret = count;
>> -error:
>> -	mutex_unlock(&data->update_lock);
>> -	return ret;
>> +	return -EOPNOTSUPP;
>>   }
>>
>> -/*****************************************************************************/
>> -/* SysFS structs							     */
>> -/*****************************************************************************/
>> -
>> -/* Temperature sensors */
>> -static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_value, 0);
>> -static SENSOR_DEVICE_ATTR_RO(temp2_input, temp_value, 1);
>> -static SENSOR_DEVICE_ATTR_RO(temp3_input, temp_value, 2);
>> -static SENSOR_DEVICE_ATTR_RO(temp4_input, temp_value, 3);
>> -static SENSOR_DEVICE_ATTR_RO(temp5_input, temp_value, 4);
>> -static SENSOR_DEVICE_ATTR_RO(temp6_input, temp_value, 5);
>> -static SENSOR_DEVICE_ATTR_RO(temp7_input, temp_value, 6);
>> -static SENSOR_DEVICE_ATTR_RO(temp8_input, temp_value, 7);
>> -static SENSOR_DEVICE_ATTR_RO(temp9_input, temp_value, 8);
>> -static SENSOR_DEVICE_ATTR_RO(temp10_input, temp_value, 9);
>> -static SENSOR_DEVICE_ATTR_RO(temp11_input, temp_value, 10);
>> -static SENSOR_DEVICE_ATTR_RO(temp12_input, temp_value, 11);
>> -static SENSOR_DEVICE_ATTR_RO(temp13_input, temp_value, 12);
>> -static SENSOR_DEVICE_ATTR_RO(temp14_input, temp_value, 13);
>> -static SENSOR_DEVICE_ATTR_RO(temp15_input, temp_value, 14);
>> -static SENSOR_DEVICE_ATTR_RO(temp16_input, temp_value, 15);
>> -
>> -static SENSOR_DEVICE_ATTR_RO(temp1_fault, temp_fault, 0);
>> -static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp_fault, 1);
>> -static SENSOR_DEVICE_ATTR_RO(temp3_fault, temp_fault, 2);
>> -static SENSOR_DEVICE_ATTR_RO(temp4_fault, temp_fault, 3);
>> -static SENSOR_DEVICE_ATTR_RO(temp5_fault, temp_fault, 4);
>> -static SENSOR_DEVICE_ATTR_RO(temp6_fault, temp_fault, 5);
>> -static SENSOR_DEVICE_ATTR_RO(temp7_fault, temp_fault, 6);
>> -static SENSOR_DEVICE_ATTR_RO(temp8_fault, temp_fault, 7);
>> -static SENSOR_DEVICE_ATTR_RO(temp9_fault, temp_fault, 8);
>> -static SENSOR_DEVICE_ATTR_RO(temp10_fault, temp_fault, 9);
>> -static SENSOR_DEVICE_ATTR_RO(temp11_fault, temp_fault, 10);
>> -static SENSOR_DEVICE_ATTR_RO(temp12_fault, temp_fault, 11);
>> -static SENSOR_DEVICE_ATTR_RO(temp13_fault, temp_fault, 12);
>> -static SENSOR_DEVICE_ATTR_RO(temp14_fault, temp_fault, 13);
>> -static SENSOR_DEVICE_ATTR_RO(temp15_fault, temp_fault, 14);
>> -static SENSOR_DEVICE_ATTR_RO(temp16_fault, temp_fault, 15);
>> -
>> -static SENSOR_DEVICE_ATTR_RW(temp1_alarm, temp_alarm, 0);
>> -static SENSOR_DEVICE_ATTR_RW(temp2_alarm, temp_alarm, 1);
>> -static SENSOR_DEVICE_ATTR_RW(temp3_alarm, temp_alarm, 2);
>> -static SENSOR_DEVICE_ATTR_RW(temp4_alarm, temp_alarm, 3);
>> -static SENSOR_DEVICE_ATTR_RW(temp5_alarm, temp_alarm, 4);
>> -static SENSOR_DEVICE_ATTR_RW(temp6_alarm, temp_alarm, 5);
>> -static SENSOR_DEVICE_ATTR_RW(temp7_alarm, temp_alarm, 6);
>> -static SENSOR_DEVICE_ATTR_RW(temp8_alarm, temp_alarm, 7);
>> -static SENSOR_DEVICE_ATTR_RW(temp9_alarm, temp_alarm, 8);
>> -static SENSOR_DEVICE_ATTR_RW(temp10_alarm, temp_alarm, 9);
>> -static SENSOR_DEVICE_ATTR_RW(temp11_alarm, temp_alarm, 10);
>> -static SENSOR_DEVICE_ATTR_RW(temp12_alarm, temp_alarm, 11);
>> -static SENSOR_DEVICE_ATTR_RW(temp13_alarm, temp_alarm, 12);
>> -static SENSOR_DEVICE_ATTR_RW(temp14_alarm, temp_alarm, 13);
>> -static SENSOR_DEVICE_ATTR_RW(temp15_alarm, temp_alarm, 14);
>> -static SENSOR_DEVICE_ATTR_RW(temp16_alarm, temp_alarm, 15);
>> -
>> -static struct attribute *fts_temp_attrs[] = {
>> -	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp2_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp3_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp4_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp5_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp6_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp7_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp8_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp9_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp10_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp11_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp12_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp13_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp14_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp15_input.dev_attr.attr,
>> -	&sensor_dev_attr_temp16_input.dev_attr.attr,
>> -
>> -	&sensor_dev_attr_temp1_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp2_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp3_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp4_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp5_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp6_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp7_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp8_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp9_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp10_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp11_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp12_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp13_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp14_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp15_fault.dev_attr.attr,
>> -	&sensor_dev_attr_temp16_fault.dev_attr.attr,
>> -
>> -	&sensor_dev_attr_temp1_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp4_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp5_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp6_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp7_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp8_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp9_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp10_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp11_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp12_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp13_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp14_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp15_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_temp16_alarm.dev_attr.attr,
>> -	NULL
>> -};
>> -
>> -/* Fans */
>> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan_value, 0);
>> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan_value, 1);
>> -static SENSOR_DEVICE_ATTR_RO(fan3_input, fan_value, 2);
>> -static SENSOR_DEVICE_ATTR_RO(fan4_input, fan_value, 3);
>> -static SENSOR_DEVICE_ATTR_RO(fan5_input, fan_value, 4);
>> -static SENSOR_DEVICE_ATTR_RO(fan6_input, fan_value, 5);
>> -static SENSOR_DEVICE_ATTR_RO(fan7_input, fan_value, 6);
>> -static SENSOR_DEVICE_ATTR_RO(fan8_input, fan_value, 7);
>> -
>> -static SENSOR_DEVICE_ATTR_RO(fan1_source, fan_source, 0);
>> -static SENSOR_DEVICE_ATTR_RO(fan2_source, fan_source, 1);
>> -static SENSOR_DEVICE_ATTR_RO(fan3_source, fan_source, 2);
>> -static SENSOR_DEVICE_ATTR_RO(fan4_source, fan_source, 3);
>> -static SENSOR_DEVICE_ATTR_RO(fan5_source, fan_source, 4);
>> -static SENSOR_DEVICE_ATTR_RO(fan6_source, fan_source, 5);
>> -static SENSOR_DEVICE_ATTR_RO(fan7_source, fan_source, 6);
>> -static SENSOR_DEVICE_ATTR_RO(fan8_source, fan_source, 7);
>> -
>> -static SENSOR_DEVICE_ATTR_RW(fan1_alarm, fan_alarm, 0);
>> -static SENSOR_DEVICE_ATTR_RW(fan2_alarm, fan_alarm, 1);
>> -static SENSOR_DEVICE_ATTR_RW(fan3_alarm, fan_alarm, 2);
>> -static SENSOR_DEVICE_ATTR_RW(fan4_alarm, fan_alarm, 3);
>> -static SENSOR_DEVICE_ATTR_RW(fan5_alarm, fan_alarm, 4);
>> -static SENSOR_DEVICE_ATTR_RW(fan6_alarm, fan_alarm, 5);
>> -static SENSOR_DEVICE_ATTR_RW(fan7_alarm, fan_alarm, 6);
>> -static SENSOR_DEVICE_ATTR_RW(fan8_alarm, fan_alarm, 7);
>> -
>> -static struct attribute *fts_fan_attrs[] = {
>> -	&sensor_dev_attr_fan1_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan2_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan3_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan4_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan5_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan6_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan7_input.dev_attr.attr,
>> -	&sensor_dev_attr_fan8_input.dev_attr.attr,
>> -
>> -	&sensor_dev_attr_fan1_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan2_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan3_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan4_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan5_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan6_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan7_source.dev_attr.attr,
>> -	&sensor_dev_attr_fan8_source.dev_attr.attr,
>> -
>> -	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan2_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan3_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan4_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan5_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan6_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan7_alarm.dev_attr.attr,
>> -	&sensor_dev_attr_fan8_alarm.dev_attr.attr,
>> -	NULL
>> +static const struct hwmon_ops fts_ops = {
>> +	.is_visible = fts_is_visible,
>> +	.read = fts_read,
>> +	.write = fts_write,
>>   };
>>
>> -/* Voltages */
>> -static SENSOR_DEVICE_ATTR_RO(in1_input, in_value, 0);
>> -static SENSOR_DEVICE_ATTR_RO(in2_input, in_value, 1);
>> -static SENSOR_DEVICE_ATTR_RO(in3_input, in_value, 2);
>> -static SENSOR_DEVICE_ATTR_RO(in4_input, in_value, 3);
>> -static struct attribute *fts_voltage_attrs[] = {
>> -	&sensor_dev_attr_in1_input.dev_attr.attr,
>> -	&sensor_dev_attr_in2_input.dev_attr.attr,
>> -	&sensor_dev_attr_in3_input.dev_attr.attr,
>> -	&sensor_dev_attr_in4_input.dev_attr.attr,
>> +static const struct hwmon_channel_info *fts_info[] = {
>> +	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
>> +	HWMON_CHANNEL_INFO(temp,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT,
>> +			   HWMON_T_INPUT | HWMON_T_ALARM | HWMON_T_FAULT
>> +			   ),
>> +	HWMON_CHANNEL_INFO(fan,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT,
>> +			   HWMON_F_INPUT | HWMON_F_ALARM | HWMON_F_FAULT
>> +			   ),
>> +	HWMON_CHANNEL_INFO(pwm,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
>> +			   HWMON_PWM_AUTO_CHANNELS_TEMP
>> +			   ),
>> +	HWMON_CHANNEL_INFO(in,
>> +			   HWMON_I_INPUT,
>> +			   HWMON_I_INPUT,
>> +			   HWMON_I_INPUT,
>> +			   HWMON_I_INPUT
>> +			   ),
>>   	NULL
>>   };
>>
>> -static const struct attribute_group fts_voltage_attr_group = {
>> -	.attrs = fts_voltage_attrs
>> -};
>> -
>> -static const struct attribute_group fts_temp_attr_group = {
>> -	.attrs = fts_temp_attrs
>> -};
>> -
>> -static const struct attribute_group fts_fan_attr_group = {
>> -	.attrs = fts_fan_attrs
>> -};
>> -
>> -static const struct attribute_group *fts_attr_groups[] = {
>> -	&fts_voltage_attr_group,
>> -	&fts_temp_attr_group,
>> -	&fts_fan_attr_group,
>> -	NULL
>> +static const struct hwmon_chip_info fts_chip_info = {
>> +	.ops = &fts_ops,
>> +	.info = fts_info,
>>   };
>>
>>   /*****************************************************************************/
>> @@ -800,10 +660,8 @@ static int fts_probe(struct i2c_client *client)
>>   		return err;
>>   	revision = err;
>>
>> -	hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
>> -							   "ftsteutates",
>> -							   data,
>> -							   fts_attr_groups);
>> +	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, "ftsteutates", data,
>> +							 &fts_chip_info, NULL);
>>   	if (IS_ERR(hwmon_dev))
>>   		return PTR_ERR(hwmon_dev);
>>
>> --
>> 2.30.2
>>

Re: [PATCH 2/3] hwmon: (ftsteutates) Convert to devm_hwmon_device_register_with_info()

[Guenter Roeck]

On Sat, Dec 31, 2022 at 12:40:40PM +0100, Armin Wolf wrote:
> Am 24.12.22 um 16:18 schrieb Guenter Roeck:
> 
> > On Sat, Dec 24, 2022 at 05:18:54AM +0100, Armin Wolf wrote:
> > > Convert driver to use devm_hwmon_device_register_with_info()
> > > to reduce module size by 40%. The non-standard fan_source
> > > attribute is replaced with the standard pwmX_auto_channels_temp
> > > attribute and its special behaviour being documented.
> > > Also export fan present status as fanX_fault.
> > There are multiple changes in this patch which are not related
> > to the actual conversion. Please split into separate patches for
> > each functional change.
> > 
> > While I agree with the fan_source -> pwmX_auto_channels_temp
> > change, it needs to be done separately after the conversion,
> > because it is an ABI change which we may have to undo if someone
> > complains.
> > 
> > "export fan present status as fanX_fault" also needs to be a
> > separate patch.
> > 
> > In this context, normally alarms are supposed to auto-clear
> > after reading an attribute, if the condition no longer exists.
> > Is it really necessary to explicitly clear the condition, or
> > would it be possible to auto-clear it after reading the attribute ?
> > 
> > Thanks,
> > Guenter
> 
> According to the datasheet, the alarm flags are "sticky" and need to
> be reset explicitly. Unfortunately, the BMC does not immediately reassign
> those flags in case of the alarm condition being present, but does this at
> the next "poll cycle". In order to emulate auto-clearable alarms, the driver
> would have to constantly poll the BMC, which would still glitch since we do
> not know the duration of a "poll cycle".

Many chips have that problem. The solution is to either accept it
(which many drivers do) or to work around it by cashing reported alarms
and only clearing the alarm bit form the cache after a poll cycle.
The lm90 driver is probably the most sophisticated of those.
There is no need to do anything special, much less anything that prevents
standard applications such as "sensors" from working.

Guenter