[PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Converting the driver to use regmap makes it more generic. It also makes
it a lot easier to debug through debugfs.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 drivers/hwmon/Kconfig    |   1 +
 drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
 2 files changed, 133 insertions(+), 122 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1ecf697d8d99..9f11d036c316 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
 config SENSORS_MAX31790
 	tristate "Maxim MAX31790 sensor chip"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for 6-Channel PWM-Output
 	  Fan RPM Controller.
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 2c6b333a28e9..e3765ce4444a 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 /* MAX31790 registers */
@@ -46,92 +47,53 @@
 
 #define NR_CHANNEL			6
 
+#define MAX31790_REG_USER_BYTE_67	0x67
+
+#define BULK_TO_U16(msb, lsb)		(((msb) << 8) + (lsb))
+#define U16_MSB(num)			(((num) & 0xFF00) >> 8)
+#define U16_LSB(num)			((num) & 0x00FF)
+
+static const struct regmap_range max31790_ro_range = {
+	.range_min = MAX31790_REG_TACH_COUNT(0),
+	.range_max = MAX31790_REG_PWMOUT(0) - 1,
+};
+
+static const struct regmap_access_table max31790_wr_table = {
+	.no_ranges = &max31790_ro_range,
+	.n_no_ranges = 1,
+};
+
+static const struct regmap_range max31790_volatile_ranges[] = {
+	regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
+	regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
+};
+
+static const struct regmap_access_table max31790_volatile_table = {
+	.no_ranges = max31790_volatile_ranges,
+	.n_no_ranges = 2,
+	.n_yes_ranges = 0
+};
+
+static const struct regmap_config max31790_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_register = MAX31790_REG_USER_BYTE_67,
+	.wr_table = &max31790_wr_table,
+	.volatile_table = &max31790_volatile_table
+};
+
 /*
  * Client data (each client gets its own)
  */
 struct max31790_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
+
 	struct mutex update_lock;
-	bool valid; /* zero until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
-
-	/* register values */
 	u8 fan_config[NR_CHANNEL];
 	u8 fan_dynamics[NR_CHANNEL];
-	u16 fault_status;
-	u16 tach[NR_CHANNEL * 2];
-	u16 pwm[NR_CHANNEL];
-	u16 target_count[NR_CHANNEL];
 };
 
-static struct max31790_data *max31790_update_device(struct device *dev)
-{
-	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	struct max31790_data *ret = data;
-	int i;
-	int rv;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS1);
-		if (rv < 0)
-			goto abort;
-		data->fault_status = rv & 0x3F;
-
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS2);
-		if (rv < 0)
-			goto abort;
-		data->fault_status |= (rv & 0x3F) << 6;
-
-		for (i = 0; i < NR_CHANNEL; i++) {
-			rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(i));
-			if (rv < 0)
-				goto abort;
-			data->tach[i] = rv;
-
-			if (data->fan_config[i]
-			    & MAX31790_FAN_CFG_TACH_INPUT) {
-				rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(NR_CHANNEL
-								+ i));
-				if (rv < 0)
-					goto abort;
-				data->tach[NR_CHANNEL + i] = rv;
-			} else {
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_PWMOUT(i));
-				if (rv < 0)
-					goto abort;
-				data->pwm[i] = rv;
-
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_TARGET_COUNT(i));
-				if (rv < 0)
-					goto abort;
-				data->target_count[i] = rv;
-			}
-		}
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	goto done;
-
-abort:
-	data->valid = false;
-	ret = ERR_PTR(rv);
-
-done:
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
 static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
 
 static u8 get_tach_period(u8 fan_dynamics)
@@ -159,28 +121,75 @@ static u8 bits_for_tach_period(int rpm)
 	return bits;
 }
 
+static int read_reg_byte(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	int val;
+
+	rv = regmap_read(regmap, reg, &val);
+	if (rv < 0)
+		return rv;
+
+	return val;
+}
+
+static int read_reg_word(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	u8 val_bulk[2];
+
+	rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
+	if (rv < 0)
+		return rv;
+
+	return BULK_TO_U16(val_bulk[0], val_bulk[1]);
+}
+
+static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
+{
+	u8 bulk_val[2];
+
+	bulk_val[0] = U16_MSB(val);
+	bulk_val[1] = U16_LSB(val);
+
+	return regmap_bulk_write(regmap, reg, bulk_val, 2);
+}
+
 static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	int sr, rpm;
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int tach, fault;
 
 	switch (attr) {
 	case hwmon_fan_input:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->tach[channel], sr);
-		*val = rpm;
+		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_target:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->target_count[channel], sr);
-		*val = rpm;
+		tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_fault:
-		*val = !!(data->fault_status & (1 << channel));
+		if (channel > 6)
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
+		else
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
+
+		if (fault < 0)
+			return fault;
+
+		if (channel > 6)
+			*val = !!(fault & (1 << (channel - 6)));
+		else
+			*val = !!(fault & (1 << channel));
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -191,7 +200,7 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	int target_count;
 	int err = 0;
 	u8 bits;
@@ -207,9 +216,10 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 			((data->fan_dynamics[channel] &
 			  ~MAX31790_FAN_DYN_SR_MASK) |
 			 (bits << MAX31790_FAN_DYN_SR_SHIFT));
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_DYNAMICS(channel),
-					data->fan_dynamics[channel]);
+
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_DYNAMICS(channel),
+				   data->fan_dynamics[channel]);
 		if (err < 0)
 			break;
 
@@ -217,11 +227,11 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 		target_count = RPM_TO_REG(val, sr);
 		target_count = clamp_val(target_count, 0x1, 0x7FF);
 
-		data->target_count[channel] = target_count << 5;
+		target_count = target_count << 5;
 
-		err = i2c_smbus_write_word_swapped(client,
-					MAX31790_REG_TARGET_COUNT(channel),
-					data->target_count[channel]);
+		err = write_reg_word(regmap,
+				     MAX31790_REG_TARGET_COUNT(channel),
+				     target_count);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -258,22 +268,22 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	u8 fan_config;
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	fan_config = data->fan_config[channel];
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int read;
 
 	switch (attr) {
 	case hwmon_pwm_input:
-		*val = data->pwm[channel] >> 8;
+		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
+		if (read < 0)
+			return read;
+
+		*val = read >> 8;
 		return 0;
 	case hwmon_pwm_enable:
-		if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
+		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
 			*val = 2;
-		else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
+		else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
 			*val = 1;
 		else
 			*val = 0;
@@ -287,7 +297,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	u8 fan_config;
 	int err = 0;
 
@@ -299,10 +309,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			err = -EINVAL;
 			break;
 		}
-		data->pwm[channel] = val << 8;
-		err = i2c_smbus_write_word_swapped(client,
-						   MAX31790_REG_PWMOUT(channel),
-						   data->pwm[channel]);
+		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
 		break;
 	case hwmon_pwm_enable:
 		fan_config = data->fan_config[channel];
@@ -321,9 +328,9 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			break;
 		}
 		data->fan_config[channel] = fan_config;
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_CONFIG(channel),
-					fan_config);
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_CONFIG(channel),
+				   fan_config);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -426,20 +433,18 @@ static const struct hwmon_chip_info max31790_chip_info = {
 	.info = max31790_info,
 };
 
-static int max31790_init_client(struct i2c_client *client,
+static int max31790_init_client(struct regmap *regmap,
 				struct max31790_data *data)
 {
 	int i, rv;
 
 	for (i = 0; i < NR_CHANNEL; i++) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_CONFIG(i));
+		rv = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(i % NR_CHANNEL));
 		if (rv < 0)
 			return rv;
 		data->fan_config[i] = rv;
 
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_DYNAMICS(i));
+		rv = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(i));
 		if (rv < 0)
 			return rv;
 		data->fan_dynamics[i] = rv;
@@ -464,13 +469,18 @@ static int max31790_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
 	mutex_init(&data->update_lock);
 
+	data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
 	/*
 	 * Initialize the max31790 chip
 	 */
-	err = max31790_init_client(client, data);
+	err = max31790_init_client(data->regmap, data);
 	if (err)
 		return err;
 
-- 
2.31.1

[PATCH v2 2/5] hwmon: (max31790) Fix and split pwm*_enable

[Václav Kubernát]

In the old code, pwm*_enable does two things. Firstly, it sets whether
the chip should run in PWM or RPM mode. Secondly, it tells the chip
whether it should monitor fan RPM. However, these two settings aren't
tied together, so they shouldn't be set with a single value. In the new
code, fan*_enable now controls fan RPM monitoring (pwm*_enable no longer
controls that).

According to the sysfs hwmon documentation, pwm*_enable has three
possible values, 0 for "no control / full-speed", 1 for manual mode, and
2+ for automatic. The old code works fine for 1 and 2, but 0 only
differs from 1 in that it just turns off fan speed monitoring. The chip
actually does have a way to turn off fan controls (and only monitor),
but what that does is that it sets PWM to 0% duty cycle (which is the
opposite to full-speed) AND it also turns off fan speed monitoring.
Because of this, I implemented the 0 value by setting PWM mode to 100%.
This method does come with a problem: it is impossible to differentiate
between full-speed and PWM mode just from the values on the chip. The
new code solves this by saving a value indicating whether we're in
full-speed mode. This value is initialized to 0, so full-speed mode
won't persist across reboots.

These two changes are closely connected together, mainly because the
detection of the pwm*_enable value depended on whether fan speed
monitoring is enabled (which is now controlled as written in the first
paragraph).

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst |  8 +--
 drivers/hwmon/max31790.c         | 87 ++++++++++++++++++++++----------
 2 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index f301385d8cef..8979c8a02cd1 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -34,10 +34,12 @@ also be configured to serve as tachometer inputs.
 Sysfs entries
 -------------
 
-================== === =======================================================
+================== === =============================================================
+fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
 fan[1-6]_target    RW  desired fan speed in RPM
-pwm[1-6]_enable    RW  regulator mode, 0=disabled, 1=manual mode, 2=rpm mode
+pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
+                       setting rpm mode sets fan*_enable to 1
 pwm[1-6]           RW  fan target duty cycle (0-255)
-================== === =======================================================
+================== === =============================================================
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index e3765ce4444a..ecdd55e12ffe 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -39,6 +39,7 @@
 
 #define FAN_RPM_MIN			120
 #define FAN_RPM_MAX			7864320
+#define MAX_PWM				0XFF80
 
 #define RPM_FROM_REG(reg, sr)		(((reg) >> 4) ? \
 					 ((60 * (sr) * 8192) / ((reg) >> 4)) : \
@@ -90,6 +91,7 @@ struct max31790_data {
 	struct regmap *regmap;
 
 	struct mutex update_lock;
+	bool full_speed[NR_CHANNEL];
 	u8 fan_config[NR_CHANNEL];
 	u8 fan_dynamics[NR_CHANNEL];
 };
@@ -191,6 +193,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 		else
 			*val = !!(fault & (1 << channel));
 		return 0;
+	case hwmon_fan_enable:
+		*val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -233,6 +238,15 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 				     MAX31790_REG_TARGET_COUNT(channel),
 				     target_count);
 		break;
+	case hwmon_fan_enable:
+		if (val == 0)
+			data->fan_config[channel] &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
+		else
+			data->fan_config[channel] |= MAX31790_FAN_CFG_TACH_INPUT_EN;
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_CONFIG(channel),
+				   data->fan_config[channel]);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -260,6 +274,11 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
 			return 0644;
 		return 0;
+	case hwmon_fan_enable:
+		if (channel < NR_CHANNEL ||
+		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
+			return 0644;
+		return 0;
 	default:
 		return 0;
 	}
@@ -281,12 +300,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
 		*val = read >> 8;
 		return 0;
 	case hwmon_pwm_enable:
-		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
+		if (data->full_speed[channel])
+			*val = 0;
+		else if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
 			*val = 2;
-		else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
+		else
 			*val = 1;
-		else
-			*val = 0;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -305,28 +324,42 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 
 	switch (attr) {
 	case hwmon_pwm_input:
-		if (val < 0 || val > 255) {
+		if (data->full_speed[channel] || val < 0 || val > 255) {
 			err = -EINVAL;
 			break;
 		}
+
 		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
 		break;
 	case hwmon_pwm_enable:
 		fan_config = data->fan_config[channel];
-		if (val == 0) {
-			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
-					MAX31790_FAN_CFG_RPM_MODE);
-		} else if (val == 1) {
-			fan_config = (fan_config |
-				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
-				     ~MAX31790_FAN_CFG_RPM_MODE;
+		if (val == 0 || val == 1) {
+			fan_config &= ~MAX31790_FAN_CFG_RPM_MODE;
 		} else if (val == 2) {
-			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
-				      MAX31790_FAN_CFG_RPM_MODE;
+			fan_config |= MAX31790_FAN_CFG_RPM_MODE;
 		} else {
 			err = -EINVAL;
 			break;
 		}
+
+		/*
+		 * The chip sets PWM to zero when using its "monitor only" mode
+		 * and 0 means full speed.
+		 */
+		if (val == 0) {
+			data->full_speed[channel] = true;
+			err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), MAX_PWM);
+		} else {
+			data->full_speed[channel] = false;
+		}
+
+		/*
+		 * RPM mode implies enabled TACH input, so enable it in RPM
+		 * mode.
+		 */
+		if (val == 2)
+			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
+
 		data->fan_config[channel] = fan_config;
 		err = regmap_write(regmap,
 				   MAX31790_REG_FAN_CONFIG(channel),
@@ -400,18 +433,18 @@ static umode_t max31790_is_visible(const void *data,
 
 static const struct hwmon_channel_info *max31790_info[] = {
 	HWMON_CHANNEL_INFO(fan,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT),
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
 	HWMON_CHANNEL_INFO(pwm,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
@@ -448,6 +481,8 @@ static int max31790_init_client(struct regmap *regmap,
 		if (rv < 0)
 			return rv;
 		data->fan_dynamics[i] = rv;
+
+		data->full_speed[i] = false;
 	}
 
 	return 0;
-- 
2.31.1

Re: [PATCH v2 2/5] hwmon: (max31790) Fix and split pwm*_enable

[Václav Kubernát]

Okay, no problem, I'll get rid of the full-speed mode (and update the
documentation) in the next version of the patch. Do you think I should
disable the value "0" in pwm*_enable?

Václav

út 20. 4. 2021 v 0:33 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On Tue, Apr 13, 2021 at 04:59:45AM +0200, Václav Kubernát wrote:
> > In the old code, pwm*_enable does two things. Firstly, it sets whether
> > the chip should run in PWM or RPM mode. Secondly, it tells the chip
> > whether it should monitor fan RPM. However, these two settings aren't
> > tied together, so they shouldn't be set with a single value. In the new
> > code, fan*_enable now controls fan RPM monitoring (pwm*_enable no longer
> > controls that).
> >
> > According to the sysfs hwmon documentation, pwm*_enable has three
> > possible values, 0 for "no control / full-speed", 1 for manual mode, and
> > 2+ for automatic. The old code works fine for 1 and 2, but 0 only
> > differs from 1 in that it just turns off fan speed monitoring. The chip
> > actually does have a way to turn off fan controls (and only monitor),
> > but what that does is that it sets PWM to 0% duty cycle (which is the
> > opposite to full-speed) AND it also turns off fan speed monitoring.
> > Because of this, I implemented the 0 value by setting PWM mode to 100%.
> > This method does come with a problem: it is impossible to differentiate
> > between full-speed and PWM mode just from the values on the chip. The
> > new code solves this by saving a value indicating whether we're in
> > full-speed mode. This value is initialized to 0, so full-speed mode
> > won't persist across reboots.
> >
> I don't think this is a good idea, sorry. It is not just a problem across
> reboots, but also when unloading and reloading the driver. I think we
> should stick with chip capabilities and adjust the documentation
> accordingly.
>
> Guenter
>
> > These two changes are closely connected together, mainly because the
> > detection of the pwm*_enable value depended on whether fan speed
> > monitoring is enabled (which is now controlled as written in the first
> > paragraph).
> >
> > Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> > ---
> >  Documentation/hwmon/max31790.rst |  8 +--
> >  drivers/hwmon/max31790.c         | 87 ++++++++++++++++++++++----------
> >  2 files changed, 66 insertions(+), 29 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> > index f301385d8cef..8979c8a02cd1 100644
> > --- a/Documentation/hwmon/max31790.rst
> > +++ b/Documentation/hwmon/max31790.rst
> > @@ -34,10 +34,12 @@ also be configured to serve as tachometer inputs.
> >  Sysfs entries
> >  -------------
> >
> > -================== === =======================================================
> > +================== === =============================================================
> > +fan[1-12]_enable   RW  enable fan speed monitoring
> >  fan[1-12]_input    RO  fan tachometer speed in RPM
> >  fan[1-12]_fault    RO  fan experienced fault
> >  fan[1-6]_target    RW  desired fan speed in RPM
> > -pwm[1-6]_enable    RW  regulator mode, 0=disabled, 1=manual mode, 2=rpm mode
> > +pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
> > +                       setting rpm mode sets fan*_enable to 1
> >  pwm[1-6]           RW  fan target duty cycle (0-255)
> > -================== === =======================================================
> > +================== === =============================================================
> > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> > index e3765ce4444a..ecdd55e12ffe 100644
> > --- a/drivers/hwmon/max31790.c
> > +++ b/drivers/hwmon/max31790.c
> > @@ -39,6 +39,7 @@
> >
> >  #define FAN_RPM_MIN                  120
> >  #define FAN_RPM_MAX                  7864320
> > +#define MAX_PWM                              0XFF80
> >
> >  #define RPM_FROM_REG(reg, sr)                (((reg) >> 4) ? \
> >                                        ((60 * (sr) * 8192) / ((reg) >> 4)) : \
> > @@ -90,6 +91,7 @@ struct max31790_data {
> >       struct regmap *regmap;
> >
> >       struct mutex update_lock;
> > +     bool full_speed[NR_CHANNEL];
> >       u8 fan_config[NR_CHANNEL];
> >       u8 fan_dynamics[NR_CHANNEL];
> >  };
> > @@ -191,6 +193,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> >               else
> >                       *val = !!(fault & (1 << channel));
> >               return 0;
> > +     case hwmon_fan_enable:
> > +             *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN);
> > +             return 0;
> >       default:
> >               return -EOPNOTSUPP;
> >       }
> > @@ -233,6 +238,15 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
> >                                    MAX31790_REG_TARGET_COUNT(channel),
> >                                    target_count);
> >               break;
> > +     case hwmon_fan_enable:
> > +             if (val == 0)
> > +                     data->fan_config[channel] &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
> > +             else
> > +                     data->fan_config[channel] |= MAX31790_FAN_CFG_TACH_INPUT_EN;
> > +             err = regmap_write(regmap,
> > +                                MAX31790_REG_FAN_CONFIG(channel),
> > +                                data->fan_config[channel]);
> > +             break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> > @@ -260,6 +274,11 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> >                   !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> >                       return 0644;
> >               return 0;
> > +     case hwmon_fan_enable:
> > +             if (channel < NR_CHANNEL ||
> > +                 (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> > +                     return 0644;
> > +             return 0;
> >       default:
> >               return 0;
> >       }
> > @@ -281,12 +300,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
> >               *val = read >> 8;
> >               return 0;
> >       case hwmon_pwm_enable:
> > -             if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
> > +             if (data->full_speed[channel])
> > +                     *val = 0;
> > +             else if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
> >                       *val = 2;
> > -             else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
> > +             else
> >                       *val = 1;
> > -             else
> > -                     *val = 0;
> >               return 0;
> >       default:
> >               return -EOPNOTSUPP;
> > @@ -305,28 +324,42 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> >
> >       switch (attr) {
> >       case hwmon_pwm_input:
> > -             if (val < 0 || val > 255) {
> > +             if (data->full_speed[channel] || val < 0 || val > 255) {
> >                       err = -EINVAL;
> >                       break;
> >               }
> > +
> >               err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
> >               break;
> >       case hwmon_pwm_enable:
> >               fan_config = data->fan_config[channel];
> > -             if (val == 0) {
> > -                     fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
> > -                                     MAX31790_FAN_CFG_RPM_MODE);
> > -             } else if (val == 1) {
> > -                     fan_config = (fan_config |
> > -                                   MAX31790_FAN_CFG_TACH_INPUT_EN) &
> > -                                  ~MAX31790_FAN_CFG_RPM_MODE;
> > +             if (val == 0 || val == 1) {
> > +                     fan_config &= ~MAX31790_FAN_CFG_RPM_MODE;
> >               } else if (val == 2) {
> > -                     fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> > -                                   MAX31790_FAN_CFG_RPM_MODE;
> > +                     fan_config |= MAX31790_FAN_CFG_RPM_MODE;
> >               } else {
> >                       err = -EINVAL;
> >                       break;
> >               }
> > +
> > +             /*
> > +              * The chip sets PWM to zero when using its "monitor only" mode
> > +              * and 0 means full speed.
> > +              */
> > +             if (val == 0) {
> > +                     data->full_speed[channel] = true;
> > +                     err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), MAX_PWM);
> > +             } else {
> > +                     data->full_speed[channel] = false;
> > +             }
> > +
> > +             /*
> > +              * RPM mode implies enabled TACH input, so enable it in RPM
> > +              * mode.
> > +              */
> > +             if (val == 2)
> > +                     fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
> > +
> >               data->fan_config[channel] = fan_config;
> >               err = regmap_write(regmap,
> >                                  MAX31790_REG_FAN_CONFIG(channel),
> > @@ -400,18 +433,18 @@ static umode_t max31790_is_visible(const void *data,
> >
> >  static const struct hwmon_channel_info *max31790_info[] = {
> >       HWMON_CHANNEL_INFO(fan,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT,
> > -                        HWMON_F_INPUT | HWMON_F_FAULT),
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
> > +                        HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
> >       HWMON_CHANNEL_INFO(pwm,
> >                          HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> >                          HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> > @@ -448,6 +481,8 @@ static int max31790_init_client(struct regmap *regmap,
> >               if (rv < 0)
> >                       return rv;
> >               data->fan_dynamics[i] = rv;
> > +
> > +             data->full_speed[i] = false;
> >       }
> >
> >       return 0;

[PATCH v2 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled

[Václav Kubernát]

When fan speed input is disabled, it makes no sense to show values in
fan*_input and fan*_fault.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 drivers/hwmon/max31790.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index ecdd55e12ffe..4fea32ff63bb 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -166,6 +166,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 
 	switch (attr) {
 	case hwmon_fan_input:
+		if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN))
+			return -ENODATA;
+
 		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
 		if (tach < 0)
 			return tach;
@@ -180,6 +183,11 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_fault:
+		if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)) {
+			*val = 0;
+			return 0;
+		}
+
 		if (channel > 6)
 			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
 		else
-- 
2.31.1

[PATCH v2 4/5] hwmon: (max31790) Allow setting fan*_div

[Václav Kubernát]

Right now, the divisor (which determines the speed range) is only set
when in RPM mode. However, the speed range also affects the input RPM,
which means, to get more accurate readings, this speed range needs to be
set.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst |  1 +
 drivers/hwmon/max31790.c         | 66 ++++++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index 8979c8a02cd1..2979addeac8f 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -38,6 +38,7 @@ Sysfs entries
 fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
+fan[1-12]_div      RW  set the measurable speed range, not available in RPM mode
 fan[1-6]_target    RW  desired fan speed in RPM
 pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
                        setting rpm mode sets fan*_enable to 1
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 4fea32ff63bb..5fdfbf0f2809 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -157,6 +157,26 @@ static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
 	return regmap_bulk_write(regmap, reg, bulk_val, 2);
 }
 
+static int bits_for_speed_range(long speed_range)
+{
+	switch (speed_range) {
+	case 1:
+		return 0x0;
+	case 2:
+		return 0x1;
+	case 4:
+		return 0x2;
+	case 8:
+		return 0x3;
+	case 16:
+		return 0x4;
+	case 32:
+		return 0x5;
+	default:
+		return -1;
+	}
+}
+
 static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
@@ -204,6 +224,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 	case hwmon_fan_enable:
 		*val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN);
 		return 0;
+	case hwmon_fan_div:
+		*val = get_tach_period(data->fan_config[channel]);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -255,6 +278,24 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 				   MAX31790_REG_FAN_CONFIG(channel),
 				   data->fan_config[channel]);
 		break;
+	case hwmon_fan_div:
+		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE) {
+			err = -EINVAL;
+			break;
+		}
+		sr = bits_for_speed_range(val);
+		if (sr < 0) {
+			err = -EINVAL;
+			break;
+		}
+
+		data->fan_dynamics[channel] = ((data->fan_dynamics[channel] &
+						~MAX31790_FAN_DYN_SR_MASK) |
+					       (sr << MAX31790_FAN_DYN_SR_SHIFT));
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_DYNAMICS(channel),
+				   data->fan_dynamics[channel]);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -283,6 +324,7 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 			return 0644;
 		return 0;
 	case hwmon_fan_enable:
+	case hwmon_fan_div:
 		if (channel < NR_CHANNEL ||
 		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
 			return 0644;
@@ -441,18 +483,18 @@ static umode_t max31790_is_visible(const void *data,
 
 static const struct hwmon_channel_info *max31790_info[] = {
 	HWMON_CHANNEL_INFO(fan,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
 	HWMON_CHANNEL_INFO(pwm,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-- 
2.31.1

[PATCH v2 5/5] hwmon: (max31790) Update documentation

[Václav Kubernát]

The conditions for fan fault and its connection to the PWM mode are now
documented.

The pwm_rate_of_change and fan_window are now mentioned. According to
our testing with Sunon PF36281BX-000U-S99, these values are crucial in
how RPM mode works and how long it takes for the RPM to stabilize. For
example, setting 5000 RPM (the fan goes up to 23000), the
pwm_rate_of_change needed to be changed to the lowest possible value,
otherwise the chip would just go from pwm 0 to pwm 60 back and forth and
never achieving 5000 RPM (and also signaling fan fault). Based on this
testing, we found out that the pwm_rate_of_change and fan_window values
need to be changed manually by the user, based on the user's exact fan
configuration.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst | 41 +++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index 2979addeac8f..6056b67c3a95 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -30,6 +30,44 @@ monitoring and control of fan RPM as well as detection of fan failure.
 Six pins are dedicated tachometer inputs. Any of the six PWM outputs can
 also be configured to serve as tachometer inputs.
 
+About pwm[1-6]_enable
+---------------------
+0 - full-speed
+    The chip doesn't have a specific way to set "full speed", so setting
+    pwm[1-6]_enable to 0 is just "set PWM mode with 255 duty cycle".
+1 - PWM mode
+    Fan speed is controlled by writing a value to pwm[1-6].
+2 - RPM mode
+    Fan speed is controlled by writing a value to fan[1-6]_target.
+
+About fan[1-6]_fault
+--------------------
+In PWM (or full-speed) mode, if the input RPM goes below what is set
+in fan[1-6]_target, fan[1-6]_fault gets set to 1. In other words,
+fan[1-6]_target works as the minimum input RPM before a fan fault goes off.
+
+In RPM mode, fan fault is set when the fan spins "too slowly" (exact
+conditions are in the datasheet). RPM mode depends on four variables:
+    target_speed:        This is set by fan[1-6]_target.
+    speed_range:         This is set automatically when setting target_speed
+                         or manually by fan[1-12]_div.
+    pwm_rate_of_change:  NOT set by the driver.
+    fan_window:          NOT set by the driver.
+
+The last two values are not set by the driver, because there's no generic way to
+compute them. You should set them manually through i2c (in the bootloader for
+example). Check the datasheet for details.
+
+The fan fault value latches, to reset it, set a value to pwm[1-6]
+or fan[1-6]_target.
+
+About fan[1-12]_div
+-------------------
+This value affects the measurable range of the chip. The driver sets this value
+automatically in RPM based on fan[1-6]_target. In PWM mode, you should set this
+value manually based on the details from the datasheet. Setting the speed range
+is disabled while in RPM mode to prevent overwriting the automatically
+calculated value.
 
 Sysfs entries
 -------------
@@ -39,7 +77,8 @@ fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
 fan[1-12]_div      RW  set the measurable speed range, not available in RPM mode
-fan[1-6]_target    RW  desired fan speed in RPM
+fan[1-6]_target    RW  RPM mode = desired fan speed
+                       PWM mode = minimum fan speed until fault
 pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
                        setting rpm mode sets fan*_enable to 1
 pwm[1-6]           RW  fan target duty cycle (0-255)
-- 
2.31.1

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hello,

I'm uploading a new version of my patches on max31790. This is a "v3"
patch, but I have mistakenly tagged it as "v2". Hopefully, this is not
a big issue.

Changes:
- I have reintroduced locking. However, I'm not sure if it's enough, I
think, locking needs to happen even when reading, but I'm not sure
- fan_config and fan_dynamics are now saved locally
- I have fixed formatting issues

Václav

út 13. 4. 2021 v 5:02 odesílatel Václav Kubernát <kubernat@cesnet.cz> napsal:
>
> Converting the driver to use regmap makes it more generic. It also makes
> it a lot easier to debug through debugfs.
>
> Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> ---
>  drivers/hwmon/Kconfig    |   1 +
>  drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
>  2 files changed, 133 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..9f11d036c316 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
>  config SENSORS_MAX31790
>         tristate "Maxim MAX31790 sensor chip"
>         depends on I2C
> +       select REGMAP_I2C
>         help
>           If you say yes here you get support for 6-Channel PWM-Output
>           Fan RPM Controller.
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 2c6b333a28e9..e3765ce4444a 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -12,6 +12,7 @@
>  #include <linux/init.h>
>  #include <linux/jiffies.h>
>  #include <linux/module.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>
>  /* MAX31790 registers */
> @@ -46,92 +47,53 @@
>
>  #define NR_CHANNEL                     6
>
> +#define MAX31790_REG_USER_BYTE_67      0x67
> +
> +#define BULK_TO_U16(msb, lsb)          (((msb) << 8) + (lsb))
> +#define U16_MSB(num)                   (((num) & 0xFF00) >> 8)
> +#define U16_LSB(num)                   ((num) & 0x00FF)
> +
> +static const struct regmap_range max31790_ro_range = {
> +       .range_min = MAX31790_REG_TACH_COUNT(0),
> +       .range_max = MAX31790_REG_PWMOUT(0) - 1,
> +};
> +
> +static const struct regmap_access_table max31790_wr_table = {
> +       .no_ranges = &max31790_ro_range,
> +       .n_no_ranges = 1,
> +};
> +
> +static const struct regmap_range max31790_volatile_ranges[] = {
> +       regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> +       regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> +};
> +
> +static const struct regmap_access_table max31790_volatile_table = {
> +       .no_ranges = max31790_volatile_ranges,
> +       .n_no_ranges = 2,
> +       .n_yes_ranges = 0
> +};
> +
> +static const struct regmap_config max31790_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .reg_stride = 1,
> +       .max_register = MAX31790_REG_USER_BYTE_67,
> +       .wr_table = &max31790_wr_table,
> +       .volatile_table = &max31790_volatile_table
> +};
> +
>  /*
>   * Client data (each client gets its own)
>   */
>  struct max31790_data {
> -       struct i2c_client *client;
> +       struct regmap *regmap;
> +
>         struct mutex update_lock;
> -       bool valid; /* zero until following fields are valid */
> -       unsigned long last_updated; /* in jiffies */
> -
> -       /* register values */
>         u8 fan_config[NR_CHANNEL];
>         u8 fan_dynamics[NR_CHANNEL];
> -       u16 fault_status;
> -       u16 tach[NR_CHANNEL * 2];
> -       u16 pwm[NR_CHANNEL];
> -       u16 target_count[NR_CHANNEL];
>  };
>
> -static struct max31790_data *max31790_update_device(struct device *dev)
> -{
> -       struct max31790_data *data = dev_get_drvdata(dev);
> -       struct i2c_client *client = data->client;
> -       struct max31790_data *ret = data;
> -       int i;
> -       int rv;
> -
> -       mutex_lock(&data->update_lock);
> -
> -       if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> -               rv = i2c_smbus_read_byte_data(client,
> -                               MAX31790_REG_FAN_FAULT_STATUS1);
> -               if (rv < 0)
> -                       goto abort;
> -               data->fault_status = rv & 0x3F;
> -
> -               rv = i2c_smbus_read_byte_data(client,
> -                               MAX31790_REG_FAN_FAULT_STATUS2);
> -               if (rv < 0)
> -                       goto abort;
> -               data->fault_status |= (rv & 0x3F) << 6;
> -
> -               for (i = 0; i < NR_CHANNEL; i++) {
> -                       rv = i2c_smbus_read_word_swapped(client,
> -                                       MAX31790_REG_TACH_COUNT(i));
> -                       if (rv < 0)
> -                               goto abort;
> -                       data->tach[i] = rv;
> -
> -                       if (data->fan_config[i]
> -                           & MAX31790_FAN_CFG_TACH_INPUT) {
> -                               rv = i2c_smbus_read_word_swapped(client,
> -                                       MAX31790_REG_TACH_COUNT(NR_CHANNEL
> -                                                               + i));
> -                               if (rv < 0)
> -                                       goto abort;
> -                               data->tach[NR_CHANNEL + i] = rv;
> -                       } else {
> -                               rv = i2c_smbus_read_word_swapped(client,
> -                                               MAX31790_REG_PWMOUT(i));
> -                               if (rv < 0)
> -                                       goto abort;
> -                               data->pwm[i] = rv;
> -
> -                               rv = i2c_smbus_read_word_swapped(client,
> -                                               MAX31790_REG_TARGET_COUNT(i));
> -                               if (rv < 0)
> -                                       goto abort;
> -                               data->target_count[i] = rv;
> -                       }
> -               }
> -
> -               data->last_updated = jiffies;
> -               data->valid = true;
> -       }
> -       goto done;
> -
> -abort:
> -       data->valid = false;
> -       ret = ERR_PTR(rv);
> -
> -done:
> -       mutex_unlock(&data->update_lock);
> -
> -       return ret;
> -}
> -
>  static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
>
>  static u8 get_tach_period(u8 fan_dynamics)
> @@ -159,28 +121,75 @@ static u8 bits_for_tach_period(int rpm)
>         return bits;
>  }
>
> +static int read_reg_byte(struct regmap *regmap, u8 reg)
> +{
> +       int rv;
> +       int val;
> +
> +       rv = regmap_read(regmap, reg, &val);
> +       if (rv < 0)
> +               return rv;
> +
> +       return val;
> +}
> +
> +static int read_reg_word(struct regmap *regmap, u8 reg)
> +{
> +       int rv;
> +       u8 val_bulk[2];
> +
> +       rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
> +       if (rv < 0)
> +               return rv;
> +
> +       return BULK_TO_U16(val_bulk[0], val_bulk[1]);
> +}
> +
> +static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
> +{
> +       u8 bulk_val[2];
> +
> +       bulk_val[0] = U16_MSB(val);
> +       bulk_val[1] = U16_LSB(val);
> +
> +       return regmap_bulk_write(regmap, reg, bulk_val, 2);
> +}
> +
>  static int max31790_read_fan(struct device *dev, u32 attr, int channel,
>                              long *val)
>  {
> -       struct max31790_data *data = max31790_update_device(dev);
> -       int sr, rpm;
> -
> -       if (IS_ERR(data))
> -               return PTR_ERR(data);
> +       struct max31790_data *data = dev_get_drvdata(dev);
> +       struct regmap *regmap = data->regmap;
> +       int tach, fault;
>
>         switch (attr) {
>         case hwmon_fan_input:
> -               sr = get_tach_period(data->fan_dynamics[channel]);
> -               rpm = RPM_FROM_REG(data->tach[channel], sr);
> -               *val = rpm;
> +               tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
> +               if (tach < 0)
> +                       return tach;
> +
> +               *val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
>                 return 0;
>         case hwmon_fan_target:
> -               sr = get_tach_period(data->fan_dynamics[channel]);
> -               rpm = RPM_FROM_REG(data->target_count[channel], sr);
> -               *val = rpm;
> +               tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
> +               if (tach < 0)
> +                       return tach;
> +
> +               *val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
>                 return 0;
>         case hwmon_fan_fault:
> -               *val = !!(data->fault_status & (1 << channel));
> +               if (channel > 6)
> +                       fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
> +               else
> +                       fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
> +
> +               if (fault < 0)
> +                       return fault;
> +
> +               if (channel > 6)
> +                       *val = !!(fault & (1 << (channel - 6)));
> +               else
> +                       *val = !!(fault & (1 << channel));
>                 return 0;
>         default:
>                 return -EOPNOTSUPP;
> @@ -191,7 +200,7 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>                               long val)
>  {
>         struct max31790_data *data = dev_get_drvdata(dev);
> -       struct i2c_client *client = data->client;
> +       struct regmap *regmap = data->regmap;
>         int target_count;
>         int err = 0;
>         u8 bits;
> @@ -207,9 +216,10 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>                         ((data->fan_dynamics[channel] &
>                           ~MAX31790_FAN_DYN_SR_MASK) |
>                          (bits << MAX31790_FAN_DYN_SR_SHIFT));
> -               err = i2c_smbus_write_byte_data(client,
> -                                       MAX31790_REG_FAN_DYNAMICS(channel),
> -                                       data->fan_dynamics[channel]);
> +
> +               err = regmap_write(regmap,
> +                                  MAX31790_REG_FAN_DYNAMICS(channel),
> +                                  data->fan_dynamics[channel]);
>                 if (err < 0)
>                         break;
>
> @@ -217,11 +227,11 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
>                 target_count = RPM_TO_REG(val, sr);
>                 target_count = clamp_val(target_count, 0x1, 0x7FF);
>
> -               data->target_count[channel] = target_count << 5;
> +               target_count = target_count << 5;
>
> -               err = i2c_smbus_write_word_swapped(client,
> -                                       MAX31790_REG_TARGET_COUNT(channel),
> -                                       data->target_count[channel]);
> +               err = write_reg_word(regmap,
> +                                    MAX31790_REG_TARGET_COUNT(channel),
> +                                    target_count);
>                 break;
>         default:
>                 err = -EOPNOTSUPP;
> @@ -258,22 +268,22 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
>  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
>                              long *val)
>  {
> -       struct max31790_data *data = max31790_update_device(dev);
> -       u8 fan_config;
> -
> -       if (IS_ERR(data))
> -               return PTR_ERR(data);
> -
> -       fan_config = data->fan_config[channel];
> +       struct max31790_data *data = dev_get_drvdata(dev);
> +       struct regmap *regmap = data->regmap;
> +       int read;
>
>         switch (attr) {
>         case hwmon_pwm_input:
> -               *val = data->pwm[channel] >> 8;
> +               read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> +               if (read < 0)
> +                       return read;
> +
> +               *val = read >> 8;
>                 return 0;
>         case hwmon_pwm_enable:
> -               if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
> +               if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
>                         *val = 2;
> -               else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
> +               else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
>                         *val = 1;
>                 else
>                         *val = 0;
> @@ -287,7 +297,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>                               long val)
>  {
>         struct max31790_data *data = dev_get_drvdata(dev);
> -       struct i2c_client *client = data->client;
> +       struct regmap *regmap = data->regmap;
>         u8 fan_config;
>         int err = 0;
>
> @@ -299,10 +309,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>                         err = -EINVAL;
>                         break;
>                 }
> -               data->pwm[channel] = val << 8;
> -               err = i2c_smbus_write_word_swapped(client,
> -                                                  MAX31790_REG_PWMOUT(channel),
> -                                                  data->pwm[channel]);
> +               err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
>                 break;
>         case hwmon_pwm_enable:
>                 fan_config = data->fan_config[channel];
> @@ -321,9 +328,9 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>                         break;
>                 }
>                 data->fan_config[channel] = fan_config;
> -               err = i2c_smbus_write_byte_data(client,
> -                                       MAX31790_REG_FAN_CONFIG(channel),
> -                                       fan_config);
> +               err = regmap_write(regmap,
> +                                  MAX31790_REG_FAN_CONFIG(channel),
> +                                  fan_config);
>                 break;
>         default:
>                 err = -EOPNOTSUPP;
> @@ -426,20 +433,18 @@ static const struct hwmon_chip_info max31790_chip_info = {
>         .info = max31790_info,
>  };
>
> -static int max31790_init_client(struct i2c_client *client,
> +static int max31790_init_client(struct regmap *regmap,
>                                 struct max31790_data *data)
>  {
>         int i, rv;
>
>         for (i = 0; i < NR_CHANNEL; i++) {
> -               rv = i2c_smbus_read_byte_data(client,
> -                               MAX31790_REG_FAN_CONFIG(i));
> +               rv = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(i % NR_CHANNEL));
>                 if (rv < 0)
>                         return rv;
>                 data->fan_config[i] = rv;
>
> -               rv = i2c_smbus_read_byte_data(client,
> -                               MAX31790_REG_FAN_DYNAMICS(i));
> +               rv = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(i));
>                 if (rv < 0)
>                         return rv;
>                 data->fan_dynamics[i] = rv;
> @@ -464,13 +469,18 @@ static int max31790_probe(struct i2c_client *client)
>         if (!data)
>                 return -ENOMEM;
>
> -       data->client = client;
>         mutex_init(&data->update_lock);
>
> +       data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
> +       if (IS_ERR(data->regmap)) {
> +               dev_err(dev, "failed to allocate register map\n");
> +               return PTR_ERR(data->regmap);
> +       }
> +
>         /*
>          * Initialize the max31790 chip
>          */
> -       err = max31790_init_client(client, data);
> +       err = max31790_init_client(data->regmap, data);
>         if (err)
>                 return err;
>
> --
> 2.31.1
>

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Guenter Roeck]

On 4/12/21 7:59 PM, Václav Kubernát wrote:
> Converting the driver to use regmap makes it more generic. It also makes
> it a lot easier to debug through debugfs.
> 
> Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> ---
>  drivers/hwmon/Kconfig    |   1 +
>  drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
>  2 files changed, 133 insertions(+), 122 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 1ecf697d8d99..9f11d036c316 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
>  config SENSORS_MAX31790
>  	tristate "Maxim MAX31790 sensor chip"
>  	depends on I2C
> +	select REGMAP_I2C
>  	help
>  	  If you say yes here you get support for 6-Channel PWM-Output
>  	  Fan RPM Controller.
> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> index 2c6b333a28e9..e3765ce4444a 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -12,6 +12,7 @@
>  #include <linux/init.h>
>  #include <linux/jiffies.h>
>  #include <linux/module.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  
>  /* MAX31790 registers */
> @@ -46,92 +47,53 @@
>  
>  #define NR_CHANNEL			6
>  
> +#define MAX31790_REG_USER_BYTE_67	0x67
> +
> +#define BULK_TO_U16(msb, lsb)		(((msb) << 8) + (lsb))
> +#define U16_MSB(num)			(((num) & 0xFF00) >> 8)
> +#define U16_LSB(num)			((num) & 0x00FF)
> +
> +static const struct regmap_range max31790_ro_range = {
> +	.range_min = MAX31790_REG_TACH_COUNT(0),
> +	.range_max = MAX31790_REG_PWMOUT(0) - 1,
> +};
> +
> +static const struct regmap_access_table max31790_wr_table = {
> +	.no_ranges = &max31790_ro_range,
> +	.n_no_ranges = 1,
> +};
> +
> +static const struct regmap_range max31790_volatile_ranges[] = {
> +	regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> +	regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> +};
> +
> +static const struct regmap_access_table max31790_volatile_table = {
> +	.no_ranges = max31790_volatile_ranges,
> +	.n_no_ranges = 2,
> +	.n_yes_ranges = 0
> +};

Looks like my reply to this got lost. Other regmap code suggests that
volatile register ranges are identified with yes_ranges, not with no_ranges.
"no" seems to mean "not volatile". Please verify and confirm if the
above code does what you want it to do.

Thanks,
Guenter

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hello.

I agree that it makes sense to swap yes_ranges and no_ranges. I tested
that, but it seems like it doesn't have an effect, I could still see
fan*_input changing (that's where I don't want caching). Does caching
work automatically? As in, all registers are cached by default in
regmap, and only registers that are in the volatile yes_ranges aren't?

Václav

čt 22. 4. 2021 v 3:31 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On 4/12/21 7:59 PM, Václav Kubernát wrote:
> > Converting the driver to use regmap makes it more generic. It also makes
> > it a lot easier to debug through debugfs.
> >
> > Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> > ---
> >  drivers/hwmon/Kconfig    |   1 +
> >  drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
> >  2 files changed, 133 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 1ecf697d8d99..9f11d036c316 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
> >  config SENSORS_MAX31790
> >       tristate "Maxim MAX31790 sensor chip"
> >       depends on I2C
> > +     select REGMAP_I2C
> >       help
> >         If you say yes here you get support for 6-Channel PWM-Output
> >         Fan RPM Controller.
> > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> > index 2c6b333a28e9..e3765ce4444a 100644
> > --- a/drivers/hwmon/max31790.c
> > +++ b/drivers/hwmon/max31790.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/init.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/module.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >
> >  /* MAX31790 registers */
> > @@ -46,92 +47,53 @@
> >
> >  #define NR_CHANNEL                   6
> >
> > +#define MAX31790_REG_USER_BYTE_67    0x67
> > +
> > +#define BULK_TO_U16(msb, lsb)                (((msb) << 8) + (lsb))
> > +#define U16_MSB(num)                 (((num) & 0xFF00) >> 8)
> > +#define U16_LSB(num)                 ((num) & 0x00FF)
> > +
> > +static const struct regmap_range max31790_ro_range = {
> > +     .range_min = MAX31790_REG_TACH_COUNT(0),
> > +     .range_max = MAX31790_REG_PWMOUT(0) - 1,
> > +};
> > +
> > +static const struct regmap_access_table max31790_wr_table = {
> > +     .no_ranges = &max31790_ro_range,
> > +     .n_no_ranges = 1,
> > +};
> > +
> > +static const struct regmap_range max31790_volatile_ranges[] = {
> > +     regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> > +     regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> > +};
> > +
> > +static const struct regmap_access_table max31790_volatile_table = {
> > +     .no_ranges = max31790_volatile_ranges,
> > +     .n_no_ranges = 2,
> > +     .n_yes_ranges = 0
> > +};
>
> Looks like my reply to this got lost. Other regmap code suggests that
> volatile register ranges are identified with yes_ranges, not with no_ranges.
> "no" seems to mean "not volatile". Please verify and confirm if the
> above code does what you want it to do.
>
> Thanks,
> Guenter

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hello.

I'm sending a new version of my patch on max31790. This new version
fixes the cache issue and actually makes it work by setting
.cache_type. You were right about the "yes/no" ranges, so I flipped
those.

By the way, it seems that the reason your reply got lost is because of
weird addresses in the "Cc:" email field, they end with "cesnet.cz",
so it could be that I'm sending email incorrectly. Let me know if I'm
doing something wrong.

Thanks,
Václav

čt 22. 4. 2021 v 3:31 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On 4/12/21 7:59 PM, Václav Kubernát wrote:
> > Converting the driver to use regmap makes it more generic. It also makes
> > it a lot easier to debug through debugfs.
> >
> > Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> > ---
> >  drivers/hwmon/Kconfig    |   1 +
> >  drivers/hwmon/max31790.c | 254 ++++++++++++++++++++-------------------
> >  2 files changed, 133 insertions(+), 122 deletions(-)
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 1ecf697d8d99..9f11d036c316 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
> >  config SENSORS_MAX31790
> >       tristate "Maxim MAX31790 sensor chip"
> >       depends on I2C
> > +     select REGMAP_I2C
> >       help
> >         If you say yes here you get support for 6-Channel PWM-Output
> >         Fan RPM Controller.
> > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> > index 2c6b333a28e9..e3765ce4444a 100644
> > --- a/drivers/hwmon/max31790.c
> > +++ b/drivers/hwmon/max31790.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/init.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/module.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >
> >  /* MAX31790 registers */
> > @@ -46,92 +47,53 @@
> >
> >  #define NR_CHANNEL                   6
> >
> > +#define MAX31790_REG_USER_BYTE_67    0x67
> > +
> > +#define BULK_TO_U16(msb, lsb)                (((msb) << 8) + (lsb))
> > +#define U16_MSB(num)                 (((num) & 0xFF00) >> 8)
> > +#define U16_LSB(num)                 ((num) & 0x00FF)
> > +
> > +static const struct regmap_range max31790_ro_range = {
> > +     .range_min = MAX31790_REG_TACH_COUNT(0),
> > +     .range_max = MAX31790_REG_PWMOUT(0) - 1,
> > +};
> > +
> > +static const struct regmap_access_table max31790_wr_table = {
> > +     .no_ranges = &max31790_ro_range,
> > +     .n_no_ranges = 1,
> > +};
> > +
> > +static const struct regmap_range max31790_volatile_ranges[] = {
> > +     regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> > +     regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> > +};
> > +
> > +static const struct regmap_access_table max31790_volatile_table = {
> > +     .no_ranges = max31790_volatile_ranges,
> > +     .n_no_ranges = 2,
> > +     .n_yes_ranges = 0
> > +};
>
> Looks like my reply to this got lost. Other regmap code suggests that
> volatile register ranges are identified with yes_ranges, not with no_ranges.
> "no" seems to mean "not volatile". Please verify and confirm if the
> above code does what you want it to do.
>
> Thanks,
> Guenter

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Guenter Roeck]

On Mon, Apr 26, 2021 at 02:46:27PM +0200, Václav Kubernát wrote:
> Hello.
> 
> I'm sending a new version of my patch on max31790. This new version
> fixes the cache issue and actually makes it work by setting
> .cache_type. You were right about the "yes/no" ranges, so I flipped
> those.
> 
> By the way, it seems that the reason your reply got lost is because of
> weird addresses in the "Cc:" email field, they end with "cesnet.cz",
> so it could be that I'm sending email incorrectly. Let me know if I'm
> doing something wrong.
> 

Yes, the To: field of your series is either empty (for the first patch
of the series), or it is something like:
	To: unlisted-recipients: no To-header on input <;

Also, you send your follow-up series as response of the previous series
which doesn't follow the guidance for submitting patches and may result
in the entire series getting lost.

Guenter

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

po 26. 4. 2021 v 16:18 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On Mon, Apr 26, 2021 at 02:46:27PM +0200, Václav Kubernát wrote:
> > Hello.
> >
> > I'm sending a new version of my patch on max31790. This new version
> > fixes the cache issue and actually makes it work by setting
> > .cache_type. You were right about the "yes/no" ranges, so I flipped
> > those.
> >
> > By the way, it seems that the reason your reply got lost is because of
> > weird addresses in the "Cc:" email field, they end with "cesnet.cz",
> > so it could be that I'm sending email incorrectly. Let me know if I'm
> > doing something wrong.
> >
>
> Yes, the To: field of your series is either empty (for the first patch
> of the series), or it is something like:
>         To: unlisted-recipients: no To-header on input <;
>
> Also, you send your follow-up series as response of the previous series
> which doesn't follow the guidance for submitting patches and may result
> in the entire series getting lost.
>

Sorry, I will fix my email-sending procedure. Should I resend the
patch series without the In-Reply-To field?

Václav

> Guenter

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Guenter Roeck]

On Mon, Apr 26, 2021 at 04:29:55PM +0200, Václav Kubernát wrote:
> po 26. 4. 2021 v 16:18 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
> >
> > On Mon, Apr 26, 2021 at 02:46:27PM +0200, Václav Kubernát wrote:
> > > Hello.
> > >
> > > I'm sending a new version of my patch on max31790. This new version
> > > fixes the cache issue and actually makes it work by setting
> > > .cache_type. You were right about the "yes/no" ranges, so I flipped
> > > those.
> > >
> > > By the way, it seems that the reason your reply got lost is because of
> > > weird addresses in the "Cc:" email field, they end with "cesnet.cz",
> > > so it could be that I'm sending email incorrectly. Let me know if I'm
> > > doing something wrong.
> > >
> >
> > Yes, the To: field of your series is either empty (for the first patch
> > of the series), or it is something like:
> >         To: unlisted-recipients: no To-header on input <;
> >
> > Also, you send your follow-up series as response of the previous series
> > which doesn't follow the guidance for submitting patches and may result
> > in the entire series getting lost.
> >
> 
> Sorry, I will fix my email-sending procedure. Should I resend the
> patch series without the In-Reply-To field?
> 
No, just keep it in mind for next time.

Thanks,
Guenter

[PATCH v4 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Converting the driver to use regmap makes it more generic. It also makes
it a lot easier to debug through debugfs.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 drivers/hwmon/Kconfig    |   1 +
 drivers/hwmon/max31790.c | 255 ++++++++++++++++++++-------------------
 2 files changed, 134 insertions(+), 122 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 1ecf697d8d99..9f11d036c316 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1095,6 +1095,7 @@ config SENSORS_MAX6697
 config SENSORS_MAX31790
 	tristate "Maxim MAX31790 sensor chip"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for 6-Channel PWM-Output
 	  Fan RPM Controller.
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 86e6c71db685..8b90fcc685f5 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 /* MAX31790 registers */
@@ -46,92 +47,54 @@
 
 #define NR_CHANNEL			6
 
+#define MAX31790_REG_USER_BYTE_67	0x67
+
+#define BULK_TO_U16(msb, lsb)		(((msb) << 8) + (lsb))
+#define U16_MSB(num)			(((num) & 0xFF00) >> 8)
+#define U16_LSB(num)			((num) & 0x00FF)
+
+static const struct regmap_range max31790_ro_range = {
+	.range_min = MAX31790_REG_TACH_COUNT(0),
+	.range_max = MAX31790_REG_PWMOUT(0) - 1,
+};
+
+static const struct regmap_access_table max31790_wr_table = {
+	.no_ranges = &max31790_ro_range,
+	.n_no_ranges = 1,
+};
+
+static const struct regmap_range max31790_volatile_ranges[] = {
+	regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
+	regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
+};
+
+static const struct regmap_access_table max31790_volatile_table = {
+	.yes_ranges = max31790_volatile_ranges,
+	.n_no_ranges = 0,
+	.n_yes_ranges = 2
+};
+
+static const struct regmap_config max31790_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_register = MAX31790_REG_USER_BYTE_67,
+	.wr_table = &max31790_wr_table,
+	.volatile_table = &max31790_volatile_table,
+	.cache_type = REGCACHE_RBTREE
+};
+
 /*
  * Client data (each client gets its own)
  */
 struct max31790_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
+
 	struct mutex update_lock;
-	bool valid; /* zero until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
-
-	/* register values */
 	u8 fan_config[NR_CHANNEL];
 	u8 fan_dynamics[NR_CHANNEL];
-	u16 fault_status;
-	u16 tach[NR_CHANNEL * 2];
-	u16 pwm[NR_CHANNEL];
-	u16 target_count[NR_CHANNEL];
 };
 
-static struct max31790_data *max31790_update_device(struct device *dev)
-{
-	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	struct max31790_data *ret = data;
-	int i;
-	int rv;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS1);
-		if (rv < 0)
-			goto abort;
-		data->fault_status = rv & 0x3F;
-
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS2);
-		if (rv < 0)
-			goto abort;
-		data->fault_status |= (rv & 0x3F) << 6;
-
-		for (i = 0; i < NR_CHANNEL; i++) {
-			rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(i));
-			if (rv < 0)
-				goto abort;
-			data->tach[i] = rv;
-
-			if (data->fan_config[i]
-			    & MAX31790_FAN_CFG_TACH_INPUT) {
-				rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(NR_CHANNEL
-								+ i));
-				if (rv < 0)
-					goto abort;
-				data->tach[NR_CHANNEL + i] = rv;
-			} else {
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_PWMOUT(i));
-				if (rv < 0)
-					goto abort;
-				data->pwm[i] = rv;
-
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_TARGET_COUNT(i));
-				if (rv < 0)
-					goto abort;
-				data->target_count[i] = rv;
-			}
-		}
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	goto done;
-
-abort:
-	data->valid = false;
-	ret = ERR_PTR(rv);
-
-done:
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
 static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
 
 static u8 get_tach_period(u8 fan_dynamics)
@@ -159,28 +122,75 @@ static u8 bits_for_tach_period(int rpm)
 	return bits;
 }
 
+static int read_reg_byte(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	int val;
+
+	rv = regmap_read(regmap, reg, &val);
+	if (rv < 0)
+		return rv;
+
+	return val;
+}
+
+static int read_reg_word(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	u8 val_bulk[2];
+
+	rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
+	if (rv < 0)
+		return rv;
+
+	return BULK_TO_U16(val_bulk[0], val_bulk[1]);
+}
+
+static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
+{
+	u8 bulk_val[2];
+
+	bulk_val[0] = U16_MSB(val);
+	bulk_val[1] = U16_LSB(val);
+
+	return regmap_bulk_write(regmap, reg, bulk_val, 2);
+}
+
 static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	int sr, rpm;
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int tach, fault;
 
 	switch (attr) {
 	case hwmon_fan_input:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->tach[channel], sr);
-		*val = rpm;
+		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_target:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->target_count[channel], sr);
-		*val = rpm;
+		tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_fault:
-		*val = !!(data->fault_status & (1 << channel));
+		if (channel > 6)
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
+		else
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
+
+		if (fault < 0)
+			return fault;
+
+		if (channel > 6)
+			*val = !!(fault & (1 << (channel - 6)));
+		else
+			*val = !!(fault & (1 << channel));
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -191,7 +201,7 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	int target_count;
 	int err = 0;
 	u8 bits;
@@ -207,9 +217,10 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 			((data->fan_dynamics[channel] &
 			  ~MAX31790_FAN_DYN_SR_MASK) |
 			 (bits << MAX31790_FAN_DYN_SR_SHIFT));
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_DYNAMICS(channel),
-					data->fan_dynamics[channel]);
+
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_DYNAMICS(channel),
+				   data->fan_dynamics[channel]);
 		if (err < 0)
 			break;
 
@@ -217,11 +228,11 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 		target_count = RPM_TO_REG(val, sr);
 		target_count = clamp_val(target_count, 0x1, 0x7FF);
 
-		data->target_count[channel] = target_count << 5;
+		target_count = target_count << 5;
 
-		err = i2c_smbus_write_word_swapped(client,
-					MAX31790_REG_TARGET_COUNT(channel),
-					data->target_count[channel]);
+		err = write_reg_word(regmap,
+				     MAX31790_REG_TARGET_COUNT(channel),
+				     target_count);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -258,22 +269,22 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	u8 fan_config;
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
-	fan_config = data->fan_config[channel];
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int read;
 
 	switch (attr) {
 	case hwmon_pwm_input:
-		*val = data->pwm[channel] >> 8;
+		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
+		if (read < 0)
+			return read;
+
+		*val = read >> 8;
 		return 0;
 	case hwmon_pwm_enable:
-		if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
+		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
 			*val = 2;
-		else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
+		else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
 			*val = 1;
 		else
 			*val = 0;
@@ -287,7 +298,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	u8 fan_config;
 	int err = 0;
 
@@ -299,10 +310,7 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			err = -EINVAL;
 			break;
 		}
-		data->pwm[channel] = val << 8;
-		err = i2c_smbus_write_word_swapped(client,
-						   MAX31790_REG_PWMOUT(channel),
-						   data->pwm[channel]);
+		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
 		break;
 	case hwmon_pwm_enable:
 		fan_config = data->fan_config[channel];
@@ -321,9 +329,9 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			break;
 		}
 		data->fan_config[channel] = fan_config;
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_CONFIG(channel),
-					fan_config);
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_CONFIG(channel),
+				   fan_config);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -426,20 +434,18 @@ static const struct hwmon_chip_info max31790_chip_info = {
 	.info = max31790_info,
 };
 
-static int max31790_init_client(struct i2c_client *client,
+static int max31790_init_client(struct regmap *regmap,
 				struct max31790_data *data)
 {
 	int i, rv;
 
 	for (i = 0; i < NR_CHANNEL; i++) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_CONFIG(i));
+		rv = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(i % NR_CHANNEL));
 		if (rv < 0)
 			return rv;
 		data->fan_config[i] = rv;
 
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_DYNAMICS(i));
+		rv = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(i));
 		if (rv < 0)
 			return rv;
 		data->fan_dynamics[i] = rv;
@@ -464,13 +470,18 @@ static int max31790_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
 	mutex_init(&data->update_lock);
 
+	data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
 	/*
 	 * Initialize the max31790 chip
 	 */
-	err = max31790_init_client(client, data);
+	err = max31790_init_client(data->regmap, data);
 	if (err)
 		return err;
 
-- 
2.31.1

[PATCH v4 2/5] hwmon: (max31790) Fix and split pwm*_enable

[Václav Kubernát]

In the old code, pwm*_enable does two things. Firstly, it sets whether
the chip should run in PWM or RPM mode. Secondly, it tells the chip
whether it should monitor fan RPM. However, these two settings aren't
tied together, so they shouldn't be set with a single value. In the new
code, fan*_enable now controls fan RPM monitoring (pwm*_enable no longer
controls that).

According to the sysfs hwmon documentation, pwm*_enable has three
possible values, 0 for "no control / full-speed", 1 for manual mode, and
2+ for automatic. The old code works fine for 1 and 2, but 0 only
differs from 1 in that it just turns off fan speed monitoring. The chip
actually does have a way to turn off fan controls (and only monitor),
but what that does is that it sets PWM to 0% duty cycle (which is the
opposite to full-speed) AND it also turns off fan speed monitoring.
Because of this, I implemented the 0 value by setting PWM mode to 100%.
This method does come with a problem: it is impossible to differentiate
between full-speed and PWM mode just from the values on the chip. The
new code solves this by saving a value indicating whether we're in
full-speed mode. This value is initialized to 0, so full-speed mode
won't persist across reboots.

These two changes are closely connected together, mainly because the
detection of the pwm*_enable value depended on whether fan speed
monitoring is enabled (which is now controlled as written in the first
paragraph).

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst |  8 +--
 drivers/hwmon/max31790.c         | 87 ++++++++++++++++++++++----------
 2 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index f301385d8cef..8979c8a02cd1 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -34,10 +34,12 @@ also be configured to serve as tachometer inputs.
 Sysfs entries
 -------------
 
-================== === =======================================================
+================== === =============================================================
+fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
 fan[1-6]_target    RW  desired fan speed in RPM
-pwm[1-6]_enable    RW  regulator mode, 0=disabled, 1=manual mode, 2=rpm mode
+pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
+                       setting rpm mode sets fan*_enable to 1
 pwm[1-6]           RW  fan target duty cycle (0-255)
-================== === =======================================================
+================== === =============================================================
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 8b90fcc685f5..82d7b8518743 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -39,6 +39,7 @@
 
 #define FAN_RPM_MIN			120
 #define FAN_RPM_MAX			7864320
+#define MAX_PWM				0XFF80
 
 #define RPM_FROM_REG(reg, sr)		(((reg) >> 4) ? \
 					 ((60 * (sr) * 8192) / ((reg) >> 4)) : \
@@ -91,6 +92,7 @@ struct max31790_data {
 	struct regmap *regmap;
 
 	struct mutex update_lock;
+	bool full_speed[NR_CHANNEL];
 	u8 fan_config[NR_CHANNEL];
 	u8 fan_dynamics[NR_CHANNEL];
 };
@@ -192,6 +194,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 		else
 			*val = !!(fault & (1 << channel));
 		return 0;
+	case hwmon_fan_enable:
+		*val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -234,6 +239,15 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 				     MAX31790_REG_TARGET_COUNT(channel),
 				     target_count);
 		break;
+	case hwmon_fan_enable:
+		if (val == 0)
+			data->fan_config[channel] &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
+		else
+			data->fan_config[channel] |= MAX31790_FAN_CFG_TACH_INPUT_EN;
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_CONFIG(channel),
+				   data->fan_config[channel]);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -261,6 +275,11 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
 			return 0644;
 		return 0;
+	case hwmon_fan_enable:
+		if (channel < NR_CHANNEL ||
+		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
+			return 0644;
+		return 0;
 	default:
 		return 0;
 	}
@@ -282,12 +301,12 @@ static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
 		*val = read >> 8;
 		return 0;
 	case hwmon_pwm_enable:
-		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
+		if (data->full_speed[channel])
+			*val = 0;
+		else if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE)
 			*val = 2;
-		else if (data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)
+		else
 			*val = 1;
-		else
-			*val = 0;
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -306,28 +325,42 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 
 	switch (attr) {
 	case hwmon_pwm_input:
-		if (val < 0 || val > 255) {
+		if (data->full_speed[channel] || val < 0 || val > 255) {
 			err = -EINVAL;
 			break;
 		}
+
 		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
 		break;
 	case hwmon_pwm_enable:
 		fan_config = data->fan_config[channel];
-		if (val == 0) {
-			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
-					MAX31790_FAN_CFG_RPM_MODE);
-		} else if (val == 1) {
-			fan_config = (fan_config |
-				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
-				     ~MAX31790_FAN_CFG_RPM_MODE;
+		if (val == 0 || val == 1) {
+			fan_config &= ~MAX31790_FAN_CFG_RPM_MODE;
 		} else if (val == 2) {
-			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
-				      MAX31790_FAN_CFG_RPM_MODE;
+			fan_config |= MAX31790_FAN_CFG_RPM_MODE;
 		} else {
 			err = -EINVAL;
 			break;
 		}
+
+		/*
+		 * The chip sets PWM to zero when using its "monitor only" mode
+		 * and 0 means full speed.
+		 */
+		if (val == 0) {
+			data->full_speed[channel] = true;
+			err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), MAX_PWM);
+		} else {
+			data->full_speed[channel] = false;
+		}
+
+		/*
+		 * RPM mode implies enabled TACH input, so enable it in RPM
+		 * mode.
+		 */
+		if (val == 2)
+			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
+
 		data->fan_config[channel] = fan_config;
 		err = regmap_write(regmap,
 				   MAX31790_REG_FAN_CONFIG(channel),
@@ -401,18 +434,18 @@ static umode_t max31790_is_visible(const void *data,
 
 static const struct hwmon_channel_info *max31790_info[] = {
 	HWMON_CHANNEL_INFO(fan,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT),
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
 	HWMON_CHANNEL_INFO(pwm,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
@@ -449,6 +482,8 @@ static int max31790_init_client(struct regmap *regmap,
 		if (rv < 0)
 			return rv;
 		data->fan_dynamics[i] = rv;
+
+		data->full_speed[i] = false;
 	}
 
 	return 0;
-- 
2.31.1

[PATCH v4 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled

[Václav Kubernát]

When fan speed input is disabled, it makes no sense to show values in
fan*_input and fan*_fault.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 drivers/hwmon/max31790.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 82d7b8518743..842749482ba4 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -167,6 +167,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 
 	switch (attr) {
 	case hwmon_fan_input:
+		if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN))
+			return -ENODATA;
+
 		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
 		if (tach < 0)
 			return tach;
@@ -181,6 +184,11 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 		*val = RPM_FROM_REG(tach, get_tach_period(data->fan_dynamics[channel]));
 		return 0;
 	case hwmon_fan_fault:
+		if (!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN)) {
+			*val = 0;
+			return 0;
+		}
+
 		if (channel > 6)
 			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
 		else
-- 
2.31.1

[PATCH v4 4/5] hwmon: (max31790) Allow setting fan*_div

[Václav Kubernát]

Right now, the divisor (which determines the speed range) is only set
when in RPM mode. However, the speed range also affects the input RPM,
which means, to get more accurate readings, this speed range needs to be
set.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst |  1 +
 drivers/hwmon/max31790.c         | 66 ++++++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index 8979c8a02cd1..2979addeac8f 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -38,6 +38,7 @@ Sysfs entries
 fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
+fan[1-12]_div      RW  set the measurable speed range, not available in RPM mode
 fan[1-6]_target    RW  desired fan speed in RPM
 pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
                        setting rpm mode sets fan*_enable to 1
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 842749482ba4..9f8562d28a3b 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -158,6 +158,26 @@ static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
 	return regmap_bulk_write(regmap, reg, bulk_val, 2);
 }
 
+static int bits_for_speed_range(long speed_range)
+{
+	switch (speed_range) {
+	case 1:
+		return 0x0;
+	case 2:
+		return 0x1;
+	case 4:
+		return 0x2;
+	case 8:
+		return 0x3;
+	case 16:
+		return 0x4;
+	case 32:
+		return 0x5;
+	default:
+		return -1;
+	}
+}
+
 static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
@@ -205,6 +225,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 	case hwmon_fan_enable:
 		*val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN);
 		return 0;
+	case hwmon_fan_div:
+		*val = get_tach_period(data->fan_config[channel]);
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -256,6 +279,24 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 				   MAX31790_REG_FAN_CONFIG(channel),
 				   data->fan_config[channel]);
 		break;
+	case hwmon_fan_div:
+		if (data->fan_config[channel] & MAX31790_FAN_CFG_RPM_MODE) {
+			err = -EINVAL;
+			break;
+		}
+		sr = bits_for_speed_range(val);
+		if (sr < 0) {
+			err = -EINVAL;
+			break;
+		}
+
+		data->fan_dynamics[channel] = ((data->fan_dynamics[channel] &
+						~MAX31790_FAN_DYN_SR_MASK) |
+					       (sr << MAX31790_FAN_DYN_SR_SHIFT));
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_DYNAMICS(channel),
+				   data->fan_dynamics[channel]);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -284,6 +325,7 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 			return 0644;
 		return 0;
 	case hwmon_fan_enable:
+	case hwmon_fan_div:
 		if (channel < NR_CHANNEL ||
 		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
 			return 0644;
@@ -442,18 +484,18 @@ static umode_t max31790_is_visible(const void *data,
 
 static const struct hwmon_channel_info *max31790_info[] = {
 	HWMON_CHANNEL_INFO(fan,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT,
+			   HWMON_F_DIV | HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT),
 	HWMON_CHANNEL_INFO(pwm,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
 			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-- 
2.31.1

[PATCH v4 5/5] hwmon: (max31790) Update documentation

[Václav Kubernát]

The conditions for fan fault and its connection to the PWM mode are now
documented.

The pwm_rate_of_change and fan_window are now mentioned. According to
our testing with Sunon PF36281BX-000U-S99, these values are crucial in
how RPM mode works and how long it takes for the RPM to stabilize. For
example, setting 5000 RPM (the fan goes up to 23000), the
pwm_rate_of_change needed to be changed to the lowest possible value,
otherwise the chip would just go from pwm 0 to pwm 60 back and forth and
never achieving 5000 RPM (and also signaling fan fault). Based on this
testing, we found out that the pwm_rate_of_change and fan_window values
need to be changed manually by the user, based on the user's exact fan
configuration.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 Documentation/hwmon/max31790.rst | 41 +++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index 2979addeac8f..6056b67c3a95 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -30,6 +30,44 @@ monitoring and control of fan RPM as well as detection of fan failure.
 Six pins are dedicated tachometer inputs. Any of the six PWM outputs can
 also be configured to serve as tachometer inputs.
 
+About pwm[1-6]_enable
+---------------------
+0 - full-speed
+    The chip doesn't have a specific way to set "full speed", so setting
+    pwm[1-6]_enable to 0 is just "set PWM mode with 255 duty cycle".
+1 - PWM mode
+    Fan speed is controlled by writing a value to pwm[1-6].
+2 - RPM mode
+    Fan speed is controlled by writing a value to fan[1-6]_target.
+
+About fan[1-6]_fault
+--------------------
+In PWM (or full-speed) mode, if the input RPM goes below what is set
+in fan[1-6]_target, fan[1-6]_fault gets set to 1. In other words,
+fan[1-6]_target works as the minimum input RPM before a fan fault goes off.
+
+In RPM mode, fan fault is set when the fan spins "too slowly" (exact
+conditions are in the datasheet). RPM mode depends on four variables:
+    target_speed:        This is set by fan[1-6]_target.
+    speed_range:         This is set automatically when setting target_speed
+                         or manually by fan[1-12]_div.
+    pwm_rate_of_change:  NOT set by the driver.
+    fan_window:          NOT set by the driver.
+
+The last two values are not set by the driver, because there's no generic way to
+compute them. You should set them manually through i2c (in the bootloader for
+example). Check the datasheet for details.
+
+The fan fault value latches, to reset it, set a value to pwm[1-6]
+or fan[1-6]_target.
+
+About fan[1-12]_div
+-------------------
+This value affects the measurable range of the chip. The driver sets this value
+automatically in RPM based on fan[1-6]_target. In PWM mode, you should set this
+value manually based on the details from the datasheet. Setting the speed range
+is disabled while in RPM mode to prevent overwriting the automatically
+calculated value.
 
 Sysfs entries
 -------------
@@ -39,7 +77,8 @@ fan[1-12]_enable   RW  enable fan speed monitoring
 fan[1-12]_input    RO  fan tachometer speed in RPM
 fan[1-12]_fault    RO  fan experienced fault
 fan[1-12]_div      RW  set the measurable speed range, not available in RPM mode
-fan[1-6]_target    RW  desired fan speed in RPM
+fan[1-6]_target    RW  RPM mode = desired fan speed
+                       PWM mode = minimum fan speed until fault
 pwm[1-6]_enable    RW  regulator mode, 0=full speed, 1=manual (pwm) mode, 2=rpm mode
                        setting rpm mode sets fan*_enable to 1
 pwm[1-6]           RW  fan target duty cycle (0-255)
-- 
2.31.1

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11237 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210316175503.1003051-1-kubernat@cesnet.cz>
References: <20210316175503.1003051-1-kubernat@cesnet.cz>
TO: "Václav Kubernát" <kubernat@cesnet.cz>
CC: "Václav Kubernát" <kubernat@cesnet.cz>
CC: Jean Delvare <jdelvare@suse.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Jonathan Corbet <corbet@lwn.net>
CC: linux-hwmon(a)vger.kernel.org
CC: linux-doc(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org

Hi "Václav,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.12-rc3 next-20210316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
config: x86_64-randconfig-m001-20210316 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'

vim +263 drivers/hwmon/max31790.c

195a4b4298a795 Il Han          2015-08-30  256  
54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  258  {
54187ff9d766b2 Guenter Roeck   2016-07-01  259  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  260  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  261  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  262  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @263  	if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  264  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  265  
54187ff9d766b2 Guenter Roeck   2016-07-01  266  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  267  	case hwmon_fan_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  268  	case hwmon_fan_fault:
54187ff9d766b2 Guenter Roeck   2016-07-01  269  		if (channel < NR_CHANNEL ||
54187ff9d766b2 Guenter Roeck   2016-07-01  270  		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271  			return 0444;
54187ff9d766b2 Guenter Roeck   2016-07-01  272  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  273  	case hwmon_fan_target:
54187ff9d766b2 Guenter Roeck   2016-07-01  274  		if (channel < NR_CHANNEL &&
54187ff9d766b2 Guenter Roeck   2016-07-01  275  		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  277  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  278  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  279  		return 0;
195a4b4298a795 Il Han          2015-08-30  280  	}
195a4b4298a795 Il Han          2015-08-30  281  }
195a4b4298a795 Il Han          2015-08-30  282  
54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  284  			     long *val)
195a4b4298a795 Il Han          2015-08-30  285  {
2c8602cfaeab63 Václav Kubernát 2021-03-16  286  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  287  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  288  	int read;
195a4b4298a795 Il Han          2015-08-30  289  
195a4b4298a795 Il Han          2015-08-30  290  	if (IS_ERR(data))
195a4b4298a795 Il Han          2015-08-30  291  		return PTR_ERR(data);
195a4b4298a795 Il Han          2015-08-30  292  
54187ff9d766b2 Guenter Roeck   2016-07-01  293  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  294  	case hwmon_pwm_input:
2c8602cfaeab63 Václav Kubernát 2021-03-16  295  		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  296  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  297  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  298  
2c8602cfaeab63 Václav Kubernát 2021-03-16  299  		*val = read >> 8;
54187ff9d766b2 Guenter Roeck   2016-07-01  300  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  301  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  302  		read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  303  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  304  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  305  
2c8602cfaeab63 Václav Kubernát 2021-03-16  306  		if (read & MAX31790_FAN_CFG_RPM_MODE)
54187ff9d766b2 Guenter Roeck   2016-07-01  307  			*val = 2;
2c8602cfaeab63 Václav Kubernát 2021-03-16  308  		else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
54187ff9d766b2 Guenter Roeck   2016-07-01  309  			*val = 1;
195a4b4298a795 Il Han          2015-08-30  310  		else
54187ff9d766b2 Guenter Roeck   2016-07-01  311  			*val = 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  312  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  313  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  314  		return -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  315  	}
195a4b4298a795 Il Han          2015-08-30  316  }
195a4b4298a795 Il Han          2015-08-30  317  
54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  319  			      long val)
195a4b4298a795 Il Han          2015-08-30  320  {
195a4b4298a795 Il Han          2015-08-30  321  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  322  	struct regmap *regmap = data->regmap;
54187ff9d766b2 Guenter Roeck   2016-07-01  323  	u8 fan_config;
54187ff9d766b2 Guenter Roeck   2016-07-01  324  	int err = 0;
195a4b4298a795 Il Han          2015-08-30  325  
54187ff9d766b2 Guenter Roeck   2016-07-01  326  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  327  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  328  		if (val < 0 || val > 255) {
54187ff9d766b2 Guenter Roeck   2016-07-01  329  			err = -EINVAL;
54187ff9d766b2 Guenter Roeck   2016-07-01  330  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  331  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  332  		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
195a4b4298a795 Il Han          2015-08-30  333  		break;
54187ff9d766b2 Guenter Roeck   2016-07-01  334  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  335  		fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  336  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @337  		if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  338  			return fan_config;
2c8602cfaeab63 Václav Kubernát 2021-03-16  339  
54187ff9d766b2 Guenter Roeck   2016-07-01  340  		if (val == 0) {
54187ff9d766b2 Guenter Roeck   2016-07-01  341  			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  342  					MAX31790_FAN_CFG_RPM_MODE);
54187ff9d766b2 Guenter Roeck   2016-07-01  343  		} else if (val == 1) {
54187ff9d766b2 Guenter Roeck   2016-07-01  344  			fan_config = (fan_config |
54187ff9d766b2 Guenter Roeck   2016-07-01  345  				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
54187ff9d766b2 Guenter Roeck   2016-07-01  346  				     ~MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  347  		} else if (val == 2) {
54187ff9d766b2 Guenter Roeck   2016-07-01  348  			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  349  				      MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  350  		} else {
54187ff9d766b2 Guenter Roeck   2016-07-01  351  			err = -EINVAL;
195a4b4298a795 Il Han          2015-08-30  352  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  353  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  354  		err = regmap_write(regmap,
54187ff9d766b2 Guenter Roeck   2016-07-01  355  				   MAX31790_REG_FAN_CONFIG(channel),
54187ff9d766b2 Guenter Roeck   2016-07-01  356  				   fan_config);
195a4b4298a795 Il Han          2015-08-30  357  		break;
195a4b4298a795 Il Han          2015-08-30  358  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  359  		err = -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  360  		break;
195a4b4298a795 Il Han          2015-08-30  361  	}
195a4b4298a795 Il Han          2015-08-30  362  
195a4b4298a795 Il Han          2015-08-30  363  	return err;
195a4b4298a795 Il Han          2015-08-30  364  }
195a4b4298a795 Il Han          2015-08-30  365  
54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  367  {
54187ff9d766b2 Guenter Roeck   2016-07-01  368  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  369  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  370  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  371  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @372  	if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  373  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  374  
54187ff9d766b2 Guenter Roeck   2016-07-01  375  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  376  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  377  	case hwmon_pwm_enable:
54187ff9d766b2 Guenter Roeck   2016-07-01  378  		if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  380  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  381  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  382  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  383  	}
54187ff9d766b2 Guenter Roeck   2016-07-01  384  }
195a4b4298a795 Il Han          2015-08-30  385  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31803 bytes --]

[PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Converting the driver to use regmap makes it more generic. It also makes
it a lot easier to debug through debugfs.

Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
---
 drivers/hwmon/Kconfig    |   1 +
 drivers/hwmon/max31790.c | 318 ++++++++++++++++++++-------------------
 2 files changed, 163 insertions(+), 156 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..c2ec57672c4e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1092,6 +1092,7 @@ config SENSORS_MAX6697
 config SENSORS_MAX31790
 	tristate "Maxim MAX31790 sensor chip"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for 6-Channel PWM-Output
 	  Fan RPM Controller.
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 86e6c71db685..4e5add567890 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 /* MAX31790 registers */
@@ -46,92 +47,49 @@
 
 #define NR_CHANNEL			6
 
+#define MAX31790_REG_USER_BYTE_67	0x67
+
+#define BULK_TO_U16(msb, lsb)		(((msb) << 8) + (lsb))
+#define U16_MSB(num)			(((num) & 0xFF00) >> 8)
+#define U16_LSB(num)			((num) & 0x00FF)
+
+static const struct regmap_range max31790_ro_range = {
+	.range_min = MAX31790_REG_TACH_COUNT(0),
+	.range_max = MAX31790_REG_PWMOUT(0) - 1,
+};
+
+static const struct regmap_access_table max31790_wr_table = {
+	.no_ranges = &max31790_ro_range,
+	.n_no_ranges = 1,
+};
+
+static const struct regmap_range max31790_volatile_ranges[] = {
+	regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
+	regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
+};
+
+static const struct regmap_access_table max31790_volatile_table = {
+	.no_ranges = max31790_volatile_ranges,
+	.n_no_ranges = 2,
+	.n_yes_ranges = 0
+};
+
+static const struct regmap_config max31790_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_register = MAX31790_REG_USER_BYTE_67,
+	.wr_table = &max31790_wr_table,
+	.volatile_table = &max31790_volatile_table
+};
+
 /*
  * Client data (each client gets its own)
  */
 struct max31790_data {
-	struct i2c_client *client;
-	struct mutex update_lock;
-	bool valid; /* zero until following fields are valid */
-	unsigned long last_updated; /* in jiffies */
-
-	/* register values */
-	u8 fan_config[NR_CHANNEL];
-	u8 fan_dynamics[NR_CHANNEL];
-	u16 fault_status;
-	u16 tach[NR_CHANNEL * 2];
-	u16 pwm[NR_CHANNEL];
-	u16 target_count[NR_CHANNEL];
+	struct regmap *regmap;
 };
 
-static struct max31790_data *max31790_update_device(struct device *dev)
-{
-	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	struct max31790_data *ret = data;
-	int i;
-	int rv;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS1);
-		if (rv < 0)
-			goto abort;
-		data->fault_status = rv & 0x3F;
-
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_FAULT_STATUS2);
-		if (rv < 0)
-			goto abort;
-		data->fault_status |= (rv & 0x3F) << 6;
-
-		for (i = 0; i < NR_CHANNEL; i++) {
-			rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(i));
-			if (rv < 0)
-				goto abort;
-			data->tach[i] = rv;
-
-			if (data->fan_config[i]
-			    & MAX31790_FAN_CFG_TACH_INPUT) {
-				rv = i2c_smbus_read_word_swapped(client,
-					MAX31790_REG_TACH_COUNT(NR_CHANNEL
-								+ i));
-				if (rv < 0)
-					goto abort;
-				data->tach[NR_CHANNEL + i] = rv;
-			} else {
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_PWMOUT(i));
-				if (rv < 0)
-					goto abort;
-				data->pwm[i] = rv;
-
-				rv = i2c_smbus_read_word_swapped(client,
-						MAX31790_REG_TARGET_COUNT(i));
-				if (rv < 0)
-					goto abort;
-				data->target_count[i] = rv;
-			}
-		}
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	goto done;
-
-abort:
-	data->valid = false;
-	ret = ERR_PTR(rv);
-
-done:
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
 static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
 
 static u8 get_tach_period(u8 fan_dynamics)
@@ -159,28 +117,89 @@ static u8 bits_for_tach_period(int rpm)
 	return bits;
 }
 
+static int read_reg_byte(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	int val;
+
+	rv = regmap_read(regmap, reg, &val);
+
+	if (rv < 0)
+		return rv;
+
+	return val;
+}
+
+static int read_reg_word(struct regmap *regmap, u8 reg)
+{
+	int rv;
+	u8 val_bulk[2];
+
+	rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
+	if (rv < 0)
+		return rv;
+
+	return BULK_TO_U16(val_bulk[0], val_bulk[1]);
+}
+
+static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
+{
+	u8 bulk_val[2];
+
+	bulk_val[0] = U16_MSB(val);
+	bulk_val[1] = U16_LSB(val);
+
+	return regmap_bulk_write(regmap, reg, bulk_val, 2);
+}
+
 static int max31790_read_fan(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	int sr, rpm;
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int rpm, dynamics, tach, fault;
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	switch (attr) {
 	case hwmon_fan_input:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->tach[channel], sr);
+		dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
+		if (dynamics < 0)
+			return dynamics;
+
+		tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
 		*val = rpm;
 		return 0;
 	case hwmon_fan_target:
-		sr = get_tach_period(data->fan_dynamics[channel]);
-		rpm = RPM_FROM_REG(data->target_count[channel], sr);
+		dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
+		if (dynamics < 0)
+			return dynamics;
+
+		tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
+		if (tach < 0)
+			return tach;
+
+		rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
 		*val = rpm;
 		return 0;
 	case hwmon_fan_fault:
-		*val = !!(data->fault_status & (1 << channel));
+		if (channel > 6)
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
+		else
+			fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
+
+		if (fault < 0)
+			return fault;
+
+		if (channel > 6)
+			*val = !!(fault & (1 << (channel - 6)));
+		else
+			*val = !!(fault & (1 << channel));
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -191,52 +210,58 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	int target_count;
 	int err = 0;
 	u8 bits;
 	int sr;
-
-	mutex_lock(&data->update_lock);
+	int fan_dynamics;
 
 	switch (attr) {
 	case hwmon_fan_target:
 		val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
 		bits = bits_for_tach_period(val);
-		data->fan_dynamics[channel] =
-			((data->fan_dynamics[channel] &
+		fan_dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
+
+		if (fan_dynamics < 0)
+			return fan_dynamics;
+
+		fan_dynamics =
+			((fan_dynamics &
 			  ~MAX31790_FAN_DYN_SR_MASK) |
 			 (bits << MAX31790_FAN_DYN_SR_SHIFT));
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_DYNAMICS(channel),
-					data->fan_dynamics[channel]);
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_DYNAMICS(channel),
+				   fan_dynamics);
 		if (err < 0)
 			break;
 
-		sr = get_tach_period(data->fan_dynamics[channel]);
+		sr = get_tach_period(fan_dynamics);
 		target_count = RPM_TO_REG(val, sr);
 		target_count = clamp_val(target_count, 0x1, 0x7FF);
 
-		data->target_count[channel] = target_count << 5;
+		target_count = target_count << 5;
 
-		err = i2c_smbus_write_word_swapped(client,
-					MAX31790_REG_TARGET_COUNT(channel),
-					data->target_count[channel]);
+		err = write_reg_word(regmap,
+				     MAX31790_REG_TARGET_COUNT(channel),
+				     target_count);
 		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return err;
 }
 
 static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 {
 	const struct max31790_data *data = _data;
-	u8 fan_config = data->fan_config[channel % NR_CHANNEL];
+	struct regmap *regmap = data->regmap;
+	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
+
+	if (fan_config < 0)
+		return 0;
 
 	switch (attr) {
 	case hwmon_fan_input:
@@ -258,22 +283,29 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
 static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
 			     long *val)
 {
-	struct max31790_data *data = max31790_update_device(dev);
-	u8 fan_config;
+	struct max31790_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int read;
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
-	fan_config = data->fan_config[channel];
-
 	switch (attr) {
 	case hwmon_pwm_input:
-		*val = data->pwm[channel] >> 8;
+		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
+		if (read < 0)
+			return read;
+
+		*val = read >> 8;
 		return 0;
 	case hwmon_pwm_enable:
-		if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
+		read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
+		if (read < 0)
+			return read;
+
+		if (read & MAX31790_FAN_CFG_RPM_MODE)
 			*val = 2;
-		else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
+		else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
 			*val = 1;
 		else
 			*val = 0;
@@ -287,25 +319,24 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			      long val)
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
+	struct regmap *regmap = data->regmap;
 	u8 fan_config;
 	int err = 0;
 
-	mutex_lock(&data->update_lock);
-
 	switch (attr) {
 	case hwmon_pwm_input:
 		if (val < 0 || val > 255) {
 			err = -EINVAL;
 			break;
 		}
-		data->pwm[channel] = val << 8;
-		err = i2c_smbus_write_word_swapped(client,
-						   MAX31790_REG_PWMOUT(channel),
-						   data->pwm[channel]);
+		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
 		break;
 	case hwmon_pwm_enable:
-		fan_config = data->fan_config[channel];
+		fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
+
+		if (fan_config < 0)
+			return fan_config;
+
 		if (val == 0) {
 			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
 					MAX31790_FAN_CFG_RPM_MODE);
@@ -320,25 +351,26 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 			err = -EINVAL;
 			break;
 		}
-		data->fan_config[channel] = fan_config;
-		err = i2c_smbus_write_byte_data(client,
-					MAX31790_REG_FAN_CONFIG(channel),
-					fan_config);
+		err = regmap_write(regmap,
+				   MAX31790_REG_FAN_CONFIG(channel),
+				   fan_config);
 		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return err;
 }
 
 static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
 {
 	const struct max31790_data *data = _data;
-	u8 fan_config = data->fan_config[channel];
+	struct regmap *regmap = data->regmap;
+	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
+
+	if (fan_config < 0)
+		return 0;
 
 	switch (attr) {
 	case hwmon_pwm_input:
@@ -426,35 +458,12 @@ static const struct hwmon_chip_info max31790_chip_info = {
 	.info = max31790_info,
 };
 
-static int max31790_init_client(struct i2c_client *client,
-				struct max31790_data *data)
-{
-	int i, rv;
-
-	for (i = 0; i < NR_CHANNEL; i++) {
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_CONFIG(i));
-		if (rv < 0)
-			return rv;
-		data->fan_config[i] = rv;
-
-		rv = i2c_smbus_read_byte_data(client,
-				MAX31790_REG_FAN_DYNAMICS(i));
-		if (rv < 0)
-			return rv;
-		data->fan_dynamics[i] = rv;
-	}
-
-	return 0;
-}
-
 static int max31790_probe(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
 	struct device *dev = &client->dev;
 	struct max31790_data *data;
 	struct device *hwmon_dev;
-	int err;
 
 	if (!i2c_check_functionality(adapter,
 			I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
@@ -464,15 +473,12 @@ static int max31790_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
-	mutex_init(&data->update_lock);
+	data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
 
-	/*
-	 * Initialize the max31790 chip
-	 */
-	err = max31790_init_client(client, data);
-	if (err)
-		return err;
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
 							 data,
-- 
2.30.2

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hi Guenther,

I'm posting v2 of my series on max31790. These are the changes from v1:
1) The driver uses regmap caching instead of local caching.
2) I got rid of the "refactor" patch and also the "pulses" patch.
3) I got rid of unnecessary whitespace changes.
4) fan*_input now returns -ENODATA when fan*_enable is 0.
5) Changed the argument of bits_for_speed_range to long.

Vaclav

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 10142 bytes --]

Hi "Václav,

url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20210316 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'

vim +263 drivers/hwmon/max31790.c

54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  258  {
54187ff9d766b2 Guenter Roeck   2016-07-01  259  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  260  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  261  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  262  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @263  	if (fan_config < 0)
                                                            ^^^^^^^^^^^^^^
A u8 can't be negative.

2c8602cfaeab63 Václav Kubernát 2021-03-16  264  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  265  
54187ff9d766b2 Guenter Roeck   2016-07-01  266  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  267  	case hwmon_fan_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  268  	case hwmon_fan_fault:
54187ff9d766b2 Guenter Roeck   2016-07-01  269  		if (channel < NR_CHANNEL ||
54187ff9d766b2 Guenter Roeck   2016-07-01  270  		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271  			return 0444;
54187ff9d766b2 Guenter Roeck   2016-07-01  272  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  273  	case hwmon_fan_target:
54187ff9d766b2 Guenter Roeck   2016-07-01  274  		if (channel < NR_CHANNEL &&
54187ff9d766b2 Guenter Roeck   2016-07-01  275  		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  277  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  278  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  279  		return 0;
195a4b4298a795 Il Han          2015-08-30  280  	}
195a4b4298a795 Il Han          2015-08-30  281  }
195a4b4298a795 Il Han          2015-08-30  282  
54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  284  			     long *val)
195a4b4298a795 Il Han          2015-08-30  285  {
2c8602cfaeab63 Václav Kubernát 2021-03-16  286  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  287  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  288  	int read;
195a4b4298a795 Il Han          2015-08-30  289  
195a4b4298a795 Il Han          2015-08-30  290  	if (IS_ERR(data))
195a4b4298a795 Il Han          2015-08-30  291  		return PTR_ERR(data);
195a4b4298a795 Il Han          2015-08-30  292  
54187ff9d766b2 Guenter Roeck   2016-07-01  293  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  294  	case hwmon_pwm_input:
2c8602cfaeab63 Václav Kubernát 2021-03-16  295  		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  296  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  297  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  298  
2c8602cfaeab63 Václav Kubernát 2021-03-16  299  		*val = read >> 8;
54187ff9d766b2 Guenter Roeck   2016-07-01  300  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  301  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  302  		read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  303  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  304  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  305  
2c8602cfaeab63 Václav Kubernát 2021-03-16  306  		if (read & MAX31790_FAN_CFG_RPM_MODE)
54187ff9d766b2 Guenter Roeck   2016-07-01  307  			*val = 2;
2c8602cfaeab63 Václav Kubernát 2021-03-16  308  		else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
54187ff9d766b2 Guenter Roeck   2016-07-01  309  			*val = 1;
195a4b4298a795 Il Han          2015-08-30  310  		else
54187ff9d766b2 Guenter Roeck   2016-07-01  311  			*val = 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  312  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  313  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  314  		return -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  315  	}
195a4b4298a795 Il Han          2015-08-30  316  }
195a4b4298a795 Il Han          2015-08-30  317  
54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  319  			      long val)
195a4b4298a795 Il Han          2015-08-30  320  {
195a4b4298a795 Il Han          2015-08-30  321  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  322  	struct regmap *regmap = data->regmap;
54187ff9d766b2 Guenter Roeck   2016-07-01  323  	u8 fan_config;
54187ff9d766b2 Guenter Roeck   2016-07-01  324  	int err = 0;
195a4b4298a795 Il Han          2015-08-30  325  
54187ff9d766b2 Guenter Roeck   2016-07-01  326  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  327  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  328  		if (val < 0 || val > 255) {
54187ff9d766b2 Guenter Roeck   2016-07-01  329  			err = -EINVAL;
54187ff9d766b2 Guenter Roeck   2016-07-01  330  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  331  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  332  		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
195a4b4298a795 Il Han          2015-08-30  333  		break;
54187ff9d766b2 Guenter Roeck   2016-07-01  334  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  335  		fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  336  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @337  		if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  338  			return fan_config;
2c8602cfaeab63 Václav Kubernát 2021-03-16  339  
54187ff9d766b2 Guenter Roeck   2016-07-01  340  		if (val == 0) {
54187ff9d766b2 Guenter Roeck   2016-07-01  341  			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  342  					MAX31790_FAN_CFG_RPM_MODE);
54187ff9d766b2 Guenter Roeck   2016-07-01  343  		} else if (val == 1) {
54187ff9d766b2 Guenter Roeck   2016-07-01  344  			fan_config = (fan_config |
54187ff9d766b2 Guenter Roeck   2016-07-01  345  				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
54187ff9d766b2 Guenter Roeck   2016-07-01  346  				     ~MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  347  		} else if (val == 2) {
54187ff9d766b2 Guenter Roeck   2016-07-01  348  			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  349  				      MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  350  		} else {
54187ff9d766b2 Guenter Roeck   2016-07-01  351  			err = -EINVAL;
195a4b4298a795 Il Han          2015-08-30  352  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  353  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  354  		err = regmap_write(regmap,
54187ff9d766b2 Guenter Roeck   2016-07-01  355  				   MAX31790_REG_FAN_CONFIG(channel),
54187ff9d766b2 Guenter Roeck   2016-07-01  356  				   fan_config);
195a4b4298a795 Il Han          2015-08-30  357  		break;
195a4b4298a795 Il Han          2015-08-30  358  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  359  		err = -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  360  		break;
195a4b4298a795 Il Han          2015-08-30  361  	}
195a4b4298a795 Il Han          2015-08-30  362  
195a4b4298a795 Il Han          2015-08-30  363  	return err;
195a4b4298a795 Il Han          2015-08-30  364  }
195a4b4298a795 Il Han          2015-08-30  365  
54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  367  {
54187ff9d766b2 Guenter Roeck   2016-07-01  368  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  369  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  370  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  371  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @372  	if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  373  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  374  
54187ff9d766b2 Guenter Roeck   2016-07-01  375  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  376  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  377  	case hwmon_pwm_enable:
54187ff9d766b2 Guenter Roeck   2016-07-01  378  		if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  380  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  381  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  382  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  383  	}
54187ff9d766b2 Guenter Roeck   2016-07-01  384  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31803 bytes --]

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 10360 bytes --]

Hi "Václav,

url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20210316 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'

vim +263 drivers/hwmon/max31790.c

54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  258  {
54187ff9d766b2 Guenter Roeck   2016-07-01  259  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  260  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  261  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  262  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @263  	if (fan_config < 0)
                                                            ^^^^^^^^^^^^^^
A u8 can't be negative.

2c8602cfaeab63 Václav Kubernát 2021-03-16  264  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  265  
54187ff9d766b2 Guenter Roeck   2016-07-01  266  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  267  	case hwmon_fan_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  268  	case hwmon_fan_fault:
54187ff9d766b2 Guenter Roeck   2016-07-01  269  		if (channel < NR_CHANNEL ||
54187ff9d766b2 Guenter Roeck   2016-07-01  270  		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271  			return 0444;
54187ff9d766b2 Guenter Roeck   2016-07-01  272  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  273  	case hwmon_fan_target:
54187ff9d766b2 Guenter Roeck   2016-07-01  274  		if (channel < NR_CHANNEL &&
54187ff9d766b2 Guenter Roeck   2016-07-01  275  		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  277  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  278  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  279  		return 0;
195a4b4298a795 Il Han          2015-08-30  280  	}
195a4b4298a795 Il Han          2015-08-30  281  }
195a4b4298a795 Il Han          2015-08-30  282  
54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  284  			     long *val)
195a4b4298a795 Il Han          2015-08-30  285  {
2c8602cfaeab63 Václav Kubernát 2021-03-16  286  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  287  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  288  	int read;
195a4b4298a795 Il Han          2015-08-30  289  
195a4b4298a795 Il Han          2015-08-30  290  	if (IS_ERR(data))
195a4b4298a795 Il Han          2015-08-30  291  		return PTR_ERR(data);
195a4b4298a795 Il Han          2015-08-30  292  
54187ff9d766b2 Guenter Roeck   2016-07-01  293  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  294  	case hwmon_pwm_input:
2c8602cfaeab63 Václav Kubernát 2021-03-16  295  		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  296  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  297  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  298  
2c8602cfaeab63 Václav Kubernát 2021-03-16  299  		*val = read >> 8;
54187ff9d766b2 Guenter Roeck   2016-07-01  300  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  301  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  302  		read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  303  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  304  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  305  
2c8602cfaeab63 Václav Kubernát 2021-03-16  306  		if (read & MAX31790_FAN_CFG_RPM_MODE)
54187ff9d766b2 Guenter Roeck   2016-07-01  307  			*val = 2;
2c8602cfaeab63 Václav Kubernát 2021-03-16  308  		else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
54187ff9d766b2 Guenter Roeck   2016-07-01  309  			*val = 1;
195a4b4298a795 Il Han          2015-08-30  310  		else
54187ff9d766b2 Guenter Roeck   2016-07-01  311  			*val = 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  312  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  313  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  314  		return -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  315  	}
195a4b4298a795 Il Han          2015-08-30  316  }
195a4b4298a795 Il Han          2015-08-30  317  
54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  319  			      long val)
195a4b4298a795 Il Han          2015-08-30  320  {
195a4b4298a795 Il Han          2015-08-30  321  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  322  	struct regmap *regmap = data->regmap;
54187ff9d766b2 Guenter Roeck   2016-07-01  323  	u8 fan_config;
54187ff9d766b2 Guenter Roeck   2016-07-01  324  	int err = 0;
195a4b4298a795 Il Han          2015-08-30  325  
54187ff9d766b2 Guenter Roeck   2016-07-01  326  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  327  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  328  		if (val < 0 || val > 255) {
54187ff9d766b2 Guenter Roeck   2016-07-01  329  			err = -EINVAL;
54187ff9d766b2 Guenter Roeck   2016-07-01  330  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  331  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  332  		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
195a4b4298a795 Il Han          2015-08-30  333  		break;
54187ff9d766b2 Guenter Roeck   2016-07-01  334  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  335  		fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  336  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @337  		if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  338  			return fan_config;
2c8602cfaeab63 Václav Kubernát 2021-03-16  339  
54187ff9d766b2 Guenter Roeck   2016-07-01  340  		if (val == 0) {
54187ff9d766b2 Guenter Roeck   2016-07-01  341  			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  342  					MAX31790_FAN_CFG_RPM_MODE);
54187ff9d766b2 Guenter Roeck   2016-07-01  343  		} else if (val == 1) {
54187ff9d766b2 Guenter Roeck   2016-07-01  344  			fan_config = (fan_config |
54187ff9d766b2 Guenter Roeck   2016-07-01  345  				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
54187ff9d766b2 Guenter Roeck   2016-07-01  346  				     ~MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  347  		} else if (val == 2) {
54187ff9d766b2 Guenter Roeck   2016-07-01  348  			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  349  				      MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  350  		} else {
54187ff9d766b2 Guenter Roeck   2016-07-01  351  			err = -EINVAL;
195a4b4298a795 Il Han          2015-08-30  352  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  353  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  354  		err = regmap_write(regmap,
54187ff9d766b2 Guenter Roeck   2016-07-01  355  				   MAX31790_REG_FAN_CONFIG(channel),
54187ff9d766b2 Guenter Roeck   2016-07-01  356  				   fan_config);
195a4b4298a795 Il Han          2015-08-30  357  		break;
195a4b4298a795 Il Han          2015-08-30  358  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  359  		err = -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  360  		break;
195a4b4298a795 Il Han          2015-08-30  361  	}
195a4b4298a795 Il Han          2015-08-30  362  
195a4b4298a795 Il Han          2015-08-30  363  	return err;
195a4b4298a795 Il Han          2015-08-30  364  }
195a4b4298a795 Il Han          2015-08-30  365  
54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  367  {
54187ff9d766b2 Guenter Roeck   2016-07-01  368  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  369  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  370  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  371  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @372  	if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  373  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  374  
54187ff9d766b2 Guenter Roeck   2016-07-01  375  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  376  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  377  	case hwmon_pwm_enable:
54187ff9d766b2 Guenter Roeck   2016-07-01  378  		if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  380  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  381  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  382  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  383  	}
54187ff9d766b2 Guenter Roeck   2016-07-01  384  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31803 bytes --]

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 10360 bytes --]

Hi "Václav,

url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-m001-20210316 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'

vim +263 drivers/hwmon/max31790.c

54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  258  {
54187ff9d766b2 Guenter Roeck   2016-07-01  259  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  260  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  261  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  262  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @263  	if (fan_config < 0)
                                                            ^^^^^^^^^^^^^^
A u8 can't be negative.

2c8602cfaeab63 Václav Kubernát 2021-03-16  264  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  265  
54187ff9d766b2 Guenter Roeck   2016-07-01  266  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  267  	case hwmon_fan_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  268  	case hwmon_fan_fault:
54187ff9d766b2 Guenter Roeck   2016-07-01  269  		if (channel < NR_CHANNEL ||
54187ff9d766b2 Guenter Roeck   2016-07-01  270  		    (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271  			return 0444;
54187ff9d766b2 Guenter Roeck   2016-07-01  272  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  273  	case hwmon_fan_target:
54187ff9d766b2 Guenter Roeck   2016-07-01  274  		if (channel < NR_CHANNEL &&
54187ff9d766b2 Guenter Roeck   2016-07-01  275  		    !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  277  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  278  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  279  		return 0;
195a4b4298a795 Il Han          2015-08-30  280  	}
195a4b4298a795 Il Han          2015-08-30  281  }
195a4b4298a795 Il Han          2015-08-30  282  
54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  284  			     long *val)
195a4b4298a795 Il Han          2015-08-30  285  {
2c8602cfaeab63 Václav Kubernát 2021-03-16  286  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  287  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  288  	int read;
195a4b4298a795 Il Han          2015-08-30  289  
195a4b4298a795 Il Han          2015-08-30  290  	if (IS_ERR(data))
195a4b4298a795 Il Han          2015-08-30  291  		return PTR_ERR(data);
195a4b4298a795 Il Han          2015-08-30  292  
54187ff9d766b2 Guenter Roeck   2016-07-01  293  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  294  	case hwmon_pwm_input:
2c8602cfaeab63 Václav Kubernát 2021-03-16  295  		read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  296  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  297  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  298  
2c8602cfaeab63 Václav Kubernát 2021-03-16  299  		*val = read >> 8;
54187ff9d766b2 Guenter Roeck   2016-07-01  300  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  301  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  302  		read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
2c8602cfaeab63 Václav Kubernát 2021-03-16  303  		if (read < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  304  			return read;
2c8602cfaeab63 Václav Kubernát 2021-03-16  305  
2c8602cfaeab63 Václav Kubernát 2021-03-16  306  		if (read & MAX31790_FAN_CFG_RPM_MODE)
54187ff9d766b2 Guenter Roeck   2016-07-01  307  			*val = 2;
2c8602cfaeab63 Václav Kubernát 2021-03-16  308  		else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
54187ff9d766b2 Guenter Roeck   2016-07-01  309  			*val = 1;
195a4b4298a795 Il Han          2015-08-30  310  		else
54187ff9d766b2 Guenter Roeck   2016-07-01  311  			*val = 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  312  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  313  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  314  		return -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  315  	}
195a4b4298a795 Il Han          2015-08-30  316  }
195a4b4298a795 Il Han          2015-08-30  317  
54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
54187ff9d766b2 Guenter Roeck   2016-07-01  319  			      long val)
195a4b4298a795 Il Han          2015-08-30  320  {
195a4b4298a795 Il Han          2015-08-30  321  	struct max31790_data *data = dev_get_drvdata(dev);
2c8602cfaeab63 Václav Kubernát 2021-03-16  322  	struct regmap *regmap = data->regmap;
54187ff9d766b2 Guenter Roeck   2016-07-01  323  	u8 fan_config;
54187ff9d766b2 Guenter Roeck   2016-07-01  324  	int err = 0;
195a4b4298a795 Il Han          2015-08-30  325  
54187ff9d766b2 Guenter Roeck   2016-07-01  326  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  327  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  328  		if (val < 0 || val > 255) {
54187ff9d766b2 Guenter Roeck   2016-07-01  329  			err = -EINVAL;
54187ff9d766b2 Guenter Roeck   2016-07-01  330  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  331  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  332  		err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
195a4b4298a795 Il Han          2015-08-30  333  		break;
54187ff9d766b2 Guenter Roeck   2016-07-01  334  	case hwmon_pwm_enable:
2c8602cfaeab63 Václav Kubernát 2021-03-16  335  		fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  336  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @337  		if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  338  			return fan_config;
2c8602cfaeab63 Václav Kubernát 2021-03-16  339  
54187ff9d766b2 Guenter Roeck   2016-07-01  340  		if (val == 0) {
54187ff9d766b2 Guenter Roeck   2016-07-01  341  			fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  342  					MAX31790_FAN_CFG_RPM_MODE);
54187ff9d766b2 Guenter Roeck   2016-07-01  343  		} else if (val == 1) {
54187ff9d766b2 Guenter Roeck   2016-07-01  344  			fan_config = (fan_config |
54187ff9d766b2 Guenter Roeck   2016-07-01  345  				      MAX31790_FAN_CFG_TACH_INPUT_EN) &
54187ff9d766b2 Guenter Roeck   2016-07-01  346  				     ~MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  347  		} else if (val == 2) {
54187ff9d766b2 Guenter Roeck   2016-07-01  348  			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
54187ff9d766b2 Guenter Roeck   2016-07-01  349  				      MAX31790_FAN_CFG_RPM_MODE;
54187ff9d766b2 Guenter Roeck   2016-07-01  350  		} else {
54187ff9d766b2 Guenter Roeck   2016-07-01  351  			err = -EINVAL;
195a4b4298a795 Il Han          2015-08-30  352  			break;
54187ff9d766b2 Guenter Roeck   2016-07-01  353  		}
2c8602cfaeab63 Václav Kubernát 2021-03-16  354  		err = regmap_write(regmap,
54187ff9d766b2 Guenter Roeck   2016-07-01  355  				   MAX31790_REG_FAN_CONFIG(channel),
54187ff9d766b2 Guenter Roeck   2016-07-01  356  				   fan_config);
195a4b4298a795 Il Han          2015-08-30  357  		break;
195a4b4298a795 Il Han          2015-08-30  358  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  359  		err = -EOPNOTSUPP;
54187ff9d766b2 Guenter Roeck   2016-07-01  360  		break;
195a4b4298a795 Il Han          2015-08-30  361  	}
195a4b4298a795 Il Han          2015-08-30  362  
195a4b4298a795 Il Han          2015-08-30  363  	return err;
195a4b4298a795 Il Han          2015-08-30  364  }
195a4b4298a795 Il Han          2015-08-30  365  
54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
195a4b4298a795 Il Han          2015-08-30  367  {
54187ff9d766b2 Guenter Roeck   2016-07-01  368  	const struct max31790_data *data = _data;
2c8602cfaeab63 Václav Kubernát 2021-03-16  369  	struct regmap *regmap = data->regmap;
2c8602cfaeab63 Václav Kubernát 2021-03-16  370  	u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
2c8602cfaeab63 Václav Kubernát 2021-03-16  371  
2c8602cfaeab63 Václav Kubernát 2021-03-16 @372  	if (fan_config < 0)
2c8602cfaeab63 Václav Kubernát 2021-03-16  373  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  374  
54187ff9d766b2 Guenter Roeck   2016-07-01  375  	switch (attr) {
54187ff9d766b2 Guenter Roeck   2016-07-01  376  	case hwmon_pwm_input:
54187ff9d766b2 Guenter Roeck   2016-07-01  377  	case hwmon_pwm_enable:
54187ff9d766b2 Guenter Roeck   2016-07-01  378  		if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379  			return 0644;
54187ff9d766b2 Guenter Roeck   2016-07-01  380  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  381  	default:
54187ff9d766b2 Guenter Roeck   2016-07-01  382  		return 0;
54187ff9d766b2 Guenter Roeck   2016-07-01  383  	}
54187ff9d766b2 Guenter Roeck   2016-07-01  384  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31803 bytes --]

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hi Dan,

thanks, I will fix this in v3 of this patch series.

Vaclav

st 17. 3. 2021 v 6:14 odesílatel Dan Carpenter
<dan.carpenter@oracle.com> napsal:
>
> Hi "Václav,
>
> url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: x86_64-randconfig-m001-20210316 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
> drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
> drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
>
> vim +263 drivers/hwmon/max31790.c
>
> 54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> 195a4b4298a795 Il Han          2015-08-30  258  {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  259          const struct max31790_data *data = _data;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  260          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  261          u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  262
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @263          if (fan_config < 0)
>                                                             ^^^^^^^^^^^^^^
> A u8 can't be negative.
>
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  264                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  265
> 54187ff9d766b2 Guenter Roeck   2016-07-01  266          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  267          case hwmon_fan_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  268          case hwmon_fan_fault:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  269                  if (channel < NR_CHANNEL ||
> 54187ff9d766b2 Guenter Roeck   2016-07-01  270                      (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271                          return 0444;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  272                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  273          case hwmon_fan_target:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  274                  if (channel < NR_CHANNEL &&
> 54187ff9d766b2 Guenter Roeck   2016-07-01  275                      !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276                          return 0644;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  277                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  278          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  279                  return 0;
> 195a4b4298a795 Il Han          2015-08-30  280          }
> 195a4b4298a795 Il Han          2015-08-30  281  }
> 195a4b4298a795 Il Han          2015-08-30  282
> 54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  284                               long *val)
> 195a4b4298a795 Il Han          2015-08-30  285  {
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  286          struct max31790_data *data = dev_get_drvdata(dev);
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  287          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  288          int read;
> 195a4b4298a795 Il Han          2015-08-30  289
> 195a4b4298a795 Il Han          2015-08-30  290          if (IS_ERR(data))
> 195a4b4298a795 Il Han          2015-08-30  291                  return PTR_ERR(data);
> 195a4b4298a795 Il Han          2015-08-30  292
> 54187ff9d766b2 Guenter Roeck   2016-07-01  293          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  294          case hwmon_pwm_input:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  295                  read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  296                  if (read < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  297                          return read;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  298
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  299                  *val = read >> 8;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  300                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  301          case hwmon_pwm_enable:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  302                  read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  303                  if (read < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  304                          return read;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  305
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  306                  if (read & MAX31790_FAN_CFG_RPM_MODE)
> 54187ff9d766b2 Guenter Roeck   2016-07-01  307                          *val = 2;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  308                  else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
> 54187ff9d766b2 Guenter Roeck   2016-07-01  309                          *val = 1;
> 195a4b4298a795 Il Han          2015-08-30  310                  else
> 54187ff9d766b2 Guenter Roeck   2016-07-01  311                          *val = 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  312                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  313          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  314                  return -EOPNOTSUPP;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  315          }
> 195a4b4298a795 Il Han          2015-08-30  316  }
> 195a4b4298a795 Il Han          2015-08-30  317
> 54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  319                                long val)
> 195a4b4298a795 Il Han          2015-08-30  320  {
> 195a4b4298a795 Il Han          2015-08-30  321          struct max31790_data *data = dev_get_drvdata(dev);
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  322          struct regmap *regmap = data->regmap;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  323          u8 fan_config;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  324          int err = 0;
> 195a4b4298a795 Il Han          2015-08-30  325
> 54187ff9d766b2 Guenter Roeck   2016-07-01  326          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  327          case hwmon_pwm_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  328                  if (val < 0 || val > 255) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  329                          err = -EINVAL;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  330                          break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  331                  }
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  332                  err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
> 195a4b4298a795 Il Han          2015-08-30  333                  break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  334          case hwmon_pwm_enable:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  335                  fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  336
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @337                  if (fan_config < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  338                          return fan_config;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  339
> 54187ff9d766b2 Guenter Roeck   2016-07-01  340                  if (val == 0) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  341                          fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  342                                          MAX31790_FAN_CFG_RPM_MODE);
> 54187ff9d766b2 Guenter Roeck   2016-07-01  343                  } else if (val == 1) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  344                          fan_config = (fan_config |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  345                                        MAX31790_FAN_CFG_TACH_INPUT_EN) &
> 54187ff9d766b2 Guenter Roeck   2016-07-01  346                                       ~MAX31790_FAN_CFG_RPM_MODE;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  347                  } else if (val == 2) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  348                          fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  349                                        MAX31790_FAN_CFG_RPM_MODE;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  350                  } else {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  351                          err = -EINVAL;
> 195a4b4298a795 Il Han          2015-08-30  352                          break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  353                  }
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  354                  err = regmap_write(regmap,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  355                                     MAX31790_REG_FAN_CONFIG(channel),
> 54187ff9d766b2 Guenter Roeck   2016-07-01  356                                     fan_config);
> 195a4b4298a795 Il Han          2015-08-30  357                  break;
> 195a4b4298a795 Il Han          2015-08-30  358          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  359                  err = -EOPNOTSUPP;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  360                  break;
> 195a4b4298a795 Il Han          2015-08-30  361          }
> 195a4b4298a795 Il Han          2015-08-30  362
> 195a4b4298a795 Il Han          2015-08-30  363          return err;
> 195a4b4298a795 Il Han          2015-08-30  364  }
> 195a4b4298a795 Il Han          2015-08-30  365
> 54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
> 195a4b4298a795 Il Han          2015-08-30  367  {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  368          const struct max31790_data *data = _data;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  369          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  370          u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  371
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @372          if (fan_config < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  373                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  374
> 54187ff9d766b2 Guenter Roeck   2016-07-01  375          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  376          case hwmon_pwm_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  377          case hwmon_pwm_enable:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  378                  if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379                          return 0644;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  380                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  381          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  382                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  383          }
> 54187ff9d766b2 Guenter Roeck   2016-07-01  384  }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

[-- Attachment #1: Type: text/plain, Size: 12158 bytes --]

Hi Dan,

thanks, I will fix this in v3 of this patch series.

Vaclav

st 17. 3. 2021 v 6:14 odesílatel Dan Carpenter
<dan.carpenter@oracle.com> napsal:
>
> Hi "Václav,
>
> url:    https://github.com/0day-ci/linux/commits/V-clav-Kubern-t/hwmon-max31790-Rework-to-use-regmap/20210317-015931
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: x86_64-randconfig-m001-20210316 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> drivers/hwmon/max31790.c:263 max31790_fan_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
> drivers/hwmon/max31790.c:337 max31790_write_pwm() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
> drivers/hwmon/max31790.c:372 max31790_pwm_is_visible() warn: impossible condition '(fan_config < 0) => (0-255 < 0)'
>
> vim +263 drivers/hwmon/max31790.c
>
> 54187ff9d766b2 Guenter Roeck   2016-07-01  257  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> 195a4b4298a795 Il Han          2015-08-30  258  {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  259          const struct max31790_data *data = _data;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  260          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  261          u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  262
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @263          if (fan_config < 0)
>                                                             ^^^^^^^^^^^^^^
> A u8 can't be negative.
>
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  264                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  265
> 54187ff9d766b2 Guenter Roeck   2016-07-01  266          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  267          case hwmon_fan_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  268          case hwmon_fan_fault:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  269                  if (channel < NR_CHANNEL ||
> 54187ff9d766b2 Guenter Roeck   2016-07-01  270                      (fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  271                          return 0444;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  272                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  273          case hwmon_fan_target:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  274                  if (channel < NR_CHANNEL &&
> 54187ff9d766b2 Guenter Roeck   2016-07-01  275                      !(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  276                          return 0644;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  277                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  278          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  279                  return 0;
> 195a4b4298a795 Il Han          2015-08-30  280          }
> 195a4b4298a795 Il Han          2015-08-30  281  }
> 195a4b4298a795 Il Han          2015-08-30  282
> 54187ff9d766b2 Guenter Roeck   2016-07-01  283  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  284                               long *val)
> 195a4b4298a795 Il Han          2015-08-30  285  {
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  286          struct max31790_data *data = dev_get_drvdata(dev);
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  287          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  288          int read;
> 195a4b4298a795 Il Han          2015-08-30  289
> 195a4b4298a795 Il Han          2015-08-30  290          if (IS_ERR(data))
> 195a4b4298a795 Il Han          2015-08-30  291                  return PTR_ERR(data);
> 195a4b4298a795 Il Han          2015-08-30  292
> 54187ff9d766b2 Guenter Roeck   2016-07-01  293          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  294          case hwmon_pwm_input:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  295                  read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  296                  if (read < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  297                          return read;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  298
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  299                  *val = read >> 8;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  300                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  301          case hwmon_pwm_enable:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  302                  read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  303                  if (read < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  304                          return read;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  305
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  306                  if (read & MAX31790_FAN_CFG_RPM_MODE)
> 54187ff9d766b2 Guenter Roeck   2016-07-01  307                          *val = 2;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  308                  else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
> 54187ff9d766b2 Guenter Roeck   2016-07-01  309                          *val = 1;
> 195a4b4298a795 Il Han          2015-08-30  310                  else
> 54187ff9d766b2 Guenter Roeck   2016-07-01  311                          *val = 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  312                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  313          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  314                  return -EOPNOTSUPP;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  315          }
> 195a4b4298a795 Il Han          2015-08-30  316  }
> 195a4b4298a795 Il Han          2015-08-30  317
> 54187ff9d766b2 Guenter Roeck   2016-07-01  318  static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  319                                long val)
> 195a4b4298a795 Il Han          2015-08-30  320  {
> 195a4b4298a795 Il Han          2015-08-30  321          struct max31790_data *data = dev_get_drvdata(dev);
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  322          struct regmap *regmap = data->regmap;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  323          u8 fan_config;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  324          int err = 0;
> 195a4b4298a795 Il Han          2015-08-30  325
> 54187ff9d766b2 Guenter Roeck   2016-07-01  326          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  327          case hwmon_pwm_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  328                  if (val < 0 || val > 255) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  329                          err = -EINVAL;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  330                          break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  331                  }
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  332                  err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
> 195a4b4298a795 Il Han          2015-08-30  333                  break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  334          case hwmon_pwm_enable:
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  335                  fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  336
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @337                  if (fan_config < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  338                          return fan_config;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  339
> 54187ff9d766b2 Guenter Roeck   2016-07-01  340                  if (val == 0) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  341                          fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  342                                          MAX31790_FAN_CFG_RPM_MODE);
> 54187ff9d766b2 Guenter Roeck   2016-07-01  343                  } else if (val == 1) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  344                          fan_config = (fan_config |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  345                                        MAX31790_FAN_CFG_TACH_INPUT_EN) &
> 54187ff9d766b2 Guenter Roeck   2016-07-01  346                                       ~MAX31790_FAN_CFG_RPM_MODE;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  347                  } else if (val == 2) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  348                          fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> 54187ff9d766b2 Guenter Roeck   2016-07-01  349                                        MAX31790_FAN_CFG_RPM_MODE;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  350                  } else {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  351                          err = -EINVAL;
> 195a4b4298a795 Il Han          2015-08-30  352                          break;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  353                  }
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  354                  err = regmap_write(regmap,
> 54187ff9d766b2 Guenter Roeck   2016-07-01  355                                     MAX31790_REG_FAN_CONFIG(channel),
> 54187ff9d766b2 Guenter Roeck   2016-07-01  356                                     fan_config);
> 195a4b4298a795 Il Han          2015-08-30  357                  break;
> 195a4b4298a795 Il Han          2015-08-30  358          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  359                  err = -EOPNOTSUPP;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  360                  break;
> 195a4b4298a795 Il Han          2015-08-30  361          }
> 195a4b4298a795 Il Han          2015-08-30  362
> 195a4b4298a795 Il Han          2015-08-30  363          return err;
> 195a4b4298a795 Il Han          2015-08-30  364  }
> 195a4b4298a795 Il Han          2015-08-30  365
> 54187ff9d766b2 Guenter Roeck   2016-07-01  366  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
> 195a4b4298a795 Il Han          2015-08-30  367  {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  368          const struct max31790_data *data = _data;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  369          struct regmap *regmap = data->regmap;
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  370          u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  371
> 2c8602cfaeab63 Václav Kubernát 2021-03-16 @372          if (fan_config < 0)
> 2c8602cfaeab63 Václav Kubernát 2021-03-16  373                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  374
> 54187ff9d766b2 Guenter Roeck   2016-07-01  375          switch (attr) {
> 54187ff9d766b2 Guenter Roeck   2016-07-01  376          case hwmon_pwm_input:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  377          case hwmon_pwm_enable:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  378                  if (!(fan_config & MAX31790_FAN_CFG_TACH_INPUT))
> dc8dbb4d7672b7 Guenter Roeck   2018-12-10  379                          return 0644;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  380                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  381          default:
> 54187ff9d766b2 Guenter Roeck   2016-07-01  382                  return 0;
> 54187ff9d766b2 Guenter Roeck   2016-07-01  383          }
> 54187ff9d766b2 Guenter Roeck   2016-07-01  384  }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v2 1/5] hwmon: (max31790) Rework to use regmap

[Václav Kubernát]

Hi Guenter,

Thank you for the comments. I will fix the issues in a V3 patch.

About the mutex: I was looking at regmap and saw it did locking by
itself. But I suppose writing still has to be locked, because the
write function writes more than once. I will add the mutex back.

Václav

út 30. 3. 2021 v 0:27 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On Tue, Mar 16, 2021 at 06:54:58PM +0100, Václav Kubernát wrote:
> > Converting the driver to use regmap makes it more generic. It also makes
> > it a lot easier to debug through debugfs.
> >
> > Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
>
> Comments are in addition to comments from Dan and 0-day.
>
> > ---
> >  drivers/hwmon/Kconfig    |   1 +
> >  drivers/hwmon/max31790.c | 318 ++++++++++++++++++++-------------------
> >  2 files changed, 163 insertions(+), 156 deletions(-)
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 54f04e61fb83..c2ec57672c4e 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1092,6 +1092,7 @@ config SENSORS_MAX6697
> >  config SENSORS_MAX31790
> >       tristate "Maxim MAX31790 sensor chip"
> >       depends on I2C
> > +     select REGMAP_I2C
> >       help
> >         If you say yes here you get support for 6-Channel PWM-Output
> >         Fan RPM Controller.
> > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> > index 86e6c71db685..4e5add567890 100644
> > --- a/drivers/hwmon/max31790.c
> > +++ b/drivers/hwmon/max31790.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/init.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/module.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >
> >  /* MAX31790 registers */
> > @@ -46,92 +47,49 @@
> >
> >  #define NR_CHANNEL                   6
> >
> > +#define MAX31790_REG_USER_BYTE_67    0x67
> > +
> > +#define BULK_TO_U16(msb, lsb)                (((msb) << 8) + (lsb))
> > +#define U16_MSB(num)                 (((num) & 0xFF00) >> 8)
> > +#define U16_LSB(num)                 ((num) & 0x00FF)
> > +
> > +static const struct regmap_range max31790_ro_range = {
> > +     .range_min = MAX31790_REG_TACH_COUNT(0),
> > +     .range_max = MAX31790_REG_PWMOUT(0) - 1,
> > +};
> > +
> > +static const struct regmap_access_table max31790_wr_table = {
> > +     .no_ranges = &max31790_ro_range,
> > +     .n_no_ranges = 1,
> > +};
> > +
> > +static const struct regmap_range max31790_volatile_ranges[] = {
> > +     regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> > +     regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> > +};
> > +
> > +static const struct regmap_access_table max31790_volatile_table = {
> > +     .no_ranges = max31790_volatile_ranges,
> > +     .n_no_ranges = 2,
> > +     .n_yes_ranges = 0
> > +};
> > +
> > +static const struct regmap_config max31790_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .reg_stride = 1,
> > +     .max_register = MAX31790_REG_USER_BYTE_67,
> > +     .wr_table = &max31790_wr_table,
> > +     .volatile_table = &max31790_volatile_table
> > +};
> > +
> >  /*
> >   * Client data (each client gets its own)
> >   */
> >  struct max31790_data {
> > -     struct i2c_client *client;
> > -     struct mutex update_lock;
> > -     bool valid; /* zero until following fields are valid */
> > -     unsigned long last_updated; /* in jiffies */
> > -
> > -     /* register values */
> > -     u8 fan_config[NR_CHANNEL];
> > -     u8 fan_dynamics[NR_CHANNEL];
> > -     u16 fault_status;
> > -     u16 tach[NR_CHANNEL * 2];
> > -     u16 pwm[NR_CHANNEL];
> > -     u16 target_count[NR_CHANNEL];
> > +     struct regmap *regmap;
> >  };
> >
> > -static struct max31790_data *max31790_update_device(struct device *dev)
> > -{
> > -     struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > -     struct max31790_data *ret = data;
> > -     int i;
> > -     int rv;
> > -
> > -     mutex_lock(&data->update_lock);
> > -
> > -     if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_FAULT_STATUS1);
> > -             if (rv < 0)
> > -                     goto abort;
> > -             data->fault_status = rv & 0x3F;
> > -
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_FAULT_STATUS2);
> > -             if (rv < 0)
> > -                     goto abort;
> > -             data->fault_status |= (rv & 0x3F) << 6;
> > -
> > -             for (i = 0; i < NR_CHANNEL; i++) {
> > -                     rv = i2c_smbus_read_word_swapped(client,
> > -                                     MAX31790_REG_TACH_COUNT(i));
> > -                     if (rv < 0)
> > -                             goto abort;
> > -                     data->tach[i] = rv;
> > -
> > -                     if (data->fan_config[i]
> > -                         & MAX31790_FAN_CFG_TACH_INPUT) {
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                     MAX31790_REG_TACH_COUNT(NR_CHANNEL
> > -                                                             + i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->tach[NR_CHANNEL + i] = rv;
> > -                     } else {
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                             MAX31790_REG_PWMOUT(i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->pwm[i] = rv;
> > -
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                             MAX31790_REG_TARGET_COUNT(i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->target_count[i] = rv;
> > -                     }
> > -             }
> > -
> > -             data->last_updated = jiffies;
> > -             data->valid = true;
> > -     }
> > -     goto done;
> > -
> > -abort:
> > -     data->valid = false;
> > -     ret = ERR_PTR(rv);
> > -
> > -done:
> > -     mutex_unlock(&data->update_lock);
> > -
> > -     return ret;
> > -}
> > -
> >  static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
> >
> >  static u8 get_tach_period(u8 fan_dynamics)
> > @@ -159,28 +117,89 @@ static u8 bits_for_tach_period(int rpm)
> >       return bits;
> >  }
> >
> > +static int read_reg_byte(struct regmap *regmap, u8 reg)
> > +{
> > +     int rv;
> > +     int val;
> > +
> > +     rv = regmap_read(regmap, reg, &val);
> > +
>
> lease no empty line between assignment and check.
>
> > +     if (rv < 0)
> > +             return rv;
> > +
> > +     return val;
> > +}
> > +
> > +static int read_reg_word(struct regmap *regmap, u8 reg)
> > +{
> > +     int rv;
> > +     u8 val_bulk[2];
> > +
> > +     rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
> > +     if (rv < 0)
> > +             return rv;
> > +
> > +     return BULK_TO_U16(val_bulk[0], val_bulk[1]);
> > +}
> > +
> > +static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
> > +{
> > +     u8 bulk_val[2];
> > +
> > +     bulk_val[0] = U16_MSB(val);
> > +     bulk_val[1] = U16_LSB(val);
> > +
> > +     return regmap_bulk_write(regmap, reg, bulk_val, 2);
> > +}
> > +
> >  static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> >                            long *val)
> >  {
> > -     struct max31790_data *data = max31790_update_device(dev);
> > -     int sr, rpm;
> > +     struct max31790_data *data = dev_get_drvdata(dev);
> > +     struct regmap *regmap = data->regmap;
> > +     int rpm, dynamics, tach, fault;
> >
> >       if (IS_ERR(data))
> >               return PTR_ERR(data);
>
> Now unnecessary.
>
> >
> >       switch (attr) {
> >       case hwmon_fan_input:
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > -             rpm = RPM_FROM_REG(data->tach[channel], sr);
> > +             dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +             if (dynamics < 0)
> > +                     return dynamics;
> > +
> > +             tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
> > +             if (tach < 0)
> > +                     return tach;
> > +
> > +             rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
> >               *val = rpm;
>
>                 *val = RPM_FROM_REG(tach, get_tach_period(dynamics));
>
> >               return 0;
> >       case hwmon_fan_target:
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > -             rpm = RPM_FROM_REG(data->target_count[channel], sr);
> > +             dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +             if (dynamics < 0)
> > +                     return dynamics;
> > +
> > +             tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
> > +             if (tach < 0)
> > +                     return tach;
> > +
> > +             rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
> >               *val = rpm;
>
>                 *val = RPM_FROM_REG(tach, get_tach_period(dynamics));
>
> >               return 0;
> >       case hwmon_fan_fault:
> > -             *val = !!(data->fault_status & (1 << channel));
> > +             if (channel > 6)
> > +                     fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
> > +             else
> > +                     fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
> > +
> > +             if (fault < 0)
> > +                     return fault;
> > +
> > +             if (channel > 6)
> > +                     *val = !!(fault & (1 << (channel - 6)));
> > +             else
> > +                     *val = !!(fault & (1 << channel));
> >               return 0;
> >       default:
> >               return -EOPNOTSUPP;
> > @@ -191,52 +210,58 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
> >                             long val)
> >  {
> >       struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > +     struct regmap *regmap = data->regmap;
> >       int target_count;
> >       int err = 0;
> >       u8 bits;
> >       int sr;
> > -
> > -     mutex_lock(&data->update_lock);
> > +     int fan_dynamics;
> >
> >       switch (attr) {
> >       case hwmon_fan_target:
> >               val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
> >               bits = bits_for_tach_period(val);
> > -             data->fan_dynamics[channel] =
> > -                     ((data->fan_dynamics[channel] &
> > +             fan_dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +
> Unnecessary empty line.
>
> > +             if (fan_dynamics < 0)
> > +                     return fan_dynamics;
> > +
> > +             fan_dynamics =
> > +                     ((fan_dynamics &
> >                         ~MAX31790_FAN_DYN_SR_MASK) |
> >                        (bits << MAX31790_FAN_DYN_SR_SHIFT));
> > -             err = i2c_smbus_write_byte_data(client,
> > -                                     MAX31790_REG_FAN_DYNAMICS(channel),
> > -                                     data->fan_dynamics[channel]);
> > +             err = regmap_write(regmap,
> > +                                MAX31790_REG_FAN_DYNAMICS(channel),
> > +                                fan_dynamics);
> >               if (err < 0)
> >                       break;
> >
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > +             sr = get_tach_period(fan_dynamics);
> >               target_count = RPM_TO_REG(val, sr);
> >               target_count = clamp_val(target_count, 0x1, 0x7FF);
> >
> > -             data->target_count[channel] = target_count << 5;
> > +             target_count = target_count << 5;
> >
> > -             err = i2c_smbus_write_word_swapped(client,
> > -                                     MAX31790_REG_TARGET_COUNT(channel),
> > -                                     data->target_count[channel]);
> > +             err = write_reg_word(regmap,
> > +                                  MAX31790_REG_TARGET_COUNT(channel),
> > +                                  target_count);
> >               break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> >       }
> >
> > -     mutex_unlock(&data->update_lock);
> > -
> Why is this lock no longer required ? There are still multiple writes
> when writing hwmon_fan_target.
>
> >       return err;
> >  }
> >
> >  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> >  {
> >       const struct max31790_data *data = _data;
> > -     u8 fan_config = data->fan_config[channel % NR_CHANNEL];
> > +     struct regmap *regmap = data->regmap;
> > +     u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +     if (fan_config < 0)
> > +             return 0;
>
> fan_config needs to be int. Also, this is a poor way of handling
> this problem. Since fan_config does not change dynamically,
> this is one set of values that would make sense to keep cached
> locally.
>
> >
> >       switch (attr) {
> >       case hwmon_fan_input:
> > @@ -258,22 +283,29 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> >  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
> >                            long *val)
> >  {
> > -     struct max31790_data *data = max31790_update_device(dev);
> > -     u8 fan_config;
> > +     struct max31790_data *data = dev_get_drvdata(dev);
> > +     struct regmap *regmap = data->regmap;
> > +     int read;
> >
> >       if (IS_ERR(data))
> >               return PTR_ERR(data);
>
> Now unnecessary.
>
> >
> > -     fan_config = data->fan_config[channel];
> > -
> >       switch (attr) {
> >       case hwmon_pwm_input:
> > -             *val = data->pwm[channel] >> 8;
> > +             read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> > +             if (read < 0)
> > +                     return read;
> > +
> > +             *val = read >> 8;
> >               return 0;
> >       case hwmon_pwm_enable:
> > -             if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
> > +             read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
> > +             if (read < 0)
> > +                     return read;
> > +
> > +             if (read & MAX31790_FAN_CFG_RPM_MODE)
> >                       *val = 2;
> > -             else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
> > +             else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
> >                       *val = 1;
> >               else
> >                       *val = 0;
> > @@ -287,25 +319,24 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> >                             long val)
> >  {
> >       struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > +     struct regmap *regmap = data->regmap;
> >       u8 fan_config;
> >       int err = 0;
> >
> > -     mutex_lock(&data->update_lock);
> > -
> >       switch (attr) {
> >       case hwmon_pwm_input:
> >               if (val < 0 || val > 255) {
> >                       err = -EINVAL;
> >                       break;
> >               }
> > -             data->pwm[channel] = val << 8;
> > -             err = i2c_smbus_write_word_swapped(client,
> > -                                                MAX31790_REG_PWMOUT(channel),
> > -                                                data->pwm[channel]);
> > +             err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
> >               break;
> >       case hwmon_pwm_enable:
> > -             fan_config = data->fan_config[channel];
> > +             fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +             if (fan_config < 0)
> > +                     return fan_config;
> > +
> >               if (val == 0) {
> >                       fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
> >                                       MAX31790_FAN_CFG_RPM_MODE);
> > @@ -320,25 +351,26 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> >                       err = -EINVAL;
> >                       break;
> >               }
> > -             data->fan_config[channel] = fan_config;
> > -             err = i2c_smbus_write_byte_data(client,
> > -                                     MAX31790_REG_FAN_CONFIG(channel),
> > -                                     fan_config);
> > +             err = regmap_write(regmap,
> > +                                MAX31790_REG_FAN_CONFIG(channel),
> > +                                fan_config);
> >               break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> >       }
> >
> > -     mutex_unlock(&data->update_lock);
> > -
> Are you sure this mutex is no longer needed here, ie that there
> can not be an interaction with multiple writes from multiple processes
> at the same time ?
>
> >       return err;
> >  }
> >
> >  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
> >  {
> >       const struct max31790_data *data = _data;
> > -     u8 fan_config = data->fan_config[channel];
> > +     struct regmap *regmap = data->regmap;
> > +     u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +     if (fan_config < 0)
> > +             return 0;
>
> int problem again.
>
> >
> >       switch (attr) {
> >       case hwmon_pwm_input:
> > @@ -426,35 +458,12 @@ static const struct hwmon_chip_info max31790_chip_info = {
> >       .info = max31790_info,
> >  };
> >
> > -static int max31790_init_client(struct i2c_client *client,
> > -                             struct max31790_data *data)
> > -{
> > -     int i, rv;
> > -
> > -     for (i = 0; i < NR_CHANNEL; i++) {
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_CONFIG(i));
> > -             if (rv < 0)
> > -                     return rv;
> > -             data->fan_config[i] = rv;
> > -
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_DYNAMICS(i));
> > -             if (rv < 0)
> > -                     return rv;
> > -             data->fan_dynamics[i] = rv;
>
> The above "cached" values are static, and it did make sense to keep those
> locally to avoid requiring unnecessary error handling (and to detect issues
> with the chip early).
>
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> >  static int max31790_probe(struct i2c_client *client)
> >  {
> >       struct i2c_adapter *adapter = client->adapter;
> >       struct device *dev = &client->dev;
> >       struct max31790_data *data;
> >       struct device *hwmon_dev;
> > -     int err;
> >
> >       if (!i2c_check_functionality(adapter,
> >                       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> > @@ -464,15 +473,12 @@ static int max31790_probe(struct i2c_client *client)
> >       if (!data)
> >               return -ENOMEM;
> >
> > -     data->client = client;
> > -     mutex_init(&data->update_lock);
> > +     data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
> >
> > -     /*
> > -      * Initialize the max31790 chip
> > -      */
> > -     err = max31790_init_client(client, data);
> > -     if (err)
> > -             return err;
> > +     if (IS_ERR(data->regmap)) {
> > +             dev_err(dev, "failed to allocate register map\n");
> > +             return PTR_ERR(data->regmap);
> > +     }
> >
> >       hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> >                                                        data,

út 30. 3. 2021 v 0:27 odesílatel Guenter Roeck <linux@roeck-us.net> napsal:
>
> On Tue, Mar 16, 2021 at 06:54:58PM +0100, Václav Kubernát wrote:
> > Converting the driver to use regmap makes it more generic. It also makes
> > it a lot easier to debug through debugfs.
> >
> > Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
>
> Comments are in addition to comments from Dan and 0-day.
>
> > ---
> >  drivers/hwmon/Kconfig    |   1 +
> >  drivers/hwmon/max31790.c | 318 ++++++++++++++++++++-------------------
> >  2 files changed, 163 insertions(+), 156 deletions(-)
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 54f04e61fb83..c2ec57672c4e 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1092,6 +1092,7 @@ config SENSORS_MAX6697
> >  config SENSORS_MAX31790
> >       tristate "Maxim MAX31790 sensor chip"
> >       depends on I2C
> > +     select REGMAP_I2C
> >       help
> >         If you say yes here you get support for 6-Channel PWM-Output
> >         Fan RPM Controller.
> > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
> > index 86e6c71db685..4e5add567890 100644
> > --- a/drivers/hwmon/max31790.c
> > +++ b/drivers/hwmon/max31790.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/init.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/module.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >
> >  /* MAX31790 registers */
> > @@ -46,92 +47,49 @@
> >
> >  #define NR_CHANNEL                   6
> >
> > +#define MAX31790_REG_USER_BYTE_67    0x67
> > +
> > +#define BULK_TO_U16(msb, lsb)                (((msb) << 8) + (lsb))
> > +#define U16_MSB(num)                 (((num) & 0xFF00) >> 8)
> > +#define U16_LSB(num)                 ((num) & 0x00FF)
> > +
> > +static const struct regmap_range max31790_ro_range = {
> > +     .range_min = MAX31790_REG_TACH_COUNT(0),
> > +     .range_max = MAX31790_REG_PWMOUT(0) - 1,
> > +};
> > +
> > +static const struct regmap_access_table max31790_wr_table = {
> > +     .no_ranges = &max31790_ro_range,
> > +     .n_no_ranges = 1,
> > +};
> > +
> > +static const struct regmap_range max31790_volatile_ranges[] = {
> > +     regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(12)),
> > +     regmap_reg_range(MAX31790_REG_FAN_FAULT_STATUS2, MAX31790_REG_FAN_FAULT_STATUS1),
> > +};
> > +
> > +static const struct regmap_access_table max31790_volatile_table = {
> > +     .no_ranges = max31790_volatile_ranges,
> > +     .n_no_ranges = 2,
> > +     .n_yes_ranges = 0
> > +};
> > +
> > +static const struct regmap_config max31790_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .reg_stride = 1,
> > +     .max_register = MAX31790_REG_USER_BYTE_67,
> > +     .wr_table = &max31790_wr_table,
> > +     .volatile_table = &max31790_volatile_table
> > +};
> > +
> >  /*
> >   * Client data (each client gets its own)
> >   */
> >  struct max31790_data {
> > -     struct i2c_client *client;
> > -     struct mutex update_lock;
> > -     bool valid; /* zero until following fields are valid */
> > -     unsigned long last_updated; /* in jiffies */
> > -
> > -     /* register values */
> > -     u8 fan_config[NR_CHANNEL];
> > -     u8 fan_dynamics[NR_CHANNEL];
> > -     u16 fault_status;
> > -     u16 tach[NR_CHANNEL * 2];
> > -     u16 pwm[NR_CHANNEL];
> > -     u16 target_count[NR_CHANNEL];
> > +     struct regmap *regmap;
> >  };
> >
> > -static struct max31790_data *max31790_update_device(struct device *dev)
> > -{
> > -     struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > -     struct max31790_data *ret = data;
> > -     int i;
> > -     int rv;
> > -
> > -     mutex_lock(&data->update_lock);
> > -
> > -     if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_FAULT_STATUS1);
> > -             if (rv < 0)
> > -                     goto abort;
> > -             data->fault_status = rv & 0x3F;
> > -
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_FAULT_STATUS2);
> > -             if (rv < 0)
> > -                     goto abort;
> > -             data->fault_status |= (rv & 0x3F) << 6;
> > -
> > -             for (i = 0; i < NR_CHANNEL; i++) {
> > -                     rv = i2c_smbus_read_word_swapped(client,
> > -                                     MAX31790_REG_TACH_COUNT(i));
> > -                     if (rv < 0)
> > -                             goto abort;
> > -                     data->tach[i] = rv;
> > -
> > -                     if (data->fan_config[i]
> > -                         & MAX31790_FAN_CFG_TACH_INPUT) {
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                     MAX31790_REG_TACH_COUNT(NR_CHANNEL
> > -                                                             + i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->tach[NR_CHANNEL + i] = rv;
> > -                     } else {
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                             MAX31790_REG_PWMOUT(i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->pwm[i] = rv;
> > -
> > -                             rv = i2c_smbus_read_word_swapped(client,
> > -                                             MAX31790_REG_TARGET_COUNT(i));
> > -                             if (rv < 0)
> > -                                     goto abort;
> > -                             data->target_count[i] = rv;
> > -                     }
> > -             }
> > -
> > -             data->last_updated = jiffies;
> > -             data->valid = true;
> > -     }
> > -     goto done;
> > -
> > -abort:
> > -     data->valid = false;
> > -     ret = ERR_PTR(rv);
> > -
> > -done:
> > -     mutex_unlock(&data->update_lock);
> > -
> > -     return ret;
> > -}
> > -
> >  static const u8 tach_period[8] = { 1, 2, 4, 8, 16, 32, 32, 32 };
> >
> >  static u8 get_tach_period(u8 fan_dynamics)
> > @@ -159,28 +117,89 @@ static u8 bits_for_tach_period(int rpm)
> >       return bits;
> >  }
> >
> > +static int read_reg_byte(struct regmap *regmap, u8 reg)
> > +{
> > +     int rv;
> > +     int val;
> > +
> > +     rv = regmap_read(regmap, reg, &val);
> > +
>
> lease no empty line between assignment and check.
>
> > +     if (rv < 0)
> > +             return rv;
> > +
> > +     return val;
> > +}
> > +
> > +static int read_reg_word(struct regmap *regmap, u8 reg)
> > +{
> > +     int rv;
> > +     u8 val_bulk[2];
> > +
> > +     rv = regmap_bulk_read(regmap, reg, val_bulk, 2);
> > +     if (rv < 0)
> > +             return rv;
> > +
> > +     return BULK_TO_U16(val_bulk[0], val_bulk[1]);
> > +}
> > +
> > +static int write_reg_word(struct regmap *regmap, u8 reg, u16 val)
> > +{
> > +     u8 bulk_val[2];
> > +
> > +     bulk_val[0] = U16_MSB(val);
> > +     bulk_val[1] = U16_LSB(val);
> > +
> > +     return regmap_bulk_write(regmap, reg, bulk_val, 2);
> > +}
> > +
> >  static int max31790_read_fan(struct device *dev, u32 attr, int channel,
> >                            long *val)
> >  {
> > -     struct max31790_data *data = max31790_update_device(dev);
> > -     int sr, rpm;
> > +     struct max31790_data *data = dev_get_drvdata(dev);
> > +     struct regmap *regmap = data->regmap;
> > +     int rpm, dynamics, tach, fault;
> >
> >       if (IS_ERR(data))
> >               return PTR_ERR(data);
>
> Now unnecessary.
>
> >
> >       switch (attr) {
> >       case hwmon_fan_input:
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > -             rpm = RPM_FROM_REG(data->tach[channel], sr);
> > +             dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +             if (dynamics < 0)
> > +                     return dynamics;
> > +
> > +             tach = read_reg_word(regmap, MAX31790_REG_TACH_COUNT(channel));
> > +             if (tach < 0)
> > +                     return tach;
> > +
> > +             rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
> >               *val = rpm;
>
>                 *val = RPM_FROM_REG(tach, get_tach_period(dynamics));
>
> >               return 0;
> >       case hwmon_fan_target:
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > -             rpm = RPM_FROM_REG(data->target_count[channel], sr);
> > +             dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +             if (dynamics < 0)
> > +                     return dynamics;
> > +
> > +             tach = read_reg_word(regmap, MAX31790_REG_TARGET_COUNT(channel));
> > +             if (tach < 0)
> > +                     return tach;
> > +
> > +             rpm = RPM_FROM_REG(tach, get_tach_period(dynamics));
> >               *val = rpm;
>
>                 *val = RPM_FROM_REG(tach, get_tach_period(dynamics));
>
> >               return 0;
> >       case hwmon_fan_fault:
> > -             *val = !!(data->fault_status & (1 << channel));
> > +             if (channel > 6)
> > +                     fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS2);
> > +             else
> > +                     fault = read_reg_byte(regmap, MAX31790_REG_FAN_FAULT_STATUS1);
> > +
> > +             if (fault < 0)
> > +                     return fault;
> > +
> > +             if (channel > 6)
> > +                     *val = !!(fault & (1 << (channel - 6)));
> > +             else
> > +                     *val = !!(fault & (1 << channel));
> >               return 0;
> >       default:
> >               return -EOPNOTSUPP;
> > @@ -191,52 +210,58 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel,
> >                             long val)
> >  {
> >       struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > +     struct regmap *regmap = data->regmap;
> >       int target_count;
> >       int err = 0;
> >       u8 bits;
> >       int sr;
> > -
> > -     mutex_lock(&data->update_lock);
> > +     int fan_dynamics;
> >
> >       switch (attr) {
> >       case hwmon_fan_target:
> >               val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX);
> >               bits = bits_for_tach_period(val);
> > -             data->fan_dynamics[channel] =
> > -                     ((data->fan_dynamics[channel] &
> > +             fan_dynamics = read_reg_byte(regmap, MAX31790_REG_FAN_DYNAMICS(channel));
> > +
> Unnecessary empty line.
>
> > +             if (fan_dynamics < 0)
> > +                     return fan_dynamics;
> > +
> > +             fan_dynamics =
> > +                     ((fan_dynamics &
> >                         ~MAX31790_FAN_DYN_SR_MASK) |
> >                        (bits << MAX31790_FAN_DYN_SR_SHIFT));
> > -             err = i2c_smbus_write_byte_data(client,
> > -                                     MAX31790_REG_FAN_DYNAMICS(channel),
> > -                                     data->fan_dynamics[channel]);
> > +             err = regmap_write(regmap,
> > +                                MAX31790_REG_FAN_DYNAMICS(channel),
> > +                                fan_dynamics);
> >               if (err < 0)
> >                       break;
> >
> > -             sr = get_tach_period(data->fan_dynamics[channel]);
> > +             sr = get_tach_period(fan_dynamics);
> >               target_count = RPM_TO_REG(val, sr);
> >               target_count = clamp_val(target_count, 0x1, 0x7FF);
> >
> > -             data->target_count[channel] = target_count << 5;
> > +             target_count = target_count << 5;
> >
> > -             err = i2c_smbus_write_word_swapped(client,
> > -                                     MAX31790_REG_TARGET_COUNT(channel),
> > -                                     data->target_count[channel]);
> > +             err = write_reg_word(regmap,
> > +                                  MAX31790_REG_TARGET_COUNT(channel),
> > +                                  target_count);
> >               break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> >       }
> >
> > -     mutex_unlock(&data->update_lock);
> > -
> Why is this lock no longer required ? There are still multiple writes
> when writing hwmon_fan_target.
>
> >       return err;
> >  }
> >
> >  static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> >  {
> >       const struct max31790_data *data = _data;
> > -     u8 fan_config = data->fan_config[channel % NR_CHANNEL];
> > +     struct regmap *regmap = data->regmap;
> > +     u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +     if (fan_config < 0)
> > +             return 0;
>
> fan_config needs to be int. Also, this is a poor way of handling
> this problem. Since fan_config does not change dynamically,
> this is one set of values that would make sense to keep cached
> locally.
>
> >
> >       switch (attr) {
> >       case hwmon_fan_input:
> > @@ -258,22 +283,29 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel)
> >  static int max31790_read_pwm(struct device *dev, u32 attr, int channel,
> >                            long *val)
> >  {
> > -     struct max31790_data *data = max31790_update_device(dev);
> > -     u8 fan_config;
> > +     struct max31790_data *data = dev_get_drvdata(dev);
> > +     struct regmap *regmap = data->regmap;
> > +     int read;
> >
> >       if (IS_ERR(data))
> >               return PTR_ERR(data);
>
> Now unnecessary.
>
> >
> > -     fan_config = data->fan_config[channel];
> > -
> >       switch (attr) {
> >       case hwmon_pwm_input:
> > -             *val = data->pwm[channel] >> 8;
> > +             read = read_reg_word(regmap, MAX31790_REG_PWMOUT(channel));
> > +             if (read < 0)
> > +                     return read;
> > +
> > +             *val = read >> 8;
> >               return 0;
> >       case hwmon_pwm_enable:
> > -             if (fan_config & MAX31790_FAN_CFG_RPM_MODE)
> > +             read = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel));
> > +             if (read < 0)
> > +                     return read;
> > +
> > +             if (read & MAX31790_FAN_CFG_RPM_MODE)
> >                       *val = 2;
> > -             else if (fan_config & MAX31790_FAN_CFG_TACH_INPUT_EN)
> > +             else if (read & MAX31790_FAN_CFG_TACH_INPUT_EN)
> >                       *val = 1;
> >               else
> >                       *val = 0;
> > @@ -287,25 +319,24 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> >                             long val)
> >  {
> >       struct max31790_data *data = dev_get_drvdata(dev);
> > -     struct i2c_client *client = data->client;
> > +     struct regmap *regmap = data->regmap;
> >       u8 fan_config;
> >       int err = 0;
> >
> > -     mutex_lock(&data->update_lock);
> > -
> >       switch (attr) {
> >       case hwmon_pwm_input:
> >               if (val < 0 || val > 255) {
> >                       err = -EINVAL;
> >                       break;
> >               }
> > -             data->pwm[channel] = val << 8;
> > -             err = i2c_smbus_write_word_swapped(client,
> > -                                                MAX31790_REG_PWMOUT(channel),
> > -                                                data->pwm[channel]);
> > +             err = write_reg_word(regmap, MAX31790_REG_PWMOUT(channel), val << 8);
> >               break;
> >       case hwmon_pwm_enable:
> > -             fan_config = data->fan_config[channel];
> > +             fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +             if (fan_config < 0)
> > +                     return fan_config;
> > +
> >               if (val == 0) {
> >                       fan_config &= ~(MAX31790_FAN_CFG_TACH_INPUT_EN |
> >                                       MAX31790_FAN_CFG_RPM_MODE);
> > @@ -320,25 +351,26 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
> >                       err = -EINVAL;
> >                       break;
> >               }
> > -             data->fan_config[channel] = fan_config;
> > -             err = i2c_smbus_write_byte_data(client,
> > -                                     MAX31790_REG_FAN_CONFIG(channel),
> > -                                     fan_config);
> > +             err = regmap_write(regmap,
> > +                                MAX31790_REG_FAN_CONFIG(channel),
> > +                                fan_config);
> >               break;
> >       default:
> >               err = -EOPNOTSUPP;
> >               break;
> >       }
> >
> > -     mutex_unlock(&data->update_lock);
> > -
> Are you sure this mutex is no longer needed here, ie that there
> can not be an interaction with multiple writes from multiple processes
> at the same time ?
>
> >       return err;
> >  }
> >
> >  static umode_t max31790_pwm_is_visible(const void *_data, u32 attr, int channel)
> >  {
> >       const struct max31790_data *data = _data;
> > -     u8 fan_config = data->fan_config[channel];
> > +     struct regmap *regmap = data->regmap;
> > +     u8 fan_config = read_reg_byte(regmap, MAX31790_REG_FAN_CONFIG(channel % NR_CHANNEL));
> > +
> > +     if (fan_config < 0)
> > +             return 0;
>
> int problem again.
>
> >
> >       switch (attr) {
> >       case hwmon_pwm_input:
> > @@ -426,35 +458,12 @@ static const struct hwmon_chip_info max31790_chip_info = {
> >       .info = max31790_info,
> >  };
> >
> > -static int max31790_init_client(struct i2c_client *client,
> > -                             struct max31790_data *data)
> > -{
> > -     int i, rv;
> > -
> > -     for (i = 0; i < NR_CHANNEL; i++) {
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_CONFIG(i));
> > -             if (rv < 0)
> > -                     return rv;
> > -             data->fan_config[i] = rv;
> > -
> > -             rv = i2c_smbus_read_byte_data(client,
> > -                             MAX31790_REG_FAN_DYNAMICS(i));
> > -             if (rv < 0)
> > -                     return rv;
> > -             data->fan_dynamics[i] = rv;
>
> The above "cached" values are static, and it did make sense to keep those
> locally to avoid requiring unnecessary error handling (and to detect issues
> with the chip early).
>
> > -     }
> > -
> > -     return 0;
> > -}
> > -
> >  static int max31790_probe(struct i2c_client *client)
> >  {
> >       struct i2c_adapter *adapter = client->adapter;
> >       struct device *dev = &client->dev;
> >       struct max31790_data *data;
> >       struct device *hwmon_dev;
> > -     int err;
> >
> >       if (!i2c_check_functionality(adapter,
> >                       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA))
> > @@ -464,15 +473,12 @@ static int max31790_probe(struct i2c_client *client)
> >       if (!data)
> >               return -ENOMEM;
> >
> > -     data->client = client;
> > -     mutex_init(&data->update_lock);
> > +     data->regmap = devm_regmap_init_i2c(client, &max31790_regmap_config);
> >
> > -     /*
> > -      * Initialize the max31790 chip
> > -      */
> > -     err = max31790_init_client(client, data);
> > -     if (err)
> > -             return err;
> > +     if (IS_ERR(data->regmap)) {
> > +             dev_err(dev, "failed to allocate register map\n");
> > +             return PTR_ERR(data->regmap);
> > +     }
> >
> >       hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> >                                                        data,