linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap
@ 2021-05-12  1:30 Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:30 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel
  Cc: 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 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 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
@ 2021-05-12  1:30 ` Václav Kubernát
  2021-05-12  1:32   ` Václav Kubernát
  2021-05-19 23:11   ` Guenter Roeck
  2021-05-12  1:30 ` [PATCH v5 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled Václav Kubernát
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:30 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel
  Cc: 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),
and what that does is that it sets PWM to 0% duty cycle. In the new
code, 0 in pwm*_enable turns off the "control" feature of the chip.

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 | 10 ++--
 drivers/hwmon/max31790.c         | 78 +++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 30 deletions(-)

diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
index f301385d8cef..d2ac4e926905 100644
--- a/Documentation/hwmon/max31790.rst
+++ b/Documentation/hwmon/max31790.rst
@@ -34,10 +34,14 @@ 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=no control, sets 0% PWM,
+				       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..5d4c551df010 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -30,6 +30,7 @@
 #define MAX31790_FAN_CFG_RPM_MODE	0x80
 #define MAX31790_FAN_CFG_TACH_INPUT_EN	0x08
 #define MAX31790_FAN_CFG_TACH_INPUT	0x01
+#define MAX31790_FAN_CFG_CTRL_MON	0x10
 
 /* Fan Dynamics register bits */
 #define MAX31790_FAN_DYN_SR_SHIFT	5
@@ -191,6 +192,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 +237,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 +273,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 +299,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->fan_config[channel] & MAX31790_FAN_CFG_CTRL_MON)
+			*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;
@@ -298,35 +316,41 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
 {
 	struct max31790_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
-	u8 fan_config;
+	u8 fan_config = data->fan_config[channel];
 	int err = 0;
 
 	mutex_lock(&data->update_lock);
 
 	switch (attr) {
 	case hwmon_pwm_input:
-		if (val < 0 || val > 255) {
+		if (fan_config & MAX31790_FAN_CFG_CTRL_MON || 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)
+			fan_config |= MAX31790_FAN_CFG_CTRL_MON;
+		else if (val == 1) {
+			fan_config &= ~(MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_CTRL_MON);
 		} else if (val == 2) {
-			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
-				      MAX31790_FAN_CFG_RPM_MODE;
+			fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON);
+			fan_config |= MAX31790_FAN_CFG_RPM_MODE;
 		} else {
 			err = -EINVAL;
 			break;
 		}
+
+		/*
+		 * 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 +424,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,
-- 
2.31.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
@ 2021-05-12  1:30 ` Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 4/5] hwmon: (max31790) Allow setting fan*_div Václav Kubernát
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:30 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel
  Cc: 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 5d4c551df010..6e0ffbcf0d28 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -165,6 +165,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;
@@ -179,6 +182,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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 4/5] hwmon: (max31790) Allow setting fan*_div
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled Václav Kubernát
@ 2021-05-12  1:30 ` Václav Kubernát
  2021-05-12  1:30 ` [PATCH v5 5/5] hwmon: (max31790) Update documentation Václav Kubernát
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:30 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel
  Cc: 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 d2ac4e926905..35afa5f0395a 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=no control, sets 0% PWM,
 				       1=manual (pwm) mode,
diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c
index 6e0ffbcf0d28..8b04f4c01752 100644
--- a/drivers/hwmon/max31790.c
+++ b/drivers/hwmon/max31790.c
@@ -156,6 +156,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)
 {
@@ -203,6 +223,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;
 	}
@@ -254,6 +277,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;
@@ -282,6 +323,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;
@@ -432,18 +474,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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5 5/5] hwmon: (max31790) Update documentation
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
                   ` (2 preceding siblings ...)
  2021-05-12  1:30 ` [PATCH v5 4/5] hwmon: (max31790) Allow setting fan*_div Václav Kubernát
@ 2021-05-12  1:30 ` Václav Kubernát
  2021-05-19 19:00 ` [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Guenter Roeck
  2021-05-20  5:12 ` Guenter Roeck
  5 siblings, 0 replies; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:30 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel
  Cc: 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 35afa5f0395a..e70aae9f71a2 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=no control, sets 0% PWM,
 				       1=manual (pwm) mode,
 				       2=rpm mode
-- 
2.31.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
@ 2021-05-12  1:32   ` Václav Kubernát
  2021-05-18 14:59     ` Guenter Roeck
  2021-05-19 23:11   ` Guenter Roeck
  1 sibling, 1 reply; 15+ messages in thread
From: Václav Kubernát @ 2021-05-12  1:32 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon,
	linux-doc, linux-kernel

Hello,

I have updated the code and got rid of the "fullspeed" mode as
requested. Let me know what you think.

Thanks
Václav

st 12. 5. 2021 v 3:31 odesílatel Václav Kubernát <kubernat@cesnet.cz> napsal:
>
> 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),
> and what that does is that it sets PWM to 0% duty cycle. In the new
> code, 0 in pwm*_enable turns off the "control" feature of the chip.
>
> 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 | 10 ++--
>  drivers/hwmon/max31790.c         | 78 +++++++++++++++++++++-----------
>  2 files changed, 58 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index f301385d8cef..d2ac4e926905 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -34,10 +34,14 @@ 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=no control, sets 0% PWM,
> +                                      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..5d4c551df010 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -30,6 +30,7 @@
>  #define MAX31790_FAN_CFG_RPM_MODE      0x80
>  #define MAX31790_FAN_CFG_TACH_INPUT_EN 0x08
>  #define MAX31790_FAN_CFG_TACH_INPUT    0x01
> +#define MAX31790_FAN_CFG_CTRL_MON      0x10
>
>  /* Fan Dynamics register bits */
>  #define MAX31790_FAN_DYN_SR_SHIFT      5
> @@ -191,6 +192,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 +237,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 +273,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 +299,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->fan_config[channel] & MAX31790_FAN_CFG_CTRL_MON)
> +                       *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;
> @@ -298,35 +316,41 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>  {
>         struct max31790_data *data = dev_get_drvdata(dev);
>         struct regmap *regmap = data->regmap;
> -       u8 fan_config;
> +       u8 fan_config = data->fan_config[channel];
>         int err = 0;
>
>         mutex_lock(&data->update_lock);
>
>         switch (attr) {
>         case hwmon_pwm_input:
> -               if (val < 0 || val > 255) {
> +               if (fan_config & MAX31790_FAN_CFG_CTRL_MON || 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)
> +                       fan_config |= MAX31790_FAN_CFG_CTRL_MON;
> +               else if (val == 1) {
> +                       fan_config &= ~(MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_CTRL_MON);
>                 } else if (val == 2) {
> -                       fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> -                                     MAX31790_FAN_CFG_RPM_MODE;
> +                       fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON);
> +                       fan_config |= MAX31790_FAN_CFG_RPM_MODE;
>                 } else {
>                         err = -EINVAL;
>                         break;
>                 }
> +
> +               /*
> +                * 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 +424,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,
> --
> 2.31.1
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-12  1:32   ` Václav Kubernát
@ 2021-05-18 14:59     ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-05-18 14:59 UTC (permalink / raw)
  To: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

On 5/11/21 6:32 PM, Václav Kubernát wrote:
> Hello,
> 
> I have updated the code and got rid of the "fullspeed" mode as
> requested. Let me know what you think.
> 

My major problem right now is that I can't reliably test the code, and I am
only going to apply it after some thorough testing (especially after the
problem with regmap volatiles in v1 I'll want to make sure that volatile
registers are handled correctly). I am working on improving my module test
code, but it will take a while.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
                   ` (3 preceding siblings ...)
  2021-05-12  1:30 ` [PATCH v5 5/5] hwmon: (max31790) Update documentation Václav Kubernát
@ 2021-05-19 19:00 ` Guenter Roeck
  2021-05-20  5:12 ` Guenter Roeck
  5 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-05-19 19:00 UTC (permalink / raw)
  To: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

On 5/11/21 6:30 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 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 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,

This should be:
	.range_max = MAX31790_REG_PWM_DUTY_CYCLE(5) + 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),

This should be:

         regmap_reg_range(MAX31790_REG_PWM_DUTY_CYCLE(0), MAX31790_REG_PWM_DUTY_CYCLE(5) + 1),
         regmap_reg_range(MAX31790_REG_TACH_COUNT(0), MAX31790_REG_TACH_COUNT(11) + 1),
         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

         .yes_ranges = max31790_volatile_ranges,
         .n_yes_ranges = ARRAY_SIZE(max31790_volatile_ranges),

Initializing with 0 is not necessary for static variables.

> +};
> +

As I have already said and now confirmed, specifying volatile ranges as no_ranges
is wrong. This only "works" because the cache method is specified implicitly as
REGCACHE_NONE, ie it has no effect.

> +static const struct regmap_config max31790_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.reg_stride = 1,
> +	.max_register = MAX31790_REG_USER_BYTE_67,

Add:
	.num_reg_defaults_raw = MAX31790_REG_USER_BYTE_67 + 1,
	.cache_type = REGCACHE_FLAT,

for caching to really work.
	
> +	.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;
>   
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
  2021-05-12  1:32   ` Václav Kubernát
@ 2021-05-19 23:11   ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-05-19 23:11 UTC (permalink / raw)
  To: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

On 5/11/21 6:30 PM, 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),
> and what that does is that it sets PWM to 0% duty cycle. In the new
> code, 0 in pwm*_enable turns off the "control" feature of the chip.
> 
> 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).
> 
The above is an indication that the current code is simply wrong.
pwmX_enable does not and should not have anything to do with
fan speed monitoring (even though it may implicitly enable it).

Fixing pwmX_enable therefore needs to be a separate patch from
introducing fanX_enable, and like the other fixes it needs to come
first, before changing the code to use regmap (to enable backporting).

More comments below.

Thanks,
Guenter

> Signed-off-by: Václav Kubernát <kubernat@cesnet.cz>
> ---
>   Documentation/hwmon/max31790.rst | 10 ++--
>   drivers/hwmon/max31790.c         | 78 +++++++++++++++++++++-----------
>   2 files changed, 58 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index f301385d8cef..d2ac4e926905 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -34,10 +34,14 @@ 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=no control, sets 0% PWM,
> +				       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..5d4c551df010 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -30,6 +30,7 @@
>   #define MAX31790_FAN_CFG_RPM_MODE	0x80
>   #define MAX31790_FAN_CFG_TACH_INPUT_EN	0x08
>   #define MAX31790_FAN_CFG_TACH_INPUT	0x01
> +#define MAX31790_FAN_CFG_CTRL_MON	0x10

This define should be above MAX31790_FAN_CFG_TACH_INPUT_EN
to maintain sequence/order.

>   
>   /* Fan Dynamics register bits */
>   #define MAX31790_FAN_DYN_SR_SHIFT	5
> @@ -191,6 +192,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 +237,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 +273,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 +299,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->fan_config[channel] & MAX31790_FAN_CFG_CTRL_MON)
> +			*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;
> @@ -298,35 +316,41 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>   {
>   	struct max31790_data *data = dev_get_drvdata(dev);
>   	struct regmap *regmap = data->regmap;
> -	u8 fan_config;
> +	u8 fan_config = data->fan_config[channel];
>   	int err = 0;
>   
>   	mutex_lock(&data->update_lock);
>   
>   	switch (attr) {
>   	case hwmon_pwm_input:
> -		if (val < 0 || val > 255) {
> +		if (fan_config & MAX31790_FAN_CFG_CTRL_MON || val < 0 || val > 255) {

No. It has to be possible to set the target pwm value before enabling pwm control.

>   			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)
> +			fan_config |= MAX31790_FAN_CFG_CTRL_MON;
> +		else if (val == 1) {
> +			fan_config &= ~(MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_CTRL_MON);
>   		} else if (val == 2) {
> -			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> -				      MAX31790_FAN_CFG_RPM_MODE;
> +			fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON);

Unnecessary ( )

> +			fan_config |= MAX31790_FAN_CFG_RPM_MODE;
>   		} else {
>   			err = -EINVAL;
>   			break;
>   		}
> +
> +		/*
> +		 * RPM mode implies enabled TACH input, so enable it in RPM
> +		 * mode.
> +		 */
> +		if (val == 2)
> +			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN;
> +

The datasheet says this is automatically enabled in rpm mode, meaning there
is no need to explicitly set the bit. I don't know if the bit is set
automatically. If not, it should not be set here. Either case, there is
already an "if (val ==2)" check above. If needed, this bit should be set there.

>   		data->fan_config[channel] = fan_config;
>   		err = regmap_write(regmap,
>   				   MAX31790_REG_FAN_CONFIG(channel),
> @@ -400,18 +424,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,
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap
  2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
                   ` (4 preceding siblings ...)
  2021-05-19 19:00 ` [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Guenter Roeck
@ 2021-05-20  5:12 ` Guenter Roeck
  5 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-05-20  5:12 UTC (permalink / raw)
  To: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

On 5/11/21 6:30 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 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 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)

Please use the NR_CHANNEL constant.

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

Better written without conditional as
		*val = !!(fault & (1 << (channel % NR_CHANNEL)));

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-20 11:29     ` Jan Kundrát
@ 2021-05-20 11:50       ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-05-20 11:50 UTC (permalink / raw)
  To: Jan Kundrát, Václav Kubernát
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel

On 5/20/21 4:29 AM, Jan Kundrát wrote:
>> As for fan[7-12]_enable, I don't even know if those can be enabled
>> separately. I see two options: Drop those attributes entirely (
>> assuming that those fan inputs are always enabled if the associated
>> pins are configured as inputs), or align them with fan[1-6]_enable.
> 
> I think we need to decide first who provides the initial configuration for this chip. There's always at least six TACH inputs, and then there's six more pins where each can be either a PWM output or a TACH input. Who decides that? Is the kernel supposed to export six knobs to the userspace? So far, I've assumed that this should be driven via sysfs. But perhaps you would you like to rely on the FW (?) to set this up properly? (On our board, that would be a few random calls to `i2cset` from a U-Boot boot script. Not pretty, but doable. Just one more place to keep track of.)
> 

It has to be done by firmware or with devicetree properties.
It can not be done with sysfs attributes.

> It's proabably "tricky" to do this at runtime -- and I don't expect to see many boards where you have such a big freedom of reconnecting the actual fans once manufactured, anyway. So, either some DT parameters, or an autodetection based on whatever is in the registers at power up, which would make an explicit assumption that "something" has set up the nPWM/TACH bits properly in the Fan Configuration Register. OK, that might work, but the kernel must not ever reset that chip afterwards.
> 
> There's also the Fan Fault Mask register, which controls which fans propagate their failures to the nFAN_FAIL output pin. This one requires a semi-independent control than the nPWM/TACH bit above. It's feasible that not all TACH inputs have an actual fan connected, and this can well vary between products. For example, ours has just four fan connectors, so we don't want "failures" of fans 5 and 6 to assert the nFAN_FAIL pin. Also, there should be a check which prevents unmasking these failures for those TACH channels which are configured as PWM outputs. Or we can once again ignore this one and rely on the FW.
> 

Power-up default is that all faults are masked. If that is not the case,
some entity must have unmasked some bits. Assuming that is the case, we
can as well assume that the same entity is able to configure bit 0
of the configuration register as needed.

> The current kernel code in max31790_read_fan() reads beyond the end of data->fan_dynamics, hitting the content of `fault_status` or `tach` fields instead, and therefore returning garbage. Not a big deal, just a missing % operator I guess, but to me, that's a pretty strong suggestion that nobody has used or even tested monitoring more than six fans on this chip, ever. (And yeah, the datasheet is not clear on how it's supposed to work anyway. Using a modulo is just a guess.)
> 

Probably it has not been tested, but that is not an argument for removal.

> Neither Vaclav nor me have any way of testing this feature -- hence my proposal to only improve what we need, and ignore TACH channels 7-12. But I guess it's not OK from your point of view?
> 

No, that is not acceptable.

FWIW, the evaluation board, including shipping, cost about $130 at Mouser.
I should get it no late than early next week. Arguing about it was more
expensive (in terms of time spent) than just buying it. Once I have the
board and had a chance to test the code I'll submit a set of changes
to fix all the problems I have found so far using module test code and
code inspection.

- Fix fan speed reporting for fan7..12
- Report correct current pwm duty cycles
- Fix pwmX_enable attributes

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-19 13:55   ` Guenter Roeck
@ 2021-05-20 11:29     ` Jan Kundrát
  2021-05-20 11:50       ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kundrát @ 2021-05-20 11:29 UTC (permalink / raw)
  To: Guenter Roeck, Václav Kubernát
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel

> As for fan[7-12]_enable, I don't even know if those can be enabled
> separately. I see two options: Drop those attributes entirely (
> assuming that those fan inputs are always enabled if the associated
> pins are configured as inputs), or align them with fan[1-6]_enable.

I think we need to decide first who provides the initial configuration for 
this chip. There's always at least six TACH inputs, and then there's six 
more pins where each can be either a PWM output or a TACH input. Who 
decides that? Is the kernel supposed to export six knobs to the userspace? 
So far, I've assumed that this should be driven via sysfs. But perhaps you 
would you like to rely on the FW (?) to set this up properly? (On our 
board, that would be a few random calls to `i2cset` from a U-Boot boot 
script. Not pretty, but doable. Just one more place to keep track of.)

It's proabably "tricky" to do this at runtime -- and I don't expect to see 
many boards where you have such a big freedom of reconnecting the actual 
fans once manufactured, anyway. So, either some DT parameters, or an 
autodetection based on whatever is in the registers at power up, which 
would make an explicit assumption that "something" has set up the nPWM/TACH 
bits properly in the Fan Configuration Register. OK, that might work, but 
the kernel must not ever reset that chip afterwards.

There's also the Fan Fault Mask register, which controls which fans 
propagate their failures to the nFAN_FAIL output pin. This one requires a 
semi-independent control than the nPWM/TACH bit above. It's feasible that 
not all TACH inputs have an actual fan connected, and this can well vary 
between products. For example, ours has just four fan connectors, so we 
don't want "failures" of fans 5 and 6 to assert the nFAN_FAIL pin. Also, 
there should be a check which prevents unmasking these failures for those 
TACH channels which are configured as PWM outputs. Or we can once again 
ignore this one and rely on the FW.

The current kernel code in max31790_read_fan() reads beyond the end of 
data->fan_dynamics, hitting the content of `fault_status` or `tach` fields 
instead, and therefore returning garbage. Not a big deal, just a missing % 
operator I guess, but to me, that's a pretty strong suggestion that nobody 
has used or even tested monitoring more than six fans on this chip, ever. 
(And yeah, the datasheet is not clear on how it's supposed to work anyway. 
Using a modulo is just a guess.)

Neither Vaclav nor me have any way of testing this feature -- hence my 
proposal to only improve what we need, and ignore TACH channels 7-12. But I 
guess it's not OK from your point of view?

With kind regards,
Jan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-19  9:10 ` Jan Kundrát
@ 2021-05-19 13:55   ` Guenter Roeck
  2021-05-20 11:29     ` Jan Kundrát
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-05-19 13:55 UTC (permalink / raw)
  To: Jan Kundrát
  Cc: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

On 5/19/21 2:10 AM, Jan Kundrát wrote:
>> As it turns out, even the current code doesn't really work for fans 7..12.
>>         sr = get_tach_period(data->fan_dynamics[channel]);
>> However, the data->fan_dynamics array has only 6 entries, not 12, so
>> reading fan[7-12]_input will result in bad/random values.
> 
> Hi Guenter, I'm Vaclav's colleague. The chip can indeed reconfigure each PWMOUT pin either as a PWM output or as a TACH input, but that's not something that's correctly implemented in the current code, and we have no use for that either (and we cannot test that on our PCBs easily, we do not have the manufacturer's eval kit).
> 
> It looks to me that the original bug is that the current docs mention 12 fan inputs. Would you be OK with a patch series which fixes the docs so that the chip always exports 6 TACH inputs and 6 PWMOUT channels?
> 
That would not be appropriate. The chip does support 12 fan inputs,
so that is not a bug. Its support has a bug, and the datasheet is kind
of vague when it comes to details, but that doesn't mean we can just
remove its support.

I see two bugs in the current code:
- pwm values should be read from the duty cycle registers,
   not from the target duty cycle registers.
- fan[1-12]_input needs to use a correct divider value.
   Unfortunately the datasheet is a bit vague when it comes
   to deciding which divider value to use, so the best we can
   do is going to use the values from fan[1-6].

Fixing this will require two patches, which should come first.
Let me know if you want to do that; if not I'll write patches
in the next few days.

As for fan[7-12]_enable, I don't even know if those can be enabled
separately. I see two options: Drop those attributes entirely (
assuming that those fan inputs are always enabled if the associated
pins are configured as inputs), or align them with fan[1-6]_enable.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
  2021-05-18 21:16 [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Guenter Roeck
@ 2021-05-19  9:10 ` Jan Kundrát
  2021-05-19 13:55   ` Guenter Roeck
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kundrát @ 2021-05-19  9:10 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Václav Kubernát, Jean Delvare, Jonathan Corbet,
	linux-hwmon, linux-doc, linux-kernel

> As it turns out, even the current code doesn't really work for fans 7..12.
> 		sr = get_tach_period(data->fan_dynamics[channel]);
> However, the data->fan_dynamics array has only 6 entries, not 12, so
> reading fan[7-12]_input will result in bad/random values.

Hi Guenter, I'm Vaclav's colleague. The chip can indeed reconfigure each 
PWMOUT pin either as a PWM output or as a TACH input, but that's not 
something that's correctly implemented in the current code, and we have no 
use for that either (and we cannot test that on our PCBs easily, we do not 
have the manufacturer's eval kit).

It looks to me that the original bug is that the current docs mention 12 
fan inputs. Would you be OK with a patch series which fixes the docs so 
that the chip always exports 6 TACH inputs and 6 PWMOUT channels?

Cheers,
Jan

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable
@ 2021-05-18 21:16 Guenter Roeck
  2021-05-19  9:10 ` Jan Kundrát
  0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-05-18 21:16 UTC (permalink / raw)
  To: Václav Kubernát
  Cc: Jean Delvare, Jonathan Corbet, linux-hwmon, linux-doc, linux-kernel

On Wed, May 12, 2021 at 03:30:48AM +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),
> and what that does is that it sets PWM to 0% duty cycle. In the new
> code, 0 in pwm*_enable turns off the "control" feature of the chip.
> 
> 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 | 10 ++--
>  drivers/hwmon/max31790.c         | 78 +++++++++++++++++++++-----------
>  2 files changed, 58 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst
> index f301385d8cef..d2ac4e926905 100644
> --- a/Documentation/hwmon/max31790.rst
> +++ b/Documentation/hwmon/max31790.rst
> @@ -34,10 +34,14 @@ 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=no control, sets 0% PWM,
> +				       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..5d4c551df010 100644
> --- a/drivers/hwmon/max31790.c
> +++ b/drivers/hwmon/max31790.c
> @@ -30,6 +30,7 @@
>  #define MAX31790_FAN_CFG_RPM_MODE	0x80
>  #define MAX31790_FAN_CFG_TACH_INPUT_EN	0x08
>  #define MAX31790_FAN_CFG_TACH_INPUT	0x01
> +#define MAX31790_FAN_CFG_CTRL_MON	0x10
>  
>  /* Fan Dynamics register bits */
>  #define MAX31790_FAN_DYN_SR_SHIFT	5
> @@ -191,6 +192,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);

fan_config array size is 6, and there are up to 12 fans ... so this won't
work for fans 7..12.

> +		return 0;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -233,6 +237,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;

Same as above.

As it turns out, even the current code doesn't really work for fans 7..12.
		sr = get_tach_period(data->fan_dynamics[channel]);
However, the data->fan_dynamics array has only 6 entries, not 12, so
reading fan[7-12]_input will result in bad/random values.

>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> @@ -260,6 +273,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 +299,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->fan_config[channel] & MAX31790_FAN_CFG_CTRL_MON)
> +			*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;
> @@ -298,35 +316,41 @@ static int max31790_write_pwm(struct device *dev, u32 attr, int channel,
>  {
>  	struct max31790_data *data = dev_get_drvdata(dev);
>  	struct regmap *regmap = data->regmap;
> -	u8 fan_config;
> +	u8 fan_config = data->fan_config[channel];
>  	int err = 0;
>  
>  	mutex_lock(&data->update_lock);
>  
>  	switch (attr) {
>  	case hwmon_pwm_input:
> -		if (val < 0 || val > 255) {
> +		if (fan_config & MAX31790_FAN_CFG_CTRL_MON || 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)
> +			fan_config |= MAX31790_FAN_CFG_CTRL_MON;
> +		else if (val == 1) {
> +			fan_config &= ~(MAX31790_FAN_CFG_RPM_MODE | MAX31790_FAN_CFG_CTRL_MON);
>  		} else if (val == 2) {
> -			fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN |
> -				      MAX31790_FAN_CFG_RPM_MODE;
> +			fan_config &= ~(MAX31790_FAN_CFG_CTRL_MON);
> +			fan_config |= MAX31790_FAN_CFG_RPM_MODE;
>  		} else {
>  			err = -EINVAL;
>  			break;
>  		}
> +
> +		/*
> +		 * 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 +424,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,

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-05-20 12:36 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12  1:30 [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Václav Kubernát
2021-05-12  1:32   ` Václav Kubernát
2021-05-18 14:59     ` Guenter Roeck
2021-05-19 23:11   ` Guenter Roeck
2021-05-12  1:30 ` [PATCH v5 3/5] hwmon: (max31790) Show 0 RPM/fault when input disabled Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 4/5] hwmon: (max31790) Allow setting fan*_div Václav Kubernát
2021-05-12  1:30 ` [PATCH v5 5/5] hwmon: (max31790) Update documentation Václav Kubernát
2021-05-19 19:00 ` [PATCH v5 1/5] hwmon: (max31790) Rework to use regmap Guenter Roeck
2021-05-20  5:12 ` Guenter Roeck
2021-05-18 21:16 [PATCH v5 2/5] hwmon: (max31790) Fix and split pwm*_enable Guenter Roeck
2021-05-19  9:10 ` Jan Kundrát
2021-05-19 13:55   ` Guenter Roeck
2021-05-20 11:29     ` Jan Kundrát
2021-05-20 11:50       ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).