[PATCH v2 00/11] hwmon: (amc6821) Various improvements

[PATCH v2 00/11] hwmon: (amc6821) Various improvements

[Guenter Roeck]

Cleanup and modernize the amc2821 driver.

Summary of changes:

- Stop accepting invalid pwm values.
- Improve consistency of reading vs. writing fan speed limits
- Rename fan1_div to fan1_pulses
- Add support for fan1_target and pwm1_enable mode 4
- Reorder include files, drop unnecessary ones
- Use tabs for column alignment in defines
- Use BIT() and GENMASK()
- Drop unnecessary enum chips
- Convert to use regmap
- Convert to with_info API
- Add support for pwm1_mode attribute

v2:
- Use kstrtou8() instead of kstrtol() where possible
- Limit range of pwm1_auto_point_pwm to 0..254 in patch 1
  instead of limiting it later, and do not accept invalid
  values for the attribute
- Do not accept negative fan speed values
- Display fan speed and speed limit as 0 if register value is 0
  (instead of 6000000), as in original code
- Only permit writing 0 (unlimited) for the maximum fan speed
- Add Reviewed-by: tags where given
- Fix definition of AMC6821_CONF1_FDRC1 in patch 7/10
- Use sign_extend32() instead of odd type cast
- Drop remaining spurious debug message in patch 9 instead of patch 10
- Add missing "select REGMAP_I2C" to Kconfig
- Change misleading variable name from 'mask' to 'mode'
- Use sysfs_emit instead of sprintf everywhere
- Add support for pwm1_mode attribute

----------------------------------------------------------------
Guenter Roeck (11):
      hwmon: (amc6821) Stop accepting invalid pwm values
      hwmon: (amc6821) Make reading and writing fan speed limits consistent
      hwmon: (amc6821) Rename fan1_div to fan1_pulses
      hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4
      hwmon: (amc2821) Reorder include files, drop unnecessary ones
      hwmon: (amc6821) Use tabs for column alignment in defines
      hwmon: (amc2821) Use BIT() and GENMASK()
      hwmon: (amc6821) Drop unnecessary enum chips
      hwmon: (amc6821) Convert to use regmap
      hwmon: (amc6821) Convert to with_info API
      hwmon: (amc6821) Add support for pwm1_mode attribute

 Documentation/hwmon/amc6821.rst |    7 +-
 drivers/hwmon/Kconfig           |    1 +
 drivers/hwmon/amc6821.c         | 1299 ++++++++++++++++++++-------------------
 3 files changed, 660 insertions(+), 647 deletions(-)

[PATCH v2 01/11] hwmon: (amc6821) Stop accepting invalid pwm values

[Guenter Roeck]

The pwm value range is well defined from 0..255. Don't accept
any values outside this range.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Use kstrtou8() instead of kstrtol() where possible.
    Limit range of pwm1_auto_point_pwm to 0..254 in patch 1
    instead of limiting it later, and do not accept invalid
    values for the attribute.

 drivers/hwmon/amc6821.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 9b02b304c2f5..eb2d5592a41a 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev,
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	long val;
-	int ret = kstrtol(buf, 10, &val);
+	u8 val;
+	int ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		return ret;
 
 	mutex_lock(&data->update_lock);
-	data->pwm1 = clamp_val(val , 0, 255);
+	data->pwm1 = val;
 	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
 	mutex_unlock(&data->update_lock);
 	return count;
@@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
 	int dpwm;
-	long val;
-	int ret = kstrtol(buf, 10, &val);
+	u8 val;
+	int ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		return ret;
 
+	if (val > 254)
+		return -EINVAL;
+
 	mutex_lock(&data->update_lock);
-	data->pwm1_auto_point_pwm[1] = clamp_val(val, 0, 254);
+	data->pwm1_auto_point_pwm[1] = val;
 	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
 			data->pwm1_auto_point_pwm[1])) {
 		dev_err(&client->dev, "Register write error, aborting.\n");
-- 
2.39.2

[PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

The default value of the maximum fan speed limit register is 0,
essentially translating to an unlimited fan speed. When reading
the limit, a value of 0 is reported in this case. However, writing
a value of 0 results in writing a value of 0xffff into the register,
which is inconsistent.

To solve the problem, permit writing a limit of 0 for the maximim fan
speed, effectively translating to "no limit". Write 0 into the register
if a limit value of 0 is written. Otherwise limit the range to
<1..6000000> and write 1..0xffff into the register. This ensures that
reading and writing from and to a limit register return the same value
while at the same time not changing reported values when reading the
speed or limits.

While at it, restrict fan limit writes to non-negative numbers; writing
a negative limit does not make sense and should be reported instead of
being corrected.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Do not accept negative fan speed values
    Display fan speed and speed limit as 0 if register value is 0
    (instead of 6000000), as in original code.
    Only permit writing 0 (unlimited) for the maximum fan speed.

 drivers/hwmon/amc6821.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index eb2d5592a41a..9c19d4d278ec 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	long val;
+	unsigned long val;
 	int ix = to_sensor_dev_attr(attr)->index;
-	int ret = kstrtol(buf, 10, &val);
+	int ret = kstrtoul(buf, 10, &val);
 	if (ret)
 		return ret;
-	val = 1 > val ? 0xFFFF : 6000000/val;
+
+	/* The minimum fan speed must not be unlimited (0) */
+	if (ix == IDX_FAN1_MIN && !val)
+		return -EINVAL;
+
+	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
 
 	mutex_lock(&data->update_lock);
-	data->fan[ix] = (u16) clamp_val(val, 1, 0xFFFF);
+	data->fan[ix] = clamp_val(val, 0, 0xFFFF);
 	if (i2c_smbus_write_byte_data(client, fan_reg_low[ix],
 			data->fan[ix] & 0xFF)) {
 		dev_err(&client->dev, "Register write error, aborting.\n");
-- 
2.39.2

[PATCH v2 03/11] hwmon: (amc6821) Rename fan1_div to fan1_pulses

[Guenter Roeck]

The chip does not have a fan divisor. What it does have is a configuration
to set either 2 or 4 pulses per fan rotation. Rename the attribute to
reflect its use. Update documentation accordingly.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Quentin's Reviewed-by: tag

 Documentation/hwmon/amc6821.rst |  2 +-
 drivers/hwmon/amc6821.c         | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
index 5ddb2849da90..4ce67c268e52 100644
--- a/Documentation/hwmon/amc6821.rst
+++ b/Documentation/hwmon/amc6821.rst
@@ -47,7 +47,7 @@ fan1_input		ro	tachometer speed
 fan1_min		rw	"
 fan1_max		rw	"
 fan1_fault		ro	"
-fan1_div		rw	Fan divisor can be either 2 or 4.
+fan1_pulses		rw	Pulses per revolution can be either 2 or 4.
 
 pwm1			rw	pwm1
 pwm1_enable		rw	regulator mode, 1=open loop, 2=fan controlled
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 9c19d4d278ec..ed98d470a308 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -148,7 +148,7 @@ struct amc6821_data {
 	int temp[TEMP_IDX_LEN];
 
 	u16 fan[FAN1_IDX_LEN];
-	u8 fan1_div;
+	u8 fan1_pulses;
 
 	u8 pwm1;
 	u8 temp1_auto_point_temp[3];
@@ -193,9 +193,9 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
 					client,
 					fan_reg_hi[i]) << 8;
 		}
-		data->fan1_div = i2c_smbus_read_byte_data(client,
+		data->fan1_pulses = i2c_smbus_read_byte_data(client,
 			AMC6821_REG_CONF4);
-		data->fan1_div = data->fan1_div & AMC6821_CONF4_PSPR ? 4 : 2;
+		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
 
 		data->pwm1_auto_point_pwm[0] = 0;
 		data->pwm1_auto_point_pwm[2] = 255;
@@ -647,16 +647,16 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static ssize_t fan1_div_show(struct device *dev,
-			     struct device_attribute *devattr, char *buf)
+static ssize_t fan1_pulses_show(struct device *dev,
+				struct device_attribute *devattr, char *buf)
 {
 	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->fan1_div);
+	return sprintf(buf, "%d\n", data->fan1_pulses);
 }
 
-static ssize_t fan1_div_store(struct device *dev,
-			      struct device_attribute *attr, const char *buf,
-			      size_t count)
+static ssize_t fan1_pulses_store(struct device *dev,
+				 struct device_attribute *attr, const char *buf,
+				 size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
@@ -676,11 +676,11 @@ static ssize_t fan1_div_store(struct device *dev,
 	switch (val) {
 	case 2:
 		config &= ~AMC6821_CONF4_PSPR;
-		data->fan1_div = 2;
+		data->fan1_pulses = 2;
 		break;
 	case 4:
 		config |= AMC6821_CONF4_PSPR;
-		data->fan1_div = 4;
+		data->fan1_pulses = 4;
 		break;
 	default:
 		count = -EINVAL;
@@ -715,7 +715,7 @@ static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, IDX_FAN1_INPUT);
 static SENSOR_DEVICE_ATTR_RW(fan1_min, fan, IDX_FAN1_MIN);
 static SENSOR_DEVICE_ATTR_RW(fan1_max, fan, IDX_FAN1_MAX);
 static SENSOR_DEVICE_ATTR_RO(fan1_fault, fan1_fault, 0);
-static SENSOR_DEVICE_ATTR_RW(fan1_div, fan1_div, 0);
+static SENSOR_DEVICE_ATTR_RW(fan1_pulses, fan1_pulses, 0);
 
 static SENSOR_DEVICE_ATTR_RW(pwm1, pwm1, 0);
 static SENSOR_DEVICE_ATTR_RW(pwm1_enable, pwm1_enable, 0);
@@ -758,7 +758,7 @@ static struct attribute *amc6821_attrs[] = {
 	&sensor_dev_attr_fan1_min.dev_attr.attr,
 	&sensor_dev_attr_fan1_max.dev_attr.attr,
 	&sensor_dev_attr_fan1_fault.dev_attr.attr,
-	&sensor_dev_attr_fan1_div.dev_attr.attr,
+	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
 	&sensor_dev_attr_pwm1.dev_attr.attr,
 	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
-- 
2.39.2

[PATCH v2 04/11] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Guenter Roeck]

After setting fan1_target and setting pwm1_enable to 4,
the fan controller tries to achieve the requested fan speed.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Do not permit writing negative or unlimited target fan speed

 Documentation/hwmon/amc6821.rst |  4 ++++
 drivers/hwmon/amc6821.c         | 25 +++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
index 4ce67c268e52..96e604c5ea8e 100644
--- a/Documentation/hwmon/amc6821.rst
+++ b/Documentation/hwmon/amc6821.rst
@@ -48,12 +48,16 @@ fan1_min		rw	"
 fan1_max		rw	"
 fan1_fault		ro	"
 fan1_pulses		rw	Pulses per revolution can be either 2 or 4.
+fan1_target		rw	Target fan speed, to be used with pwm1_enable
+				mode 4.
 
 pwm1			rw	pwm1
 pwm1_enable		rw	regulator mode, 1=open loop, 2=fan controlled
 				by remote temperature, 3=fan controlled by
 				combination of the on-chip temperature and
 				remote-sensor temperature,
+				4=fan controlled by target rpm set with
+				fan1_target attribute.
 pwm1_auto_channels_temp ro	1 if pwm_enable==2, 3 if pwm_enable==3
 pwm1_auto_point1_pwm	ro	Hardwired to 0, shared for both
 				temperature channels.
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index ed98d470a308..caf720ff674c 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -66,6 +66,8 @@ enum chips { amc6821 };
 #define AMC6821_REG_TACH_LLIMITH 0x11
 #define AMC6821_REG_TACH_HLIMITL 0x12
 #define AMC6821_REG_TACH_HLIMITH 0x13
+#define AMC6821_REG_TACH_SETTINGL 0x1e
+#define AMC6821_REG_TACH_SETTINGH 0x1f
 
 #define AMC6821_CONF1_START 0x01
 #define AMC6821_CONF1_FAN_INT_EN 0x02
@@ -122,17 +124,18 @@ static const u8 temp_reg[] = {AMC6821_REG_LTEMP_HI,
 			AMC6821_REG_RTEMP_LIMIT_MAX,
 			AMC6821_REG_RTEMP_CRIT, };
 
-enum {IDX_FAN1_INPUT = 0, IDX_FAN1_MIN, IDX_FAN1_MAX,
+enum {IDX_FAN1_INPUT = 0, IDX_FAN1_MIN, IDX_FAN1_MAX, IDX_FAN1_TARGET,
 	FAN1_IDX_LEN, };
 
 static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
 			AMC6821_REG_TACH_LLIMITL,
-			AMC6821_REG_TACH_HLIMITL, };
-
+			AMC6821_REG_TACH_HLIMITL,
+			AMC6821_REG_TACH_SETTINGL, };
 
 static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
 			AMC6821_REG_TACH_LLIMITH,
-			AMC6821_REG_TACH_HLIMITH, };
+			AMC6821_REG_TACH_HLIMITH,
+			AMC6821_REG_TACH_SETTINGH, };
 
 /*
  * Client data (each client gets its own)
@@ -250,10 +253,10 @@ static struct amc6821_data *amc6821_update_device(struct device *dev)
 			break;
 		case 1: /*
 			 * semi-open loop: software sets rpm, chip controls
-			 * pwm1, currently not implemented
+			 * pwm1
 			 */
 			data->pwm1_auto_channels_temp = 0;
-			data->pwm1_enable = 0;
+			data->pwm1_enable = 4;
 			break;
 		}
 
@@ -407,6 +410,10 @@ static ssize_t pwm1_enable_store(struct device *dev,
 		config |= AMC6821_CONF1_FDRC0;
 		config |= AMC6821_CONF1_FDRC1;
 		break;
+	case 4:
+		config |= AMC6821_CONF1_FDRC0;
+		config &= ~AMC6821_CONF1_FDRC1;
+		break;
 	default:
 		count = -EINVAL;
 		goto unlock;
@@ -623,8 +630,8 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 	if (ret)
 		return ret;
 
-	/* The minimum fan speed must not be unlimited (0) */
-	if (ix == IDX_FAN1_MIN && !val)
+	/* Minimum and target fan speed must not be unlimited (0) */
+	if ((ix == IDX_FAN1_MIN || ix == IDX_FAN1_TARGET) && !val)
 		return -EINVAL;
 
 	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
@@ -714,6 +721,7 @@ static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, temp_alarm, IDX_TEMP2_CRIT);
 static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, IDX_FAN1_INPUT);
 static SENSOR_DEVICE_ATTR_RW(fan1_min, fan, IDX_FAN1_MIN);
 static SENSOR_DEVICE_ATTR_RW(fan1_max, fan, IDX_FAN1_MAX);
+static SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET);
 static SENSOR_DEVICE_ATTR_RO(fan1_fault, fan1_fault, 0);
 static SENSOR_DEVICE_ATTR_RW(fan1_pulses, fan1_pulses, 0);
 
@@ -757,6 +765,7 @@ static struct attribute *amc6821_attrs[] = {
 	&sensor_dev_attr_fan1_input.dev_attr.attr,
 	&sensor_dev_attr_fan1_min.dev_attr.attr,
 	&sensor_dev_attr_fan1_max.dev_attr.attr,
+	&sensor_dev_attr_fan1_target.dev_attr.attr,
 	&sensor_dev_attr_fan1_fault.dev_attr.attr,
 	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
 	&sensor_dev_attr_pwm1.dev_attr.attr,
-- 
2.39.2

[PATCH v2 05/11] hwmon: (amc2821) Reorder include files, drop unnecessary ones

[Guenter Roeck]

Reorder include files to alphabetic order to simplify maintenance,
and drop the unnecessary kernel.h include.

No functional change intended.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Quentin's Reviewed-by: tag

 drivers/hwmon/amc6821.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index caf720ff674c..e9d345c8064e 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,16 +8,15 @@
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
  */
 
-#include <linux/kernel.h>	/* Needed for KERN_INFO */
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/jiffies.h>
-#include <linux/i2c.h>
+#include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/slab.h>
 
 /*
  * Addresses to scan.
-- 
2.39.2

[PATCH v2 06/11] hwmon: (amc6821) Use tabs for column alignment in defines

[Guenter Roeck]

Using tabs for column alignment makes the code easier to read.

No functional change intended.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Quentin's Reviewed-by: tag

 drivers/hwmon/amc6821.c | 128 ++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index e9d345c8064e..23111c6cb142 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -37,77 +37,77 @@ module_param(init, int, 0444);
 
 enum chips { amc6821 };
 
-#define AMC6821_REG_DEV_ID 0x3D
-#define AMC6821_REG_COMP_ID 0x3E
-#define AMC6821_REG_CONF1 0x00
-#define AMC6821_REG_CONF2 0x01
-#define AMC6821_REG_CONF3 0x3F
-#define AMC6821_REG_CONF4 0x04
-#define AMC6821_REG_STAT1 0x02
-#define AMC6821_REG_STAT2 0x03
-#define AMC6821_REG_TDATA_LOW 0x08
-#define AMC6821_REG_TDATA_HI 0x09
-#define AMC6821_REG_LTEMP_HI 0x0A
-#define AMC6821_REG_RTEMP_HI 0x0B
-#define AMC6821_REG_LTEMP_LIMIT_MIN 0x15
-#define AMC6821_REG_LTEMP_LIMIT_MAX 0x14
-#define AMC6821_REG_RTEMP_LIMIT_MIN 0x19
-#define AMC6821_REG_RTEMP_LIMIT_MAX 0x18
-#define AMC6821_REG_LTEMP_CRIT 0x1B
-#define AMC6821_REG_RTEMP_CRIT 0x1D
-#define AMC6821_REG_PSV_TEMP 0x1C
-#define AMC6821_REG_DCY 0x22
-#define AMC6821_REG_LTEMP_FAN_CTRL 0x24
-#define AMC6821_REG_RTEMP_FAN_CTRL 0x25
-#define AMC6821_REG_DCY_LOW_TEMP 0x21
+#define AMC6821_REG_DEV_ID		0x3D
+#define AMC6821_REG_COMP_ID		0x3E
+#define AMC6821_REG_CONF1		0x00
+#define AMC6821_REG_CONF2		0x01
+#define AMC6821_REG_CONF3		0x3F
+#define AMC6821_REG_CONF4		0x04
+#define AMC6821_REG_STAT1		0x02
+#define AMC6821_REG_STAT2		0x03
+#define AMC6821_REG_TDATA_LOW		0x08
+#define AMC6821_REG_TDATA_HI		0x09
+#define AMC6821_REG_LTEMP_HI		0x0A
+#define AMC6821_REG_RTEMP_HI		0x0B
+#define AMC6821_REG_LTEMP_LIMIT_MIN	0x15
+#define AMC6821_REG_LTEMP_LIMIT_MAX	0x14
+#define AMC6821_REG_RTEMP_LIMIT_MIN	0x19
+#define AMC6821_REG_RTEMP_LIMIT_MAX	0x18
+#define AMC6821_REG_LTEMP_CRIT		0x1B
+#define AMC6821_REG_RTEMP_CRIT		0x1D
+#define AMC6821_REG_PSV_TEMP		0x1C
+#define AMC6821_REG_DCY			0x22
+#define AMC6821_REG_LTEMP_FAN_CTRL	0x24
+#define AMC6821_REG_RTEMP_FAN_CTRL	0x25
+#define AMC6821_REG_DCY_LOW_TEMP	0x21
 
-#define AMC6821_REG_TACH_LLIMITL 0x10
-#define AMC6821_REG_TACH_LLIMITH 0x11
-#define AMC6821_REG_TACH_HLIMITL 0x12
-#define AMC6821_REG_TACH_HLIMITH 0x13
-#define AMC6821_REG_TACH_SETTINGL 0x1e
-#define AMC6821_REG_TACH_SETTINGH 0x1f
+#define AMC6821_REG_TACH_LLIMITL	0x10
+#define AMC6821_REG_TACH_LLIMITH	0x11
+#define AMC6821_REG_TACH_HLIMITL	0x12
+#define AMC6821_REG_TACH_HLIMITH	0x13
+#define AMC6821_REG_TACH_SETTINGL	0x1e
+#define AMC6821_REG_TACH_SETTINGH	0x1f
 
-#define AMC6821_CONF1_START 0x01
-#define AMC6821_CONF1_FAN_INT_EN 0x02
-#define AMC6821_CONF1_FANIE 0x04
-#define AMC6821_CONF1_PWMINV 0x08
-#define AMC6821_CONF1_FAN_FAULT_EN 0x10
-#define AMC6821_CONF1_FDRC0 0x20
-#define AMC6821_CONF1_FDRC1 0x40
-#define AMC6821_CONF1_THERMOVIE 0x80
+#define AMC6821_CONF1_START		0x01
+#define AMC6821_CONF1_FAN_INT_EN	0x02
+#define AMC6821_CONF1_FANIE		0x04
+#define AMC6821_CONF1_PWMINV		0x08
+#define AMC6821_CONF1_FAN_FAULT_EN	0x10
+#define AMC6821_CONF1_FDRC0		0x20
+#define AMC6821_CONF1_FDRC1		0x40
+#define AMC6821_CONF1_THERMOVIE		0x80
 
-#define AMC6821_CONF2_PWM_EN 0x01
-#define AMC6821_CONF2_TACH_MODE 0x02
-#define AMC6821_CONF2_TACH_EN 0x04
-#define AMC6821_CONF2_RTFIE 0x08
-#define AMC6821_CONF2_LTOIE 0x10
-#define AMC6821_CONF2_RTOIE 0x20
-#define AMC6821_CONF2_PSVIE 0x40
-#define AMC6821_CONF2_RST 0x80
+#define AMC6821_CONF2_PWM_EN		0x01
+#define AMC6821_CONF2_TACH_MODE		0x02
+#define AMC6821_CONF2_TACH_EN		0x04
+#define AMC6821_CONF2_RTFIE		0x08
+#define AMC6821_CONF2_LTOIE		0x10
+#define AMC6821_CONF2_RTOIE		0x20
+#define AMC6821_CONF2_PSVIE		0x40
+#define AMC6821_CONF2_RST		0x80
 
-#define AMC6821_CONF3_THERM_FAN_EN 0x80
-#define AMC6821_CONF3_REV_MASK 0x0F
+#define AMC6821_CONF3_THERM_FAN_EN	0x80
+#define AMC6821_CONF3_REV_MASK		0x0F
 
-#define AMC6821_CONF4_OVREN 0x10
-#define AMC6821_CONF4_TACH_FAST 0x20
-#define AMC6821_CONF4_PSPR 0x40
-#define AMC6821_CONF4_MODE 0x80
+#define AMC6821_CONF4_OVREN		0x10
+#define AMC6821_CONF4_TACH_FAST		0x20
+#define AMC6821_CONF4_PSPR		0x40
+#define AMC6821_CONF4_MODE		0x80
 
-#define AMC6821_STAT1_RPM_ALARM 0x01
-#define AMC6821_STAT1_FANS 0x02
-#define AMC6821_STAT1_RTH 0x04
-#define AMC6821_STAT1_RTL 0x08
-#define AMC6821_STAT1_R_THERM 0x10
-#define AMC6821_STAT1_RTF 0x20
-#define AMC6821_STAT1_LTH 0x40
-#define AMC6821_STAT1_LTL 0x80
+#define AMC6821_STAT1_RPM_ALARM		0x01
+#define AMC6821_STAT1_FANS		0x02
+#define AMC6821_STAT1_RTH		0x04
+#define AMC6821_STAT1_RTL		0x08
+#define AMC6821_STAT1_R_THERM		0x10
+#define AMC6821_STAT1_RTF		0x20
+#define AMC6821_STAT1_LTH		0x40
+#define AMC6821_STAT1_LTL		0x80
 
-#define AMC6821_STAT2_RTC 0x08
-#define AMC6821_STAT2_LTC 0x10
-#define AMC6821_STAT2_LPSV 0x20
-#define AMC6821_STAT2_L_THERM 0x40
-#define AMC6821_STAT2_THERM_IN 0x80
+#define AMC6821_STAT2_RTC		0x08
+#define AMC6821_STAT2_LTC		0x10
+#define AMC6821_STAT2_LPSV		0x20
+#define AMC6821_STAT2_L_THERM		0x40
+#define AMC6821_STAT2_THERM_IN		0x80
 
 enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
 	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-- 
2.39.2

[PATCH v2 07/11] hwmon: (amc2821) Use BIT() and GENMASK()

[Guenter Roeck]

Use BIT() and GENMASK() for bit and mask definitions
to help distinguish bit and mask definitions from other
defines and to make the code easier to read.

No functional change intended.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fix definition of AMC6821_CONF1_FDRC1 in this patch

 drivers/hwmon/amc6821.c | 71 +++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 23111c6cb142..fa9f64c743ff 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
  */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -68,46 +69,46 @@ enum chips { amc6821 };
 #define AMC6821_REG_TACH_SETTINGL	0x1e
 #define AMC6821_REG_TACH_SETTINGH	0x1f
 
-#define AMC6821_CONF1_START		0x01
-#define AMC6821_CONF1_FAN_INT_EN	0x02
-#define AMC6821_CONF1_FANIE		0x04
-#define AMC6821_CONF1_PWMINV		0x08
-#define AMC6821_CONF1_FAN_FAULT_EN	0x10
-#define AMC6821_CONF1_FDRC0		0x20
-#define AMC6821_CONF1_FDRC1		0x40
-#define AMC6821_CONF1_THERMOVIE		0x80
+#define AMC6821_CONF1_START		BIT(0)
+#define AMC6821_CONF1_FAN_INT_EN	BIT(1)
+#define AMC6821_CONF1_FANIE		BIT(2)
+#define AMC6821_CONF1_PWMINV		BIT(3)
+#define AMC6821_CONF1_FAN_FAULT_EN	BIT(4)
+#define AMC6821_CONF1_FDRC0		BIT(5)
+#define AMC6821_CONF1_FDRC1		BIT(6)
+#define AMC6821_CONF1_THERMOVIE		BIT(7)
 
-#define AMC6821_CONF2_PWM_EN		0x01
-#define AMC6821_CONF2_TACH_MODE		0x02
-#define AMC6821_CONF2_TACH_EN		0x04
-#define AMC6821_CONF2_RTFIE		0x08
-#define AMC6821_CONF2_LTOIE		0x10
-#define AMC6821_CONF2_RTOIE		0x20
-#define AMC6821_CONF2_PSVIE		0x40
-#define AMC6821_CONF2_RST		0x80
+#define AMC6821_CONF2_PWM_EN		BIT(0)
+#define AMC6821_CONF2_TACH_MODE		BIT(1)
+#define AMC6821_CONF2_TACH_EN		BIT(2)
+#define AMC6821_CONF2_RTFIE		BIT(3)
+#define AMC6821_CONF2_LTOIE		BIT(4)
+#define AMC6821_CONF2_RTOIE		BIT(5)
+#define AMC6821_CONF2_PSVIE		BIT(6)
+#define AMC6821_CONF2_RST		BIT(7)
 
-#define AMC6821_CONF3_THERM_FAN_EN	0x80
-#define AMC6821_CONF3_REV_MASK		0x0F
+#define AMC6821_CONF3_THERM_FAN_EN	BIT(7)
+#define AMC6821_CONF3_REV_MASK		GENMASK(3, 0)
 
-#define AMC6821_CONF4_OVREN		0x10
-#define AMC6821_CONF4_TACH_FAST		0x20
-#define AMC6821_CONF4_PSPR		0x40
-#define AMC6821_CONF4_MODE		0x80
+#define AMC6821_CONF4_OVREN		BIT(4)
+#define AMC6821_CONF4_TACH_FAST		BIT(5)
+#define AMC6821_CONF4_PSPR		BIT(6)
+#define AMC6821_CONF4_MODE		BIT(7)
 
-#define AMC6821_STAT1_RPM_ALARM		0x01
-#define AMC6821_STAT1_FANS		0x02
-#define AMC6821_STAT1_RTH		0x04
-#define AMC6821_STAT1_RTL		0x08
-#define AMC6821_STAT1_R_THERM		0x10
-#define AMC6821_STAT1_RTF		0x20
-#define AMC6821_STAT1_LTH		0x40
-#define AMC6821_STAT1_LTL		0x80
+#define AMC6821_STAT1_RPM_ALARM		BIT(0)
+#define AMC6821_STAT1_FANS		BIT(1)
+#define AMC6821_STAT1_RTH		BIT(2)
+#define AMC6821_STAT1_RTL		BIT(3)
+#define AMC6821_STAT1_R_THERM		BIT(4)
+#define AMC6821_STAT1_RTF		BIT(5)
+#define AMC6821_STAT1_LTH		BIT(6)
+#define AMC6821_STAT1_LTL		BIT(7)
 
-#define AMC6821_STAT2_RTC		0x08
-#define AMC6821_STAT2_LTC		0x10
-#define AMC6821_STAT2_LPSV		0x20
-#define AMC6821_STAT2_L_THERM		0x40
-#define AMC6821_STAT2_THERM_IN		0x80
+#define AMC6821_STAT2_RTC		BIT(3)
+#define AMC6821_STAT2_LTC		BIT(4)
+#define AMC6821_STAT2_LPSV		BIT(5)
+#define AMC6821_STAT2_L_THERM		BIT(6)
+#define AMC6821_STAT2_THERM_IN		BIT(7)
 
 enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
 	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-- 
2.39.2

[PATCH v2 08/11] hwmon: (amc6821) Drop unnecessary enum chips

[Guenter Roeck]

The driver only supports a single chip, so an enum
to determine the chip type is unnecessary. Drop it.

No functional change intended.

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add Quentin's Reviewed-by: tag

 drivers/hwmon/amc6821.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index fa9f64c743ff..028998d3bedf 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -36,8 +36,6 @@ module_param(pwminv, int, 0444);
 static int init = 1; /*Power-on initialization.*/
 module_param(init, int, 0444);
 
-enum chips { amc6821 };
-
 #define AMC6821_REG_DEV_ID		0x3D
 #define AMC6821_REG_COMP_ID		0x3E
 #define AMC6821_REG_CONF1		0x00
@@ -945,7 +943,7 @@ static int amc6821_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id amc6821_id[] = {
-	{ "amc6821", amc6821 },
+	{ "amc6821", 0 },
 	{ }
 };
 
@@ -954,7 +952,6 @@ MODULE_DEVICE_TABLE(i2c, amc6821_id);
 static const struct of_device_id __maybe_unused amc6821_of_match[] = {
 	{
 		.compatible = "ti,amc6821",
-		.data = (void *)amc6821,
 	},
 	{ }
 };
-- 
2.39.2

[PATCH v2 09/11] hwmon: (amc6821) Convert to use regmap

[Guenter Roeck]

Use regmap for register accesses and for most caching.

While at it, use sysfs_emit() instead of sprintf() to write sysfs
attribute data, and remove spurious debug messages which would
only be seen as result of a bug in the code.

No functional change intended.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Drop another spurious debug message in this patch instead of patch 10
    Add missing "select REGMAP_I2C" to Kconfig
    Change misleading variable name from 'mask' to 'mode'.
    Use sysfs_emit instead of sprintf everywhere

 drivers/hwmon/Kconfig   |   1 +
 drivers/hwmon/amc6821.c | 713 ++++++++++++++++++----------------------
 2 files changed, 329 insertions(+), 385 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..a8fa87a96e8f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
 config SENSORS_AMC6821
 	tristate "Texas Instruments AMC6821"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for the Texas Instruments
 	  AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 028998d3bedf..3fe0bfeac843 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -8,15 +8,16 @@
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
  */
 
+#include <linux/bitops.h>
 #include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/jiffies.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 /*
@@ -44,6 +45,7 @@ module_param(init, int, 0444);
 #define AMC6821_REG_CONF4		0x04
 #define AMC6821_REG_STAT1		0x02
 #define AMC6821_REG_STAT2		0x03
+#define AMC6821_REG_TEMP_LO		0x06
 #define AMC6821_REG_TDATA_LOW		0x08
 #define AMC6821_REG_TDATA_HI		0x09
 #define AMC6821_REG_LTEMP_HI		0x0A
@@ -61,11 +63,8 @@ module_param(init, int, 0444);
 #define AMC6821_REG_DCY_LOW_TEMP	0x21
 
 #define AMC6821_REG_TACH_LLIMITL	0x10
-#define AMC6821_REG_TACH_LLIMITH	0x11
 #define AMC6821_REG_TACH_HLIMITL	0x12
-#define AMC6821_REG_TACH_HLIMITH	0x13
 #define AMC6821_REG_TACH_SETTINGL	0x1e
-#define AMC6821_REG_TACH_SETTINGH	0x1f
 
 #define AMC6821_CONF1_START		BIT(0)
 #define AMC6821_CONF1_FAN_INT_EN	BIT(1)
@@ -130,224 +129,169 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
 			AMC6821_REG_TACH_HLIMITL,
 			AMC6821_REG_TACH_SETTINGL, };
 
-static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
-			AMC6821_REG_TACH_LLIMITH,
-			AMC6821_REG_TACH_HLIMITH,
-			AMC6821_REG_TACH_SETTINGH, };
-
 /*
  * Client data (each client gets its own)
  */
 
 struct amc6821_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	struct mutex update_lock;
-	bool valid; /* false until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
 
-	/* register values */
-	int temp[TEMP_IDX_LEN];
-
-	u16 fan[FAN1_IDX_LEN];
-	u8 fan1_pulses;
-
-	u8 pwm1;
 	u8 temp1_auto_point_temp[3];
 	u8 temp2_auto_point_temp[3];
-	u8 pwm1_auto_point_pwm[3];
-	u8 pwm1_enable;
-	u8 pwm1_auto_channels_temp;
-
-	u8 stat1;
-	u8 stat2;
 };
 
-static struct amc6821_data *amc6821_update_device(struct device *dev)
+static int amc6821_init_auto_point_data(struct amc6821_data *data)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int timeout = HZ;
-	u8 reg;
-	int i;
+	struct regmap *regmap = data->regmap;
+	u32 pwm, regval;
+	int err;
 
-	mutex_lock(&data->update_lock);
+	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+	if (err)
+		return err;
 
-	if (time_after(jiffies, data->last_updated + timeout) ||
-			!data->valid) {
+	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
+	if (err)
+		return err;
+	data->temp1_auto_point_temp[0] = regval;
+	data->temp2_auto_point_temp[0] = data->temp1_auto_point_temp[0];
 
-		for (i = 0; i < TEMP_IDX_LEN; i++)
-			data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
-				client, temp_reg[i]);
+	err = regmap_read(regmap, AMC6821_REG_LTEMP_FAN_CTRL, &regval);
+	if (err)
+		return err;
+	data->temp1_auto_point_temp[1] = (regval & 0xF8) >> 1;
 
-		data->stat1 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_STAT1);
-		data->stat2 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_STAT2);
+	regval &= 0x07;
+	regval = 0x20 >> regval;
+	if (regval)
+		data->temp1_auto_point_temp[2] =
+			data->temp1_auto_point_temp[1] +
+			(255 - pwm) / regval;
+	else
+		data->temp1_auto_point_temp[2] = 255;
 
-		data->pwm1 = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_DCY);
-		for (i = 0; i < FAN1_IDX_LEN; i++) {
-			data->fan[i] = i2c_smbus_read_byte_data(
-					client,
-					fan_reg_low[i]);
-			data->fan[i] += i2c_smbus_read_byte_data(
-					client,
-					fan_reg_hi[i]) << 8;
-		}
-		data->fan1_pulses = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_CONF4);
-		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
+	err = regmap_read(regmap, AMC6821_REG_RTEMP_FAN_CTRL, &regval);
+	if (err)
+		return err;
 
-		data->pwm1_auto_point_pwm[0] = 0;
-		data->pwm1_auto_point_pwm[2] = 255;
-		data->pwm1_auto_point_pwm[1] = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_DCY_LOW_TEMP);
+	data->temp2_auto_point_temp[1] = (regval & 0xF8) >> 1;
+	regval &= 0x07;
+	regval = 0x20 >> regval;
 
-		data->temp1_auto_point_temp[0] =
-			i2c_smbus_read_byte_data(client,
-					AMC6821_REG_PSV_TEMP);
-		data->temp2_auto_point_temp[0] =
-				data->temp1_auto_point_temp[0];
-		reg = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_LTEMP_FAN_CTRL);
-		data->temp1_auto_point_temp[1] = (reg & 0xF8) >> 1;
-		reg &= 0x07;
-		reg = 0x20 >> reg;
-		if (reg > 0)
-			data->temp1_auto_point_temp[2] =
-				data->temp1_auto_point_temp[1] +
-				(data->pwm1_auto_point_pwm[2] -
-				data->pwm1_auto_point_pwm[1]) / reg;
-		else
-			data->temp1_auto_point_temp[2] = 255;
+	if (regval)
+		data->temp2_auto_point_temp[2] =
+			data->temp2_auto_point_temp[1] +
+			(255 - pwm) / regval;
+	else
+		data->temp2_auto_point_temp[2] = 255;
 
-		reg = i2c_smbus_read_byte_data(client,
-			AMC6821_REG_RTEMP_FAN_CTRL);
-		data->temp2_auto_point_temp[1] = (reg & 0xF8) >> 1;
-		reg &= 0x07;
-		reg = 0x20 >> reg;
-		if (reg > 0)
-			data->temp2_auto_point_temp[2] =
-				data->temp2_auto_point_temp[1] +
-				(data->pwm1_auto_point_pwm[2] -
-				data->pwm1_auto_point_pwm[1]) / reg;
-		else
-			data->temp2_auto_point_temp[2] = 255;
-
-		reg = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-		reg = (reg >> 5) & 0x3;
-		switch (reg) {
-		case 0: /*open loop: software sets pwm1*/
-			data->pwm1_auto_channels_temp = 0;
-			data->pwm1_enable = 1;
-			break;
-		case 2: /*closed loop: remote T (temp2)*/
-			data->pwm1_auto_channels_temp = 2;
-			data->pwm1_enable = 2;
-			break;
-		case 3: /*closed loop: local and remote T (temp2)*/
-			data->pwm1_auto_channels_temp = 3;
-			data->pwm1_enable = 3;
-			break;
-		case 1: /*
-			 * semi-open loop: software sets rpm, chip controls
-			 * pwm1
-			 */
-			data->pwm1_auto_channels_temp = 0;
-			data->pwm1_enable = 4;
-			break;
-		}
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	mutex_unlock(&data->update_lock);
-	return data;
+	return 0;
 }
 
 static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
+	u32 regval;
+	int err;
 
-	return sprintf(buf, "%d\n", data->temp[ix] * 1000);
+	err = regmap_read(data->regmap, temp_reg[ix], &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", sign_extend32(regval, 7) * 1000);
 }
 
 static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int ix = to_sensor_dev_attr(attr)->index;
 	long val;
+	int err;
 
 	int ret = kstrtol(buf, 10, &val);
 	if (ret)
 		return ret;
 	val = clamp_val(val / 1000, -128, 127);
 
-	mutex_lock(&data->update_lock);
-	data->temp[ix] = val;
-	if (i2c_smbus_write_byte_data(client, temp_reg[ix], data->temp[ix])) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-	}
-	mutex_unlock(&data->update_lock);
+	err = regmap_write(data->regmap, temp_reg[ix], val);
+	if (err)
+		return err;
+
 	return count;
 }
 
 static ssize_t temp_alarm_show(struct device *dev,
 			       struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	u8 flag;
+	u32 regval, mask, reg;
+	int err;
 
 	switch (ix) {
 	case IDX_TEMP1_MIN:
-		flag = data->stat1 & AMC6821_STAT1_LTL;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_LTL;
 		break;
 	case IDX_TEMP1_MAX:
-		flag = data->stat1 & AMC6821_STAT1_LTH;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_LTH;
 		break;
 	case IDX_TEMP1_CRIT:
-		flag = data->stat2 & AMC6821_STAT2_LTC;
+		reg = AMC6821_REG_STAT2;
+		mask = AMC6821_STAT2_LTC;
 		break;
 	case IDX_TEMP2_MIN:
-		flag = data->stat1 & AMC6821_STAT1_RTL;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_RTL;
 		break;
 	case IDX_TEMP2_MAX:
-		flag = data->stat1 & AMC6821_STAT1_RTH;
+		reg = AMC6821_REG_STAT1;
+		mask = AMC6821_STAT1_RTH;
 		break;
 	case IDX_TEMP2_CRIT:
-		flag = data->stat2 & AMC6821_STAT2_RTC;
+		reg = AMC6821_REG_STAT2;
+		mask = AMC6821_STAT2_RTC;
 		break;
 	default:
-		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
 		return -EINVAL;
 	}
-	if (flag)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	err = regmap_read(data->regmap, reg, &regval);
+	if (err)
+		return err;
+	return sysfs_emit(buf, "%d\n", !!(regval & mask));
 }
 
 static ssize_t temp2_fault_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	if (data->stat1 & AMC6821_STAT1_RTF)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
 }
 
 static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", regval);
 }
 
 static ssize_t pwm1_store(struct device *dev,
@@ -355,24 +299,43 @@ static ssize_t pwm1_store(struct device *dev,
 			  size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	u8 val;
 	int ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		return ret;
 
-	mutex_lock(&data->update_lock);
-	data->pwm1 = val;
-	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
+	if (ret)
+		return ret;
+
 	return count;
 }
 
 static ssize_t pwm1_enable_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_enable);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	int err;
+	u32 val;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+	if (err)
+		return err;
+	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+	case 0:
+		val = 1;	/* manual */
+		break;
+	case AMC6821_CONF1_FDRC0:
+		val = 4;	/* target rpm (fan1_target) controlled */
+		break;
+	case AMC6821_CONF1_FDRC1:
+		val = 2;	/* remote temp controlled */
+		break;
+	default:
+		val = 3;	/* max(local, remote) temp controlled */
+		break;
+	}
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t pwm1_enable_store(struct device *dev,
@@ -380,49 +343,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
 				 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	long val;
-	int config = kstrtol(buf, 10, &val);
-	if (config)
-		return config;
+	u32 mode;
+	int err;
 
-	mutex_lock(&data->update_lock);
-	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-	if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-			count = config;
-			goto unlock;
-	}
+	err = kstrtol(buf, 10, &val);
+	if (err)
+		return err;
 
 	switch (val) {
 	case 1:
-		config &= ~AMC6821_CONF1_FDRC0;
-		config &= ~AMC6821_CONF1_FDRC1;
+		mode = 0;
 		break;
 	case 2:
-		config &= ~AMC6821_CONF1_FDRC0;
-		config |= AMC6821_CONF1_FDRC1;
+		mode = AMC6821_CONF1_FDRC1;
 		break;
 	case 3:
-		config |= AMC6821_CONF1_FDRC0;
-		config |= AMC6821_CONF1_FDRC1;
+		mode = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
 		break;
 	case 4:
-		config |= AMC6821_CONF1_FDRC0;
-		config &= ~AMC6821_CONF1_FDRC1;
+		mode = AMC6821_CONF1_FDRC0;
 		break;
 	default:
-		count = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF1, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			count = -EIO;
-	}
-unlock:
-	mutex_unlock(&data->update_lock);
+
+	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
+				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
+				 mode);
+	if (err)
+		return err;
+
 	return count;
 }
 
@@ -430,26 +381,45 @@ static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
 					    struct device_attribute *devattr,
 					    char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_auto_channels_temp);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 val;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
+	if (err)
+		return err;
+	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+	case 0:
+	case AMC6821_CONF1_FDRC0:
+		val = 0;	/* manual or target rpm controlled */
+		break;
+	case AMC6821_CONF1_FDRC1:
+		val = 2;	/* remote temp controlled */
+		break;
+	default:
+		val = 3;	/* max(local, remote) temp controlled */
+		break;
+	}
+
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t temp_auto_point_temp_show(struct device *dev,
 					 struct device_attribute *devattr,
 					 char *buf)
 {
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr_2(devattr)->index;
 	int nr = to_sensor_dev_attr_2(devattr)->nr;
-	struct amc6821_data *data = amc6821_update_device(dev);
+
 	switch (nr) {
 	case 1:
-		return sprintf(buf, "%d\n",
-			data->temp1_auto_point_temp[ix] * 1000);
+		return sysfs_emit(buf, "%d\n",
+				  data->temp1_auto_point_temp[ix] * 1000);
 	case 2:
-		return sprintf(buf, "%d\n",
-			data->temp2_auto_point_temp[ix] * 1000);
+		return sysfs_emit(buf, "%d\n",
+				  data->temp2_auto_point_temp[ix] * 1000);
 	default:
-		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
 		return -EINVAL;
 	}
 }
@@ -458,44 +428,59 @@ static ssize_t pwm1_auto_point_pwm_show(struct device *dev,
 					struct device_attribute *devattr,
 					char *buf)
 {
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm1_auto_point_pwm[ix]);
+	u32 val;
+	int err;
+
+	switch (ix) {
+	case 0:
+		val = 0;
+		break;
+	case 1:
+		err = regmap_read(data->regmap, AMC6821_REG_DCY_LOW_TEMP, &val);
+		if (err)
+			return err;
+		break;
+	default:
+		val = 255;
+		break;
+	}
+	return sysfs_emit(buf, "%d\n", val);
 }
 
-static inline ssize_t set_slope_register(struct i2c_client *client,
-		u8 reg,
-		u8 dpwm,
-		u8 *ptemp)
+static inline int set_slope_register(struct regmap *regmap,
+				     u8 reg, u8 *ptemp)
 {
-	int dt;
-	u8 tmp;
+	u8 tmp, dpwm;
+	int err, dt;
+	u32 pwm;
 
-	dt = ptemp[2]-ptemp[1];
+	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+	if (err)
+		return err;
+
+	dpwm = 255 - pwm;
+
+	dt = ptemp[2] - ptemp[1];
 	for (tmp = 4; tmp > 0; tmp--) {
 		if (dt * (0x20 >> tmp) >= dpwm)
 			break;
 	}
 	tmp |= (ptemp[1] & 0x7C) << 1;
-	if (i2c_smbus_write_byte_data(client,
-			reg, tmp)) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		return -EIO;
-	}
-	return 0;
+	return regmap_write(regmap, reg, tmp);
 }
 
 static ssize_t temp_auto_point_temp_store(struct device *dev,
 					  struct device_attribute *attr,
 					  const char *buf, size_t count)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	struct i2c_client *client = data->client;
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr_2(attr)->index;
 	int nr = to_sensor_dev_attr_2(attr)->nr;
+	struct regmap *regmap = data->regmap;
 	u8 *ptemp;
 	u8 reg;
-	int dpwm;
 	long val;
 	int ret = kstrtol(buf, 10, &val);
 	if (ret)
@@ -511,12 +496,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 		reg = AMC6821_REG_RTEMP_FAN_CTRL;
 		break;
 	default:
-		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
 		return -EINVAL;
 	}
 
 	mutex_lock(&data->update_lock);
-	data->valid = false;
 
 	switch (ix) {
 	case 0:
@@ -525,13 +508,9 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 		ptemp[0] = clamp_val(ptemp[0], 0,
 				     data->temp2_auto_point_temp[1]);
 		ptemp[0] = clamp_val(ptemp[0], 0, 63);
-		if (i2c_smbus_write_byte_data(
-					client,
-					AMC6821_REG_PSV_TEMP,
-					ptemp[0])) {
-				dev_err(&client->dev,
-					"Register write error, aborting.\n");
-				count = -EIO;
+		if (regmap_write(regmap, AMC6821_REG_PSV_TEMP, ptemp[0])) {
+			dev_err(dev, "Register write error, aborting.\n");
+			count = -EIO;
 		}
 		goto EXIT;
 	case 1:
@@ -543,12 +522,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
 		ptemp[2] = clamp_val(val / 1000, ptemp[1]+1, 255);
 		break;
 	default:
-		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
 		count = -EINVAL;
 		goto EXIT;
 	}
-	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
-	if (set_slope_register(client, reg, dpwm, ptemp))
+	if (set_slope_register(regmap, reg, ptemp))
 		count = -EIO;
 
 EXIT:
@@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 					 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int dpwm;
+	struct regmap *regmap = data->regmap;
 	u8 val;
-	int ret = kstrtou8(buf, 10, &val);
+	int ret;
+
+	ret = kstrtou8(buf, 10, &val);
 	if (ret)
 		return ret;
 
@@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 		return -EINVAL;
 
 	mutex_lock(&data->update_lock);
-	data->pwm1_auto_point_pwm[1] = val;
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
-			data->pwm1_auto_point_pwm[1])) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-		goto EXIT;
+	ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
+	if (ret)
+		goto unlock;
+
+	ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
+				 data->temp1_auto_point_temp);
+	if (ret) {
+		count = ret;
+		goto unlock;
 	}
-	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
-	if (set_slope_register(client, AMC6821_REG_LTEMP_FAN_CTRL, dpwm,
-			data->temp1_auto_point_temp)) {
-		count = -EIO;
-		goto EXIT;
-	}
-	if (set_slope_register(client, AMC6821_REG_RTEMP_FAN_CTRL, dpwm,
-			data->temp2_auto_point_temp)) {
-		count = -EIO;
-		goto EXIT;
+	ret = set_slope_register(regmap, AMC6821_REG_RTEMP_FAN_CTRL,
+				 data->temp2_auto_point_temp);
+	if (ret) {
+		count = ret;
+		goto unlock;
 	}
 
-EXIT:
-	data->valid = false;
+unlock:
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -600,63 +575,76 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
 			char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
+	struct amc6821_data *data = dev_get_drvdata(dev);
 	int ix = to_sensor_dev_attr(devattr)->index;
-	if (0 == data->fan[ix])
-		return sprintf(buf, "0");
-	return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
+	u32 regval;
+	u8 regs[2];
+	int err;
+
+	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
+	if (err)
+		return err;
+	regval = (regs[1] << 8) | regs[0];
+
+	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
 }
 
 static ssize_t fan1_fault_show(struct device *dev,
 			       struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	if (data->stat1 & AMC6821_STAT1_FANS)
-		return sprintf(buf, "1");
-	else
-		return sprintf(buf, "0");
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
 }
 
 static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
 			 const char *buf, size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	unsigned long val;
 	int ix = to_sensor_dev_attr(attr)->index;
-	int ret = kstrtoul(buf, 10, &val);
-	if (ret)
-		return ret;
+	unsigned long val;
+	u8 regs[2];
+	int err;
+
+	err = kstrtoul(buf, 10, &val);
+	if (err)
+		return err;
 
 	/* Minimum and target fan speed must not be unlimited (0) */
 	if ((ix == IDX_FAN1_MIN || ix == IDX_FAN1_TARGET) && !val)
 		return -EINVAL;
 
 	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
+	val = clamp_val(val, 0, 0xFFFF);
+
+	regs[0] = val & 0xff;
+	regs[1] = val >> 8;
+
+	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
+	if (err)
+		return err;
 
-	mutex_lock(&data->update_lock);
-	data->fan[ix] = clamp_val(val, 0, 0xFFFF);
-	if (i2c_smbus_write_byte_data(client, fan_reg_low[ix],
-			data->fan[ix] & 0xFF)) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-		goto EXIT;
-	}
-	if (i2c_smbus_write_byte_data(client,
-			fan_reg_hi[ix], data->fan[ix] >> 8)) {
-		dev_err(&client->dev, "Register write error, aborting.\n");
-		count = -EIO;
-	}
-EXIT:
-	mutex_unlock(&data->update_lock);
 	return count;
 }
 
 static ssize_t fan1_pulses_show(struct device *dev,
 				struct device_attribute *devattr, char *buf)
 {
-	struct amc6821_data *data = amc6821_update_device(dev);
-	return sprintf(buf, "%d\n", data->fan1_pulses);
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int err;
+
+	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
+	if (err)
+		return err;
+
+	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
 }
 
 static ssize_t fan1_pulses_store(struct device *dev,
@@ -664,40 +652,22 @@ static ssize_t fan1_pulses_store(struct device *dev,
 				 size_t count)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	long val;
-	int config = kstrtol(buf, 10, &val);
-	if (config)
-		return config;
+	int err;
+
+	err = kstrtol(buf, 10, &val);
+	if (err)
+		return err;
+
+	if (val != 2 && val != 4)
+		return -EINVAL;
+
+	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
+				 AMC6821_CONF4_PSPR,
+				 val == 4 ? AMC6821_CONF4_PSPR : 0);
+	if (err)
+		return err;
 
-	mutex_lock(&data->update_lock);
-	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
-	if (config < 0) {
-		dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-		count = config;
-		goto EXIT;
-	}
-	switch (val) {
-	case 2:
-		config &= ~AMC6821_CONF4_PSPR;
-		data->fan1_pulses = 2;
-		break;
-	case 4:
-		config |= AMC6821_CONF4_PSPR;
-		data->fan1_pulses = 4;
-		break;
-	default:
-		count = -EINVAL;
-		goto EXIT;
-	}
-	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4, config)) {
-		dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-		count = -EIO;
-	}
-EXIT:
-	mutex_unlock(&data->update_lock);
 	return count;
 }
 
@@ -829,110 +799,83 @@ static int amc6821_detect(
 	return 0;
 }
 
-static int amc6821_init_client(struct i2c_client *client)
+static int amc6821_init_client(struct amc6821_data *data)
 {
-	int config;
-	int err = -EIO;
+	struct regmap *regmap = data->regmap;
+	int err;
+
+	err = amc6821_init_auto_point_data(data);
+	if (err)
+		return err;
 
 	if (init) {
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF4);
-
-		if (config < 0) {
-				dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-				return err;
-		}
-
-		config |= AMC6821_CONF4_MODE;
-
-		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF4,
-				config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
+		err = regmap_set_bits(regmap, AMC6821_REG_CONF4, AMC6821_CONF4_MODE);
+		if (err)
 			return err;
-		}
-
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF3);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
+		err = regmap_clear_bits(regmap, AMC6821_REG_CONF3, AMC6821_CONF3_THERM_FAN_EN);
+		if (err)
 			return err;
-		}
-
-		dev_info(&client->dev, "Revision %d\n", config & 0x0f);
-
-		config &= ~AMC6821_CONF3_THERM_FAN_EN;
-
-		if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF3,
-				config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
+		err = regmap_clear_bits(regmap, AMC6821_REG_CONF2,
+					AMC6821_CONF2_RTFIE |
+					AMC6821_CONF2_LTOIE |
+					AMC6821_CONF2_RTOIE);
+		if (err)
 			return err;
-		}
 
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF2);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
+		err = regmap_update_bits(regmap, AMC6821_REG_CONF1,
+					 AMC6821_CONF1_THERMOVIE | AMC6821_CONF1_FANIE |
+					 AMC6821_CONF1_START | AMC6821_CONF1_PWMINV,
+					 AMC6821_CONF1_START |
+					 (pwminv ? AMC6821_CONF1_PWMINV : 0));
+		if (err)
 			return err;
-		}
-
-		config &= ~AMC6821_CONF2_RTFIE;
-		config &= ~AMC6821_CONF2_LTOIE;
-		config &= ~AMC6821_CONF2_RTOIE;
-		if (i2c_smbus_write_byte_data(client,
-				AMC6821_REG_CONF2, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			return err;
-		}
-
-		config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
-
-		if (config < 0) {
-			dev_err(&client->dev,
-			"Error reading configuration register, aborting.\n");
-			return err;
-		}
-
-		config &= ~AMC6821_CONF1_THERMOVIE;
-		config &= ~AMC6821_CONF1_FANIE;
-		config |= AMC6821_CONF1_START;
-		if (pwminv)
-			config |= AMC6821_CONF1_PWMINV;
-		else
-			config &= ~AMC6821_CONF1_PWMINV;
-
-		if (i2c_smbus_write_byte_data(
-				client, AMC6821_REG_CONF1, config)) {
-			dev_err(&client->dev,
-			"Configuration register write error, aborting.\n");
-			return err;
-		}
 	}
 	return 0;
 }
 
+static bool amc6821_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case AMC6821_REG_STAT1:
+	case AMC6821_REG_STAT2:
+	case AMC6821_REG_TEMP_LO:
+	case AMC6821_REG_TDATA_LOW:
+	case AMC6821_REG_LTEMP_HI:
+	case AMC6821_REG_RTEMP_HI:
+	case AMC6821_REG_TDATA_HI:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config amc6821_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AMC6821_REG_CONF3,
+	.volatile_reg = amc6821_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
 static int amc6821_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct amc6821_data *data;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
 	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct amc6821_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
-	mutex_init(&data->update_lock);
+	regmap = devm_regmap_init_i2c(client, &amc6821_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize regmap\n");
+	data->regmap = regmap;
 
-	/*
-	 * Initialize the amc6821 chip
-	 */
-	err = amc6821_init_client(client);
+	err = amc6821_init_client(data);
 	if (err)
 		return err;
 
-- 
2.39.2

[PATCH v2 10/11] hwmon: (amc6821) Convert to with_info API

[Guenter Roeck]

Convert to use with_info API to simplify the code and make it easier
to maintain. This also reduces code size by approximately 20%.

No functional change intended.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Adjust to changes made in preceding patches

 drivers/hwmon/amc6821.c | 744 +++++++++++++++++++++-------------------
 1 file changed, 390 insertions(+), 354 deletions(-)

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 3fe0bfeac843..5a3c089ae06f 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -6,6 +6,9 @@
  *
  * Based on max6650.c:
  * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
+ *
+ * Conversion to regmap and with_info API:
+ * Copyright (C) 2024 Guenter Roeck <linux@roeck-us.net>
  */
 
 #include <linux/bitops.h>
@@ -107,28 +110,6 @@ module_param(init, int, 0444);
 #define AMC6821_STAT2_L_THERM		BIT(6)
 #define AMC6821_STAT2_THERM_IN		BIT(7)
 
-enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
-	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
-	IDX_TEMP2_MAX, IDX_TEMP2_CRIT,
-	TEMP_IDX_LEN, };
-
-static const u8 temp_reg[] = {AMC6821_REG_LTEMP_HI,
-			AMC6821_REG_LTEMP_LIMIT_MIN,
-			AMC6821_REG_LTEMP_LIMIT_MAX,
-			AMC6821_REG_LTEMP_CRIT,
-			AMC6821_REG_RTEMP_HI,
-			AMC6821_REG_RTEMP_LIMIT_MIN,
-			AMC6821_REG_RTEMP_LIMIT_MAX,
-			AMC6821_REG_RTEMP_CRIT, };
-
-enum {IDX_FAN1_INPUT = 0, IDX_FAN1_MIN, IDX_FAN1_MAX, IDX_FAN1_TARGET,
-	FAN1_IDX_LEN, };
-
-static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
-			AMC6821_REG_TACH_LLIMITL,
-			AMC6821_REG_TACH_HLIMITL,
-			AMC6821_REG_TACH_SETTINGL, };
-
 /*
  * Client data (each client gets its own)
  */
@@ -189,219 +170,319 @@ static int amc6821_init_auto_point_data(struct amc6821_data *data)
 	return 0;
 }
 
-static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
+static int amc6821_temp_read_values(struct regmap *regmap, u32 attr, int channel, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
+	int reg, err;
 	u32 regval;
-	int err;
 
-	err = regmap_read(data->regmap, temp_reg[ix], &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", sign_extend32(regval, 7) * 1000);
-}
-
-static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(attr)->index;
-	long val;
-	int err;
-
-	int ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-	val = clamp_val(val / 1000, -128, 127);
-
-	err = regmap_write(data->regmap, temp_reg[ix], val);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static ssize_t temp_alarm_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
-	u32 regval, mask, reg;
-	int err;
-
-	switch (ix) {
-	case IDX_TEMP1_MIN:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_LTL;
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = channel ? AMC6821_REG_RTEMP_HI : AMC6821_REG_LTEMP_HI;
 		break;
-	case IDX_TEMP1_MAX:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_LTH;
+	case hwmon_temp_min:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
 		break;
-	case IDX_TEMP1_CRIT:
-		reg = AMC6821_REG_STAT2;
-		mask = AMC6821_STAT2_LTC;
+	case hwmon_temp_max:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
 		break;
-	case IDX_TEMP2_MIN:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_RTL;
-		break;
-	case IDX_TEMP2_MAX:
-		reg = AMC6821_REG_STAT1;
-		mask = AMC6821_STAT1_RTH;
-		break;
-	case IDX_TEMP2_CRIT:
-		reg = AMC6821_REG_STAT2;
-		mask = AMC6821_STAT2_RTC;
+	case hwmon_temp_crit:
+		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
 		break;
 	default:
-		return -EINVAL;
+		return -EOPNOTSUPP;
 	}
-	err = regmap_read(data->regmap, reg, &regval);
+	err = regmap_read(regmap, reg, &regval);
 	if (err)
 		return err;
-	return sysfs_emit(buf, "%d\n", !!(regval & mask));
+	*val = sign_extend32(regval, 7) * 1000;
+	return 0;
 }
 
-static ssize_t temp2_fault_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
+static int amc6821_read_alarms(struct regmap *regmap, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
+	int reg, mask, err;
 	u32 regval;
-	int err;
 
-	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
-}
-
-static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", regval);
-}
-
-static ssize_t pwm1_store(struct device *dev,
-			  struct device_attribute *devattr, const char *buf,
-			  size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u8 val;
-	int ret = kstrtou8(buf, 10, &val);
-	if (ret)
-		return ret;
-
-	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
-	if (ret)
-		return ret;
-
-	return count;
-}
-
-static ssize_t pwm1_enable_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int err;
-	u32 val;
-
-	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
-	if (err)
-		return err;
-	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
-	case 0:
-		val = 1;	/* manual */
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_min_alarm:
+			reg = AMC6821_REG_STAT1;
+			mask = channel ? AMC6821_STAT1_RTL : AMC6821_STAT1_LTL;
+			break;
+		case hwmon_temp_max_alarm:
+			reg = AMC6821_REG_STAT1;
+			mask = channel ? AMC6821_STAT1_RTH : AMC6821_STAT1_LTH;
+			break;
+		case hwmon_temp_crit_alarm:
+			reg = AMC6821_REG_STAT2;
+			mask = channel ? AMC6821_STAT2_RTC : AMC6821_STAT2_LTC;
+			break;
+		case hwmon_temp_fault:
+			reg = AMC6821_REG_STAT1;
+			mask = AMC6821_STAT1_RTF;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
 		break;
-	case AMC6821_CONF1_FDRC0:
-		val = 4;	/* target rpm (fan1_target) controlled */
-		break;
-	case AMC6821_CONF1_FDRC1:
-		val = 2;	/* remote temp controlled */
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_fault:
+			reg = AMC6821_REG_STAT1;
+			mask = AMC6821_STAT1_FANS;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
 		break;
 	default:
-		val = 3;	/* max(local, remote) temp controlled */
-		break;
+		return -EOPNOTSUPP;
 	}
-	return sysfs_emit(buf, "%d\n", val);
+	err = regmap_read(regmap, reg, &regval);
+	if (err)
+		return err;
+	*val = !!(regval & mask);
+	return 0;
 }
 
-static ssize_t pwm1_enable_store(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
+static int amc6821_temp_read(struct device *dev, u32 attr, int channel, long *val)
 {
 	struct amc6821_data *data = dev_get_drvdata(dev);
-	long val;
+
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+	case hwmon_temp_crit:
+		return amc6821_temp_read_values(data->regmap, attr, channel, val);
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+	case hwmon_temp_crit_alarm:
+	case hwmon_temp_fault:
+		return amc6821_read_alarms(data->regmap, hwmon_temp, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_temp_write(struct device *dev, u32 attr, int channel, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	int reg;
+
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -128000, 127000), 1000);
+
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MIN : AMC6821_REG_LTEMP_LIMIT_MIN;
+		break;
+	case hwmon_temp_max:
+		reg = channel ? AMC6821_REG_RTEMP_LIMIT_MAX : AMC6821_REG_LTEMP_LIMIT_MAX;
+		break;
+	case hwmon_temp_crit:
+		reg = channel ? AMC6821_REG_RTEMP_CRIT : AMC6821_REG_LTEMP_CRIT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return regmap_write(data->regmap, reg, val);
+}
+
+static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_pwm_enable:
+		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
+		if (err)
+			return err;
+		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+		case 0:
+			*val = 1;	/* manual */
+			break;
+		case AMC6821_CONF1_FDRC0:
+			*val = 4;	/* target rpm (fan1_target) controlled */
+			break;
+		case AMC6821_CONF1_FDRC1:
+			*val = 2;	/* remote temp controlled */
+			break;
+		default:
+			*val = 3;	/* max(local, remote) temp controlled */
+			break;
+		}
+		return 0;
+	case hwmon_pwm_auto_channels_temp:
+		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
+		if (err)
+			return err;
+		switch (regval & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
+		case 0:
+		case AMC6821_CONF1_FDRC0:
+			*val = 0;	/* manual or target rpm controlled */
+			break;
+		case AMC6821_CONF1_FDRC1:
+			*val = 2;	/* remote temp controlled */
+			break;
+		default:
+			*val = 3;	/* max(local, remote) temp controlled */
+			break;
+		}
+		return 0;
+	case hwmon_pwm_input:
+		err = regmap_read(regmap, AMC6821_REG_DCY, &regval);
+		if (err)
+			return err;
+		*val = regval;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
 	u32 mode;
-	int err;
 
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	switch (val) {
-	case 1:
-		mode = 0;
-		break;
-	case 2:
-		mode = AMC6821_CONF1_FDRC1;
-		break;
-	case 3:
-		mode = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
-		break;
-	case 4:
-		mode = AMC6821_CONF1_FDRC0;
-		break;
+	switch (attr) {
+	case hwmon_pwm_enable:
+		switch (val) {
+		case 1:
+			mode = 0;
+			break;
+		case 2:
+			mode = AMC6821_CONF1_FDRC1;
+			break;
+		case 3:
+			mode = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
+			break;
+		case 4:
+			mode = AMC6821_CONF1_FDRC0;
+			break;
+		default:
+			return -EINVAL;
+		}
+		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
+					  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
+					  mode);
+	case hwmon_pwm_input:
+		if (val < 0 || val > 255)
+			return -EINVAL;
+		return regmap_write(regmap, AMC6821_REG_DCY, val);
 	default:
-		return -EINVAL;
+		return -EOPNOTSUPP;
 	}
-
-	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
-				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
-				 mode);
-	if (err)
-		return err;
-
-	return count;
 }
 
-static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
-					    struct device_attribute *devattr,
-					    char *buf)
+static int amc6821_fan_read_rpm(struct regmap *regmap, u32 attr, long *val)
 {
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 val;
-	int err;
+	int reg, err;
+	u8 regs[2];
+	u32 regval;
 
-	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
-	if (err)
-		return err;
-	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
-	case 0:
-	case AMC6821_CONF1_FDRC0:
-		val = 0;	/* manual or target rpm controlled */
+	switch (attr) {
+	case hwmon_fan_input:
+		reg = AMC6821_REG_TDATA_LOW;
 		break;
-	case AMC6821_CONF1_FDRC1:
-		val = 2;	/* remote temp controlled */
+	case hwmon_fan_min:
+		reg = AMC6821_REG_TACH_LLIMITL;
+		break;
+	case hwmon_fan_max:
+		reg = AMC6821_REG_TACH_HLIMITL;
+		break;
+	case hwmon_fan_target:
+		reg = AMC6821_REG_TACH_SETTINGL;
 		break;
 	default:
-		val = 3;	/* max(local, remote) temp controlled */
-		break;
+		return -EOPNOTSUPP;
 	}
 
-	return sysfs_emit(buf, "%d\n", val);
+	err = regmap_bulk_read(regmap, reg, regs, 2);
+	if (err)
+		return err;
+
+	regval = (regs[1] << 8) | regs[0];
+	*val = regval ? 6000000 / regval : 0;
+
+	return 0;
+}
+
+static int amc6821_fan_read(struct device *dev, u32 attr, long *val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval;
+	int err;
+
+	switch (attr) {
+	case hwmon_fan_input:
+	case hwmon_fan_min:
+	case hwmon_fan_max:
+	case hwmon_fan_target:
+		return amc6821_fan_read_rpm(regmap, attr, val);
+	case hwmon_fan_fault:
+		return amc6821_read_alarms(regmap, hwmon_fan, attr, 0, val);
+	case hwmon_fan_pulses:
+		err = regmap_read(regmap, AMC6821_REG_CONF4, &regval);
+		if (err)
+			return err;
+		*val = (regval & AMC6821_CONF4_PSPR) ? 4 : 2;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_fan_write(struct device *dev, u32 attr, long val)
+{
+	struct amc6821_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u8 regs[2];
+	int reg;
+
+	if (attr == hwmon_fan_pulses) {
+		if (val != 2 && val != 4)
+			return -EINVAL;
+		return regmap_update_bits(regmap, AMC6821_REG_CONF4,
+					 AMC6821_CONF4_PSPR,
+					 val == 4 ? AMC6821_CONF4_PSPR : 0);
+	}
+
+	if (val < 0)
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_fan_min:
+		if (!val)	/* no unlimited minimum speed */
+			return -EINVAL;
+		reg = AMC6821_REG_TACH_LLIMITL;
+		break;
+	case hwmon_fan_max:
+		reg = AMC6821_REG_TACH_HLIMITL;
+		break;
+	case hwmon_fan_target:
+		if (!val)	/* no unlimited target speed */
+			return -EINVAL;
+		reg = AMC6821_REG_TACH_SETTINGL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	val = val ? 6000000 / clamp_val(val, 1, 6000000) : 0;
+	val = clamp_val(val, 0, 0xffff);
+
+	regs[0] = val & 0xff;
+	regs[1] = val >> 8;
+
+	return regmap_bulk_write(data->regmap, reg, regs, 2);
 }
 
 static ssize_t temp_auto_point_temp_show(struct device *dev,
@@ -572,134 +653,9 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
 	return count;
 }
 
-static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
-			char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(devattr)->index;
-	u32 regval;
-	u8 regs[2];
-	int err;
-
-	err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
-	if (err)
-		return err;
-	regval = (regs[1] << 8) | regs[0];
-
-	return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
-}
-
-static ssize_t fan1_fault_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
-}
-
-static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
-			 const char *buf, size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	int ix = to_sensor_dev_attr(attr)->index;
-	unsigned long val;
-	u8 regs[2];
-	int err;
-
-	err = kstrtoul(buf, 10, &val);
-	if (err)
-		return err;
-
-	/* Minimum and target fan speed must not be unlimited (0) */
-	if ((ix == IDX_FAN1_MIN || ix == IDX_FAN1_TARGET) && !val)
-		return -EINVAL;
-
-	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
-	val = clamp_val(val, 0, 0xFFFF);
-
-	regs[0] = val & 0xff;
-	regs[1] = val >> 8;
-
-	err = regmap_bulk_write(data->regmap, fan_reg_low[ix], regs, 2);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static ssize_t fan1_pulses_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	u32 regval;
-	int err;
-
-	err = regmap_read(data->regmap, AMC6821_REG_CONF4, &regval);
-	if (err)
-		return err;
-
-	return sysfs_emit(buf, "%d\n", (regval & AMC6821_CONF4_PSPR) ? 4 : 2);
-}
-
-static ssize_t fan1_pulses_store(struct device *dev,
-				 struct device_attribute *attr, const char *buf,
-				 size_t count)
-{
-	struct amc6821_data *data = dev_get_drvdata(dev);
-	long val;
-	int err;
-
-	err = kstrtol(buf, 10, &val);
-	if (err)
-		return err;
-
-	if (val != 2 && val != 4)
-		return -EINVAL;
-
-	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF4,
-				 AMC6821_CONF4_PSPR,
-				 val == 4 ? AMC6821_CONF4_PSPR : 0);
-	if (err)
-		return err;
-
-	return count;
-}
-
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, IDX_TEMP1_INPUT);
-static SENSOR_DEVICE_ATTR_RW(temp1_min, temp, IDX_TEMP1_MIN);
-static SENSOR_DEVICE_ATTR_RW(temp1_max, temp, IDX_TEMP1_MAX);
-static SENSOR_DEVICE_ATTR_RW(temp1_crit, temp, IDX_TEMP1_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp1_min_alarm, temp_alarm, IDX_TEMP1_MIN);
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, temp_alarm, IDX_TEMP1_MAX);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, temp_alarm, IDX_TEMP1_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, IDX_TEMP2_INPUT);
-static SENSOR_DEVICE_ATTR_RW(temp2_min, temp, IDX_TEMP2_MIN);
-static SENSOR_DEVICE_ATTR_RW(temp2_max, temp, IDX_TEMP2_MAX);
-static SENSOR_DEVICE_ATTR_RW(temp2_crit, temp, IDX_TEMP2_CRIT);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp2_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_min_alarm, temp_alarm, IDX_TEMP2_MIN);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, temp_alarm, IDX_TEMP2_MAX);
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, temp_alarm, IDX_TEMP2_CRIT);
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, IDX_FAN1_INPUT);
-static SENSOR_DEVICE_ATTR_RW(fan1_min, fan, IDX_FAN1_MIN);
-static SENSOR_DEVICE_ATTR_RW(fan1_max, fan, IDX_FAN1_MAX);
-static SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET);
-static SENSOR_DEVICE_ATTR_RO(fan1_fault, fan1_fault, 0);
-static SENSOR_DEVICE_ATTR_RW(fan1_pulses, fan1_pulses, 0);
-
-static SENSOR_DEVICE_ATTR_RW(pwm1, pwm1, 0);
-static SENSOR_DEVICE_ATTR_RW(pwm1_enable, pwm1_enable, 0);
 static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point1_pwm, pwm1_auto_point_pwm, 0);
 static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_pwm, pwm1_auto_point_pwm, 1);
 static SENSOR_DEVICE_ATTR_RO(pwm1_auto_point3_pwm, pwm1_auto_point_pwm, 2);
-static SENSOR_DEVICE_ATTR_RO(pwm1_auto_channels_temp, pwm1_auto_channels_temp,
-			     0);
 static SENSOR_DEVICE_ATTR_2_RO(temp1_auto_point1_temp, temp_auto_point_temp,
 			       1, 0);
 static SENSOR_DEVICE_ATTR_2_RW(temp1_auto_point2_temp, temp_auto_point_temp,
@@ -715,30 +671,6 @@ static SENSOR_DEVICE_ATTR_2_RW(temp2_auto_point3_temp, temp_auto_point_temp,
 			       2, 2);
 
 static struct attribute *amc6821_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_min.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit.dev_attr.attr,
-	&sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_min.dev_attr.attr,
-	&sensor_dev_attr_temp2_max.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit.dev_attr.attr,
-	&sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	&sensor_dev_attr_fan1_min.dev_attr.attr,
-	&sensor_dev_attr_fan1_max.dev_attr.attr,
-	&sensor_dev_attr_fan1_target.dev_attr.attr,
-	&sensor_dev_attr_fan1_fault.dev_attr.attr,
-	&sensor_dev_attr_fan1_pulses.dev_attr.attr,
-	&sensor_dev_attr_pwm1.dev_attr.attr,
-	&sensor_dev_attr_pwm1_enable.dev_attr.attr,
-	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point3_pwm.dev_attr.attr,
@@ -750,13 +682,117 @@ static struct attribute *amc6821_attrs[] = {
 	&sensor_dev_attr_temp2_auto_point3_temp.dev_attr.attr,
 	NULL
 };
-
 ATTRIBUTE_GROUPS(amc6821);
 
+static int amc6821_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return amc6821_temp_read(dev, attr, channel, val);
+	case hwmon_fan:
+		return amc6821_fan_read(dev, attr, val);
+	case hwmon_pwm:
+		return amc6821_pwm_read(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int amc6821_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return amc6821_temp_write(dev, attr, channel, val);
+	case hwmon_fan:
+		return amc6821_fan_write(dev, attr, val);
+	case hwmon_pwm:
+		return amc6821_pwm_write(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t amc6821_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+		case hwmon_temp_crit_alarm:
+		case hwmon_temp_fault:
+			return 0444;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+		case hwmon_temp_crit:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_fault:
+			return 0444;
+		case hwmon_fan_pulses:
+		case hwmon_fan_min:
+		case hwmon_fan_max:
+		case hwmon_fan_target:
+			return 0644;
+		default:
+			return 0;
+		}
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_enable:
+		case hwmon_pwm_input:
+			return 0644;
+		case hwmon_pwm_auto_channels_temp:
+			return 0444;
+		default:
+			return 0;
+		}
+	default:
+		return 0;
+	}
+}
+
+static const struct hwmon_channel_info * const amc6821_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX |
+			   HWMON_T_CRIT | HWMON_T_MIN_ALARM |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX |
+			   HWMON_F_TARGET | HWMON_F_PULSES | HWMON_F_FAULT),
+	HWMON_CHANNEL_INFO(pwm,
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE |
+			   HWMON_PWM_AUTO_CHANNELS_TEMP),
+	NULL
+};
+
+static const struct hwmon_ops amc6821_hwmon_ops = {
+	.is_visible = amc6821_is_visible,
+	.read = amc6821_read,
+	.write = amc6821_write,
+};
+
+static const struct hwmon_chip_info amc6821_chip_info = {
+	.ops = &amc6821_hwmon_ops,
+	.info = amc6821_info,
+};
+
 /* Return 0 if detection is successful, -ENODEV otherwise */
-static int amc6821_detect(
-		struct i2c_client *client,
-		struct i2c_board_info *info)
+static int amc6821_detect(struct i2c_client *client, struct i2c_board_info *info)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	int address = client->addr;
@@ -879,9 +915,9 @@ static int amc6821_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data,
-							   amc6821_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data, &amc6821_chip_info,
+							 amc6821_groups);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.2

[PATCH v2 11/11] hwmon: (amc6821) Add support for pwm1_mode attribute

[Guenter Roeck]

AMC6821 supports configuring if a fan is DC or PWM controlled.
Add support for the pwm1_mode attribute to make it runtime configurable.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: New patch

 Documentation/hwmon/amc6821.rst |  1 +
 drivers/hwmon/amc6821.c         | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
index 96e604c5ea8e..dbd544cd1160 100644
--- a/Documentation/hwmon/amc6821.rst
+++ b/Documentation/hwmon/amc6821.rst
@@ -58,6 +58,7 @@ pwm1_enable		rw	regulator mode, 1=open loop, 2=fan controlled
 				remote-sensor temperature,
 				4=fan controlled by target rpm set with
 				fan1_target attribute.
+pwm1_mode		rw	Fan duty control mode (0=DC, 1=PWM)
 pwm1_auto_channels_temp ro	1 if pwm_enable==2, 3 if pwm_enable==3
 pwm1_auto_point1_pwm	ro	Hardwired to 0, shared for both
 				temperature channels.
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index 5a3c089ae06f..98a45fe529e0 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -317,6 +317,12 @@ static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
 			break;
 		}
 		return 0;
+	case hwmon_pwm_mode:
+		err = regmap_read(regmap, AMC6821_REG_CONF2, &regval);
+		if (err)
+			return err;
+		*val = !!(regval & AMC6821_CONF2_TACH_MODE);
+		return 0;
 	case hwmon_pwm_auto_channels_temp:
 		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
 		if (err)
@@ -372,6 +378,13 @@ static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
 		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
 					  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
 					  mode);
+	case hwmon_pwm_mode:
+		if (val < 0 || val > 1)
+			return -EINVAL;
+		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
+					  AMC6821_CONF2_TACH_MODE,
+					  val ? AMC6821_CONF2_TACH_MODE : 0);
+		break;
 	case hwmon_pwm_input:
 		if (val < 0 || val > 255)
 			return -EINVAL;
@@ -749,6 +762,7 @@ static umode_t amc6821_is_visible(const void *data,
 		}
 	case hwmon_pwm:
 		switch (attr) {
+		case hwmon_pwm_mode:
 		case hwmon_pwm_enable:
 		case hwmon_pwm_input:
 			return 0644;
@@ -775,7 +789,7 @@ static const struct hwmon_channel_info * const amc6821_info[] = {
 			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_MAX |
 			   HWMON_F_TARGET | HWMON_F_PULSES | HWMON_F_FAULT),
 	HWMON_CHANNEL_INFO(pwm,
-			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE |
+			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE | HWMON_PWM_MODE |
 			   HWMON_PWM_AUTO_CHANNELS_TEMP),
 	NULL
 };
-- 
2.39.2

Re: [PATCH v2 01/11] hwmon: (amc6821) Stop accepting invalid pwm values

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> The pwm value range is well defined from 0..255. Don't accept
> any values outside this range.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Use kstrtou8() instead of kstrtol() where possible.
>      Limit range of pwm1_auto_point_pwm to 0..254 in patch 1
>      instead of limiting it later, and do not accept invalid
>      values for the attribute.
> 
>   drivers/hwmon/amc6821.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 9b02b304c2f5..eb2d5592a41a 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev,
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	long val;
> -	int ret = kstrtol(buf, 10, &val);
> +	u8 val;
> +	int ret = kstrtou8(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   
>   	mutex_lock(&data->update_lock);
> -	data->pwm1 = clamp_val(val , 0, 255);
> +	data->pwm1 = val;
>   	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
>   	mutex_unlock(&data->update_lock);
>   	return count;
> @@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   	struct amc6821_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
>   	int dpwm;
> -	long val;
> -	int ret = kstrtol(buf, 10, &val);
> +	u8 val;
> +	int ret = kstrtou8(buf, 10, &val);
>   	if (ret)
>   		return ret;
>  
> +	if (val > 254)

Would have appreciated a comment as to why it's 254. My understanding is 
that the subsystem requires no overlap between multiple pwm_auto_points? 
0 being 0 and 2 being 255, we need 1 to be 255?

Actually, that doesn't explain why we allow 0 here, so maybe I'm just 
clueless :)

The change itself though:
Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> The default value of the maximum fan speed limit register is 0,
> essentially translating to an unlimited fan speed. When reading
> the limit, a value of 0 is reported in this case. However, writing
> a value of 0 results in writing a value of 0xffff into the register,
> which is inconsistent.
> 
> To solve the problem, permit writing a limit of 0 for the maximim fan
> speed, effectively translating to "no limit". Write 0 into the register
> if a limit value of 0 is written. Otherwise limit the range to
> <1..6000000> and write 1..0xffff into the register. This ensures that
> reading and writing from and to a limit register return the same value
> while at the same time not changing reported values when reading the
> speed or limits.
> 
> While at it, restrict fan limit writes to non-negative numbers; writing
> a negative limit does not make sense and should be reported instead of
> being corrected.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Do not accept negative fan speed values
>      Display fan speed and speed limit as 0 if register value is 0
>      (instead of 6000000), as in original code.
>      Only permit writing 0 (unlimited) for the maximum fan speed.
> 
>   drivers/hwmon/amc6821.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index eb2d5592a41a..9c19d4d278ec 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	long val;
> +	unsigned long val;
>   	int ix = to_sensor_dev_attr(attr)->index;
> -	int ret = kstrtol(buf, 10, &val);
> +	int ret = kstrtoul(buf, 10, &val);
>   	if (ret)
>   		return ret;
> -	val = 1 > val ? 0xFFFF : 6000000/val;
> +
> +	/* The minimum fan speed must not be unlimited (0) */
> +	if (ix == IDX_FAN1_MIN && !val)
> +		return -EINVAL;
> +
> +	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
>   

I'm wondering if we shouldn't check !val for min after this line 
instead? Otherwise we allow 6000001+RPM speeds... which is technically 
unlimited.

Nitpicking though, therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 04/11] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> After setting fan1_target and setting pwm1_enable to 4,
> the fan controller tries to achieve the requested fan speed.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 07/11] hwmon: (amc2821) Use BIT() and GENMASK()

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> Use BIT() and GENMASK() for bit and mask definitions
> to help distinguish bit and mask definitions from other
> defines and to make the code easier to read.
> 
> No functional change intended.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 11/11] hwmon: (amc6821) Add support for pwm1_mode attribute

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> AMC6821 supports configuring if a fan is DC or PWM controlled.
> Add support for the pwm1_mode attribute to make it runtime configurable.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: New patch
> 
>   Documentation/hwmon/amc6821.rst |  1 +
>   drivers/hwmon/amc6821.c         | 16 +++++++++++++++-
>   2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
> index 96e604c5ea8e..dbd544cd1160 100644
> --- a/Documentation/hwmon/amc6821.rst
> +++ b/Documentation/hwmon/amc6821.rst
> @@ -58,6 +58,7 @@ pwm1_enable		rw	regulator mode, 1=open loop, 2=fan controlled
>   				remote-sensor temperature,
>   				4=fan controlled by target rpm set with
>   				fan1_target attribute.
> +pwm1_mode		rw	Fan duty control mode (0=DC, 1=PWM)
>   pwm1_auto_channels_temp ro	1 if pwm_enable==2, 3 if pwm_enable==3
>   pwm1_auto_point1_pwm	ro	Hardwired to 0, shared for both
>   				temperature channels.
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 5a3c089ae06f..98a45fe529e0 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -317,6 +317,12 @@ static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
>   			break;
>   		}
>   		return 0;
> +	case hwmon_pwm_mode:
> +		err = regmap_read(regmap, AMC6821_REG_CONF2, &regval);
> +		if (err)
> +			return err;
> +		*val = !!(regval & AMC6821_CONF2_TACH_MODE);
> +		return 0;
>   	case hwmon_pwm_auto_channels_temp:
>   		err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
>   		if (err)
> @@ -372,6 +378,13 @@ static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
>   		return regmap_update_bits(regmap, AMC6821_REG_CONF1,
>   					  AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
>   					  mode);
> +	case hwmon_pwm_mode:
> +		if (val < 0 || val > 1)
> +			return -EINVAL;
> +		return regmap_update_bits(regmap, AMC6821_REG_CONF1,

Wrong register here, should be AMC6821_REG_CONF2.

Otherwise, looks good to me.

Cheers,
Quentin

Re: [PATCH v2 10/11] hwmon: (amc6821) Convert to with_info API

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> Convert to use with_info API to simplify the code and make it easier
> to maintain. This also reduces code size by approximately 20%.
> 
> No functional change intended.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Looks good to me,
Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH v2 09/11] hwmon: (amc6821) Convert to use regmap

[Quentin Schulz]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> Use regmap for register accesses and for most caching.
> 
> While at it, use sysfs_emit() instead of sprintf() to write sysfs
> attribute data, and remove spurious debug messages which would
> only be seen as result of a bug in the code.
> 
> No functional change intended.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Drop another spurious debug message in this patch instead of patch 10
>      Add missing "select REGMAP_I2C" to Kconfig
>      Change misleading variable name from 'mask' to 'mode'.
>      Use sysfs_emit instead of sprintf everywhere
> 
>   drivers/hwmon/Kconfig   |   1 +
>   drivers/hwmon/amc6821.c | 713 ++++++++++++++++++----------------------
>   2 files changed, 329 insertions(+), 385 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b..a8fa87a96e8f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
>   config SENSORS_AMC6821
>   	tristate "Texas Instruments AMC6821"
>   	depends on I2C
> +	select REGMAP_I2C
>   	help
>   	  If you say yes here you get support for the Texas Instruments
>   	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 028998d3bedf..3fe0bfeac843 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -8,15 +8,16 @@
>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
>    */
>   
> +#include <linux/bitops.h>
>   #include <linux/bits.h>
>   #include <linux/err.h>
>   #include <linux/hwmon.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/i2c.h>
>   #include <linux/init.h>
> -#include <linux/jiffies.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/regmap.h>
>   #include <linux/slab.h>
>   
>   /*
> @@ -44,6 +45,7 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_CONF4		0x04
>   #define AMC6821_REG_STAT1		0x02
>   #define AMC6821_REG_STAT2		0x03
> +#define AMC6821_REG_TEMP_LO		0x06
>   #define AMC6821_REG_TDATA_LOW		0x08
>   #define AMC6821_REG_TDATA_HI		0x09
>   #define AMC6821_REG_LTEMP_HI		0x0A
> @@ -61,11 +63,8 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_DCY_LOW_TEMP	0x21
>   
>   #define AMC6821_REG_TACH_LLIMITL	0x10
> -#define AMC6821_REG_TACH_LLIMITH	0x11
>   #define AMC6821_REG_TACH_HLIMITL	0x12
> -#define AMC6821_REG_TACH_HLIMITH	0x13
>   #define AMC6821_REG_TACH_SETTINGL	0x1e
> -#define AMC6821_REG_TACH_SETTINGH	0x1f
>   
>   #define AMC6821_CONF1_START		BIT(0)
>   #define AMC6821_CONF1_FAN_INT_EN	BIT(1)
> @@ -130,224 +129,169 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
>   			AMC6821_REG_TACH_HLIMITL,
>   			AMC6821_REG_TACH_SETTINGL, };
>   
> -static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
> -			AMC6821_REG_TACH_LLIMITH,
> -			AMC6821_REG_TACH_HLIMITH,
> -			AMC6821_REG_TACH_SETTINGH, };
> -
>   /*
>    * Client data (each client gets its own)
>    */
>   
>   struct amc6821_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>   	struct mutex update_lock;
> -	bool valid; /* false until following fields are valid */
> -	unsigned long last_updated; /* in jiffies */
>   
> -	/* register values */
> -	int temp[TEMP_IDX_LEN];
> -
> -	u16 fan[FAN1_IDX_LEN];
> -	u8 fan1_pulses;
> -
> -	u8 pwm1;
>   	u8 temp1_auto_point_temp[3];
>   	u8 temp2_auto_point_temp[3];
> -	u8 pwm1_auto_point_pwm[3];
> -	u8 pwm1_enable;
> -	u8 pwm1_auto_channels_temp;
> -
> -	u8 stat1;
> -	u8 stat2;
>   };
>   
> -static struct amc6821_data *amc6821_update_device(struct device *dev)
> +static int amc6821_init_auto_point_data(struct amc6821_data *data)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int timeout = HZ;
> -	u8 reg;
> -	int i;
> +	struct regmap *regmap = data->regmap;
> +	u32 pwm, regval;
> +	int err;
>   
> -	mutex_lock(&data->update_lock);
> +	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);

I think this is incorrect logic.

amc6821_init_auto_point_data is only called once in init_client, in 
probe. While we currently do not write to AMC6821_REG_DCY_LOW_TEMP, we 
could in the future. But writing to it would desynchronise the 
auto_point_temp values for the new value of the register.

I suggest we put the logic into a function and return the value for a 
given temp_auto_point (1/2 currently) so that we are calling this 
function instead of a member in the struct so that we are never caching 
it unknowingly (regmap would do it for us in any case). It's a bit odd 
anyway to have only **those** values be cached in the struct as members 
and migrating everything else.

> +	if (err)
> +		return err;
>   
> -	if (time_after(jiffies, data->last_updated + timeout) ||
> -			!data->valid) {
> +	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
> +	if (err)
> +		return err;
> +	data->temp1_auto_point_temp[0] = regval;
> +	data->temp2_auto_point_temp[0] = data->temp1_auto_point_temp[0];
>   
> -		for (i = 0; i < TEMP_IDX_LEN; i++)
> -			data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
> -				client, temp_reg[i]);
> +	err = regmap_read(regmap, AMC6821_REG_LTEMP_FAN_CTRL, &regval);
> +	if (err)
> +		return err;
> +	data->temp1_auto_point_temp[1] = (regval & 0xF8) >> 1;
>   
> -		data->stat1 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT1);
> -		data->stat2 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT2);
> +	regval &= 0x07;
> +	regval = 0x20 >> regval;
> +	if (regval)
> +		data->temp1_auto_point_temp[2] =
> +			data->temp1_auto_point_temp[1] +
> +			(255 - pwm) / regval;
> +	else
> +		data->temp1_auto_point_temp[2] = 255;
>   
> -		data->pwm1 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_DCY);
> -		for (i = 0; i < FAN1_IDX_LEN; i++) {
> -			data->fan[i] = i2c_smbus_read_byte_data(
> -					client,
> -					fan_reg_low[i]);
> -			data->fan[i] += i2c_smbus_read_byte_data(
> -					client,
> -					fan_reg_hi[i]) << 8;
> -		}
> -		data->fan1_pulses = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_CONF4);
> -		data->fan1_pulses = data->fan1_pulses & AMC6821_CONF4_PSPR ? 4 : 2;
> +	err = regmap_read(regmap, AMC6821_REG_RTEMP_FAN_CTRL, &regval);
> +	if (err)
> +		return err;
>   
> -		data->pwm1_auto_point_pwm[0] = 0;
> -		data->pwm1_auto_point_pwm[2] = 255;
> -		data->pwm1_auto_point_pwm[1] = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_DCY_LOW_TEMP);
> +	data->temp2_auto_point_temp[1] = (regval & 0xF8) >> 1;
> +	regval &= 0x07;
> +	regval = 0x20 >> regval;
>   
> -		data->temp1_auto_point_temp[0] =
> -			i2c_smbus_read_byte_data(client,
> -					AMC6821_REG_PSV_TEMP);
> -		data->temp2_auto_point_temp[0] =
> -				data->temp1_auto_point_temp[0];
> -		reg = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_LTEMP_FAN_CTRL);
> -		data->temp1_auto_point_temp[1] = (reg & 0xF8) >> 1;
> -		reg &= 0x07;
> -		reg = 0x20 >> reg;
> -		if (reg > 0)
> -			data->temp1_auto_point_temp[2] =
> -				data->temp1_auto_point_temp[1] +
> -				(data->pwm1_auto_point_pwm[2] -
> -				data->pwm1_auto_point_pwm[1]) / reg;
> -		else
> -			data->temp1_auto_point_temp[2] = 255;
> +	if (regval)
> +		data->temp2_auto_point_temp[2] =
> +			data->temp2_auto_point_temp[1] +
> +			(255 - pwm) / regval;
> +	else
> +		data->temp2_auto_point_temp[2] = 255;
>   
> -		reg = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_RTEMP_FAN_CTRL);
> -		data->temp2_auto_point_temp[1] = (reg & 0xF8) >> 1;
> -		reg &= 0x07;
> -		reg = 0x20 >> reg;
> -		if (reg > 0)
> -			data->temp2_auto_point_temp[2] =
> -				data->temp2_auto_point_temp[1] +
> -				(data->pwm1_auto_point_pwm[2] -
> -				data->pwm1_auto_point_pwm[1]) / reg;
> -		else
> -			data->temp2_auto_point_temp[2] = 255;
> -
> -		reg = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
> -		reg = (reg >> 5) & 0x3;
> -		switch (reg) {
> -		case 0: /*open loop: software sets pwm1*/
> -			data->pwm1_auto_channels_temp = 0;
> -			data->pwm1_enable = 1;
> -			break;
> -		case 2: /*closed loop: remote T (temp2)*/
> -			data->pwm1_auto_channels_temp = 2;
> -			data->pwm1_enable = 2;
> -			break;
> -		case 3: /*closed loop: local and remote T (temp2)*/
> -			data->pwm1_auto_channels_temp = 3;
> -			data->pwm1_enable = 3;
> -			break;
> -		case 1: /*
> -			 * semi-open loop: software sets rpm, chip controls
> -			 * pwm1
> -			 */
> -			data->pwm1_auto_channels_temp = 0;
> -			data->pwm1_enable = 4;
> -			break;
> -		}
> -
> -		data->last_updated = jiffies;
> -		data->valid = true;
> -	}
> -	mutex_unlock(&data->update_lock);
> -	return data;
> +	return 0;
>   }
>   
>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>   			 char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> +	u32 regval;
> +	int err;
>   
> -	return sprintf(buf, "%d\n", data->temp[ix] * 1000);
> +	err = regmap_read(data->regmap, temp_reg[ix], &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", sign_extend32(regval, 7) * 1000);
>   }
>   
>   static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
>   			  const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	int ix = to_sensor_dev_attr(attr)->index;
>   	long val;
> +	int err;
>   
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   	val = clamp_val(val / 1000, -128, 127);
>   
> -	mutex_lock(&data->update_lock);
> -	data->temp[ix] = val;
> -	if (i2c_smbus_write_byte_data(client, temp_reg[ix], data->temp[ix])) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		count = -EIO;
> -	}
> -	mutex_unlock(&data->update_lock);
> +	err = regmap_write(data->regmap, temp_reg[ix], val);
> +	if (err)
> +		return err;
> +
>   	return count;
>   }
>   
>   static ssize_t temp_alarm_show(struct device *dev,
>   			       struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> -	u8 flag;
> +	u32 regval, mask, reg;
> +	int err;
>   
>   	switch (ix) {
>   	case IDX_TEMP1_MIN:
> -		flag = data->stat1 & AMC6821_STAT1_LTL;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_LTL;
>   		break;
>   	case IDX_TEMP1_MAX:
> -		flag = data->stat1 & AMC6821_STAT1_LTH;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_LTH;
>   		break;
>   	case IDX_TEMP1_CRIT:
> -		flag = data->stat2 & AMC6821_STAT2_LTC;
> +		reg = AMC6821_REG_STAT2;
> +		mask = AMC6821_STAT2_LTC;
>   		break;
>   	case IDX_TEMP2_MIN:
> -		flag = data->stat1 & AMC6821_STAT1_RTL;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_RTL;
>   		break;
>   	case IDX_TEMP2_MAX:
> -		flag = data->stat1 & AMC6821_STAT1_RTH;
> +		reg = AMC6821_REG_STAT1;
> +		mask = AMC6821_STAT1_RTH;
>   		break;
>   	case IDX_TEMP2_CRIT:
> -		flag = data->stat2 & AMC6821_STAT2_RTC;
> +		reg = AMC6821_REG_STAT2;
> +		mask = AMC6821_STAT2_RTC;
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);

Was this an intended removal? I think we can afford keeping it in?

>   		return -EINVAL;
>   	}
> -	if (flag)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	err = regmap_read(data->regmap, reg, &regval);
> +	if (err)
> +		return err;
> +	return sysfs_emit(buf, "%d\n", !!(regval & mask));
>   }
>   
>   static ssize_t temp2_fault_show(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	if (data->stat1 & AMC6821_STAT1_RTF)
> -		return sprintf(buf, "1");
> -	else
> -		return sprintf(buf, "0");
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_RTF));
>   }
>   
>   static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
>   			 char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_DCY, &regval);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buf, "%d\n", regval);
>   }
>   
>   static ssize_t pwm1_store(struct device *dev,
> @@ -355,24 +299,43 @@ static ssize_t pwm1_store(struct device *dev,
>   			  size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	u8 val;
>   	int ret = kstrtou8(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   
> -	mutex_lock(&data->update_lock);
> -	data->pwm1 = val;
> -	i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
> -	mutex_unlock(&data->update_lock);
> +	ret = regmap_write(data->regmap, AMC6821_REG_DCY, val);
> +	if (ret)
> +		return ret;
> +
>   	return count;
>   }
>   
>   static ssize_t pwm1_enable_show(struct device *dev,
>   				struct device_attribute *devattr, char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_enable);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	int err;
> +	u32 val;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> +	if (err)
> +		return err;
> +	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +	case 0:
> +		val = 1;	/* manual */
> +		break;
> +	case AMC6821_CONF1_FDRC0:
> +		val = 4;	/* target rpm (fan1_target) controlled */
> +		break;
> +	case AMC6821_CONF1_FDRC1:
> +		val = 2;	/* remote temp controlled */
> +		break;
> +	default:
> +		val = 3;	/* max(local, remote) temp controlled */
> +		break;
> +	}
> +	return sysfs_emit(buf, "%d\n", val);
>   }
>   
>   static ssize_t pwm1_enable_store(struct device *dev,
> @@ -380,49 +343,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   				 const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>   	long val;
> -	int config = kstrtol(buf, 10, &val);
> -	if (config)
> -		return config;
> +	u32 mode;
> +	int err;
>   
> -	mutex_lock(&data->update_lock);
> -	config = i2c_smbus_read_byte_data(client, AMC6821_REG_CONF1);
> -	if (config < 0) {
> -			dev_err(&client->dev,
> -			"Error reading configuration register, aborting.\n");
> -			count = config;
> -			goto unlock;
> -	}
> +	err = kstrtol(buf, 10, &val);
> +	if (err)
> +		return err;
>   
>   	switch (val) {
>   	case 1:
> -		config &= ~AMC6821_CONF1_FDRC0;
> -		config &= ~AMC6821_CONF1_FDRC1;
> +		mode = 0;
>   		break;
>   	case 2:
> -		config &= ~AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mode = AMC6821_CONF1_FDRC1;
>   		break;
>   	case 3:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config |= AMC6821_CONF1_FDRC1;
> +		mode = AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1;
>   		break;
>   	case 4:
> -		config |= AMC6821_CONF1_FDRC0;
> -		config &= ~AMC6821_CONF1_FDRC1;
> +		mode = AMC6821_CONF1_FDRC0;
>   		break;
>   	default:
> -		count = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>   	}
> -	if (i2c_smbus_write_byte_data(client, AMC6821_REG_CONF1, config)) {
> -			dev_err(&client->dev,
> -			"Configuration register write error, aborting.\n");
> -			count = -EIO;
> -	}
> -unlock:
> -	mutex_unlock(&data->update_lock);
> +
> +	err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
> +				 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
> +				 mode);
> +	if (err)
> +		return err;
> +
>   	return count;
>   }
>   
> @@ -430,26 +381,45 @@ static ssize_t pwm1_auto_channels_temp_show(struct device *dev,
>   					    struct device_attribute *devattr,
>   					    char *buf)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_auto_channels_temp);
> +	struct amc6821_data *data = dev_get_drvdata(dev);
> +	u32 val;
> +	int err;
> +
> +	err = regmap_read(data->regmap, AMC6821_REG_CONF1, &val);
> +	if (err)
> +		return err;
> +	switch (val & (AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1)) {
> +	case 0:
> +	case AMC6821_CONF1_FDRC0:
> +		val = 0;	/* manual or target rpm controlled */
> +		break;
> +	case AMC6821_CONF1_FDRC1:
> +		val = 2;	/* remote temp controlled */
> +		break;
> +	default:
> +		val = 3;	/* max(local, remote) temp controlled */
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%d\n", val);
>   }
>   
>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>   					 struct device_attribute *devattr,
>   					 char *buf)
>   {
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr_2(devattr)->index;
>   	int nr = to_sensor_dev_attr_2(devattr)->nr;
> -	struct amc6821_data *data = amc6821_update_device(dev);
> +
>   	switch (nr) {
>   	case 1:
> -		return sprintf(buf, "%d\n",
> -			data->temp1_auto_point_temp[ix] * 1000);
> +		return sysfs_emit(buf, "%d\n",
> +				  data->temp1_auto_point_temp[ix] * 1000);
>   	case 2:
> -		return sprintf(buf, "%d\n",
> -			data->temp2_auto_point_temp[ix] * 1000);
> +		return sysfs_emit(buf, "%d\n",
> +				  data->temp2_auto_point_temp[ix] * 1000);
>   	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);

Ditto.

>   		return -EINVAL;
>   	}
>   }
> @@ -458,44 +428,59 @@ static ssize_t pwm1_auto_point_pwm_show(struct device *dev,
>   					struct device_attribute *devattr,
>   					char *buf)
>   {
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr(devattr)->index;
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm1_auto_point_pwm[ix]);
> +	u32 val;
> +	int err;
> +
> +	switch (ix) {
> +	case 0:
> +		val = 0;
> +		break;
> +	case 1:
> +		err = regmap_read(data->regmap, AMC6821_REG_DCY_LOW_TEMP, &val);
> +		if (err)
> +			return err;
> +		break;
> +	default:
> +		val = 255;
> +		break;
> +	}
> +	return sysfs_emit(buf, "%d\n", val);
>   }
>   
> -static inline ssize_t set_slope_register(struct i2c_client *client,
> -		u8 reg,
> -		u8 dpwm,
> -		u8 *ptemp)
> +static inline int set_slope_register(struct regmap *regmap,
> +				     u8 reg, u8 *ptemp)
>   {
> -	int dt;
> -	u8 tmp;
> +	u8 tmp, dpwm;
> +	int err, dt;
> +	u32 pwm;
>   
> -	dt = ptemp[2]-ptemp[1];
> +	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> +	if (err)
> +		return err;
> +
> +	dpwm = 255 - pwm;
> +
> +	dt = ptemp[2] - ptemp[1];
>   	for (tmp = 4; tmp > 0; tmp--) {
>   		if (dt * (0x20 >> tmp) >= dpwm)
>   			break;
>   	}
>   	tmp |= (ptemp[1] & 0x7C) << 1;
> -	if (i2c_smbus_write_byte_data(client,
> -			reg, tmp)) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		return -EIO;
> -	}
> -	return 0;
> +	return regmap_write(regmap, reg, tmp);
>   }
>   
>   static ssize_t temp_auto_point_temp_store(struct device *dev,
>   					  struct device_attribute *attr,
>   					  const char *buf, size_t count)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	struct i2c_client *client = data->client;
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr_2(attr)->index;
>   	int nr = to_sensor_dev_attr_2(attr)->nr;
> +	struct regmap *regmap = data->regmap;
>   	u8 *ptemp;
>   	u8 reg;
> -	int dpwm;
>   	long val;
>   	int ret = kstrtol(buf, 10, &val);
>   	if (ret)
> @@ -511,12 +496,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		reg = AMC6821_REG_RTEMP_FAN_CTRL;
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);

Ditto.

>   		return -EINVAL;
>   	}
>   
>   	mutex_lock(&data->update_lock);
> -	data->valid = false;
>   
>   	switch (ix) {
>   	case 0:
> @@ -525,13 +508,9 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		ptemp[0] = clamp_val(ptemp[0], 0,
>   				     data->temp2_auto_point_temp[1]);
>   		ptemp[0] = clamp_val(ptemp[0], 0, 63);
> -		if (i2c_smbus_write_byte_data(
> -					client,
> -					AMC6821_REG_PSV_TEMP,
> -					ptemp[0])) {
> -				dev_err(&client->dev,
> -					"Register write error, aborting.\n");
> -				count = -EIO;
> +		if (regmap_write(regmap, AMC6821_REG_PSV_TEMP, ptemp[0])) {
> +			dev_err(dev, "Register write error, aborting.\n");
> +			count = -EIO;
>   		}
>   		goto EXIT;
>   	case 1:
> @@ -543,12 +522,10 @@ static ssize_t temp_auto_point_temp_store(struct device *dev,
>   		ptemp[2] = clamp_val(val / 1000, ptemp[1]+1, 255);
>   		break;
>   	default:
> -		dev_dbg(dev, "Unknown attr->index (%d).\n", ix);

Ditto.

>   		count = -EINVAL;
>   		goto EXIT;
>   	}
> -	dpwm = data->pwm1_auto_point_pwm[2] - data->pwm1_auto_point_pwm[1];
> -	if (set_slope_register(client, reg, dpwm, ptemp))
> +	if (set_slope_register(regmap, reg, ptemp))
>   		count = -EIO;
>   
>   EXIT:
> @@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   					 const char *buf, size_t count)
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int dpwm;
> +	struct regmap *regmap = data->regmap;
>   	u8 val;
> -	int ret = kstrtou8(buf, 10, &val);
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &val);

Not sure this cosmetic change is worth it? Or maybe squash with an 
earlier commit?

>   	if (ret)
>   		return ret;
>   
> @@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>   		return -EINVAL;
>   
>   	mutex_lock(&data->update_lock);
> -	data->pwm1_auto_point_pwm[1] = val;
> -	if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
> -			data->pwm1_auto_point_pwm[1])) {
> -		dev_err(&client->dev, "Register write error, aborting.\n");
> -		count = -EIO;
> -		goto EXIT;
> +	ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
> +	if (ret)

I think we're missing a count = something here?

> +		goto unlock;
> +
> +	ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
> +				 data->temp1_auto_point_temp);
> +	if (ret) {
> +		count = ret;

In some places, we replace set_slope_register return code with -EIO 
(like it was) and sometimes we propagate it, like here. Is there some 
reason for this or can we have some kind of consistency across the code 
base here?

Looks good to me otherwise.

Cheers,
Quentin

Re: [PATCH v2 01/11] hwmon: (amc6821) Stop accepting invalid pwm values

[Guenter Roeck]

On 7/3/24 07:29, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> The pwm value range is well defined from 0..255. Don't accept
>> any values outside this range.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Use kstrtou8() instead of kstrtol() where possible.
>>      Limit range of pwm1_auto_point_pwm to 0..254 in patch 1
>>      instead of limiting it later, and do not accept invalid
>>      values for the attribute.
>>
>>   drivers/hwmon/amc6821.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 9b02b304c2f5..eb2d5592a41a 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev,
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    long val;
>> -    int ret = kstrtol(buf, 10, &val);
>> +    u8 val;
>> +    int ret = kstrtou8(buf, 10, &val);
>>       if (ret)
>>           return ret;
>>       mutex_lock(&data->update_lock);
>> -    data->pwm1 = clamp_val(val , 0, 255);
>> +    data->pwm1 = val;
>>       i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1);
>>       mutex_unlock(&data->update_lock);
>>       return count;
>> @@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>>       int dpwm;
>> -    long val;
>> -    int ret = kstrtol(buf, 10, &val);
>> +    u8 val;
>> +    int ret = kstrtou8(buf, 10, &val);
>>       if (ret)
>>           return ret;
>>
>> +    if (val > 254)
> 
> Would have appreciated a comment as to why it's 254. My understanding is that the subsystem requires no overlap between multiple pwm_auto_points? 0 being 0 and 2 being 255, we need 1 to be 255?
> 

No idea, really, I just took it from the original code. I don't find a hint
in the code suggesting why 255 would be worse than 0.

> Actually, that doesn't explain why we allow 0 here, so maybe I'm just clueless :)
> 
Yes, agreed, that doesn't really make sense. I'll change the upper limit to 255.

> The change itself though:
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
> 
... but I'll also keep that tag unless you start screaming.

Thanks,
Guenter

Re: [PATCH v2 09/11] hwmon: (amc6821) Convert to use regmap

[Guenter Roeck]

On 7/3/24 09:19, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> Use regmap for register accesses and for most caching.
>>
>> While at it, use sysfs_emit() instead of sprintf() to write sysfs
>> attribute data, and remove spurious debug messages which would
>> only be seen as result of a bug in the code.
>>
>> No functional change intended.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Drop another spurious debug message in this patch instead of patch 10
>>      Add missing "select REGMAP_I2C" to Kconfig
>>      Change misleading variable name from 'mask' to 'mode'.
>>      Use sysfs_emit instead of sprintf everywhere
>>
>>   drivers/hwmon/Kconfig   |   1 +
>>   drivers/hwmon/amc6821.c | 713 ++++++++++++++++++----------------------
>>   2 files changed, 329 insertions(+), 385 deletions(-)
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index e14ae18a973b..a8fa87a96e8f 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
>>   config SENSORS_AMC6821
>>       tristate "Texas Instruments AMC6821"
>>       depends on I2C
>> +    select REGMAP_I2C
>>       help
>>         If you say yes here you get support for the Texas Instruments
>>         AMC6821 hardware monitoring chips.
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 028998d3bedf..3fe0bfeac843 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -8,15 +8,16 @@
>>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
>>    */
>> +#include <linux/bitops.h>
>>   #include <linux/bits.h>
>>   #include <linux/err.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/hwmon-sysfs.h>
>>   #include <linux/i2c.h>
>>   #include <linux/init.h>
>> -#include <linux/jiffies.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>> +#include <linux/regmap.h>
>>   #include <linux/slab.h>
>>   /*
>> @@ -44,6 +45,7 @@ module_param(init, int, 0444);
>>   #define AMC6821_REG_CONF4        0x04
>>   #define AMC6821_REG_STAT1        0x02
>>   #define AMC6821_REG_STAT2        0x03
>> +#define AMC6821_REG_TEMP_LO        0x06
>>   #define AMC6821_REG_TDATA_LOW        0x08
>>   #define AMC6821_REG_TDATA_HI        0x09
>>   #define AMC6821_REG_LTEMP_HI        0x0A
>> @@ -61,11 +63,8 @@ module_param(init, int, 0444);
>>   #define AMC6821_REG_DCY_LOW_TEMP    0x21
>>   #define AMC6821_REG_TACH_LLIMITL    0x10
>> -#define AMC6821_REG_TACH_LLIMITH    0x11
>>   #define AMC6821_REG_TACH_HLIMITL    0x12
>> -#define AMC6821_REG_TACH_HLIMITH    0x13
>>   #define AMC6821_REG_TACH_SETTINGL    0x1e
>> -#define AMC6821_REG_TACH_SETTINGH    0x1f
>>   #define AMC6821_CONF1_START        BIT(0)
>>   #define AMC6821_CONF1_FAN_INT_EN    BIT(1)
>> @@ -130,224 +129,169 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
>>               AMC6821_REG_TACH_HLIMITL,
>>               AMC6821_REG_TACH_SETTINGL, };
>> -static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
>> -            AMC6821_REG_TACH_LLIMITH,
>> -            AMC6821_REG_TACH_HLIMITH,
>> -            AMC6821_REG_TACH_SETTINGH, };
>> -
>>   /*
>>    * Client data (each client gets its own)
>>    */
>>   struct amc6821_data {
>> -    struct i2c_client *client;
>> +    struct regmap *regmap;
>>       struct mutex update_lock;
>> -    bool valid; /* false until following fields are valid */
>> -    unsigned long last_updated; /* in jiffies */
>> -    /* register values */
>> -    int temp[TEMP_IDX_LEN];
>> -
>> -    u16 fan[FAN1_IDX_LEN];
>> -    u8 fan1_pulses;
>> -
>> -    u8 pwm1;
>>       u8 temp1_auto_point_temp[3];
>>       u8 temp2_auto_point_temp[3];
>> -    u8 pwm1_auto_point_pwm[3];
>> -    u8 pwm1_enable;
>> -    u8 pwm1_auto_channels_temp;
>> -
>> -    u8 stat1;
>> -    u8 stat2;
>>   };
>> -static struct amc6821_data *amc6821_update_device(struct device *dev)
>> +static int amc6821_init_auto_point_data(struct amc6821_data *data)
>>   {
>> -    struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int timeout = HZ;
>> -    u8 reg;
>> -    int i;
>> +    struct regmap *regmap = data->regmap;
>> +    u32 pwm, regval;
>> +    int err;
>> -    mutex_lock(&data->update_lock);
>> +    err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> 
> I think this is incorrect logic.
> 
> amc6821_init_auto_point_data is only called once in init_client, in probe. While we currently do not write to AMC6821_REG_DCY_LOW_TEMP, we could in the future. But writing to it would desynchronise the auto_point_temp values for the new value of the register.
> 

We do write the register, in pwm1_auto_point_pwm_store().

> I suggest we put the logic into a function and return the value for a given temp_auto_point (1/2 currently) so that we are calling this function instead of a member in the struct so that we are never caching it unknowingly (regmap would do it for us in any case). It's a bit odd anyway to have only **those** values be cached in the struct as members and migrating everything else.

Yes, I know, that is a bit odd. Essentially I was too lazy to clean that up.
But, yes, you are correct, that results in the cached values being wrong
after AMC6821_REG_DCY_LOW_TEMP is updated. Back to the drawing board,
and thanks for finding this.

[ ... ]

>>       default:
>> -        dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
> 
> Was this an intended removal? I think we can afford keeping it in?

Yes, it is intentional. It (and the others below) indicate a coding
error, which would have been found during development. While it does make
sense to keep the default: case to make the compiler happy, it doesn't
make sense to keep the pointless message in the code.

>> @@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>                        const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int dpwm;
>> +    struct regmap *regmap = data->regmap;
>>       u8 val;
>> -    int ret = kstrtou8(buf, 10, &val);
>> +    int ret;
>> +
>> +    ret = kstrtou8(buf, 10, &val);
> 
> Not sure this cosmetic change is worth it? Or maybe squash with an earlier commit?
> 

That slipped in, I think from the first patch of the series. I'll move it there.

>>       if (ret)
>>           return ret;
>> @@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>           return -EINVAL;
>>       mutex_lock(&data->update_lock);
>> -    data->pwm1_auto_point_pwm[1] = val;
>> -    if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
>> -            data->pwm1_auto_point_pwm[1])) {
>> -        dev_err(&client->dev, "Register write error, aborting.\n");
>> -        count = -EIO;
>> -        goto EXIT;
>> +    ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
>> +    if (ret)
> 
> I think we're missing a count = something here?
> 
Yes.

>> +        goto unlock;
>> +
>> +    ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
>> +                 data->temp1_auto_point_temp);
>> +    if (ret) {
>> +        count = ret;
> 
> In some places, we replace set_slope_register return code with -EIO (like it was) and sometimes we propagate it, like here. Is there some reason for this or can we have some kind of consistency across the code base here?
> 
Thanks for noticing. The code should be consistent and always propagate the error code.
I'll fix it.

Thanks,
Guenter

Re: [PATCH v2 11/11] hwmon: (amc6821) Add support for pwm1_mode attribute

[Guenter Roeck]

On 7/3/24 08:28, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> AMC6821 supports configuring if a fan is DC or PWM controlled.
>> Add support for the pwm1_mode attribute to make it runtime configurable.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: New patch
>>
>>   Documentation/hwmon/amc6821.rst |  1 +
>>   drivers/hwmon/amc6821.c         | 16 +++++++++++++++-
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/hwmon/amc6821.rst b/Documentation/hwmon/amc6821.rst
>> index 96e604c5ea8e..dbd544cd1160 100644
>> --- a/Documentation/hwmon/amc6821.rst
>> +++ b/Documentation/hwmon/amc6821.rst
>> @@ -58,6 +58,7 @@ pwm1_enable        rw    regulator mode, 1=open loop, 2=fan controlled
>>                   remote-sensor temperature,
>>                   4=fan controlled by target rpm set with
>>                   fan1_target attribute.
>> +pwm1_mode        rw    Fan duty control mode (0=DC, 1=PWM)
>>   pwm1_auto_channels_temp ro    1 if pwm_enable==2, 3 if pwm_enable==3
>>   pwm1_auto_point1_pwm    ro    Hardwired to 0, shared for both
>>                   temperature channels.
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index 5a3c089ae06f..98a45fe529e0 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -317,6 +317,12 @@ static int amc6821_pwm_read(struct device *dev, u32 attr, long *val)
>>               break;
>>           }
>>           return 0;
>> +    case hwmon_pwm_mode:
>> +        err = regmap_read(regmap, AMC6821_REG_CONF2, &regval);
>> +        if (err)
>> +            return err;
>> +        *val = !!(regval & AMC6821_CONF2_TACH_MODE);
>> +        return 0;
>>       case hwmon_pwm_auto_channels_temp:
>>           err = regmap_read(regmap, AMC6821_REG_CONF1, &regval);
>>           if (err)
>> @@ -372,6 +378,13 @@ static int amc6821_pwm_write(struct device *dev, u32 attr, long val)
>>           return regmap_update_bits(regmap, AMC6821_REG_CONF1,
>>                         AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
>>                         mode);
>> +    case hwmon_pwm_mode:
>> +        if (val < 0 || val > 1)
>> +            return -EINVAL;
>> +        return regmap_update_bits(regmap, AMC6821_REG_CONF1,
> 
> Wrong register here, should be AMC6821_REG_CONF2.
> 

Oops. I had a bug in my test script, and thanks to that it failed to report the problem.

Thanks for noticing!
Guenter

Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

On 7/3/24 07:35, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> The default value of the maximum fan speed limit register is 0,
>> essentially translating to an unlimited fan speed. When reading
>> the limit, a value of 0 is reported in this case. However, writing
>> a value of 0 results in writing a value of 0xffff into the register,
>> which is inconsistent.
>>
>> To solve the problem, permit writing a limit of 0 for the maximim fan
>> speed, effectively translating to "no limit". Write 0 into the register
>> if a limit value of 0 is written. Otherwise limit the range to
>> <1..6000000> and write 1..0xffff into the register. This ensures that
>> reading and writing from and to a limit register return the same value
>> while at the same time not changing reported values when reading the
>> speed or limits.
>>
>> While at it, restrict fan limit writes to non-negative numbers; writing
>> a negative limit does not make sense and should be reported instead of
>> being corrected.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Do not accept negative fan speed values
>>      Display fan speed and speed limit as 0 if register value is 0
>>      (instead of 6000000), as in original code.
>>      Only permit writing 0 (unlimited) for the maximum fan speed.
>>
>>   drivers/hwmon/amc6821.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>> index eb2d5592a41a..9c19d4d278ec 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    long val;
>> +    unsigned long val;
>>       int ix = to_sensor_dev_attr(attr)->index;
>> -    int ret = kstrtol(buf, 10, &val);
>> +    int ret = kstrtoul(buf, 10, &val);
>>       if (ret)
>>           return ret;
>> -    val = 1 > val ? 0xFFFF : 6000000/val;
>> +
>> +    /* The minimum fan speed must not be unlimited (0) */
>> +    if (ix == IDX_FAN1_MIN && !val)
>> +        return -EINVAL;
>> +
>> +    val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
> 
> I'm wondering if we shouldn't check !val for min after this line instead? Otherwise we allow 6000001+RPM speeds... which is technically unlimited.
> 

If ix == IDX_FAN1_MIN, val must be positive because of the check above.
The expression "6000000 / clamp_val(val, 1, 6000000)" is therefore always
positive as well because val is clamped. Its minimum result would be
6000000/6000000 = 1. The alternate case of the ternary expression would
never hit because it is guaranteed that val > 0. Am I missing something ?

Thanks,
Guenter

Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz]

Hi Guenter,

On 7/3/24 11:48 PM, Guenter Roeck wrote:
> On 7/3/24 07:35, Quentin Schulz wrote:
>> Hi Guenter,
>>
>> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>>> The default value of the maximum fan speed limit register is 0,
>>> essentially translating to an unlimited fan speed. When reading
>>> the limit, a value of 0 is reported in this case. However, writing
>>> a value of 0 results in writing a value of 0xffff into the register,
>>> which is inconsistent.
>>>
>>> To solve the problem, permit writing a limit of 0 for the maximim fan
>>> speed, effectively translating to "no limit". Write 0 into the register
>>> if a limit value of 0 is written. Otherwise limit the range to
>>> <1..6000000> and write 1..0xffff into the register. This ensures that
>>> reading and writing from and to a limit register return the same value
>>> while at the same time not changing reported values when reading the
>>> speed or limits.
>>>
>>> While at it, restrict fan limit writes to non-negative numbers; writing
>>> a negative limit does not make sense and should be reported instead of
>>> being corrected.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> v2: Do not accept negative fan speed values
>>>      Display fan speed and speed limit as 0 if register value is 0
>>>      (instead of 6000000), as in original code.
>>>      Only permit writing 0 (unlimited) for the maximum fan speed.
>>>
>>>   drivers/hwmon/amc6821.c | 13 +++++++++----
>>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>>> index eb2d5592a41a..9c19d4d278ec 100644
>>> --- a/drivers/hwmon/amc6821.c
>>> +++ b/drivers/hwmon/amc6821.c
>>> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, 
>>> struct device_attribute *attr,
>>>   {
>>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>>       struct i2c_client *client = data->client;
>>> -    long val;
>>> +    unsigned long val;
>>>       int ix = to_sensor_dev_attr(attr)->index;
>>> -    int ret = kstrtol(buf, 10, &val);
>>> +    int ret = kstrtoul(buf, 10, &val);
>>>       if (ret)
>>>           return ret;
>>> -    val = 1 > val ? 0xFFFF : 6000000/val;
>>> +
>>> +    /* The minimum fan speed must not be unlimited (0) */
>>> +    if (ix == IDX_FAN1_MIN && !val)
>>> +        return -EINVAL;
>>> +
>>> +    val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
>>
>> I'm wondering if we shouldn't check !val for min after this line 
>> instead? Otherwise we allow 6000001+RPM speeds... which is technically 
>> unlimited.
>>
> 
> If ix == IDX_FAN1_MIN, val must be positive because of the check above.
> The expression "6000000 / clamp_val(val, 1, 6000000)" is therefore always
> positive as well because val is clamped. Its minimum result would be
> 6000000/6000000 = 1. The alternate case of the ternary expression would
> never hit because it is guaranteed that val > 0. Am I missing something ?
> 

No, I misread the code and I didn't see the clamp_val, which means we 
cannot have the denominator be > 6000000, meaning val cannot be 0 after 
that line (well, except if it is 0 **before** already).

So no, just brain fart.

Also, we probably could swap clamp_val(val, 1, 6000000) for min(val, 
6000000) as val > 0 because of the ternary operator condition. But 
that's nothing important nor interesting.

Cheers,
Quentin

Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Guenter Roeck]

On 7/4/24 00:52, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/3/24 11:48 PM, Guenter Roeck wrote:
>> On 7/3/24 07:35, Quentin Schulz wrote:
>>> Hi Guenter,
>>>
>>> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>>>> The default value of the maximum fan speed limit register is 0,
>>>> essentially translating to an unlimited fan speed. When reading
>>>> the limit, a value of 0 is reported in this case. However, writing
>>>> a value of 0 results in writing a value of 0xffff into the register,
>>>> which is inconsistent.
>>>>
>>>> To solve the problem, permit writing a limit of 0 for the maximim fan
>>>> speed, effectively translating to "no limit". Write 0 into the register
>>>> if a limit value of 0 is written. Otherwise limit the range to
>>>> <1..6000000> and write 1..0xffff into the register. This ensures that
>>>> reading and writing from and to a limit register return the same value
>>>> while at the same time not changing reported values when reading the
>>>> speed or limits.
>>>>
>>>> While at it, restrict fan limit writes to non-negative numbers; writing
>>>> a negative limit does not make sense and should be reported instead of
>>>> being corrected.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>> v2: Do not accept negative fan speed values
>>>>      Display fan speed and speed limit as 0 if register value is 0
>>>>      (instead of 6000000), as in original code.
>>>>      Only permit writing 0 (unlimited) for the maximum fan speed.
>>>>
>>>>   drivers/hwmon/amc6821.c | 13 +++++++++----
>>>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
>>>> index eb2d5592a41a..9c19d4d278ec 100644
>>>> --- a/drivers/hwmon/amc6821.c
>>>> +++ b/drivers/hwmon/amc6821.c
>>>> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>>>>   {
>>>>       struct amc6821_data *data = dev_get_drvdata(dev);
>>>>       struct i2c_client *client = data->client;
>>>> -    long val;
>>>> +    unsigned long val;
>>>>       int ix = to_sensor_dev_attr(attr)->index;
>>>> -    int ret = kstrtol(buf, 10, &val);
>>>> +    int ret = kstrtoul(buf, 10, &val);
>>>>       if (ret)
>>>>           return ret;
>>>> -    val = 1 > val ? 0xFFFF : 6000000/val;
>>>> +
>>>> +    /* The minimum fan speed must not be unlimited (0) */
>>>> +    if (ix == IDX_FAN1_MIN && !val)
>>>> +        return -EINVAL;
>>>> +
>>>> +    val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
>>>
>>> I'm wondering if we shouldn't check !val for min after this line instead? Otherwise we allow 6000001+RPM speeds... which is technically unlimited.
>>>
>>
>> If ix == IDX_FAN1_MIN, val must be positive because of the check above.
>> The expression "6000000 / clamp_val(val, 1, 6000000)" is therefore always
>> positive as well because val is clamped. Its minimum result would be
>> 6000000/6000000 = 1. The alternate case of the ternary expression would
>> never hit because it is guaranteed that val > 0. Am I missing something ?
>>
> 
> No, I misread the code and I didn't see the clamp_val, which means we cannot have the denominator be > 6000000, meaning val cannot be 0 after that line (well, except if it is 0 **before** already).
> 
> So no, just brain fart.
> 
> Also, we probably could swap clamp_val(val, 1, 6000000) for min(val, 6000000) as val > 0 because of the ternary operator condition. But that's nothing important nor interesting.
> 

Good point. I may do that if I have to send v4 (if I remember), but for now the
existing code should be good enough.

Thanks,
Guenter