[PATCH v2 0/6] hwmon: (max6697) Cleanup, use regmap and with_info API

[PATCH v2 0/6] hwmon: (max6697) Cleanup, use regmap and with_info API

[Guenter Roeck]

Clean up driver, use regmap, convert to with_info hwmon API,
and add support for additional standard attributes.

The patch series reduces object file size by more than 30%.

v2:
- Fix include file order to be really alphabetical
- Fix devicetree configuration
- Add Reviewed-by: tags

----------------------------------------------------------------
Guenter Roeck (6):
      hwmon: (max6697) Reorder include files
      hwmon: (max6697) Drop platform data support
      hwmon: (max6697) Use bit operations where possible
      hwmon: (max6697) Convert to use regmap
      hwmon: (max6697) Convert to with_info hwmon API
      hwmon: (max6697) Add support for tempX_min and tempX_min_alarm

 drivers/hwmon/max6697.c               | 896 ++++++++++++++--------------------
 include/linux/platform_data/max6697.h |  33 --
 2 files changed, 370 insertions(+), 559 deletions(-)
 delete mode 100644 include/linux/platform_data/max6697.h

[PATCH v2 1/6] hwmon: (max6697) Reorder include files

[Guenter Roeck]

Reorder include files to alphabetic order to improve maintainability.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Really alphabetical order

 drivers/hwmon/max6697.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index 20981f9443dd..9a2a21230c7d 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -6,16 +6,16 @@
  * Copyright (c) 2011 David George <david.george@ska.ac.za>
  */
 
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/jiffies.h>
-#include <linux/i2c.h>
+#include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 
 #include <linux/platform_data/max6697.h>
 
-- 
2.39.2

Re: [PATCH v2 1/6] hwmon: (max6697) Reorder include files

[Tzung-Bi Shih]

On Tue, Jul 23, 2024 at 08:44:42AM -0700, Guenter Roeck wrote:
> Reorder include files to alphabetic order to improve maintainability.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Guenter Roeck]

Platform data is not used anywhere in the upstram kernel. Drop support
for it to simplify code maintenance.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Drop unnecessary data->chip dereference
    Fix resistance-cancellation devicetree configuration

 drivers/hwmon/max6697.c               | 173 ++++++++++++--------------
 include/linux/platform_data/max6697.h |  33 -----
 2 files changed, 80 insertions(+), 126 deletions(-)
 delete mode 100644 include/linux/platform_data/max6697.h

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index 9a2a21230c7d..62b03c79c039 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -17,8 +17,6 @@
 #include <linux/of.h>
 #include <linux/slab.h>
 
-#include <linux/platform_data/max6697.h>
-
 enum chips { max6581, max6602, max6622, max6636, max6689, max6693, max6694,
 	     max6697, max6698, max6699 };
 
@@ -558,54 +556,96 @@ static const struct attribute_group max6697_group = {
 };
 __ATTRIBUTE_GROUPS(max6697);
 
-static void max6697_get_config_of(struct device_node *node,
-				  struct max6697_platform_data *pdata)
+static int max6697_config_of(struct max6697_data *data, struct i2c_client *client)
 {
-	int len;
-	const __be32 *prop;
+	const struct max6697_chip_data *chip = data->chip;
+	struct device_node *node = client->dev.of_node;
+	int ret, confreg;
+	int factor = 0;
+	u32 vals[2];
 
-	pdata->smbus_timeout_disable =
-		of_property_read_bool(node, "smbus-timeout-disable");
-	pdata->extended_range_enable =
-		of_property_read_bool(node, "extended-range-enable");
-	pdata->beta_compensation =
-		of_property_read_bool(node, "beta-compensation-enable");
+	confreg = 0;
+	if (of_property_read_bool(node, "smbus-timeout-disable") &&
+	    (chip->valid_conf & MAX6697_CONF_TIMEOUT)) {
+		confreg |= MAX6697_CONF_TIMEOUT;
+	}
+	if (of_property_read_bool(node, "extended-range-enable") &&
+	    (chip->valid_conf & MAX6581_CONF_EXTENDED)) {
+		confreg |= MAX6581_CONF_EXTENDED;
+		data->temp_offset = 64;
+	}
+	if (of_property_read_bool(node, "beta-compensation-enable") &&
+	    (chip->valid_conf & MAX6693_CONF_BETA)) {
+		confreg |= MAX6693_CONF_BETA;
+	}
 
-	prop = of_get_property(node, "alert-mask", &len);
-	if (prop && len == sizeof(u32))
-		pdata->alert_mask = be32_to_cpu(prop[0]);
-	prop = of_get_property(node, "over-temperature-mask", &len);
-	if (prop && len == sizeof(u32))
-		pdata->over_temperature_mask = be32_to_cpu(prop[0]);
-	prop = of_get_property(node, "resistance-cancellation", &len);
-	if (prop) {
-		if (len == sizeof(u32))
-			pdata->resistance_cancellation = be32_to_cpu(prop[0]);
-		else
-			pdata->resistance_cancellation = 0xfe;
-	}
-	prop = of_get_property(node, "transistor-ideality", &len);
-	if (prop && len == 2 * sizeof(u32)) {
-			pdata->ideality_mask = be32_to_cpu(prop[0]);
-			pdata->ideality_value = be32_to_cpu(prop[1]);
+	if (of_property_read_u32(node, "alert-mask", vals))
+		vals[0] = 0;
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_ALERT_MASK,
+					MAX6697_ALERT_MAP_BITS(vals[0]));
+	if (ret)
+		return ret;
+
+	if (of_property_read_u32(node, "over-temperature-mask", vals))
+		vals[0] = 0;
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_OVERT_MASK,
+					MAX6697_OVERT_MAP_BITS(vals[0]));
+	if (ret)
+		return ret;
+
+	if (data->type != max6581) {
+		if (of_property_read_bool(node, "resistance-cancellation") &&
+		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
+			confreg |= MAX6697_CONF_RESISTANCE;
+			factor = 1;
+		}
+	} else {
+		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
+			if (of_property_read_bool(node, "resistance-cancellation"))
+				vals[0] = 0xfe;
+			else
+				vals[0] = 0;
+		}
+
+		factor = hweight8(vals[0] & 0xfe);
+		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
+						vals[0] >> 1);
+		if (ret < 0)
+			return ret;
+
+		if (of_property_read_u32_array(node, "transistor-ideality", vals, 2)) {
+			vals[0] = 0;
+			vals[1] = 0;
+		}
+
+		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_IDEALITY,
+						vals[1]);
+		if (ret < 0)
+			return ret;
+		ret = i2c_smbus_write_byte_data(client,
+						MAX6581_REG_IDEALITY_SELECT,
+						vals[0] >> 1);
+		if (ret < 0)
+			return ret;
 	}
+	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_CONFIG, confreg);
+	if (ret < 0)
+		return ret;
+	return factor;
 }
 
 static int max6697_init_chip(struct max6697_data *data,
 			     struct i2c_client *client)
 {
-	struct max6697_platform_data *pdata = dev_get_platdata(&client->dev);
-	struct max6697_platform_data p;
 	const struct max6697_chip_data *chip = data->chip;
 	int factor = chip->channels;
 	int ret, reg;
 
 	/*
-	 * Don't touch configuration if neither platform data nor OF
-	 * configuration was specified. If that is the case, use the
-	 * current chip configuration.
+	 * Don't touch configuration if there is no devicetree configuration.
+	 * If that is the case, use the current chip configuration.
 	 */
-	if (!pdata && !client->dev.of_node) {
+	if (!client->dev.of_node) {
 		reg = i2c_smbus_read_byte_data(client, MAX6697_REG_CONFIG);
 		if (reg < 0)
 			return reg;
@@ -621,67 +661,14 @@ static int max6697_init_chip(struct max6697_data *data,
 			if (reg & MAX6697_CONF_RESISTANCE)
 				factor++;
 		}
-		goto done;
-	}
-
-	if (client->dev.of_node) {
-		memset(&p, 0, sizeof(p));
-		max6697_get_config_of(client->dev.of_node, &p);
-		pdata = &p;
-	}
-
-	reg = 0;
-	if (pdata->smbus_timeout_disable &&
-	    (chip->valid_conf & MAX6697_CONF_TIMEOUT)) {
-		reg |= MAX6697_CONF_TIMEOUT;
-	}
-	if (pdata->extended_range_enable &&
-	    (chip->valid_conf & MAX6581_CONF_EXTENDED)) {
-		reg |= MAX6581_CONF_EXTENDED;
-		data->temp_offset = 64;
-	}
-	if (pdata->resistance_cancellation &&
-	    (chip->valid_conf & MAX6697_CONF_RESISTANCE)) {
-		reg |= MAX6697_CONF_RESISTANCE;
-		factor++;
-	}
-	if (pdata->beta_compensation &&
-	    (chip->valid_conf & MAX6693_CONF_BETA)) {
-		reg |= MAX6693_CONF_BETA;
-	}
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_CONFIG, reg);
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_ALERT_MASK,
-				MAX6697_ALERT_MAP_BITS(pdata->alert_mask));
-	if (ret < 0)
-		return ret;
-
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_OVERT_MASK,
-			MAX6697_OVERT_MAP_BITS(pdata->over_temperature_mask));
-	if (ret < 0)
-		return ret;
-
-	if (data->type == max6581) {
-		factor += hweight8(pdata->resistance_cancellation >> 1);
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
-					pdata->resistance_cancellation >> 1);
-		if (ret < 0)
-			return ret;
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_IDEALITY,
-						pdata->ideality_value);
-		if (ret < 0)
-			return ret;
-		ret = i2c_smbus_write_byte_data(client,
-						MAX6581_REG_IDEALITY_SELECT,
-						pdata->ideality_mask >> 1);
+		data->update_interval = factor * MAX6697_CONV_TIME;
+	} else {
+		ret = max6697_config_of(data, client);
 		if (ret < 0)
 			return ret;
+		data->update_interval = (factor + ret) * MAX6697_CONV_TIME;
 	}
-done:
-	data->update_interval = factor * MAX6697_CONV_TIME;
+
 	return 0;
 }
 
diff --git a/include/linux/platform_data/max6697.h b/include/linux/platform_data/max6697.h
deleted file mode 100644
index 6fbb70005541..000000000000
--- a/include/linux/platform_data/max6697.h
+++ /dev/null
@@ -1,33 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * max6697.h
- *     Copyright (c) 2012 Guenter Roeck <linux@roeck-us.net>
- */
-
-#ifndef MAX6697_H
-#define MAX6697_H
-
-#include <linux/types.h>
-
-/*
- * For all bit masks:
- * bit 0:    local temperature
- * bit 1..7: remote temperatures
- */
-struct max6697_platform_data {
-	bool smbus_timeout_disable;	/* set to disable SMBus timeouts */
-	bool extended_range_enable;	/* set to enable extended temp range */
-	bool beta_compensation;		/* set to enable beta compensation */
-	u8 alert_mask;			/* set bit to 1 to disable alert */
-	u8 over_temperature_mask;	/* set bit to 1 to disable */
-	u8 resistance_cancellation;	/* set bit to 0 to disable
-					 * bit mask for MAX6581,
-					 * boolean for other chips
-					 */
-	u8 ideality_mask;		/* set bit to 0 to disable */
-	u8 ideality_value;		/* transistor ideality as per
-					 * MAX6581 datasheet
-					 */
-};
-
-#endif /* MAX6697_H */
-- 
2.39.2

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Tzung-Bi Shih]

On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> +	if (data->type != max6581) {
> +		if (of_property_read_bool(node, "resistance-cancellation") &&
> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> +			confreg |= MAX6697_CONF_RESISTANCE;
> +			factor = 1;
> +		}
> +	} else {
> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> +			if (of_property_read_bool(node, "resistance-cancellation"))
> +				vals[0] = 0xfe;
> +			else
> +				vals[0] = 0;
> +		}
> +
> +		factor = hweight8(vals[0] & 0xfe);

It doesn't AND with 0xfe originally.

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Guenter Roeck]

On 7/24/24 06:03, Tzung-Bi Shih wrote:
> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>> +	if (data->type != max6581) {
>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>> +			confreg |= MAX6697_CONF_RESISTANCE;
>> +			factor = 1;
>> +		}
>> +	} else {
>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>> +				vals[0] = 0xfe;
>> +			else
>> +				vals[0] = 0;
>> +		}
>> +
>> +		factor = hweight8(vals[0] & 0xfe);
> 
> It doesn't AND with 0xfe originally.
> 

Yes, but the original code uses the value in
	factor += hweight8(pdata->resistance_cancellation >> 1);
	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
                                         pdata->resistance_cancellation >> 1);
which is effectively the same. I can't just use
	factor = hweight8(vals[0] >> 1);
because, unlike resistance_cancellation, val[] is an u32 array which would
not auto-mask the upper bits.

Thanks,
Guenter

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Tzung-Bi Shih]

On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
> On 7/24/24 06:03, Tzung-Bi Shih wrote:
> > On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> > > +	if (data->type != max6581) {
> > > +		if (of_property_read_bool(node, "resistance-cancellation") &&
> > > +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> > > +			confreg |= MAX6697_CONF_RESISTANCE;
> > > +			factor = 1;
> > > +		}
> > > +	} else {
> > > +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> > > +			if (of_property_read_bool(node, "resistance-cancellation"))
> > > +				vals[0] = 0xfe;
> > > +			else
> > > +				vals[0] = 0;
> > > +		}
> > > +
> > > +		factor = hweight8(vals[0] & 0xfe);
> > 
> > It doesn't AND with 0xfe originally.
> > 
> 
> Yes, but the original code uses the value in
> 	factor += hweight8(pdata->resistance_cancellation >> 1);
> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>                                         pdata->resistance_cancellation >> 1);
> which is effectively the same. I can't just use
> 	factor = hweight8(vals[0] >> 1);
> because, unlike resistance_cancellation, val[] is an u32 array which would
> not auto-mask the upper bits.

If you are worrying about:
* MSB, it should be fine as it should only prepend 0s for right shift on
  unsigned.
* BIT(8), other `val[0] >> 1` should also share the same concern.

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Guenter Roeck]

On 7/24/24 08:41, Tzung-Bi Shih wrote:
> On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
>> On 7/24/24 06:03, Tzung-Bi Shih wrote:
>>> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>>>> +	if (data->type != max6581) {
>>>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>>>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>>>> +			confreg |= MAX6697_CONF_RESISTANCE;
>>>> +			factor = 1;
>>>> +		}
>>>> +	} else {
>>>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>>>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>>>> +				vals[0] = 0xfe;
>>>> +			else
>>>> +				vals[0] = 0;
>>>> +		}
>>>> +
>>>> +		factor = hweight8(vals[0] & 0xfe);
>>>
>>> It doesn't AND with 0xfe originally.
>>>
>>
>> Yes, but the original code uses the value in
>> 	factor += hweight8(pdata->resistance_cancellation >> 1);
>> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>>                                          pdata->resistance_cancellation >> 1);
>> which is effectively the same. I can't just use
>> 	factor = hweight8(vals[0] >> 1);
>> because, unlike resistance_cancellation, val[] is an u32 array which would
>> not auto-mask the upper bits.
> 
> If you are worrying about:
> * MSB, it should be fine as it should only prepend 0s for right shift on
>    unsigned.
> * BIT(8), other `val[0] >> 1` should also share the same concern.

BIT(8) is the concern. Yes, you are correct, I'll change the code to

	val[0] &= 0xfe;
	factor = hweight8(vals[0]);
	...

In practice it doesn't matter since bit 7 isn't used by the chip,
but that isn't a reason for the bad code.

Thanks a lot for noticing!

Guenter

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Tzung-Bi Shih]

On Wed, Jul 24, 2024 at 10:40:20AM -0700, Guenter Roeck wrote:
> On 7/24/24 08:41, Tzung-Bi Shih wrote:
> > On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
> > > On 7/24/24 06:03, Tzung-Bi Shih wrote:
> > > > On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
> > > > > +	if (data->type != max6581) {
> > > > > +		if (of_property_read_bool(node, "resistance-cancellation") &&
> > > > > +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
> > > > > +			confreg |= MAX6697_CONF_RESISTANCE;
> > > > > +			factor = 1;
> > > > > +		}
> > > > > +	} else {
> > > > > +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
> > > > > +			if (of_property_read_bool(node, "resistance-cancellation"))
> > > > > +				vals[0] = 0xfe;
> > > > > +			else
> > > > > +				vals[0] = 0;
> > > > > +		}
> > > > > +
> > > > > +		factor = hweight8(vals[0] & 0xfe);
> > > > 
> > > > It doesn't AND with 0xfe originally.
> > > > 
> > > 
> > > Yes, but the original code uses the value in
> > > 	factor += hweight8(pdata->resistance_cancellation >> 1);
> > > 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
> > >                                          pdata->resistance_cancellation >> 1);
> > > which is effectively the same. I can't just use
> > > 	factor = hweight8(vals[0] >> 1);
> > > because, unlike resistance_cancellation, val[] is an u32 array which would
> > > not auto-mask the upper bits.
> > 
> > If you are worrying about:
> > * MSB, it should be fine as it should only prepend 0s for right shift on
> >    unsigned.
> > * BIT(8), other `val[0] >> 1` should also share the same concern.
> 
> BIT(8) is the concern. Yes, you are correct, I'll change the code to
> 
> 	val[0] &= 0xfe;
> 	factor = hweight8(vals[0]);
> 	...

With that,
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

Note: there is another `vals[0] >> 1` for MAX6581_REG_IDEALITY_SELECT.

Re: [PATCH v2 2/6] hwmon: (max6697) Drop platform data support

[Guenter Roeck]

On 7/24/24 17:54, Tzung-Bi Shih wrote:
> On Wed, Jul 24, 2024 at 10:40:20AM -0700, Guenter Roeck wrote:
>> On 7/24/24 08:41, Tzung-Bi Shih wrote:
>>> On Wed, Jul 24, 2024 at 07:25:31AM -0700, Guenter Roeck wrote:
>>>> On 7/24/24 06:03, Tzung-Bi Shih wrote:
>>>>> On Tue, Jul 23, 2024 at 08:44:43AM -0700, Guenter Roeck wrote:
>>>>>> +	if (data->type != max6581) {
>>>>>> +		if (of_property_read_bool(node, "resistance-cancellation") &&
>>>>>> +		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
>>>>>> +			confreg |= MAX6697_CONF_RESISTANCE;
>>>>>> +			factor = 1;
>>>>>> +		}
>>>>>> +	} else {
>>>>>> +		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
>>>>>> +			if (of_property_read_bool(node, "resistance-cancellation"))
>>>>>> +				vals[0] = 0xfe;
>>>>>> +			else
>>>>>> +				vals[0] = 0;
>>>>>> +		}
>>>>>> +
>>>>>> +		factor = hweight8(vals[0] & 0xfe);
>>>>>
>>>>> It doesn't AND with 0xfe originally.
>>>>>
>>>>
>>>> Yes, but the original code uses the value in
>>>> 	factor += hweight8(pdata->resistance_cancellation >> 1);
>>>> 	ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
>>>>                                           pdata->resistance_cancellation >> 1);
>>>> which is effectively the same. I can't just use
>>>> 	factor = hweight8(vals[0] >> 1);
>>>> because, unlike resistance_cancellation, val[] is an u32 array which would
>>>> not auto-mask the upper bits.
>>>
>>> If you are worrying about:
>>> * MSB, it should be fine as it should only prepend 0s for right shift on
>>>     unsigned.
>>> * BIT(8), other `val[0] >> 1` should also share the same concern.
>>
>> BIT(8) is the concern. Yes, you are correct, I'll change the code to
>>
>> 	val[0] &= 0xfe;
>> 	factor = hweight8(vals[0]);
>> 	...
> 
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
> Note: there is another `vals[0] >> 1` for MAX6581_REG_IDEALITY_SELECT.

Good point. I masked that as well.

Thanks a lot for the reviews,
Guenter

[PATCH v2 3/6] hwmon: (max6697) Use bit operations where possible

[Guenter Roeck]

Use bit operations to improve code maintainability.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change

 drivers/hwmon/max6697.c | 43 +++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index 62b03c79c039..1e7c549ef090 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -6,6 +6,8 @@
  * Copyright (c) 2011 David George <david.george@ska.ac.za>
  */
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -32,20 +34,31 @@ static const u8 MAX6697_REG_CRIT[] = {
 			0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27 };
 
 /*
- * Map device tree / platform data register bit map to chip bit map.
+ * Map device tree / internal register bit map to chip bit map.
  * Applies to alert register and over-temperature register.
  */
+
+#define MAX6697_EXTERNAL_MASK_DT	GENMASK(7, 1)
+#define MAX6697_LOCAL_MASK_DT		BIT(0)
+#define MAX6697_EXTERNAL_MASK_CHIP	GENMASK(6, 0)
+#define MAX6697_LOCAL_MASK_CHIP		BIT(7)
+
+/* alert - local channel is in bit 6 */
 #define MAX6697_ALERT_MAP_BITS(reg)	((((reg) & 0x7e) >> 1) | \
 				 (((reg) & 0x01) << 6) | ((reg) & 0x80))
-#define MAX6697_OVERT_MAP_BITS(reg) (((reg) >> 1) | (((reg) & 0x01) << 7))
+
+/* over-temperature - local channel is in bit 7 */
+#define MAX6697_OVERT_MAP_BITS(reg)	\
+	(FIELD_PREP(MAX6697_EXTERNAL_MASK_CHIP, FIELD_GET(MAX6697_EXTERNAL_MASK_DT, reg)) | \
+	 FIELD_PREP(MAX6697_LOCAL_MASK_CHIP, FIELD_GET(MAX6697_LOCAL_MASK_DT, reg)))
 
 #define MAX6697_REG_STAT(n)		(0x44 + (n))
 
 #define MAX6697_REG_CONFIG		0x41
-#define MAX6581_CONF_EXTENDED		(1 << 1)
-#define MAX6693_CONF_BETA		(1 << 2)
-#define MAX6697_CONF_RESISTANCE		(1 << 3)
-#define MAX6697_CONF_TIMEOUT		(1 << 5)
+#define MAX6581_CONF_EXTENDED		BIT(1)
+#define MAX6693_CONF_BETA		BIT(2)
+#define MAX6697_CONF_RESISTANCE		BIT(3)
+#define MAX6697_CONF_TIMEOUT		BIT(5)
 #define MAX6697_REG_ALERT_MASK		0x42
 #define MAX6697_REG_OVERT_MASK		0x43
 
@@ -193,7 +206,7 @@ static struct max6697_data *max6697_update_device(struct device *dev)
 		goto abort;
 
 	for (i = 0; i < data->chip->channels; i++) {
-		if (data->chip->have_ext & (1 << i)) {
+		if (data->chip->have_ext & BIT(i)) {
 			val = i2c_smbus_read_byte_data(client,
 						       MAX6697_REG_TEMP_EXT[i]);
 			if (unlikely(val < 0)) {
@@ -217,7 +230,7 @@ static struct max6697_data *max6697_update_device(struct device *dev)
 		}
 		data->temp[i][MAX6697_TEMP_MAX] = val;
 
-		if (data->chip->have_crit & (1 << i)) {
+		if (data->chip->have_crit & BIT(i)) {
 			val = i2c_smbus_read_byte_data(client,
 						       MAX6697_REG_CRIT[i]);
 			if (unlikely(val < 0)) {
@@ -291,7 +304,7 @@ static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
 	if (data->chip->alarm_map)
 		index = data->chip->alarm_map[index];
 
-	return sprintf(buf, "%u\n", (data->alarms >> index) & 0x1);
+	return sprintf(buf, "%u\n", !!(data->alarms & BIT(index)));
 }
 
 static ssize_t temp_store(struct device *dev,
@@ -342,20 +355,20 @@ static ssize_t offset_store(struct device *dev, struct device_attribute *devattr
 		ret = select;
 		goto abort;
 	}
-	channel_enabled = (select & (1 << (index - 1)));
+	channel_enabled = (select & BIT(index - 1));
 	temp = clamp_val(temp, MAX6581_OFFSET_MIN, MAX6581_OFFSET_MAX);
 	val = DIV_ROUND_CLOSEST(temp, 250);
 	/* disable the offset for channel if the new offset is 0 */
 	if (val == 0) {
 		if (channel_enabled)
 			ret = i2c_smbus_write_byte_data(data->client, MAX6581_REG_OFFSET_SELECT,
-							select & ~(1 << (index - 1)));
+							select & ~BIT(index - 1));
 		ret = ret < 0 ? ret : count;
 		goto abort;
 	}
 	if (!channel_enabled) {
 		ret = i2c_smbus_write_byte_data(data->client, MAX6581_REG_OFFSET_SELECT,
-						select | (1 << (index - 1)));
+						select | BIT(index - 1));
 		if (ret < 0)
 			goto abort;
 	}
@@ -378,7 +391,7 @@ static ssize_t offset_show(struct device *dev, struct device_attribute *devattr,
 	select = i2c_smbus_read_byte_data(data->client, MAX6581_REG_OFFSET_SELECT);
 	if (select < 0)
 		ret = select;
-	else if (select & (1 << (index - 1)))
+	else if (select & BIT(index - 1))
 		ret = i2c_smbus_read_byte_data(data->client, MAX6581_REG_OFFSET);
 	else
 		ret = 0;
@@ -467,9 +480,9 @@ static umode_t max6697_is_visible(struct kobject *kobj, struct attribute *attr,
 	if (channel >= chip->channels)
 		return 0;
 
-	if ((nr == 3 || nr == 4) && !(chip->have_crit & (1 << channel)))
+	if ((nr == 3 || nr == 4) && !(chip->have_crit & BIT(channel)))
 		return 0;
-	if (nr == 5 && !(chip->have_fault & (1 << channel)))
+	if (nr == 5 && !(chip->have_fault & BIT(channel)))
 		return 0;
 	/* offset reg is only supported on max6581 remote channels */
 	if (nr == 6)
-- 
2.39.2

Re: [PATCH v2 3/6] hwmon: (max6697) Use bit operations where possible

[Tzung-Bi Shih]

On Tue, Jul 23, 2024 at 08:44:44AM -0700, Guenter Roeck wrote:
> Use bit operations to improve code maintainability.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

[PATCH v2 4/6] hwmon: (max6697) Convert to use regmap

[Guenter Roeck]

Use regmap for register caching, and use regmap API for bit operations
to simplify the code.

This patch reduces object file size by approximately 10%.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max6697.c | 317 +++++++++++++++-------------------------
 1 file changed, 121 insertions(+), 196 deletions(-)

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index 1e7c549ef090..e4bec33d1d44 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -13,10 +13,10 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/jiffies.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 enum chips { max6581, max6602, max6622, max6636, max6689, max6693, max6694,
@@ -82,20 +82,15 @@ struct max6697_chip_data {
 };
 
 struct max6697_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 
 	enum chips type;
 	const struct max6697_chip_data *chip;
 
-	int update_interval;	/* in milli-seconds */
 	int temp_offset;	/* in degrees C */
 
 	struct mutex update_lock;
-	unsigned long last_updated;	/* In jiffies */
-	bool valid;		/* true if following fields are valid */
 
-	/* 1x local and up to 7x remote */
-	u8 temp[8][4];		/* [nr][0]=temp [1]=ext [2]=max [3]=crit */
 #define MAX6697_TEMP_INPUT	0
 #define MAX6697_TEMP_EXT	1
 #define MAX6697_TEMP_MAX	2
@@ -189,88 +184,22 @@ static inline int max6581_offset_to_millic(int val)
 	return sign_extend32(val, 7) * 250;
 }
 
-static struct max6697_data *max6697_update_device(struct device *dev)
-{
-	struct max6697_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	struct max6697_data *ret = data;
-	int val;
-	int i;
-	u32 alarms;
-
-	mutex_lock(&data->update_lock);
-
-	if (data->valid &&
-	    !time_after(jiffies, data->last_updated
-			+ msecs_to_jiffies(data->update_interval)))
-		goto abort;
-
-	for (i = 0; i < data->chip->channels; i++) {
-		if (data->chip->have_ext & BIT(i)) {
-			val = i2c_smbus_read_byte_data(client,
-						       MAX6697_REG_TEMP_EXT[i]);
-			if (unlikely(val < 0)) {
-				ret = ERR_PTR(val);
-				goto abort;
-			}
-			data->temp[i][MAX6697_TEMP_EXT] = val;
-		}
-
-		val = i2c_smbus_read_byte_data(client, MAX6697_REG_TEMP[i]);
-		if (unlikely(val < 0)) {
-			ret = ERR_PTR(val);
-			goto abort;
-		}
-		data->temp[i][MAX6697_TEMP_INPUT] = val;
-
-		val = i2c_smbus_read_byte_data(client, MAX6697_REG_MAX[i]);
-		if (unlikely(val < 0)) {
-			ret = ERR_PTR(val);
-			goto abort;
-		}
-		data->temp[i][MAX6697_TEMP_MAX] = val;
-
-		if (data->chip->have_crit & BIT(i)) {
-			val = i2c_smbus_read_byte_data(client,
-						       MAX6697_REG_CRIT[i]);
-			if (unlikely(val < 0)) {
-				ret = ERR_PTR(val);
-				goto abort;
-			}
-			data->temp[i][MAX6697_TEMP_CRIT] = val;
-		}
-	}
-
-	alarms = 0;
-	for (i = 0; i < 3; i++) {
-		val = i2c_smbus_read_byte_data(client, MAX6697_REG_STAT(i));
-		if (unlikely(val < 0)) {
-			ret = ERR_PTR(val);
-			goto abort;
-		}
-		alarms = (alarms << 8) | val;
-	}
-	data->alarms = alarms;
-	data->last_updated = jiffies;
-	data->valid = true;
-abort:
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
 static ssize_t temp_input_show(struct device *dev,
 			       struct device_attribute *devattr, char *buf)
 {
+	struct max6697_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(devattr)->index;
-	struct max6697_data *data = max6697_update_device(dev);
-	int temp;
+	unsigned int regs[2] = { MAX6697_REG_TEMP[index],
+				 MAX6697_REG_TEMP_EXT[index] };
+	u8 regdata[2] = { };
+	int temp, ret;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	ret = regmap_multi_reg_read(data->regmap, regs, regdata,
+				    data->chip->have_ext & BIT(index) ? 2 : 1);
+	if (ret)
+		return ret;
 
-	temp = (data->temp[index][MAX6697_TEMP_INPUT] - data->temp_offset) << 3;
-	temp |= data->temp[index][MAX6697_TEMP_EXT] >> 5;
+	temp = ((regdata[0] - data->temp_offset) << 3) | (regdata[1] >> 5);
 
 	return sprintf(buf, "%d\n", temp * 125);
 }
@@ -278,33 +207,41 @@ static ssize_t temp_input_show(struct device *dev,
 static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
 			 char *buf)
 {
-	int nr = to_sensor_dev_attr_2(devattr)->nr;
+	struct max6697_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr_2(devattr)->index;
-	struct max6697_data *data = max6697_update_device(dev);
-	int temp;
+	int nr = to_sensor_dev_attr_2(devattr)->nr;
+	unsigned int temp;
+	int reg, ret;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	if (index == MAX6697_TEMP_MAX)
+		reg = MAX6697_REG_MAX[nr];
+	else
+		reg = MAX6697_REG_CRIT[nr];
 
-	temp = data->temp[nr][index];
-	temp -= data->temp_offset;
+	ret = regmap_read(data->regmap, reg, &temp);
+	if (ret)
+		return ret;
 
-	return sprintf(buf, "%d\n", temp * 1000);
+	return sprintf(buf, "%d\n", ((int)temp - data->temp_offset) * 1000);
 }
 
 static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
+	struct max6697_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	struct max6697_data *data = max6697_update_device(dev);
-
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	unsigned int alarms;
+	int reg, ret;
 
 	if (data->chip->alarm_map)
 		index = data->chip->alarm_map[index];
 
-	return sprintf(buf, "%u\n", !!(data->alarms & BIT(index)));
+	reg = MAX6697_REG_STAT(2 - (index / 8));
+	ret = regmap_read(data->regmap, reg, &alarms);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%u\n", !!(alarms & BIT(index & 7)));
 }
 
 static ssize_t temp_store(struct device *dev,
@@ -321,82 +258,63 @@ static ssize_t temp_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&data->update_lock);
 	temp = clamp_val(temp, -1000000, 1000000);	/* prevent underflow */
 	temp = DIV_ROUND_CLOSEST(temp, 1000) + data->temp_offset;
 	temp = clamp_val(temp, 0, data->type == max6581 ? 255 : 127);
-	data->temp[nr][index] = temp;
-	ret = i2c_smbus_write_byte_data(data->client,
-					index == 2 ? MAX6697_REG_MAX[nr]
-						   : MAX6697_REG_CRIT[nr],
-					temp);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_write(data->regmap,
+			   index == 2 ? MAX6697_REG_MAX[nr]
+				      : MAX6697_REG_CRIT[nr],
+			   temp);
 
-	return ret < 0 ? ret : count;
+	return ret ? : count;
 }
 
 static ssize_t offset_store(struct device *dev, struct device_attribute *devattr, const char *buf,
 			    size_t count)
 {
-	int val, ret, index, select;
-	struct max6697_data *data;
-	bool channel_enabled;
+	struct max6697_data *data = dev_get_drvdata(dev);
+	int index = to_sensor_dev_attr(devattr)->index;
+	struct regmap *regmap = data->regmap;
 	long temp;
+	int ret;
 
-	index = to_sensor_dev_attr(devattr)->index;
-	data = dev_get_drvdata(dev);
 	ret = kstrtol(buf, 10, &temp);
 	if (ret < 0)
 		return ret;
 
 	mutex_lock(&data->update_lock);
-	select = i2c_smbus_read_byte_data(data->client, MAX6581_REG_OFFSET_SELECT);
-	if (select < 0) {
-		ret = select;
-		goto abort;
-	}
-	channel_enabled = (select & BIT(index - 1));
 	temp = clamp_val(temp, MAX6581_OFFSET_MIN, MAX6581_OFFSET_MAX);
-	val = DIV_ROUND_CLOSEST(temp, 250);
-	/* disable the offset for channel if the new offset is 0 */
-	if (val == 0) {
-		if (channel_enabled)
-			ret = i2c_smbus_write_byte_data(data->client, MAX6581_REG_OFFSET_SELECT,
-							select & ~BIT(index - 1));
-		ret = ret < 0 ? ret : count;
-		goto abort;
+	temp = DIV_ROUND_CLOSEST(temp, 250);
+	if (!temp) {	/* disable this (and only this) channel */
+		ret = regmap_clear_bits(regmap, MAX6581_REG_OFFSET_SELECT, BIT(index - 1));
+		goto unlock;
 	}
-	if (!channel_enabled) {
-		ret = i2c_smbus_write_byte_data(data->client, MAX6581_REG_OFFSET_SELECT,
-						select | BIT(index - 1));
-		if (ret < 0)
-			goto abort;
-	}
-	ret = i2c_smbus_write_byte_data(data->client, MAX6581_REG_OFFSET, val);
-	ret = ret < 0 ? ret : count;
-
-abort:
+	/* enable channel, and update offset */
+	ret = regmap_set_bits(regmap, MAX6581_REG_OFFSET_SELECT, BIT(index - 1));
+	if (ret)
+		goto unlock;
+	ret = regmap_write(regmap, MAX6581_REG_OFFSET, temp);
+unlock:
 	mutex_unlock(&data->update_lock);
-	return ret;
+	return ret ? : count;
 }
 
 static ssize_t offset_show(struct device *dev, struct device_attribute *devattr, char *buf)
 {
-	struct max6697_data *data;
-	int select, ret, index;
+	unsigned int regs[2] = { MAX6581_REG_OFFSET_SELECT, MAX6581_REG_OFFSET };
+	struct max6697_data *data = dev_get_drvdata(dev);
+	int index = to_sensor_dev_attr(devattr)->index;
+	u8 regdata[2];
+	int ret;
 
-	index = to_sensor_dev_attr(devattr)->index;
-	data = dev_get_drvdata(dev);
-	mutex_lock(&data->update_lock);
-	select = i2c_smbus_read_byte_data(data->client, MAX6581_REG_OFFSET_SELECT);
-	if (select < 0)
-		ret = select;
-	else if (select & BIT(index - 1))
-		ret = i2c_smbus_read_byte_data(data->client, MAX6581_REG_OFFSET);
-	else
-		ret = 0;
-	mutex_unlock(&data->update_lock);
-	return ret < 0 ? ret : sprintf(buf, "%d\n", max6581_offset_to_millic(ret));
+	ret = regmap_multi_reg_read(data->regmap, regs, regdata, 2);
+	if (ret)
+		return ret;
+
+	if (!(regdata[0] & BIT(index - 1)))
+		regdata[1] = 0;
+
+	return sprintf(buf, "%d\n", max6581_offset_to_millic(regdata[1]));
 }
 
 static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_input, 0);
@@ -569,12 +487,11 @@ static const struct attribute_group max6697_group = {
 };
 __ATTRIBUTE_GROUPS(max6697);
 
-static int max6697_config_of(struct max6697_data *data, struct i2c_client *client)
+static int max6697_config_of(struct device_node *node, struct max6697_data *data)
 {
 	const struct max6697_chip_data *chip = data->chip;
-	struct device_node *node = client->dev.of_node;
+	struct regmap *regmap = data->regmap;
 	int ret, confreg;
-	int factor = 0;
 	u32 vals[2];
 
 	confreg = 0;
@@ -594,15 +511,15 @@ static int max6697_config_of(struct max6697_data *data, struct i2c_client *clien
 
 	if (of_property_read_u32(node, "alert-mask", vals))
 		vals[0] = 0;
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_ALERT_MASK,
-					MAX6697_ALERT_MAP_BITS(vals[0]));
+	ret = regmap_write(regmap, MAX6697_REG_ALERT_MASK,
+			   MAX6697_ALERT_MAP_BITS(vals[0]));
 	if (ret)
 		return ret;
 
 	if (of_property_read_u32(node, "over-temperature-mask", vals))
 		vals[0] = 0;
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_OVERT_MASK,
-					MAX6697_OVERT_MAP_BITS(vals[0]));
+	ret = regmap_write(regmap, MAX6697_REG_OVERT_MASK,
+			   MAX6697_OVERT_MAP_BITS(vals[0]));
 	if (ret)
 		return ret;
 
@@ -610,7 +527,6 @@ static int max6697_config_of(struct max6697_data *data, struct i2c_client *clien
 		if (of_property_read_bool(node, "resistance-cancellation") &&
 		    chip->valid_conf & MAX6697_CONF_RESISTANCE) {
 			confreg |= MAX6697_CONF_RESISTANCE;
-			factor = 1;
 		}
 	} else {
 		if (of_property_read_u32(node, "resistance-cancellation", &vals[0])) {
@@ -620,9 +536,7 @@ static int max6697_config_of(struct max6697_data *data, struct i2c_client *clien
 				vals[0] = 0;
 		}
 
-		factor = hweight8(vals[0] & 0xfe);
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_RESISTANCE,
-						vals[0] >> 1);
+		ret = regmap_write(regmap, MAX6581_REG_RESISTANCE, vals[0] >> 1);
 		if (ret < 0)
 			return ret;
 
@@ -631,81 +545,92 @@ static int max6697_config_of(struct max6697_data *data, struct i2c_client *clien
 			vals[1] = 0;
 		}
 
-		ret = i2c_smbus_write_byte_data(client, MAX6581_REG_IDEALITY,
-						vals[1]);
+		ret = regmap_write(regmap, MAX6581_REG_IDEALITY, vals[1]);
 		if (ret < 0)
 			return ret;
-		ret = i2c_smbus_write_byte_data(client,
-						MAX6581_REG_IDEALITY_SELECT,
-						vals[0] >> 1);
+		ret = regmap_write(regmap, MAX6581_REG_IDEALITY_SELECT,
+				   vals[0] >> 1);
 		if (ret < 0)
 			return ret;
 	}
-	ret = i2c_smbus_write_byte_data(client, MAX6697_REG_CONFIG, confreg);
-	if (ret < 0)
-		return ret;
-	return factor;
+	return regmap_write(regmap, MAX6697_REG_CONFIG, confreg);
 }
 
-static int max6697_init_chip(struct max6697_data *data,
-			     struct i2c_client *client)
+static int max6697_init_chip(struct device_node *np, struct max6697_data *data)
 {
-	const struct max6697_chip_data *chip = data->chip;
-	int factor = chip->channels;
-	int ret, reg;
+	unsigned int reg;
+	int ret;
 
 	/*
 	 * Don't touch configuration if there is no devicetree configuration.
 	 * If that is the case, use the current chip configuration.
 	 */
-	if (!client->dev.of_node) {
-		reg = i2c_smbus_read_byte_data(client, MAX6697_REG_CONFIG);
-		if (reg < 0)
-			return reg;
+	if (!np) {
+		struct regmap *regmap = data->regmap;
+
+		ret = regmap_read(regmap, MAX6697_REG_CONFIG, &reg);
+		if (ret < 0)
+			return ret;
 		if (data->type == max6581) {
 			if (reg & MAX6581_CONF_EXTENDED)
 				data->temp_offset = 64;
-			reg = i2c_smbus_read_byte_data(client,
-						       MAX6581_REG_RESISTANCE);
-			if (reg < 0)
-				return reg;
-			factor += hweight8(reg);
-		} else {
-			if (reg & MAX6697_CONF_RESISTANCE)
-				factor++;
+			ret = regmap_read(regmap, MAX6581_REG_RESISTANCE, &reg);
 		}
-		data->update_interval = factor * MAX6697_CONV_TIME;
 	} else {
-		ret = max6697_config_of(data, client);
-		if (ret < 0)
-			return ret;
-		data->update_interval = (factor + ret) * MAX6697_CONV_TIME;
+		ret = max6697_config_of(np, data);
 	}
 
-	return 0;
+	return ret;
 }
 
+static bool max6697_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case 0x00 ... 0x09:	/* temperature high bytes */
+	case 0x44 ... 0x47:	/* status */
+	case 0x51 ... 0x58:	/* temperature low bytes */
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool max6697_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return reg != 0x0a && reg != 0x0f && !max6697_volatile_reg(dev, reg);
+}
+
+static const struct regmap_config max6697_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x58,
+	.writeable_reg = max6697_writeable_reg,
+	.volatile_reg = max6697_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
 static int max6697_probe(struct i2c_client *client)
 {
-	struct i2c_adapter *adapter = client->adapter;
 	struct device *dev = &client->dev;
 	struct max6697_data *data;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
 	int err;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
-		return -ENODEV;
+	regmap = regmap_init_i2c(client, &max6697_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
 
 	data = devm_kzalloc(dev, sizeof(struct max6697_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
+	data->regmap = regmap;
 	data->type = (uintptr_t)i2c_get_match_data(client);
 	data->chip = &max6697_chip_data[data->type];
-	data->client = client;
 	mutex_init(&data->update_lock);
 
-	err = max6697_init_chip(data, client);
+	err = max6697_init_chip(client->dev.of_node, data);
 	if (err)
 		return err;
 
-- 
2.39.2

[PATCH v2 5/6] hwmon: (max6697) Convert to with_info hwmon API

[Guenter Roeck]

Convert to with_info hwmon API to simplify the code and reduce its size.

This patch reduces object file size by approximately 25%.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max6697.c | 464 +++++++++++++++-------------------------
 1 file changed, 173 insertions(+), 291 deletions(-)

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index e4bec33d1d44..a81e60879c1a 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -10,7 +10,6 @@
 #include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -52,7 +51,9 @@ static const u8 MAX6697_REG_CRIT[] = {
 	(FIELD_PREP(MAX6697_EXTERNAL_MASK_CHIP, FIELD_GET(MAX6697_EXTERNAL_MASK_DT, reg)) | \
 	 FIELD_PREP(MAX6697_LOCAL_MASK_CHIP, FIELD_GET(MAX6697_LOCAL_MASK_DT, reg)))
 
-#define MAX6697_REG_STAT(n)		(0x44 + (n))
+#define MAX6697_REG_STAT_ALARM		0x44
+#define MAX6697_REG_STAT_CRIT		0x45
+#define MAX6697_REG_STAT_FAULT		0x46
 
 #define MAX6697_REG_CONFIG		0x41
 #define MAX6581_CONF_EXTENDED		BIT(1)
@@ -78,7 +79,6 @@ struct max6697_chip_data {
 	u32 have_crit;
 	u32 have_fault;
 	u8 valid_conf;
-	const u8 *alarm_map;
 };
 
 struct max6697_data {
@@ -98,11 +98,6 @@ struct max6697_data {
 	u32 alarms;
 };
 
-/* Diode fault status bits on MAX6581 are right shifted by one bit */
-static const u8 max6581_alarm_map[] = {
-	 0, 0, 1, 2, 3, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15,
-	 16, 17, 18, 19, 20, 21, 22, 23 };
-
 static const struct max6697_chip_data max6697_chip_data[] = {
 	[max6581] = {
 		.channels = 8,
@@ -110,7 +105,6 @@ static const struct max6697_chip_data max6697_chip_data[] = {
 		.have_ext = 0x7f,
 		.have_fault = 0xfe,
 		.valid_conf = MAX6581_CONF_EXTENDED | MAX6697_CONF_TIMEOUT,
-		.alarm_map = max6581_alarm_map,
 	},
 	[max6602] = {
 		.channels = 5,
@@ -179,313 +173,202 @@ static const struct max6697_chip_data max6697_chip_data[] = {
 	},
 };
 
-static inline int max6581_offset_to_millic(int val)
-{
-	return sign_extend32(val, 7) * 250;
-}
-
-static ssize_t temp_input_show(struct device *dev,
-			       struct device_attribute *devattr, char *buf)
+static int max6697_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
 {
+	unsigned int offset_regs[2] = { MAX6581_REG_OFFSET_SELECT, MAX6581_REG_OFFSET };
+	unsigned int temp_regs[2] = { MAX6697_REG_TEMP[channel],
+				      MAX6697_REG_TEMP_EXT[channel] };
 	struct max6697_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	unsigned int regs[2] = { MAX6697_REG_TEMP[index],
-				 MAX6697_REG_TEMP_EXT[index] };
-	u8 regdata[2] = { };
-	int temp, ret;
-
-	ret = regmap_multi_reg_read(data->regmap, regs, regdata,
-				    data->chip->have_ext & BIT(index) ? 2 : 1);
-	if (ret)
-		return ret;
-
-	temp = ((regdata[0] - data->temp_offset) << 3) | (regdata[1] >> 5);
-
-	return sprintf(buf, "%d\n", temp * 125);
-}
-
-static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
-			 char *buf)
-{
-	struct max6697_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr_2(devattr)->index;
-	int nr = to_sensor_dev_attr_2(devattr)->nr;
-	unsigned int temp;
-	int reg, ret;
-
-	if (index == MAX6697_TEMP_MAX)
-		reg = MAX6697_REG_MAX[nr];
-	else
-		reg = MAX6697_REG_CRIT[nr];
-
-	ret = regmap_read(data->regmap, reg, &temp);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%d\n", ((int)temp - data->temp_offset) * 1000);
-}
-
-static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
-			  char *buf)
-{
-	struct max6697_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	unsigned int alarms;
-	int reg, ret;
-
-	if (data->chip->alarm_map)
-		index = data->chip->alarm_map[index];
-
-	reg = MAX6697_REG_STAT(2 - (index / 8));
-	ret = regmap_read(data->regmap, reg, &alarms);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%u\n", !!(alarms & BIT(index & 7)));
-}
-
-static ssize_t temp_store(struct device *dev,
-			  struct device_attribute *devattr, const char *buf,
-			  size_t count)
-{
-	int nr = to_sensor_dev_attr_2(devattr)->nr;
-	int index = to_sensor_dev_attr_2(devattr)->index;
-	struct max6697_data *data = dev_get_drvdata(dev);
-	long temp;
-	int ret;
-
-	ret = kstrtol(buf, 10, &temp);
-	if (ret < 0)
-		return ret;
-
-	temp = clamp_val(temp, -1000000, 1000000);	/* prevent underflow */
-	temp = DIV_ROUND_CLOSEST(temp, 1000) + data->temp_offset;
-	temp = clamp_val(temp, 0, data->type == max6581 ? 255 : 127);
-	ret = regmap_write(data->regmap,
-			   index == 2 ? MAX6697_REG_MAX[nr]
-				      : MAX6697_REG_CRIT[nr],
-			   temp);
-
-	return ret ? : count;
-}
-
-static ssize_t offset_store(struct device *dev, struct device_attribute *devattr, const char *buf,
-			    size_t count)
-{
-	struct max6697_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
 	struct regmap *regmap = data->regmap;
-	long temp;
+	u8 regdata[2] = { };
+	u32 regval;
 	int ret;
 
-	ret = kstrtol(buf, 10, &temp);
-	if (ret < 0)
-		return ret;
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = regmap_multi_reg_read(regmap, temp_regs, regdata,
+					    data->chip->have_ext & BIT(channel) ? 2 : 1);
+		if (ret)
+			return ret;
+		*val = (((regdata[0] - data->temp_offset) << 3) | (regdata[1] >> 5)) * 125;
+		break;
+	case hwmon_temp_max:
+		ret = regmap_read(regmap, MAX6697_REG_MAX[channel], &regval);
+		if (ret)
+			return ret;
+		*val = ((int)regval - data->temp_offset) * 1000;
+		break;
+	case hwmon_temp_crit:
+		ret = regmap_read(regmap, MAX6697_REG_CRIT[channel], &regval);
+		if (ret)
+			return ret;
+		*val = ((int)regval - data->temp_offset) * 1000;
+		break;
+	case hwmon_temp_offset:
+		ret = regmap_multi_reg_read(regmap, offset_regs, regdata, 2);
+		if (ret)
+			return ret;
 
-	mutex_lock(&data->update_lock);
-	temp = clamp_val(temp, MAX6581_OFFSET_MIN, MAX6581_OFFSET_MAX);
-	temp = DIV_ROUND_CLOSEST(temp, 250);
-	if (!temp) {	/* disable this (and only this) channel */
-		ret = regmap_clear_bits(regmap, MAX6581_REG_OFFSET_SELECT, BIT(index - 1));
-		goto unlock;
+		if (!(regdata[0] & BIT(channel - 1)))
+			regdata[1] = 0;
+
+		*val = sign_extend32(regdata[1], 7) * 250;
+		break;
+	case hwmon_temp_fault:
+		ret = regmap_read(regmap, MAX6697_REG_STAT_FAULT, &regval);
+		if (ret)
+			return ret;
+		if (data->type == max6581)
+			*val = !!(regval & BIT(channel - 1));
+		else
+			*val = !!(regval & BIT(channel));
+		break;
+	case hwmon_temp_crit_alarm:
+		ret = regmap_read(regmap, MAX6697_REG_STAT_CRIT, &regval);
+		if (ret)
+			return ret;
+		*val = !!(regval & BIT(channel ? channel - 1 : 7));
+		break;
+	case hwmon_temp_max_alarm:
+		ret = regmap_read(regmap, MAX6697_REG_STAT_ALARM, &regval);
+		if (ret)
+			return ret;
+		switch (channel) {
+		case 0:
+			*val = !!(regval & BIT(6));
+			break;
+		case 7:
+			*val = !!(regval & BIT(7));
+			break;
+		default:
+			*val = !!(regval & BIT(channel - 1));
+			break;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
 	}
-	/* enable channel, and update offset */
-	ret = regmap_set_bits(regmap, MAX6581_REG_OFFSET_SELECT, BIT(index - 1));
-	if (ret)
-		goto unlock;
-	ret = regmap_write(regmap, MAX6581_REG_OFFSET, temp);
-unlock:
-	mutex_unlock(&data->update_lock);
-	return ret ? : count;
+	return 0;
 }
 
-static ssize_t offset_show(struct device *dev, struct device_attribute *devattr, char *buf)
+static int max6697_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
 {
-	unsigned int regs[2] = { MAX6581_REG_OFFSET_SELECT, MAX6581_REG_OFFSET };
 	struct max6697_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(devattr)->index;
-	u8 regdata[2];
+	struct regmap *regmap = data->regmap;
 	int ret;
 
-	ret = regmap_multi_reg_read(data->regmap, regs, regdata, 2);
-	if (ret)
+	switch (attr) {
+	case hwmon_temp_max:
+		val = clamp_val(val, -1000000, 1000000);	/* prevent underflow */
+		val = DIV_ROUND_CLOSEST(val, 1000) + data->temp_offset;
+		val = clamp_val(val, 0, data->type == max6581 ? 255 : 127);
+		return regmap_write(regmap, MAX6697_REG_MAX[channel], val);
+	case hwmon_temp_crit:
+		val = clamp_val(val, -1000000, 1000000);	/* prevent underflow */
+		val = DIV_ROUND_CLOSEST(val, 1000) + data->temp_offset;
+		val = clamp_val(val, 0, data->type == max6581 ? 255 : 127);
+		return regmap_write(regmap, MAX6697_REG_CRIT[channel], val);
+	case hwmon_temp_offset:
+		mutex_lock(&data->update_lock);
+		val = clamp_val(val, MAX6581_OFFSET_MIN, MAX6581_OFFSET_MAX);
+		val = DIV_ROUND_CLOSEST(val, 250);
+		if (!val) {	/* disable this (and only this) channel */
+			ret = regmap_clear_bits(regmap, MAX6581_REG_OFFSET_SELECT,
+						BIT(channel - 1));
+		} else {
+			/* enable channel and update offset */
+			ret = regmap_set_bits(regmap, MAX6581_REG_OFFSET_SELECT,
+					      BIT(channel - 1));
+			if (ret)
+				goto unlock;
+			ret = regmap_write(regmap, MAX6581_REG_OFFSET, val);
+		}
+unlock:
+		mutex_unlock(&data->update_lock);
 		return ret;
-
-	if (!(regdata[0] & BIT(index - 1)))
-		regdata[1] = 0;
-
-	return sprintf(buf, "%d\n", max6581_offset_to_millic(regdata[1]));
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp_input, 0);
-static SENSOR_DEVICE_ATTR_2_RW(temp1_max, temp, 0, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp1_crit, temp, 0, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp_input, 1);
-static SENSOR_DEVICE_ATTR_2_RW(temp2_max, temp, 1, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp2_crit, temp, 1, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp3_input, temp_input, 2);
-static SENSOR_DEVICE_ATTR_2_RW(temp3_max, temp, 2, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp3_crit, temp, 2, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp4_input, temp_input, 3);
-static SENSOR_DEVICE_ATTR_2_RW(temp4_max, temp, 3, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp4_crit, temp, 3, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp5_input, temp_input, 4);
-static SENSOR_DEVICE_ATTR_2_RW(temp5_max, temp, 4, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp5_crit, temp, 4, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp6_input, temp_input, 5);
-static SENSOR_DEVICE_ATTR_2_RW(temp6_max, temp, 5, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp6_crit, temp, 5, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp7_input, temp_input, 6);
-static SENSOR_DEVICE_ATTR_2_RW(temp7_max, temp, 6, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp7_crit, temp, 6, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp8_input, temp_input, 7);
-static SENSOR_DEVICE_ATTR_2_RW(temp8_max, temp, 7, MAX6697_TEMP_MAX);
-static SENSOR_DEVICE_ATTR_2_RW(temp8_crit, temp, 7, MAX6697_TEMP_CRIT);
-
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, alarm, 22);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, alarm, 16);
-static SENSOR_DEVICE_ATTR_RO(temp3_max_alarm, alarm, 17);
-static SENSOR_DEVICE_ATTR_RO(temp4_max_alarm, alarm, 18);
-static SENSOR_DEVICE_ATTR_RO(temp5_max_alarm, alarm, 19);
-static SENSOR_DEVICE_ATTR_RO(temp6_max_alarm, alarm, 20);
-static SENSOR_DEVICE_ATTR_RO(temp7_max_alarm, alarm, 21);
-static SENSOR_DEVICE_ATTR_RO(temp8_max_alarm, alarm, 23);
-
-static SENSOR_DEVICE_ATTR_RO(temp1_crit_alarm, alarm, 15);
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, alarm, 8);
-static SENSOR_DEVICE_ATTR_RO(temp3_crit_alarm, alarm, 9);
-static SENSOR_DEVICE_ATTR_RO(temp4_crit_alarm, alarm, 10);
-static SENSOR_DEVICE_ATTR_RO(temp5_crit_alarm, alarm, 11);
-static SENSOR_DEVICE_ATTR_RO(temp6_crit_alarm, alarm, 12);
-static SENSOR_DEVICE_ATTR_RO(temp7_crit_alarm, alarm, 13);
-static SENSOR_DEVICE_ATTR_RO(temp8_crit_alarm, alarm, 14);
-
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, alarm, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_fault, alarm, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_fault, alarm, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_fault, alarm, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_fault, alarm, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_fault, alarm, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_fault, alarm, 7);
-
-/* There is no offset for local temperature so starting from temp2 */
-static SENSOR_DEVICE_ATTR_RW(temp2_offset, offset, 1);
-static SENSOR_DEVICE_ATTR_RW(temp3_offset, offset, 2);
-static SENSOR_DEVICE_ATTR_RW(temp4_offset, offset, 3);
-static SENSOR_DEVICE_ATTR_RW(temp5_offset, offset, 4);
-static SENSOR_DEVICE_ATTR_RW(temp6_offset, offset, 5);
-static SENSOR_DEVICE_ATTR_RW(temp7_offset, offset, 6);
-static SENSOR_DEVICE_ATTR_RW(temp8_offset, offset, 7);
-
-static DEVICE_ATTR(dummy, 0, NULL, NULL);
-
-static umode_t max6697_is_visible(struct kobject *kobj, struct attribute *attr,
-				  int index)
+static umode_t max6697_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
 {
-	struct device *dev = kobj_to_dev(kobj);
-	struct max6697_data *data = dev_get_drvdata(dev);
+	const struct max6697_data *data = _data;
 	const struct max6697_chip_data *chip = data->chip;
-	int channel = index / 7;	/* channel number */
-	int nr = index % 7;		/* attribute index within channel */
 
 	if (channel >= chip->channels)
 		return 0;
 
-	if ((nr == 3 || nr == 4) && !(chip->have_crit & BIT(channel)))
-		return 0;
-	if (nr == 5 && !(chip->have_fault & BIT(channel)))
-		return 0;
-	/* offset reg is only supported on max6581 remote channels */
-	if (nr == 6)
-		if (data->type != max6581 || channel == 0)
-			return 0;
-
-	return attr->mode;
+	switch (attr) {
+	case hwmon_temp_max:
+		return 0644;
+	case hwmon_temp_input:
+	case hwmon_temp_max_alarm:
+		return 0444;
+	case hwmon_temp_crit:
+		if (chip->have_crit & BIT(channel))
+			return 0644;
+		break;
+	case hwmon_temp_crit_alarm:
+		if (chip->have_crit & BIT(channel))
+			return 0444;
+		break;
+	case hwmon_temp_fault:
+		if (chip->have_fault & BIT(channel))
+			return 0444;
+		break;
+	case hwmon_temp_offset:
+		if (data->type == max6581 && channel)
+			return 0644;
+		break;
+	default:
+		break;
+	}
+	return 0;
 }
 
-/*
- * max6697_is_visible uses the index into the following array to determine
- * if attributes should be created or not. Any change in order or content
- * must be matched in max6697_is_visible.
- */
-static struct attribute *max6697_attributes[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit.dev_attr.attr,
-	&sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
-	&dev_attr_dummy.attr,
-	&dev_attr_dummy.attr,
-
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_max.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp3_input.dev_attr.attr,
-	&sensor_dev_attr_temp3_max.dev_attr.attr,
-	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp3_crit.dev_attr.attr,
-	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp3_fault.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp4_input.dev_attr.attr,
-	&sensor_dev_attr_temp4_max.dev_attr.attr,
-	&sensor_dev_attr_temp4_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp4_crit.dev_attr.attr,
-	&sensor_dev_attr_temp4_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp4_fault.dev_attr.attr,
-	&sensor_dev_attr_temp4_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp5_input.dev_attr.attr,
-	&sensor_dev_attr_temp5_max.dev_attr.attr,
-	&sensor_dev_attr_temp5_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp5_crit.dev_attr.attr,
-	&sensor_dev_attr_temp5_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp5_fault.dev_attr.attr,
-	&sensor_dev_attr_temp5_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp6_input.dev_attr.attr,
-	&sensor_dev_attr_temp6_max.dev_attr.attr,
-	&sensor_dev_attr_temp6_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp6_crit.dev_attr.attr,
-	&sensor_dev_attr_temp6_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp6_fault.dev_attr.attr,
-	&sensor_dev_attr_temp6_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp7_input.dev_attr.attr,
-	&sensor_dev_attr_temp7_max.dev_attr.attr,
-	&sensor_dev_attr_temp7_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp7_crit.dev_attr.attr,
-	&sensor_dev_attr_temp7_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp7_fault.dev_attr.attr,
-	&sensor_dev_attr_temp7_offset.dev_attr.attr,
-
-	&sensor_dev_attr_temp8_input.dev_attr.attr,
-	&sensor_dev_attr_temp8_max.dev_attr.attr,
-	&sensor_dev_attr_temp8_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp8_crit.dev_attr.attr,
-	&sensor_dev_attr_temp8_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp8_fault.dev_attr.attr,
-	&sensor_dev_attr_temp8_offset.dev_attr.attr,
+/* Return 0 if detection is successful, -ENODEV otherwise */
+static const struct hwmon_channel_info * const max6697_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_FAULT | HWMON_T_OFFSET),
 	NULL
 };
 
-static const struct attribute_group max6697_group = {
-	.attrs = max6697_attributes, .is_visible = max6697_is_visible,
+static const struct hwmon_ops max6697_hwmon_ops = {
+	.is_visible = max6697_is_visible,
+	.read = max6697_read,
+	.write = max6697_write,
+};
+
+static const struct hwmon_chip_info max6697_chip_info = {
+	.ops = &max6697_hwmon_ops,
+	.info = max6697_info,
 };
-__ATTRIBUTE_GROUPS(max6697);
 
 static int max6697_config_of(struct device_node *node, struct max6697_data *data)
 {
@@ -634,9 +517,8 @@ static int max6697_probe(struct i2c_client *client)
 	if (err)
 		return err;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   data,
-							   max6697_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							 &max6697_chip_info, NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.2

[PATCH v2 6/6] hwmon: (max6697) Add support for tempX_min and tempX_min_alarm

[Guenter Roeck]

MAX6581 supports setting the minimum temperature as well as minimum
temperature alarms. Add support for it.

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added Reviewed-by: tag

 drivers/hwmon/max6697.c | 59 +++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/max6697.c b/drivers/hwmon/max6697.c
index a81e60879c1a..1b7b3f0da662 100644
--- a/drivers/hwmon/max6697.c
+++ b/drivers/hwmon/max6697.c
@@ -32,6 +32,7 @@ static const u8 MAX6697_REG_MAX[] = {
 static const u8 MAX6697_REG_CRIT[] = {
 			0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27 };
 
+#define MAX6697_REG_MIN			0x30
 /*
  * Map device tree / internal register bit map to chip bit map.
  * Applies to alert register and over-temperature register.
@@ -54,6 +55,7 @@ static const u8 MAX6697_REG_CRIT[] = {
 #define MAX6697_REG_STAT_ALARM		0x44
 #define MAX6697_REG_STAT_CRIT		0x45
 #define MAX6697_REG_STAT_FAULT		0x46
+#define MAX6697_REG_STAT_MIN_ALARM	0x47
 
 #define MAX6697_REG_CONFIG		0x41
 #define MAX6581_CONF_EXTENDED		BIT(1)
@@ -173,6 +175,18 @@ static const struct max6697_chip_data max6697_chip_data[] = {
 	},
 };
 
+static int max6697_alarm_channel_map(int channel)
+{
+	switch (channel) {
+	case 0:
+		return 6;
+	case 7:
+		return 7;
+	default:
+		return channel - 1;
+	}
+}
+
 static int max6697_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
@@ -205,6 +219,12 @@ static int max6697_read(struct device *dev, enum hwmon_sensor_types type,
 			return ret;
 		*val = ((int)regval - data->temp_offset) * 1000;
 		break;
+	case hwmon_temp_min:
+		ret = regmap_read(regmap, MAX6697_REG_MIN, &regval);
+		if (ret)
+			return ret;
+		*val = ((int)regval - data->temp_offset) * 1000;
+		break;
 	case hwmon_temp_offset:
 		ret = regmap_multi_reg_read(regmap, offset_regs, regdata, 2);
 		if (ret)
@@ -234,17 +254,13 @@ static int max6697_read(struct device *dev, enum hwmon_sensor_types type,
 		ret = regmap_read(regmap, MAX6697_REG_STAT_ALARM, &regval);
 		if (ret)
 			return ret;
-		switch (channel) {
-		case 0:
-			*val = !!(regval & BIT(6));
-			break;
-		case 7:
-			*val = !!(regval & BIT(7));
-			break;
-		default:
-			*val = !!(regval & BIT(channel - 1));
-			break;
-		}
+		*val = !!(regval & BIT(max6697_alarm_channel_map(channel)));
+		break;
+	case hwmon_temp_min_alarm:
+		ret = regmap_read(regmap, MAX6697_REG_STAT_MIN_ALARM, &regval);
+		if (ret)
+			return ret;
+		*val = !!(regval & BIT(max6697_alarm_channel_map(channel)));
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -270,6 +286,11 @@ static int max6697_write(struct device *dev, enum hwmon_sensor_types type,
 		val = DIV_ROUND_CLOSEST(val, 1000) + data->temp_offset;
 		val = clamp_val(val, 0, data->type == max6581 ? 255 : 127);
 		return regmap_write(regmap, MAX6697_REG_CRIT[channel], val);
+	case hwmon_temp_min:
+		val = clamp_val(val, -1000000, 1000000);	/* prevent underflow */
+		val = DIV_ROUND_CLOSEST(val, 1000) + data->temp_offset;
+		val = clamp_val(val, 0, 255);
+		return regmap_write(regmap, MAX6697_REG_MIN, val);
 	case hwmon_temp_offset:
 		mutex_lock(&data->update_lock);
 		val = clamp_val(val, MAX6581_OFFSET_MIN, MAX6581_OFFSET_MAX);
@@ -308,6 +329,14 @@ static umode_t max6697_is_visible(const void *_data, enum hwmon_sensor_types typ
 	case hwmon_temp_input:
 	case hwmon_temp_max_alarm:
 		return 0444;
+	case hwmon_temp_min:
+		if (data->type == max6581)
+			return channel ? 0444 : 0644;
+		break;
+	case hwmon_temp_min_alarm:
+		if (data->type == max6581)
+			return 0444;
+		break;
 	case hwmon_temp_crit:
 		if (chip->have_crit & BIT(channel))
 			return 0644;
@@ -334,27 +363,35 @@ static umode_t max6697_is_visible(const void *_data, enum hwmon_sensor_types typ
 static const struct hwmon_channel_info * const max6697_info[] = {
 	HWMON_CHANNEL_INFO(temp,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
 			   HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_MIN | HWMON_T_MIN_ALARM |
 			   HWMON_T_FAULT | HWMON_T_OFFSET),
 	NULL
 };
-- 
2.39.2