[PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device

[PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device

[Guenter Roeck]

Use devm_add_action to unregister the thermal cooling device.
This lets us drop the remove function.

At the same time, use 'dev' variable in probe function consistently.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 939953240827..6cce199dab6a 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -752,6 +752,11 @@ static const struct thermal_cooling_device_ops max6650_cooling_ops = {
 	.get_cur_state = max6650_get_cur_state,
 	.set_cur_state = max6650_set_cur_state,
 };
+
+static void max6650_thermal_unregister(void *data)
+{
+	thermal_cooling_device_unregister(data);
+}
 #endif
 
 static int max6650_probe(struct i2c_client *client,
@@ -794,27 +799,20 @@ static int max6650_probe(struct i2c_client *client,
 
 #if IS_ENABLED(CONFIG_THERMAL)
 	data->cooling_dev =
-		thermal_of_cooling_device_register(client->dev.of_node,
+		thermal_of_cooling_device_register(dev->of_node,
 						   client->name, data,
 						   &max6650_cooling_ops);
-	if (IS_ERR(data->cooling_dev))
-		dev_warn(&client->dev,
-			 "thermal cooling device register failed: %ld\n",
+	if (IS_ERR(data->cooling_dev)) {
+		dev_warn(dev, "thermal cooling device register failed: %ld\n",
 			 PTR_ERR(data->cooling_dev));
+	} else {
+		devm_add_action(dev, max6650_thermal_unregister,
+				data->cooling_dev);
+	}
 #endif
 	return 0;
 }
 
-static int max6650_remove(struct i2c_client *client)
-{
-	struct max6650_data *data = i2c_get_clientdata(client);
-
-	if (!IS_ERR(data->cooling_dev))
-		thermal_cooling_device_unregister(data->cooling_dev);
-
-	return 0;
-}
-
 static const struct i2c_device_id max6650_id[] = {
 	{ "max6650", 1 },
 	{ "max6651", 4 },
@@ -828,7 +826,6 @@ static struct i2c_driver max6650_driver = {
 		.of_match_table = of_match_ptr(max6650_dt_match),
 	},
 	.probe		= max6650_probe,
-	.remove		= max6650_remove,
 	.id_table	= max6650_id,
 };
 
-- 
2.7.4

[PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm

[Guenter Roeck]

Consolidate conversion from pwm value to dac value and from dac value
to pwm value into helper functions.

While doing this, only update the cached dac value if writing it to
the chip was successful after an update. Also, put macro argument of
DIV_FROM_REG() into ().

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 53 ++++++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 6cce199dab6a..b6b8f8edc1b0 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -105,7 +105,8 @@ module_param(clock, int, 0444);
 #define FAN_RPM_MIN 240
 #define FAN_RPM_MAX 30000
 
-#define DIV_FROM_REG(reg) (1 << (reg & 7))
+#define DIV_FROM_REG(reg)	(1 << ((reg) & 7))
+#define DAC_LIMIT(v12)		((v12) ? 180 : 76)
 
 /*
  * Client data (each client gets its own)
@@ -150,6 +151,22 @@ static const struct of_device_id __maybe_unused max6650_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, max6650_dt_match);
 
+static int dac_to_pwm(int dac, bool v12)
+{
+	/*
+	 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V fans.
+	 * Lower DAC values mean higher speeds.
+	 */
+	return clamp_val(255 - (255 * dac) / DAC_LIMIT(v12), 0, 255);
+}
+
+static u8 pwm_to_dac(unsigned int pwm, bool v12)
+{
+	int limit = DAC_LIMIT(v12);
+
+	return limit - (limit * pwm) / 255;
+}
+
 static struct max6650_data *max6650_update_device(struct device *dev)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
@@ -357,22 +374,10 @@ static ssize_t fan1_target_store(struct device *dev,
 static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	int pwm;
 	struct max6650_data *data = max6650_update_device(dev);
 
-	/*
-	 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V fans.
-	 * Lower DAC values mean higher speeds.
-	 */
-	if (data->config & MAX6650_CFG_V12)
-		pwm = 255 - (255 * (int)data->dac)/180;
-	else
-		pwm = 255 - (255 * (int)data->dac)/76;
-
-	if (pwm < 0)
-		pwm = 0;
-
-	return sprintf(buf, "%d\n", pwm);
+	return sprintf(buf, "%d\n", dac_to_pwm(data->dac,
+					       data->config & MAX6650_CFG_V12));
 }
 
 static ssize_t pwm1_store(struct device *dev,
@@ -383,6 +388,7 @@ static ssize_t pwm1_store(struct device *dev,
 	struct i2c_client *client = data->client;
 	unsigned long pwm;
 	int err;
+	u8 dac;
 
 	err = kstrtoul(buf, 10, &pwm);
 	if (err)
@@ -391,13 +397,10 @@ static ssize_t pwm1_store(struct device *dev,
 	pwm = clamp_val(pwm, 0, 255);
 
 	mutex_lock(&data->update_lock);
-
-	if (data->config & MAX6650_CFG_V12)
-		data->dac = 180 - (180 * pwm)/255;
-	else
-		data->dac = 76 - (76 * pwm)/255;
-	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
-
+	dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
+	if (!err)
+		data->dac = dac;
 	mutex_unlock(&data->update_lock);
 
 	return err < 0 ? err : count;
@@ -728,11 +731,7 @@ static int max6650_set_cur_state(struct thermal_cooling_device *cdev,
 
 	mutex_lock(&data->update_lock);
 
-	if (data->config & MAX6650_CFG_V12)
-		data->dac = 180 - (180 * state)/255;
-	else
-		data->dac = 76 - (76 * state)/255;
-
+	data->dac = pwm_to_dac(state, data->config & MAX6650_CFG_V12);
 	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
 
 	if (!err) {
-- 
2.7.4

[PATCH 03/11] hwmon: (max6650) Improve error handling in max6650_init_client

[Guenter Roeck]

Do not overwrite errors reported from i2c functions, and don't ignore
any errors.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index b6b8f8edc1b0..ac05cc5627d7 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -618,8 +618,8 @@ static int max6650_init_client(struct max6650_data *data,
 			       struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	int config;
-	int err = -EIO;
+	int reg;
+	int err;
 	u32 voltage;
 	u32 prescale;
 	u32 target_rpm;
@@ -633,21 +633,20 @@ static int max6650_init_client(struct max6650_data *data,
 				 &prescale))
 		prescale = prescaler;
 
-	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
-
-	if (config < 0) {
-		dev_err(dev, "Error reading config, aborting.\n");
-		return err;
+	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
+	if (reg < 0) {
+		dev_err(dev, "Error reading config register, aborting.\n");
+		return reg;
 	}
 
 	switch (voltage) {
 	case 0:
 		break;
 	case 5:
-		config &= ~MAX6650_CFG_V12;
+		reg &= ~MAX6650_CFG_V12;
 		break;
 	case 12:
-		config |= MAX6650_CFG_V12;
+		reg |= MAX6650_CFG_V12;
 		break;
 	default:
 		dev_err(dev, "illegal value for fan_voltage (%d)\n", voltage);
@@ -657,22 +656,22 @@ static int max6650_init_client(struct max6650_data *data,
 	case 0:
 		break;
 	case 1:
-		config &= ~MAX6650_CFG_PRESCALER_MASK;
+		reg &= ~MAX6650_CFG_PRESCALER_MASK;
 		break;
 	case 2:
-		config = (config & ~MAX6650_CFG_PRESCALER_MASK)
+		reg = (reg & ~MAX6650_CFG_PRESCALER_MASK)
 			 | MAX6650_CFG_PRESCALER_2;
 		break;
 	case  4:
-		config = (config & ~MAX6650_CFG_PRESCALER_MASK)
+		reg = (reg & ~MAX6650_CFG_PRESCALER_MASK)
 			 | MAX6650_CFG_PRESCALER_4;
 		break;
 	case  8:
-		config = (config & ~MAX6650_CFG_PRESCALER_MASK)
+		reg = (reg & ~MAX6650_CFG_PRESCALER_MASK)
 			 | MAX6650_CFG_PRESCALER_8;
 		break;
 	case 16:
-		config = (config & ~MAX6650_CFG_PRESCALER_MASK)
+		reg = (reg & ~MAX6650_CFG_PRESCALER_MASK)
 			 | MAX6650_CFG_PRESCALER_16;
 		break;
 	default:
@@ -680,16 +679,22 @@ static int max6650_init_client(struct max6650_data *data,
 	}
 
 	dev_info(dev, "Fan voltage: %dV, prescaler: %d.\n",
-		 (config & MAX6650_CFG_V12) ? 12 : 5,
-		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
+		 (reg & MAX6650_CFG_V12) ? 12 : 5,
+		 1 << (reg & MAX6650_CFG_PRESCALER_MASK));
 
-	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, reg);
+	if (err) {
 		dev_err(dev, "Config write error, aborting.\n");
 		return err;
 	}
 
-	data->config = config;
-	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
+	data->config = reg;
+	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
+	if (reg < 0) {
+		dev_err(dev, "Failed to read count register, aborting.\n");
+		return reg;
+	}
+	data->count = reg;
 
 	if (!of_property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
 				  &target_rpm)) {
-- 
2.7.4

[PATCH 04/11] hwmon: (max6650) Declare valid as boolean

[Guenter Roeck]

Declare valid as boolean to match its use case.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index ac05cc5627d7..8fa888b57efa 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -118,7 +118,7 @@ struct max6650_data {
 	struct thermal_cooling_device *cooling_dev;
 	struct mutex update_lock;
 	int nr_fans;
-	char valid; /* zero until following fields are valid */
+	bool valid; /* false until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
 
 	/* register values */
@@ -197,7 +197,7 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 							MAX6650_REG_ALARM);
 
 		data->last_updated = jiffies;
-		data->valid = 1;
+		data->valid = true;
 	}
 
 	mutex_unlock(&data->update_lock);
-- 
2.7.4

[PATCH 05/11] hwmon: (max6650) Cache alarm_en register

[Guenter Roeck]

The alarm_en register is read each time the is_visible function is called.
Since it is a configuration register, this is completely unnecessary.
Read it once and cache its value.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 8fa888b57efa..cf051f3acf26 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -128,6 +128,7 @@ struct max6650_data {
 	u8 count;
 	u8 dac;
 	u8 alarm;
+	u8 alarm_en;
 	unsigned long cooling_dev_state;
 };
 
@@ -559,8 +560,6 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	u8 alarm_en = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
 	struct device_attribute *devattr;
 
 	/*
@@ -573,7 +572,7 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 	 || devattr == &sensor_dev_attr_fan1_fault.dev_attr
 	 || devattr == &sensor_dev_attr_gpio1_alarm.dev_attr
 	 || devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
-		if (!(alarm_en & to_sensor_dev_attr(devattr)->index))
+		if (!(data->alarm_en & to_sensor_dev_attr(devattr)->index))
 			return 0;
 	}
 
@@ -696,6 +695,13 @@ static int max6650_init_client(struct max6650_data *data,
 	}
 	data->count = reg;
 
+	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN);
+	if (reg < 0) {
+		dev_err(dev, "Failed to read alarm configuration, aborting.\n");
+		return reg;
+	}
+	data->alarm_en = reg;
+
 	if (!of_property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
 				  &target_rpm)) {
 		max6650_set_target(data, target_rpm);
-- 
2.7.4

[PATCH 06/11] hwmon: (max6650) Simplify alarm handling

[Guenter Roeck]

Instead of re-reading the alarm register after reporting an alarm,
mark cached values as invalid. While this results in always reading all
data on subsequent reads, it is quite unlikely that such reads will
actually happen before the cache times out. The upside is avoiding
unnecessary unconditional i2c read operations.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index cf051f3acf26..c02694d70eee 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -526,15 +526,12 @@ static ssize_t alarm_show(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct max6650_data *data = max6650_update_device(dev);
-	struct i2c_client *client = data->client;
-	int alarm = 0;
+	bool alarm = data->alarm & attr->index;
 
-	if (data->alarm & attr->index) {
+	if (alarm) {
 		mutex_lock(&data->update_lock);
-		alarm = 1;
 		data->alarm &= ~attr->index;
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
+		data->valid = false;
 		mutex_unlock(&data->update_lock);
 	}
 
-- 
2.7.4

[PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info

[Guenter Roeck]

Convert driver to use devm_hwmon_device_register_with_info to simplify
the code and to reduce its size.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 505 +++++++++++++++++++++++-------------------------
 1 file changed, 244 insertions(+), 261 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index c02694d70eee..301184172ab1 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -114,7 +114,6 @@ module_param(clock, int, 0444);
 
 struct max6650_data {
 	struct i2c_client *client;
-	const struct attribute_group *groups[3];
 	struct thermal_cooling_device *cooling_dev;
 	struct mutex update_lock;
 	int nr_fans;
@@ -230,26 +229,6 @@ static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
 	return 0;
 }
 
-static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
-			char *buf)
-{
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct max6650_data *data = max6650_update_device(dev);
-	int rpm;
-
-	/*
-	 * Calculation details:
-	 *
-	 * Each tachometer counts over an interval given by the "count"
-	 * register (0.25, 0.5, 1 or 2 seconds). This module assumes
-	 * that the fans produce two pulses per revolution (this seems
-	 * to be the most common).
-	 */
-
-	rpm = ((data->tach[attr->index] * 120) / DIV_FROM_REG(data->count));
-	return sprintf(buf, "%d\n", rpm);
-}
-
 /*
  * Set the fan speed to the specified RPM (or read back the RPM setting).
  * This works in closed loop mode only. Use pwm1 for open loop speed setting.
@@ -291,26 +270,6 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
  * controlled.
  */
 
-static ssize_t fan1_target_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct max6650_data *data = max6650_update_device(dev);
-	int kscale, ktach, rpm;
-
-	/*
-	 * Use the datasheet equation:
-	 *
-	 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
-	 *
-	 * then multiply by 60 to give rpm.
-	 */
-
-	kscale = DIV_FROM_REG(data->config);
-	ktach = data->speed;
-	rpm = 60 * kscale * clock / (256 * (ktach + 1));
-	return sprintf(buf, "%d\n", rpm);
-}
-
 static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
 {
 	int kscale, ktach;
@@ -339,183 +298,8 @@ static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
 					 data->speed);
 }
 
-static ssize_t fan1_target_store(struct device *dev,
-				 struct device_attribute *devattr,
-				 const char *buf, size_t count)
-{
-	struct max6650_data *data = dev_get_drvdata(dev);
-	unsigned long rpm;
-	int err;
-
-	err = kstrtoul(buf, 10, &rpm);
-	if (err)
-		return err;
-
-	mutex_lock(&data->update_lock);
-
-	err = max6650_set_target(data, rpm);
-
-	mutex_unlock(&data->update_lock);
-
-	if (err < 0)
-		return err;
-
-	return count;
-}
-
-/*
- * Get/set the fan speed in open loop mode using pwm1 sysfs file.
- * Speed is given as a relative value from 0 to 255, where 255 is maximum
- * speed. Note that this is done by writing directly to the chip's DAC,
- * it won't change the closed loop speed set by fan1_target.
- * Also note that due to rounding errors it is possible that you don't read
- * back exactly the value you have set.
- */
-
-static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	struct max6650_data *data = max6650_update_device(dev);
-
-	return sprintf(buf, "%d\n", dac_to_pwm(data->dac,
-					       data->config & MAX6650_CFG_V12));
-}
-
-static ssize_t pwm1_store(struct device *dev,
-			  struct device_attribute *devattr, const char *buf,
-			  size_t count)
-{
-	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	unsigned long pwm;
-	int err;
-	u8 dac;
-
-	err = kstrtoul(buf, 10, &pwm);
-	if (err)
-		return err;
-
-	pwm = clamp_val(pwm, 0, 255);
-
-	mutex_lock(&data->update_lock);
-	dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
-	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
-	if (!err)
-		data->dac = dac;
-	mutex_unlock(&data->update_lock);
-
-	return err < 0 ? err : count;
-}
-
-/*
- * Get/Set controller mode:
- * Possible values:
- * 0 = Fan always on
- * 1 = Open loop, Voltage is set according to speed, not regulated.
- * 2 = Closed loop, RPM for all fans regulated by fan1 tachometer
- * 3 = Fan off
- */
-static ssize_t pwm1_enable_show(struct device *dev,
-				struct device_attribute *devattr, char *buf)
-{
-	struct max6650_data *data = max6650_update_device(dev);
-	int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
-	int sysfs_modes[4] = {0, 3, 2, 1};
-
-	return sprintf(buf, "%d\n", sysfs_modes[mode]);
-}
-
-static ssize_t pwm1_enable_store(struct device *dev,
-				 struct device_attribute *devattr,
-				 const char *buf, size_t count)
-{
-	struct max6650_data *data = dev_get_drvdata(dev);
-	unsigned long mode;
-	int err;
-	const u8 max6650_modes[] = {
-		MAX6650_CFG_MODE_ON,
-		MAX6650_CFG_MODE_OPEN_LOOP,
-		MAX6650_CFG_MODE_CLOSED_LOOP,
-		MAX6650_CFG_MODE_OFF,
-		};
-
-	err = kstrtoul(buf, 10, &mode);
-	if (err)
-		return err;
-
-	if (mode >= ARRAY_SIZE(max6650_modes))
-		return -EINVAL;
-
-	mutex_lock(&data->update_lock);
-
-	max6650_set_operating_mode(data, max6650_modes[mode]);
-
-	mutex_unlock(&data->update_lock);
-
-	return count;
-}
-
-/*
- * Read/write functions for fan1_div sysfs file. The MAX6650 has no such
- * divider. We handle this by converting between divider and counttime:
- *
- * (counttime == k) <==> (divider == 2^k), k = 0, 1, 2, or 3
- *
- * Lower values of k allow to connect a faster fan without the risk of
- * counter overflow. The price is lower resolution. You can also set counttime
- * using the module parameter. Note that the module parameter "prescaler" also
- * influences the behaviour. Unfortunately, there's no sysfs attribute
- * defined for that. See the data sheet for details.
- */
-
-static ssize_t fan1_div_show(struct device *dev,
-			     struct device_attribute *devattr, char *buf)
-{
-	struct max6650_data *data = max6650_update_device(dev);
-
-	return sprintf(buf, "%d\n", DIV_FROM_REG(data->count));
-}
-
-static ssize_t fan1_div_store(struct device *dev,
-			      struct device_attribute *devattr,
-			      const char *buf, size_t count)
-{
-	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	unsigned long div;
-	int err;
-
-	err = kstrtoul(buf, 10, &div);
-	if (err)
-		return err;
-
-	mutex_lock(&data->update_lock);
-	switch (div) {
-	case 1:
-		data->count = 0;
-		break;
-	case 2:
-		data->count = 1;
-		break;
-	case 4:
-		data->count = 2;
-		break;
-	case 8:
-		data->count = 3;
-		break;
-	default:
-		mutex_unlock(&data->update_lock);
-		return -EINVAL;
-	}
-
-	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
-	mutex_unlock(&data->update_lock);
-
-	return count;
-}
-
 /*
- * Get alarm stati:
+ * Get gpio alarm status:
  * Possible values:
  * 0 = no alarm
  * 1 = alarm
@@ -538,17 +322,6 @@ static ssize_t alarm_show(struct device *dev,
 	return sprintf(buf, "%d\n", alarm);
 }
 
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_input, fan, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_input, fan, 3);
-static DEVICE_ATTR_RW(fan1_target);
-static DEVICE_ATTR_RW(fan1_div);
-static DEVICE_ATTR_RW(pwm1_enable);
-static DEVICE_ATTR_RW(pwm1);
-static SENSOR_DEVICE_ATTR_RO(fan1_max_alarm, alarm, MAX6650_ALRM_MAX);
-static SENSOR_DEVICE_ATTR_RO(fan1_min_alarm, alarm, MAX6650_ALRM_MIN);
-static SENSOR_DEVICE_ATTR_RO(fan1_fault, alarm, MAX6650_ALRM_TACH);
 static SENSOR_DEVICE_ATTR_RO(gpio1_alarm, alarm, MAX6650_ALRM_GPIO1);
 static SENSOR_DEVICE_ATTR_RO(gpio2_alarm, alarm, MAX6650_ALRM_GPIO2);
 
@@ -564,11 +337,8 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 	 */
 
 	devattr = container_of(a, struct device_attribute, attr);
-	if (devattr == &sensor_dev_attr_fan1_max_alarm.dev_attr
-	 || devattr == &sensor_dev_attr_fan1_min_alarm.dev_attr
-	 || devattr == &sensor_dev_attr_fan1_fault.dev_attr
-	 || devattr == &sensor_dev_attr_gpio1_alarm.dev_attr
-	 || devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
+	if (devattr == &sensor_dev_attr_gpio1_alarm.dev_attr ||
+	    devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
 		if (!(data->alarm_en & to_sensor_dev_attr(devattr)->index))
 			return 0;
 	}
@@ -577,14 +347,6 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
 }
 
 static struct attribute *max6650_attrs[] = {
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	&dev_attr_fan1_target.attr,
-	&dev_attr_fan1_div.attr,
-	&dev_attr_pwm1_enable.attr,
-	&dev_attr_pwm1.attr,
-	&sensor_dev_attr_fan1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan1_min_alarm.dev_attr.attr,
-	&sensor_dev_attr_fan1_fault.dev_attr.attr,
 	&sensor_dev_attr_gpio1_alarm.dev_attr.attr,
 	&sensor_dev_attr_gpio2_alarm.dev_attr.attr,
 	NULL
@@ -595,21 +357,11 @@ static const struct attribute_group max6650_group = {
 	.is_visible = max6650_attrs_visible,
 };
 
-static struct attribute *max6651_attrs[] = {
-	&sensor_dev_attr_fan2_input.dev_attr.attr,
-	&sensor_dev_attr_fan3_input.dev_attr.attr,
-	&sensor_dev_attr_fan4_input.dev_attr.attr,
+static const struct attribute_group *max6650_groups[] = {
+	&max6650_group,
 	NULL
 };
 
-static const struct attribute_group max6651_group = {
-	.attrs = max6651_attrs,
-};
-
-/*
- * Real code
- */
-
 static int max6650_init_client(struct max6650_data *data,
 			       struct i2c_client *client)
 {
@@ -766,6 +518,241 @@ static void max6650_thermal_unregister(void *data)
 }
 #endif
 
+static int max6650_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct max6650_data *data = max6650_update_device(dev);
+	int mode;
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			*val = dac_to_pwm(data->dac,
+					  data->config & MAX6650_CFG_V12);
+			break;
+		case hwmon_pwm_enable:
+			/*
+			 * Possible values:
+			 * 0 = Fan always on
+			 * 1 = Open loop, Voltage is set according to speed,
+			 *     not regulated.
+			 * 2 = Closed loop, RPM for all fans regulated by fan1
+			 *     tachometer
+			 * 3 = Fan off
+			 */
+			mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
+			*val = (4 - mode) & 3; /* {0 1 2 3} -> {0 3 2 1} */
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			/*
+			 * Calculation details:
+			 *
+			 * Each tachometer counts over an interval given by the
+			 * "count" register (0.25, 0.5, 1 or 2 seconds).
+			 * The driver assumes that the fans produce two pulses
+			 * per revolution (this seems to be the most common).
+			 */
+			*val = DIV_ROUND_CLOSEST(data->tach[channel] * 120,
+						 DIV_FROM_REG(data->count));
+			break;
+		case hwmon_fan_div:
+			*val = DIV_FROM_REG(data->count);
+			break;
+		case hwmon_fan_target:
+			/*
+			 * Use the datasheet equation:
+			 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
+			 * then multiply by 60 to give rpm.
+			 */
+			*val = 60 * DIV_FROM_REG(data->config) * clock /
+				(256 * (data->speed + 1));
+			break;
+		case hwmon_fan_min_alarm:
+			*val = !!(data->alarm & MAX6650_ALRM_MIN);
+			data->alarm &= ~MAX6650_ALRM_MIN;
+			data->valid = false;
+			break;
+		case hwmon_fan_max_alarm:
+			*val = !!(data->alarm & MAX6650_ALRM_MAX);
+			data->alarm &= ~MAX6650_ALRM_MAX;
+			data->valid = false;
+			break;
+		case hwmon_fan_fault:
+			*val = !!(data->alarm & MAX6650_ALRM_TACH);
+			data->alarm &= ~MAX6650_ALRM_TACH;
+			data->valid = false;
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+const u8 max6650_pwm_modes[] = {
+	MAX6650_CFG_MODE_ON,
+	MAX6650_CFG_MODE_OPEN_LOOP,
+	MAX6650_CFG_MODE_CLOSED_LOOP,
+	MAX6650_CFG_MODE_OFF,
+};
+
+static int max6650_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	struct max6650_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+	u8 reg;
+
+	mutex_lock(&data->update_lock);
+
+	switch (type) {
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+			reg = pwm_to_dac(clamp_val(val, 0, 255),
+					 data->config & MAX6650_CFG_V12);
+			ret = i2c_smbus_write_byte_data(data->client,
+							MAX6650_REG_DAC, reg);
+			if (ret)
+				break;
+			data->dac = reg;
+			break;
+		case hwmon_pwm_enable:
+			if (val < 0 || val >= ARRAY_SIZE(max6650_pwm_modes))
+				return -EINVAL;
+			ret = max6650_set_operating_mode(data,
+						max6650_pwm_modes[val]);
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+			break;
+		}
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_div:
+			switch (val) {
+			case 1:
+				reg = 0;
+				break;
+			case 2:
+				reg = 1;
+				break;
+			case 4:
+				reg = 2;
+				break;
+			case 8:
+				reg = 3;
+				break;
+			default:
+				ret = -EINVAL;
+				goto error;
+			}
+			ret = i2c_smbus_write_byte_data(data->client,
+							MAX6650_REG_COUNT, reg);
+			if (ret)
+				break;
+			data->count = reg;
+			break;
+		case hwmon_fan_target:
+			if (val < 0) {
+				ret = -EINVAL;
+				break;
+			}
+			ret = max6650_set_target(data, val);
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+			break;
+		}
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+error:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static umode_t max6650_is_visible(const void *_data,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel)
+{
+	const struct max6650_data *data = _data;
+
+	if (channel && (channel >= data->nr_fans || type != hwmon_fan))
+		return 0;
+
+	switch (type) {
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			return 0444;
+		case hwmon_fan_target:
+		case hwmon_fan_div:
+			return 0644;
+		case hwmon_fan_min_alarm:
+			if (data->alarm_en & MAX6650_ALRM_MIN)
+				return 0444;
+			break;
+		case hwmon_fan_max_alarm:
+			if (data->alarm_en & MAX6650_ALRM_MAX)
+				return 0444;
+			break;
+		case hwmon_fan_fault:
+			if (data->alarm_en & MAX6650_ALRM_TACH)
+				return 0444;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_pwm:
+		switch (attr) {
+		case hwmon_pwm_input:
+		case hwmon_pwm_enable:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static const struct hwmon_channel_info *max6650_info[] = {
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_DIV |
+			   HWMON_F_MIN_ALARM | HWMON_F_MAX_ALARM |
+			   HWMON_F_FAULT,
+			   HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+	NULL
+};
+
+static const struct hwmon_ops max6650_hwmon_ops = {
+	.read = max6650_read,
+	.write = max6650_write,
+	.is_visible = max6650_is_visible,
+};
+
+static const struct hwmon_chip_info max6650_chip_info = {
+	.ops = &max6650_hwmon_ops,
+	.info = max6650_info,
+};
+
 static int max6650_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -792,14 +779,10 @@ static int max6650_probe(struct i2c_client *client,
 	if (err)
 		return err;
 
-	data->groups[0] = &max6650_group;
-	/* 3 additional fan inputs for the MAX6651 */
-	if (data->nr_fans == 4)
-		data->groups[1] = &max6651_group;
-
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
-							   client->name, data,
-							   data->groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev,
+							 client->name, data,
+							 &max6650_chip_info,
+							 max6650_groups);
 	err = PTR_ERR_OR_ZERO(hwmon_dev);
 	if (err)
 		return err;
-- 
2.7.4

[PATCH 08/11] hwmon: (max6650) Read non-volatile registers only once

[Guenter Roeck]

Only tachometer and alarm status registers are modified by the chip.
All other registers only need to be read only once, and reading them
repeatedly does not add any value.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 301184172ab1..7c8b1b8ededc 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -176,17 +176,10 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
-		data->speed = i2c_smbus_read_byte_data(client,
-						       MAX6650_REG_SPEED);
-		data->config = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_CONFIG);
 		for (i = 0; i < data->nr_fans; i++) {
 			data->tach[i] = i2c_smbus_read_byte_data(client,
 								 tach_reg[i]);
 		}
-		data->count = i2c_smbus_read_byte_data(client,
-							MAX6650_REG_COUNT);
-		data->dac = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
 
 		/*
 		 * Alarms are cleared on read in case the condition that
@@ -435,8 +428,22 @@ static int max6650_init_client(struct max6650_data *data,
 		dev_err(dev, "Config write error, aborting.\n");
 		return err;
 	}
-
 	data->config = reg;
+
+	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_SPEED);
+	if (reg < 0) {
+		dev_err(dev, "Failed to read speed register, aborting.\n");
+		return reg;
+	}
+	data->speed = reg;
+
+	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_DAC);
+	if (reg < 0) {
+		dev_err(dev, "Failed to read DAC register, aborting.\n");
+		return reg;
+	}
+	data->dac = reg;
+
 	reg = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
 	if (reg < 0) {
 		dev_err(dev, "Failed to read count register, aborting.\n");
-- 
2.7.4

[PATCH 09/11] hwmon: (max6650) Improve error handling in max6650_update_device

[Guenter Roeck]

Pass errors from i2c_smbus_read_byte_data() back to the caller of
max6650_update_device().

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 7c8b1b8ededc..3c79949a741c 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -171,14 +171,19 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
+	int reg, err = 0;
 	int i;
 
 	mutex_lock(&data->update_lock);
 
 	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
 		for (i = 0; i < data->nr_fans; i++) {
-			data->tach[i] = i2c_smbus_read_byte_data(client,
-								 tach_reg[i]);
+			reg = i2c_smbus_read_byte_data(client, tach_reg[i]);
+			if (reg < 0) {
+				err = reg;
+				goto error;
+			}
+			data->tach[i] = reg;
 		}
 
 		/*
@@ -186,15 +191,20 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 		 * caused the alarm is removed. Keep the value latched here
 		 * for providing the register through different alarm files.
 		 */
-		data->alarm |= i2c_smbus_read_byte_data(client,
-							MAX6650_REG_ALARM);
-
+		reg = i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM);
+		if (reg < 0) {
+			err = reg;
+			goto error;
+		}
+		data->alarm |= reg;
 		data->last_updated = jiffies;
 		data->valid = true;
 	}
 
+error:
 	mutex_unlock(&data->update_lock);
-
+	if (err)
+		data = ERR_PTR(err);
 	return data;
 }
 
@@ -303,8 +313,12 @@ static ssize_t alarm_show(struct device *dev,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct max6650_data *data = max6650_update_device(dev);
-	bool alarm = data->alarm & attr->index;
+	bool alarm;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	alarm = data->alarm & attr->index;
 	if (alarm) {
 		mutex_lock(&data->update_lock);
 		data->alarm &= ~attr->index;
@@ -531,6 +545,9 @@ static int max6650_read(struct device *dev, enum hwmon_sensor_types type,
 	struct max6650_data *data = max6650_update_device(dev);
 	int mode;
 
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
 	switch (type) {
 	case hwmon_pwm:
 		switch (attr) {
-- 
2.7.4

[PATCH 10/11] hwmon: (max6650) Use SPDX license identifier

[Guenter Roeck]

Replace boilerplace license text with SPDX license identifier.

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 3c79949a741c..83824e0c9a8e 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * max6650.c - Part of lm_sensors, Linux kernel modules for hardware
  *             monitoring.
@@ -13,22 +14,7 @@
  * chips.
  *
  * The datasheet was last seen at:
- *
  *        http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
 #include <linux/module.h>
-- 
2.7.4

[PATCH 11/11] hwmon: (max6650) Fix minor formatting issues

[Guenter Roeck]

CHECK: struct mutex definition without comment
CHECK: Alignment should match open parenthesis

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/max6650.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 83824e0c9a8e..635ef662f600 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -101,7 +101,7 @@ module_param(clock, int, 0444);
 struct max6650_data {
 	struct i2c_client *client;
 	struct thermal_cooling_device *cooling_dev;
-	struct mutex update_lock;
+	struct mutex update_lock; /* protect alarm register updates */
 	int nr_fans;
 	bool valid; /* false until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
@@ -319,7 +319,7 @@ static SENSOR_DEVICE_ATTR_RO(gpio1_alarm, alarm, MAX6650_ALRM_GPIO1);
 static SENSOR_DEVICE_ATTR_RO(gpio2_alarm, alarm, MAX6650_ALRM_GPIO2);
 
 static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
-				    int n)
+				     int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct max6650_data *data = dev_get_drvdata(dev);
@@ -500,11 +500,10 @@ static int max6650_set_cur_state(struct thermal_cooling_device *cdev,
 
 	data->dac = pwm_to_dac(state, data->config & MAX6650_CFG_V12);
 	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
-
 	if (!err) {
 		max6650_set_operating_mode(data, state ?
-						   MAX6650_CFG_MODE_OPEN_LOOP :
-						   MAX6650_CFG_MODE_OFF);
+					   MAX6650_CFG_MODE_OPEN_LOOP :
+					   MAX6650_CFG_MODE_OFF);
 		data->cooling_dev_state = state;
 	}
 
-- 
2.7.4

Re: [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues

[Jean-Francois Dagenais]

Wow, you sure had fun over the weekend! ;)

max6650 looked like it needed some love, nice work.

I am keen to try it out. We are in the integration stage of our board so it's a
good time to adopt such changes. The value of our testing would be limited
though, since we don't use the tach line at all.

I am not properly setup in my workflow to easily work with patches in emails. I
could manage, but it might be easier for me if you had this work in a branch on
github/gitlab somewhere... Any chance?

(BTW, are you guys fussed over 80 columns emails here?)

Thanks

Re: [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues

[Guenter Roeck]

On Tue, Apr 23, 2019 at 10:38:58AM -0400, Jean-Francois Dagenais wrote:
> Wow, you sure had fun over the weekend! ;)
> 
> max6650 looked like it needed some love, nice work.
> 
> I am keen to try it out. We are in the integration stage of our board so it's a
> good time to adopt such changes. The value of our testing would be limited
> though, since we don't use the tach line at all.
> 
> I am not properly setup in my workflow to easily work with patches in emails. I
> could manage, but it might be easier for me if you had this work in a branch on
> github/gitlab somewhere... Any chance?
> 
I pushed the series to
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git,
branch hwmon-playground. Let me know if it works for you.

Just in case you are interested, I tested the changes with
github.com:groeck/module-tests.git. The test script is in
scripts/max6650.sh.

> (BTW, are you guys fussed over 80 columns emails here?)
> 

Yes ... I usually don't say anything unless it gets too annoying, though.

Guenter

Re: [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm

[Jean-Francois Dagenais]

> On Apr 23, 2019, at 09:33, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> -
> +	dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
> +	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
> +	if (!err)
> +		data->dac = dac;
> 	mutex_unlock(&data->update_lock);
> 
> 	return err < 0 ? err : count;

When I created max6650_set_cur_state, I copied over the pwm1_store code. I ended
up with a "return err < 0 ? ..." which I adjusted. However, as my colleague
pointed out, the set_cur_state return style matches that of
i2c_smbus_write_byte_data so we should simply "return err;" in
max6650_set_cur_state. Since the driver is in great flux state right now, I
cannot make a patch only for that and you should probably just include that in
your series. Or I can submit it later when your series is applied?

Re: [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm

[Guenter Roeck]

On Tue, Apr 23, 2019 at 11:23:00AM -0400, Jean-Francois Dagenais wrote:
> 
> 
> > On Apr 23, 2019, at 09:33, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > -
> > +	dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
> > +	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
> > +	if (!err)
> > +		data->dac = dac;
> > 	mutex_unlock(&data->update_lock);
> > 
> > 	return err < 0 ? err : count;
> 
> When I created max6650_set_cur_state, I copied over the pwm1_store code. I ended
> up with a "return err < 0 ? ..." which I adjusted. However, as my colleague
> pointed out, the set_cur_state return style matches that of
> i2c_smbus_write_byte_data so we should simply "return err;" in
> max6650_set_cur_state. Since the driver is in great flux state right now, I
> cannot make a patch only for that and you should probably just include that in
> your series. Or I can submit it later when your series is applied?
> 
Just a patch for that should work; I don't think I changed the code
around it too much. Can you give it a try ?

Guenter

Re: [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info

[Guenter Roeck]

On Tue, Apr 23, 2019 at 06:33:07AM -0700, Guenter Roeck wrote:
> Convert driver to use devm_hwmon_device_register_with_info to simplify
> the code and to reduce its size.
> 

To keep everyone up to date, I have made the changes indicated below
to the patch. Thanks to 0day and Julia Lawall for reporting the problems.

Guenter

> Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/max6650.c | 505 +++++++++++++++++++++++-------------------------
>  1 file changed, 244 insertions(+), 261 deletions(-)
> 
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index c02694d70eee..301184172ab1 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -114,7 +114,6 @@ module_param(clock, int, 0444);
>  
>  struct max6650_data {
>  	struct i2c_client *client;
> -	const struct attribute_group *groups[3];
>  	struct thermal_cooling_device *cooling_dev;
>  	struct mutex update_lock;
>  	int nr_fans;
> @@ -230,26 +229,6 @@ static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
>  	return 0;
>  }
>  
> -static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
> -			char *buf)
> -{
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct max6650_data *data = max6650_update_device(dev);
> -	int rpm;
> -
> -	/*
> -	 * Calculation details:
> -	 *
> -	 * Each tachometer counts over an interval given by the "count"
> -	 * register (0.25, 0.5, 1 or 2 seconds). This module assumes
> -	 * that the fans produce two pulses per revolution (this seems
> -	 * to be the most common).
> -	 */
> -
> -	rpm = ((data->tach[attr->index] * 120) / DIV_FROM_REG(data->count));
> -	return sprintf(buf, "%d\n", rpm);
> -}
> -
>  /*
>   * Set the fan speed to the specified RPM (or read back the RPM setting).
>   * This works in closed loop mode only. Use pwm1 for open loop speed setting.
> @@ -291,26 +270,6 @@ static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
>   * controlled.
>   */
>  
> -static ssize_t fan1_target_show(struct device *dev,
> -				struct device_attribute *devattr, char *buf)
> -{
> -	struct max6650_data *data = max6650_update_device(dev);
> -	int kscale, ktach, rpm;
> -
> -	/*
> -	 * Use the datasheet equation:
> -	 *
> -	 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> -	 *
> -	 * then multiply by 60 to give rpm.
> -	 */
> -
> -	kscale = DIV_FROM_REG(data->config);
> -	ktach = data->speed;
> -	rpm = 60 * kscale * clock / (256 * (ktach + 1));
> -	return sprintf(buf, "%d\n", rpm);
> -}
> -
>  static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
>  {
>  	int kscale, ktach;
> @@ -339,183 +298,8 @@ static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
>  					 data->speed);
>  }
>  
> -static ssize_t fan1_target_store(struct device *dev,
> -				 struct device_attribute *devattr,
> -				 const char *buf, size_t count)
> -{
> -	struct max6650_data *data = dev_get_drvdata(dev);
> -	unsigned long rpm;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &rpm);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	err = max6650_set_target(data, rpm);
> -
> -	mutex_unlock(&data->update_lock);
> -
> -	if (err < 0)
> -		return err;
> -
> -	return count;
> -}
> -
> -/*
> - * Get/set the fan speed in open loop mode using pwm1 sysfs file.
> - * Speed is given as a relative value from 0 to 255, where 255 is maximum
> - * speed. Note that this is done by writing directly to the chip's DAC,
> - * it won't change the closed loop speed set by fan1_target.
> - * Also note that due to rounding errors it is possible that you don't read
> - * back exactly the value you have set.
> - */
> -
> -static ssize_t pwm1_show(struct device *dev, struct device_attribute *devattr,
> -			 char *buf)
> -{
> -	struct max6650_data *data = max6650_update_device(dev);
> -
> -	return sprintf(buf, "%d\n", dac_to_pwm(data->dac,
> -					       data->config & MAX6650_CFG_V12));
> -}
> -
> -static ssize_t pwm1_store(struct device *dev,
> -			  struct device_attribute *devattr, const char *buf,
> -			  size_t count)
> -{
> -	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	unsigned long pwm;
> -	int err;
> -	u8 dac;
> -
> -	err = kstrtoul(buf, 10, &pwm);
> -	if (err)
> -		return err;
> -
> -	pwm = clamp_val(pwm, 0, 255);
> -
> -	mutex_lock(&data->update_lock);
> -	dac = pwm_to_dac(pwm, data->config & MAX6650_CFG_V12);
> -	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, dac);
> -	if (!err)
> -		data->dac = dac;
> -	mutex_unlock(&data->update_lock);
> -
> -	return err < 0 ? err : count;
> -}
> -
> -/*
> - * Get/Set controller mode:
> - * Possible values:
> - * 0 = Fan always on
> - * 1 = Open loop, Voltage is set according to speed, not regulated.
> - * 2 = Closed loop, RPM for all fans regulated by fan1 tachometer
> - * 3 = Fan off
> - */
> -static ssize_t pwm1_enable_show(struct device *dev,
> -				struct device_attribute *devattr, char *buf)
> -{
> -	struct max6650_data *data = max6650_update_device(dev);
> -	int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
> -	int sysfs_modes[4] = {0, 3, 2, 1};
> -
> -	return sprintf(buf, "%d\n", sysfs_modes[mode]);
> -}
> -
> -static ssize_t pwm1_enable_store(struct device *dev,
> -				 struct device_attribute *devattr,
> -				 const char *buf, size_t count)
> -{
> -	struct max6650_data *data = dev_get_drvdata(dev);
> -	unsigned long mode;
> -	int err;
> -	const u8 max6650_modes[] = {
> -		MAX6650_CFG_MODE_ON,
> -		MAX6650_CFG_MODE_OPEN_LOOP,
> -		MAX6650_CFG_MODE_CLOSED_LOOP,
> -		MAX6650_CFG_MODE_OFF,
> -		};
> -
> -	err = kstrtoul(buf, 10, &mode);
> -	if (err)
> -		return err;
> -
> -	if (mode >= ARRAY_SIZE(max6650_modes))
> -		return -EINVAL;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	max6650_set_operating_mode(data, max6650_modes[mode]);
> -
> -	mutex_unlock(&data->update_lock);
> -
> -	return count;
> -}
> -
> -/*
> - * Read/write functions for fan1_div sysfs file. The MAX6650 has no such
> - * divider. We handle this by converting between divider and counttime:
> - *
> - * (counttime == k) <==> (divider == 2^k), k = 0, 1, 2, or 3
> - *
> - * Lower values of k allow to connect a faster fan without the risk of
> - * counter overflow. The price is lower resolution. You can also set counttime
> - * using the module parameter. Note that the module parameter "prescaler" also
> - * influences the behaviour. Unfortunately, there's no sysfs attribute
> - * defined for that. See the data sheet for details.
> - */
> -
> -static ssize_t fan1_div_show(struct device *dev,
> -			     struct device_attribute *devattr, char *buf)
> -{
> -	struct max6650_data *data = max6650_update_device(dev);
> -
> -	return sprintf(buf, "%d\n", DIV_FROM_REG(data->count));
> -}
> -
> -static ssize_t fan1_div_store(struct device *dev,
> -			      struct device_attribute *devattr,
> -			      const char *buf, size_t count)
> -{
> -	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	unsigned long div;
> -	int err;
> -
> -	err = kstrtoul(buf, 10, &div);
> -	if (err)
> -		return err;
> -
> -	mutex_lock(&data->update_lock);
> -	switch (div) {
> -	case 1:
> -		data->count = 0;
> -		break;
> -	case 2:
> -		data->count = 1;
> -		break;
> -	case 4:
> -		data->count = 2;
> -		break;
> -	case 8:
> -		data->count = 3;
> -		break;
> -	default:
> -		mutex_unlock(&data->update_lock);
> -		return -EINVAL;
> -	}
> -
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
> -	mutex_unlock(&data->update_lock);
> -
> -	return count;
> -}
> -
>  /*
> - * Get alarm stati:
> + * Get gpio alarm status:
>   * Possible values:
>   * 0 = no alarm
>   * 1 = alarm
> @@ -538,17 +322,6 @@ static ssize_t alarm_show(struct device *dev,
>  	return sprintf(buf, "%d\n", alarm);
>  }
>  
> -static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
> -static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
> -static SENSOR_DEVICE_ATTR_RO(fan3_input, fan, 2);
> -static SENSOR_DEVICE_ATTR_RO(fan4_input, fan, 3);
> -static DEVICE_ATTR_RW(fan1_target);
> -static DEVICE_ATTR_RW(fan1_div);
> -static DEVICE_ATTR_RW(pwm1_enable);
> -static DEVICE_ATTR_RW(pwm1);
> -static SENSOR_DEVICE_ATTR_RO(fan1_max_alarm, alarm, MAX6650_ALRM_MAX);
> -static SENSOR_DEVICE_ATTR_RO(fan1_min_alarm, alarm, MAX6650_ALRM_MIN);
> -static SENSOR_DEVICE_ATTR_RO(fan1_fault, alarm, MAX6650_ALRM_TACH);
>  static SENSOR_DEVICE_ATTR_RO(gpio1_alarm, alarm, MAX6650_ALRM_GPIO1);
>  static SENSOR_DEVICE_ATTR_RO(gpio2_alarm, alarm, MAX6650_ALRM_GPIO2);
>  
> @@ -564,11 +337,8 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>  	 */
>  
>  	devattr = container_of(a, struct device_attribute, attr);
> -	if (devattr == &sensor_dev_attr_fan1_max_alarm.dev_attr
> -	 || devattr == &sensor_dev_attr_fan1_min_alarm.dev_attr
> -	 || devattr == &sensor_dev_attr_fan1_fault.dev_attr
> -	 || devattr == &sensor_dev_attr_gpio1_alarm.dev_attr
> -	 || devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
> +	if (devattr == &sensor_dev_attr_gpio1_alarm.dev_attr ||
> +	    devattr == &sensor_dev_attr_gpio2_alarm.dev_attr) {
>  		if (!(data->alarm_en & to_sensor_dev_attr(devattr)->index))
>  			return 0;
>  	}
> @@ -577,14 +347,6 @@ static umode_t max6650_attrs_visible(struct kobject *kobj, struct attribute *a,
>  }
>  
>  static struct attribute *max6650_attrs[] = {
> -	&sensor_dev_attr_fan1_input.dev_attr.attr,
> -	&dev_attr_fan1_target.attr,
> -	&dev_attr_fan1_div.attr,
> -	&dev_attr_pwm1_enable.attr,
> -	&dev_attr_pwm1.attr,
> -	&sensor_dev_attr_fan1_max_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan1_min_alarm.dev_attr.attr,
> -	&sensor_dev_attr_fan1_fault.dev_attr.attr,
>  	&sensor_dev_attr_gpio1_alarm.dev_attr.attr,
>  	&sensor_dev_attr_gpio2_alarm.dev_attr.attr,
>  	NULL
> @@ -595,21 +357,11 @@ static const struct attribute_group max6650_group = {
>  	.is_visible = max6650_attrs_visible,
>  };
>  
> -static struct attribute *max6651_attrs[] = {
> -	&sensor_dev_attr_fan2_input.dev_attr.attr,
> -	&sensor_dev_attr_fan3_input.dev_attr.attr,
> -	&sensor_dev_attr_fan4_input.dev_attr.attr,
> +static const struct attribute_group *max6650_groups[] = {
> +	&max6650_group,
>  	NULL
>  };
>  
> -static const struct attribute_group max6651_group = {
> -	.attrs = max6651_attrs,
> -};
> -
> -/*
> - * Real code
> - */
> -
>  static int max6650_init_client(struct max6650_data *data,
>  			       struct i2c_client *client)
>  {
> @@ -766,6 +518,241 @@ static void max6650_thermal_unregister(void *data)
>  }
>  #endif
>  
> +static int max6650_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +	int mode;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			*val = dac_to_pwm(data->dac,
> +					  data->config & MAX6650_CFG_V12);
> +			break;
> +		case hwmon_pwm_enable:
> +			/*
> +			 * Possible values:
> +			 * 0 = Fan always on
> +			 * 1 = Open loop, Voltage is set according to speed,
> +			 *     not regulated.
> +			 * 2 = Closed loop, RPM for all fans regulated by fan1
> +			 *     tachometer
> +			 * 3 = Fan off
> +			 */
> +			mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
> +			*val = (4 - mode) & 3; /* {0 1 2 3} -> {0 3 2 1} */
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			/*
> +			 * Calculation details:
> +			 *
> +			 * Each tachometer counts over an interval given by the
> +			 * "count" register (0.25, 0.5, 1 or 2 seconds).
> +			 * The driver assumes that the fans produce two pulses
> +			 * per revolution (this seems to be the most common).
> +			 */
> +			*val = DIV_ROUND_CLOSEST(data->tach[channel] * 120,
> +						 DIV_FROM_REG(data->count));
> +			break;
> +		case hwmon_fan_div:
> +			*val = DIV_FROM_REG(data->count);
> +			break;
> +		case hwmon_fan_target:
> +			/*
> +			 * Use the datasheet equation:
> +			 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> +			 * then multiply by 60 to give rpm.
> +			 */
> +			*val = 60 * DIV_FROM_REG(data->config) * clock /
> +				(256 * (data->speed + 1));
> +			break;
> +		case hwmon_fan_min_alarm:
> +			*val = !!(data->alarm & MAX6650_ALRM_MIN);
> +			data->alarm &= ~MAX6650_ALRM_MIN;
> +			data->valid = false;
> +			break;
> +		case hwmon_fan_max_alarm:
> +			*val = !!(data->alarm & MAX6650_ALRM_MAX);
> +			data->alarm &= ~MAX6650_ALRM_MAX;
> +			data->valid = false;
> +			break;
> +		case hwmon_fan_fault:
> +			*val = !!(data->alarm & MAX6650_ALRM_TACH);
> +			data->alarm &= ~MAX6650_ALRM_TACH;
> +			data->valid = false;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +const u8 max6650_pwm_modes[] = {

static const max6650_pwm_modes

> +	MAX6650_CFG_MODE_ON,
> +	MAX6650_CFG_MODE_OPEN_LOOP,
> +	MAX6650_CFG_MODE_CLOSED_LOOP,
> +	MAX6650_CFG_MODE_OFF,
> +};
> +
> +static int max6650_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	struct max6650_data *data = dev_get_drvdata(dev);
> +	int ret = 0;
> +	u8 reg;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			reg = pwm_to_dac(clamp_val(val, 0, 255),
> +					 data->config & MAX6650_CFG_V12);
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							MAX6650_REG_DAC, reg);
> +			if (ret)
> +				break;
> +			data->dac = reg;
> +			break;
> +		case hwmon_pwm_enable:
> +			if (val < 0 || val >= ARRAY_SIZE(max6650_pwm_modes))
> +				return -EINVAL;
						{
				ret = -EINVAL;
				break;
			}

> +			ret = max6650_set_operating_mode(data,
> +						max6650_pwm_modes[val]);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_div:
> +			switch (val) {
> +			case 1:
> +				reg = 0;
> +				break;
> +			case 2:
> +				reg = 1;
> +				break;
> +			case 4:
> +				reg = 2;
> +				break;
> +			case 8:
> +				reg = 3;
> +				break;
> +			default:
> +				ret = -EINVAL;
> +				goto error;
> +			}
> +			ret = i2c_smbus_write_byte_data(data->client,
> +							MAX6650_REG_COUNT, reg);
> +			if (ret)
> +				break;
> +			data->count = reg;
> +			break;
> +		case hwmon_fan_target:
> +			if (val < 0) {
> +				ret = -EINVAL;
> +				break;
> +			}
> +			ret = max6650_set_target(data, val);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +error:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static umode_t max6650_is_visible(const void *_data,
> +				  enum hwmon_sensor_types type, u32 attr,
> +				  int channel)
> +{
> +	const struct max6650_data *data = _data;
> +
> +	if (channel && (channel >= data->nr_fans || type != hwmon_fan))
> +		return 0;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			return 0444;
> +		case hwmon_fan_target:
> +		case hwmon_fan_div:
> +			return 0644;
> +		case hwmon_fan_min_alarm:
> +			if (data->alarm_en & MAX6650_ALRM_MIN)
> +				return 0444;
> +			break;
> +		case hwmon_fan_max_alarm:
> +			if (data->alarm_en & MAX6650_ALRM_MAX)
> +				return 0444;
> +			break;
> +		case hwmon_fan_fault:
> +			if (data->alarm_en & MAX6650_ALRM_TACH)
> +				return 0444;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +		case hwmon_pwm_enable:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info *max6650_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_DIV |
> +			   HWMON_F_MIN_ALARM | HWMON_F_MAX_ALARM |
> +			   HWMON_F_FAULT,
> +			   HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +	NULL
> +};
> +
> +static const struct hwmon_ops max6650_hwmon_ops = {
> +	.read = max6650_read,
> +	.write = max6650_write,
> +	.is_visible = max6650_is_visible,
> +};
> +
> +static const struct hwmon_chip_info max6650_chip_info = {
> +	.ops = &max6650_hwmon_ops,
> +	.info = max6650_info,
> +};
> +
>  static int max6650_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -792,14 +779,10 @@ static int max6650_probe(struct i2c_client *client,
>  	if (err)
>  		return err;
>  
> -	data->groups[0] = &max6650_group;
> -	/* 3 additional fan inputs for the MAX6651 */
> -	if (data->nr_fans == 4)
> -		data->groups[1] = &max6651_group;
> -
> -	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> -							   client->name, data,
> -							   data->groups);
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev,
> +							 client->name, data,
> +							 &max6650_chip_info,
> +							 max6650_groups);
>  	err = PTR_ERR_OR_ZERO(hwmon_dev);
>  	if (err)
>  		return err;