Linux-Hwmon Archive on lore.kernel.org
 help / Atom feed
* [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device
@ 2019-04-23 13:33 Guenter Roeck
  2019-04-23 13:33 ` [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm Guenter Roeck
                   ` (9 more replies)
  0 siblings, 10 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 15:23   ` Jean-Francois Dagenais
  2019-04-23 13:33 ` [PATCH 03/11] hwmon: (max6650) Improve error handling in max6650_init_client Guenter Roeck
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 03/11] hwmon: (max6650) Improve error handling in max6650_init_client
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
  2019-04-23 13:33 ` [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 04/11] hwmon: (max6650) Declare valid as boolean Guenter Roeck
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 04/11] hwmon: (max6650) Declare valid as boolean
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
  2019-04-23 13:33 ` [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm Guenter Roeck
  2019-04-23 13:33 ` [PATCH 03/11] hwmon: (max6650) Improve error handling in max6650_init_client Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 05/11] hwmon: (max6650) Cache alarm_en register Guenter Roeck
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 05/11] hwmon: (max6650) Cache alarm_en register
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (2 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 04/11] hwmon: (max6650) Declare valid as boolean Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 06/11] hwmon: (max6650) Simplify alarm handling Guenter Roeck
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 06/11] hwmon: (max6650) Simplify alarm handling
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (3 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 05/11] hwmon: (max6650) Cache alarm_en register Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info Guenter Roeck
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (4 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 06/11] hwmon: (max6650) Simplify alarm handling Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-24 13:42   ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 08/11] hwmon: (max6650) Read non-volatile registers only once Guenter Roeck
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 08/11] hwmon: (max6650) Read non-volatile registers only once
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (5 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 09/11] hwmon: (max6650) Improve error handling in max6650_update_device Guenter Roeck
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 09/11] hwmon: (max6650) Improve error handling in max6650_update_device
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (6 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 08/11] hwmon: (max6650) Read non-volatile registers only once Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 10/11] hwmon: (max6650) Use SPDX license identifier Guenter Roeck
  2019-04-23 13:33 ` [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues Guenter Roeck
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 10/11] hwmon: (max6650) Use SPDX license identifier
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (7 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 09/11] hwmon: (max6650) Improve error handling in max6650_update_device Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 13:33 ` [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues Guenter Roeck
  9 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues
  2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
                   ` (8 preceding siblings ...)
  2019-04-23 13:33 ` [PATCH 10/11] hwmon: (max6650) Use SPDX license identifier Guenter Roeck
@ 2019-04-23 13:33 ` Guenter Roeck
  2019-04-23 14:38   ` Jean-Francois Dagenais
  9 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 13:33 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Guenter Roeck, Jean-Francois Dagenais

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


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

* Re: [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues
  2019-04-23 13:33 ` [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues Guenter Roeck
@ 2019-04-23 14:38   ` Jean-Francois Dagenais
  2019-04-23 15:18     ` Guenter Roeck
  0 siblings, 1 reply; 16+ messages in thread
From: Jean-Francois Dagenais @ 2019-04-23 14:38 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Hardware Monitoring, Jean Delvare

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

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

* Re: [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues
  2019-04-23 14:38   ` Jean-Francois Dagenais
@ 2019-04-23 15:18     ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 15:18 UTC (permalink / raw)
  To: Jean-Francois Dagenais; +Cc: Hardware Monitoring, Jean Delvare

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

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

* Re: [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm
  2019-04-23 13:33 ` [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm Guenter Roeck
@ 2019-04-23 15:23   ` Jean-Francois Dagenais
  2019-04-23 17:02     ` Guenter Roeck
  0 siblings, 1 reply; 16+ messages in thread
From: Jean-Francois Dagenais @ 2019-04-23 15:23 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Hardware Monitoring, Jean Delvare



> 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?



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

* Re: [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm
  2019-04-23 15:23   ` Jean-Francois Dagenais
@ 2019-04-23 17:02     ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-23 17:02 UTC (permalink / raw)
  To: Jean-Francois Dagenais; +Cc: Hardware Monitoring, Jean Delvare

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

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

* Re: [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info
  2019-04-23 13:33 ` [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info Guenter Roeck
@ 2019-04-24 13:42   ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2019-04-24 13:42 UTC (permalink / raw)
  To: Hardware Monitoring; +Cc: Jean Delvare, Jean-Francois Dagenais

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;

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

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 13:33 [PATCH 01/11] hwmon: (max6650) Use devm_add_action to unregister thermal device Guenter Roeck
2019-04-23 13:33 ` [PATCH 02/11] hwmon: (max6650) Introduce pwm_to_dac and dac_to_pwm Guenter Roeck
2019-04-23 15:23   ` Jean-Francois Dagenais
2019-04-23 17:02     ` Guenter Roeck
2019-04-23 13:33 ` [PATCH 03/11] hwmon: (max6650) Improve error handling in max6650_init_client Guenter Roeck
2019-04-23 13:33 ` [PATCH 04/11] hwmon: (max6650) Declare valid as boolean Guenter Roeck
2019-04-23 13:33 ` [PATCH 05/11] hwmon: (max6650) Cache alarm_en register Guenter Roeck
2019-04-23 13:33 ` [PATCH 06/11] hwmon: (max6650) Simplify alarm handling Guenter Roeck
2019-04-23 13:33 ` [PATCH 07/11] hwmon: (max6650) Convert to use devm_hwmon_device_register_with_info Guenter Roeck
2019-04-24 13:42   ` Guenter Roeck
2019-04-23 13:33 ` [PATCH 08/11] hwmon: (max6650) Read non-volatile registers only once Guenter Roeck
2019-04-23 13:33 ` [PATCH 09/11] hwmon: (max6650) Improve error handling in max6650_update_device Guenter Roeck
2019-04-23 13:33 ` [PATCH 10/11] hwmon: (max6650) Use SPDX license identifier Guenter Roeck
2019-04-23 13:33 ` [PATCH 11/11] hwmon: (max6650) Fix minor formatting issues Guenter Roeck
2019-04-23 14:38   ` Jean-Francois Dagenais
2019-04-23 15:18     ` Guenter Roeck

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org linux-hwmon@archiver.kernel.org
	public-inbox-index linux-hwmon


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/ public-inbox