[PATCH v2 01/10] hwmon: (max6650) Use devm function to register thermal device

[PATCH v2 01/10] hwmon: (max6650) Use devm function to register thermal device

[Guenter Roeck]

Use devm_thermal_of_cooling_device_register to register 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>
---
v2: Use devm_thermal_of_cooling_device_register() instead of devm_add_action()

 drivers/hwmon/max6650.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 6b9056f9483f..e540d0b0145e 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -101,7 +101,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;
 	char valid; /* zero until following fields are valid */
@@ -744,6 +743,7 @@ static const struct thermal_cooling_device_ops max6650_cooling_ops = {
 static int max6650_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	struct thermal_cooling_device *cooling_dev;
 	struct device *dev = &client->dev;
 	const struct of_device_id *of_id =
 		of_match_device(of_match_ptr(max6650_dt_match), dev);
@@ -780,28 +780,16 @@ static int max6650_probe(struct i2c_client *client,
 		return err;
 
 #if IS_ENABLED(CONFIG_THERMAL)
-	data->cooling_dev =
-		thermal_of_cooling_device_register(client->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",
-			 PTR_ERR(data->cooling_dev));
+	cooling_dev = devm_thermal_of_cooling_device_register(dev, dev->of_node,
+				client->name, data, &max6650_cooling_ops);
+	if (IS_ERR(cooling_dev)) {
+		dev_warn(dev, "thermal cooling device register failed: %ld\n",
+			 PTR_ERR(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 },
@@ -815,7 +803,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 v2 02/10] 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 (), and simplify return statement of
max6650_set_cur_state().

Cc: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: simplify return statement of max6650_set_cur_state()

 drivers/hwmon/max6650.c | 55 ++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index e540d0b0145e..461484e7828a 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -92,7 +92,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)
@@ -136,6 +137,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);
@@ -343,22 +360,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,
@@ -369,6 +374,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)
@@ -377,13 +383,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;
@@ -714,11 +717,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) {
@@ -730,7 +729,7 @@ static int max6650_set_cur_state(struct thermal_cooling_device *cdev,
 
 	mutex_unlock(&data->update_lock);
 
-	return err < 0 ? err : 0;
+	return err;
 }
 
 static const struct thermal_cooling_device_ops max6650_cooling_ops = {
-- 
2.7.4

[PATCH v2 03/10] 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>
---
v2: No change

 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 461484e7828a..caede4d3e21a 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -604,8 +604,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;
@@ -619,21 +619,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);
@@ -643,22 +642,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:
@@ -666,16 +665,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 v2 04/10] 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>
---
v2: No change

 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 caede4d3e21a..90565318aafb 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -104,7 +104,7 @@ struct max6650_data {
 	const struct attribute_group *groups[3];
 	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 */
@@ -183,7 +183,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

Re: [PATCH v2 04/10] hwmon: (max6650) Declare valid as boolean

[Jean-Francois Dagenais]

Hi Guenter,

> On Jun 7, 2019, at 13:23, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> 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>
> ---
> v2: No change
> 
> 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 caede4d3e21a..90565318aafb 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -104,7 +104,7 @@ struct max6650_data {
> 	const struct attribute_group *groups[3];
> 	struct mutex update_lock;
> 	int nr_fans;
> -	char valid; /* zero until following fields are valid */
> +	bool valid; /* false until following fields are valid */

Is there some compiler configuration that ensures this non-explicitely initialized function variable will be zero'ed?

> 	unsigned long last_updated; /* in jiffies */
> 
> 	/* register values */
> @@ -183,7 +183,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
> 

Re: [PATCH v2 04/10] hwmon: (max6650) Declare valid as boolean

[Guenter Roeck]

On 6/10/19 4:33 AM, Jean-Francois Dagenais wrote:
> Hi Guenter,
> 
>> On Jun 7, 2019, at 13:23, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> 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>
>> ---
>> v2: No change
>>
>> 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 caede4d3e21a..90565318aafb 100644
>> --- a/drivers/hwmon/max6650.c
>> +++ b/drivers/hwmon/max6650.c
>> @@ -104,7 +104,7 @@ struct max6650_data {
>> 	const struct attribute_group *groups[3];
>> 	struct mutex update_lock;
>> 	int nr_fans;
>> -	char valid; /* zero until following fields are valid */
>> +	bool valid; /* false until following fields are valid */
> 
> Is there some compiler configuration that ensures this non-explicitely initialized function variable will be zero'ed?
> 

struct max6650_data is allocated with devm_kzalloc(), which returns zero-cleared data.

Guenter

>> 	unsigned long last_updated; /* in jiffies */
>>
>> 	/* register values */
>> @@ -183,7 +183,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 v2 05/10] 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>
---
v2: No change

 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 90565318aafb..2edee4ca5cae 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -114,6 +114,7 @@ struct max6650_data {
 	u8 count;
 	u8 dac;
 	u8 alarm;
+	u8 alarm_en;
 	unsigned long cooling_dev_state;
 };
 
@@ -545,8 +546,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;
 
 	/*
@@ -559,7 +558,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;
 	}
 
@@ -682,6 +681,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 v2 06/10] 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>
---
v2: No change

 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 2edee4ca5cae..045e67f73846 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -512,15 +512,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 v2 07/10] 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>
---
v2: Add missing break; statement in max6650_write()
    Declare max6650_pwm_modes static
    Fix error return from max6650_write which sometimes
    did not release the update_lock mutex

 drivers/hwmon/max6650.c | 508 +++++++++++++++++++++++-------------------------
 1 file changed, 247 insertions(+), 261 deletions(-)

diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 045e67f73846..bcd50307d963 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -101,7 +101,6 @@ module_param(clock, int, 0444);
 
 struct max6650_data {
 	struct i2c_client *client;
-	const struct attribute_group *groups[3];
 	struct mutex update_lock;
 	int nr_fans;
 	bool valid; /* false until following fields are valid */
@@ -216,26 +215,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.
@@ -277,26 +256,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;
@@ -325,183 +284,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
@@ -524,17 +308,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);
 
@@ -550,11 +323,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;
 	}
@@ -563,14 +333,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
@@ -581,21 +343,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)
 {
@@ -747,6 +499,244 @@ static const struct thermal_cooling_device_ops max6650_cooling_ops = {
 };
 #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;
+}
+
+static 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)) {
+				ret = -EINVAL;
+				break;
+			}
+			ret = max6650_set_operating_mode(data,
+						max6650_pwm_modes[val]);
+			break;
+		default:
+			ret = -EOPNOTSUPP;
+			break;
+		}
+		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)
 {
@@ -774,14 +764,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 v2 08/10] 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>
---
v2: No change

 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 bcd50307d963..6f1a1a6eae46 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -162,17 +162,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
@@ -421,8 +414,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 v2 09/10] 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>
---
v2: No change

 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 6f1a1a6eae46..e65792020ca1 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -157,14 +157,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;
 		}
 
 		/*
@@ -172,15 +177,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;
 }
 
@@ -289,8 +299,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;
@@ -512,6 +526,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 v2 10/10] 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>
---
v2: No change
    Note: This is now patch 10/10 instead of 11/11.
    Patch 10/11 in the initial series was addressed with upstream commit
    74ba9207e1ad ("treewide: Replace GPLv2 boilerplate/reference with
    SPDX - rule 61").

 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 e65792020ca1..5fdad4645cca 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 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