[PATCH 0/6] hwmon: (lm9534) Various improvements

[PATCH 0/6] hwmon: (lm9534) Various improvements

[Guenter Roeck]

Clean up driver, convert to use regmap, and convert to use
with_info hwmon API. Finally, add support for temp_enable
attribute to support enabling or disabling channels one by
one.

----------------------------------------------------------------
Guenter Roeck (6):
      hwmon: (lm95234) Reorder include files to be in alphabetic order
      hwmon: (lm95234) Use find_closest to find matching update interval
      hwmon: (lm95234) Convert to use regmap
      hwmon: (lm95234) Convert to with_info hwmon API
      hwmon: (lm95234) Add support for tempX_enable attribute
      hwmon: (lm95234) Use multi-byte regmap operations

 drivers/hwmon/Kconfig   |   1 +
 drivers/hwmon/lm95234.c | 869 +++++++++++++++++++-----------------------------
 2 files changed, 351 insertions(+), 519 deletions(-)

[PATCH 1/6] hwmon: (lm95234) Reorder include files to be in alphabetic order

[Guenter Roeck]

Alphabetic include file order simplifies maintenance and makes it easier
to add or remove files.

No functional change.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm95234.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index 9a7afdb49895..0c509eed6a01 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -8,14 +8,14 @@
  * Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
  */
 
-#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/slab.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
 
-- 
2.39.2

Re: [PATCH 1/6] hwmon: (lm95234) Reorder include files to be in alphabetic order

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:30PM -0700, Guenter Roeck wrote:
> Alphabetic include file order simplifies maintenance and makes it easier
> to add or remove files.
> 
> No functional change.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

[PATCH 2/6] hwmon: (lm95234) Use find_closest to find matching update interval

[Guenter Roeck]

Use find_closest() instead of manually coding it to find best update
interval.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm95234.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index 0c509eed6a01..a36fa7824da8 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/sysfs.h>
+#include <linux/util_macros.h>
 
 #define DRVNAME "lm95234"
 
@@ -471,10 +472,7 @@ static ssize_t update_interval_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	for (regval = 0; regval < 3; regval++) {
-		if (val <= update_intervals[regval])
-			break;
-	}
+	regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
 
 	mutex_lock(&data->update_lock);
 	data->interval = msecs_to_jiffies(update_intervals[regval]);
-- 
2.39.2

Re: [PATCH 2/6] hwmon: (lm95234) Use find_closest to find matching update interval

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:31PM -0700, Guenter Roeck wrote:
> @@ -471,10 +472,7 @@ static ssize_t update_interval_store(struct device *dev,
>  	if (ret < 0)
>  		return ret;
>  
> -	for (regval = 0; regval < 3; regval++) {
> -		if (val <= update_intervals[regval])
> -			break;
> -	}
> +	regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));

The behavior changed.

static u16 update_intervals[] = { 143, 364, 1000, 2500 };

If val = 144,
* Originally, regval = 1.
* After applying the patch, regval = 0.

Re: [PATCH 2/6] hwmon: (lm95234) Use find_closest to find matching update interval

[Guenter Roeck]

On 7/18/24 09:39, Tzung-Bi Shih wrote:
> On Wed, Jul 17, 2024 at 08:39:31PM -0700, Guenter Roeck wrote:
>> @@ -471,10 +472,7 @@ static ssize_t update_interval_store(struct device *dev,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	for (regval = 0; regval < 3; regval++) {
>> -		if (val <= update_intervals[regval])
>> -			break;
>> -	}
>> +	regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
> 
> The behavior changed.
> 
> static u16 update_intervals[] = { 143, 364, 1000, 2500 };
> 
> If val = 144,
> * Originally, regval = 1.
> * After applying the patch, regval = 0.
> 


Yes, find_closest() rounds the value instead of using the lower match.
That was intentional. I'll add an explicit note to the commit message.

Thanks,
Guenter

Re: [PATCH 2/6] hwmon: (lm95234) Use find_closest to find matching update interval

[Guenter Roeck]

On 7/18/24 10:48, Guenter Roeck wrote:
> On 7/18/24 09:39, Tzung-Bi Shih wrote:
>> On Wed, Jul 17, 2024 at 08:39:31PM -0700, Guenter Roeck wrote:
>>> @@ -471,10 +472,7 @@ static ssize_t update_interval_store(struct device *dev,
>>>       if (ret < 0)
>>>           return ret;
>>> -    for (regval = 0; regval < 3; regval++) {
>>> -        if (val <= update_intervals[regval])
>>> -            break;
>>> -    }
>>> +    regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
>>
>> The behavior changed.
>>
>> static u16 update_intervals[] = { 143, 364, 1000, 2500 };
>>
>> If val = 144,
>> * Originally, regval = 1.
>> * After applying the patch, regval = 0.
>>
> 
> 
> Yes, find_closest() rounds the value instead of using the lower match.
> That was intentional. I'll add an explicit note to the commit message.
> 

I added this to the commit message:

     Since find_closest() uses rounding to find the best match, the resulting
     update interval will now reflect the update interval that is closest to
     the requested value, not the value that is lower or equal to the requested
     value.

Thanks,
Guenter

Re: [PATCH 2/6] hwmon: (lm95234) Use find_closest to find matching update interval

[Tzung-Bi Shih]

On Thu, Jul 18, 2024 at 10:53:19AM -0700, Guenter Roeck wrote:
> On 7/18/24 10:48, Guenter Roeck wrote:
> > On 7/18/24 09:39, Tzung-Bi Shih wrote:
> > > On Wed, Jul 17, 2024 at 08:39:31PM -0700, Guenter Roeck wrote:
> > > > @@ -471,10 +472,7 @@ static ssize_t update_interval_store(struct device *dev,
> > > >       if (ret < 0)
> > > >           return ret;
> > > > -    for (regval = 0; regval < 3; regval++) {
> > > > -        if (val <= update_intervals[regval])
> > > > -            break;
> > > > -    }
> > > > +    regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
> > > 
> > > The behavior changed.
> > > 
> > > static u16 update_intervals[] = { 143, 364, 1000, 2500 };
> > > 
> > > If val = 144,
> > > * Originally, regval = 1.
> > > * After applying the patch, regval = 0.
> > > 
> > 
> > 
> > Yes, find_closest() rounds the value instead of using the lower match.
> > That was intentional. I'll add an explicit note to the commit message.
> > 
> 
> I added this to the commit message:
> 
>     Since find_closest() uses rounding to find the best match, the resulting
>     update interval will now reflect the update interval that is closest to
>     the requested value, not the value that is lower or equal to the requested
>     value.

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

[PATCH 3/6] hwmon: (lm95234) Convert to use regmap

[Guenter Roeck]

Use regmap to replace local caching and to be able to use regmap API
functions.

No functional change.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/Kconfig   |   1 +
 drivers/hwmon/lm95234.c | 437 +++++++++++++++++-----------------------
 2 files changed, 186 insertions(+), 252 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index b60fe2e58ad6..e838a55bb3cb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1532,6 +1532,7 @@ config SENSORS_LM93
 config SENSORS_LM95234
 	tristate "National Semiconductor LM95234 and compatibles"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	  If you say yes here you get support for the LM95233 and LM95234
 	  temperature sensor chips.
diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index a36fa7824da8..7a3aff1d183a 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.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/slab.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
 #include <linux/sysfs.h>
 #include <linux/util_macros.h>
 
@@ -33,6 +33,8 @@ static const unsigned short normal_i2c[] = {
 #define LM95234_REG_STATUS		0x02
 #define LM95234_REG_CONFIG		0x03
 #define LM95234_REG_CONVRATE		0x04
+#define LM95234_REG_ENABLE		0x05
+#define LM95234_REG_FILTER		0x06
 #define LM95234_REG_STS_FAULT		0x07
 #define LM95234_REG_STS_TCRIT1		0x08
 #define LM95234_REG_STS_TCRIT2		0x09
@@ -53,181 +55,72 @@ static const unsigned short normal_i2c[] = {
 
 /* Client data (each client gets its own) */
 struct lm95234_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	const struct attribute_group *groups[3];
 	struct mutex update_lock;
-	unsigned long last_updated, interval;	/* in jiffies */
-	bool valid;		/* false until following fields are valid */
-	/* registers values */
-	int temp[5];		/* temperature (signed) */
-	u32 status;		/* fault/alarm status */
-	u8 tcrit1[5];		/* critical temperature limit */
-	u8 tcrit2[2];		/* high temperature limit */
-	s8 toffset[4];		/* remote temperature offset */
-	u8 thyst;		/* common hysteresis */
-
-	u8 sensor_type;		/* temperature sensor type */
 };
 
-static int lm95234_read_temp(struct i2c_client *client, int index, int *t)
+static int lm95234_read_temp(struct regmap *regmap, int index, int *t)
 {
-	int val;
-	u16 temp = 0;
+	int temp = 0, ret;
+	u32 val;
 
 	if (index) {
-		val = i2c_smbus_read_byte_data(client,
-					       LM95234_REG_UTEMPH(index - 1));
-		if (val < 0)
-			return val;
+		ret = regmap_read(regmap, LM95234_REG_UTEMPH(index - 1), &val);
+		if (ret)
+			return ret;
 		temp = val << 8;
-		val = i2c_smbus_read_byte_data(client,
-					       LM95234_REG_UTEMPL(index - 1));
-		if (val < 0)
-			return val;
+		ret = regmap_read(regmap, LM95234_REG_UTEMPL(index - 1), &val);
+		if (ret)
+			return ret;
 		temp |= val;
-		*t = temp;
 	}
 	/*
 	 * Read signed temperature if unsigned temperature is 0,
 	 * or if this is the local sensor.
 	 */
 	if (!temp) {
-		val = i2c_smbus_read_byte_data(client,
-					       LM95234_REG_TEMPH(index));
-		if (val < 0)
-			return val;
+		ret = regmap_read(regmap, LM95234_REG_TEMPH(index), &val);
+		if (ret)
+			return ret;
 		temp = val << 8;
-		val = i2c_smbus_read_byte_data(client,
-					       LM95234_REG_TEMPL(index));
-		if (val < 0)
-			return val;
-		temp |= val;
-		*t = (s16)temp;
+		ret = regmap_read(regmap, LM95234_REG_TEMPL(index), &val);
+		if (ret)
+			return ret;
+		temp = sign_extend32(temp | val, 15);
 	}
+	*t = DIV_ROUND_CLOSEST(temp * 125, 32);
 	return 0;
 }
 
-static u16 update_intervals[] = { 143, 364, 1000, 2500 };
-
-/* Fill value cache. Must be called with update lock held. */
-
-static int lm95234_fill_cache(struct lm95234_data *data,
-			      struct i2c_client *client)
-{
-	int i, ret;
-
-	ret = i2c_smbus_read_byte_data(client, LM95234_REG_CONVRATE);
-	if (ret < 0)
-		return ret;
-
-	data->interval = msecs_to_jiffies(update_intervals[ret & 0x03]);
-
-	for (i = 0; i < ARRAY_SIZE(data->tcrit1); i++) {
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_TCRIT1(i));
-		if (ret < 0)
-			return ret;
-		data->tcrit1[i] = ret;
-	}
-	for (i = 0; i < ARRAY_SIZE(data->tcrit2); i++) {
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_TCRIT2(i));
-		if (ret < 0)
-			return ret;
-		data->tcrit2[i] = ret;
-	}
-	for (i = 0; i < ARRAY_SIZE(data->toffset); i++) {
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_OFFSET(i));
-		if (ret < 0)
-			return ret;
-		data->toffset[i] = ret;
-	}
-
-	ret = i2c_smbus_read_byte_data(client, LM95234_REG_TCRIT_HYST);
-	if (ret < 0)
-		return ret;
-	data->thyst = ret;
-
-	ret = i2c_smbus_read_byte_data(client, LM95234_REG_REM_MODEL);
-	if (ret < 0)
-		return ret;
-	data->sensor_type = ret;
-
-	return 0;
-}
-
-static int lm95234_update_device(struct lm95234_data *data)
-{
-	struct i2c_client *client = data->client;
-	int ret;
-
-	mutex_lock(&data->update_lock);
-
-	if (time_after(jiffies, data->last_updated + data->interval) ||
-	    !data->valid) {
-		int i;
-
-		if (!data->valid) {
-			ret = lm95234_fill_cache(data, client);
-			if (ret < 0)
-				goto abort;
-		}
-
-		data->valid = false;
-		for (i = 0; i < ARRAY_SIZE(data->temp); i++) {
-			ret = lm95234_read_temp(client, i, &data->temp[i]);
-			if (ret < 0)
-				goto abort;
-		}
-
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_STS_FAULT);
-		if (ret < 0)
-			goto abort;
-		data->status = ret;
-
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_STS_TCRIT1);
-		if (ret < 0)
-			goto abort;
-		data->status |= ret << 8;
-
-		ret = i2c_smbus_read_byte_data(client, LM95234_REG_STS_TCRIT2);
-		if (ret < 0)
-			goto abort;
-		data->status |= ret << 16;
-
-		data->last_updated = jiffies;
-		data->valid = true;
-	}
-	ret = 0;
-abort:
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
 static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	int ret, temp;
 
+	ret = lm95234_read_temp(data->regmap, index, &temp);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%d\n",
-		       DIV_ROUND_CLOSEST(data->temp[index] * 125, 32));
+	return sysfs_emit(buf, "%d\n", temp);
 }
 
 static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
-	u32 mask = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	u8 mask = to_sensor_dev_attr_2(attr)->index;
+	u8 reg = to_sensor_dev_attr_2(attr)->nr;
+	int ret;
+	u32 val;
 
+	ret = regmap_read(data->regmap, reg, &val);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%u", !!(data->status & mask));
+	return sysfs_emit(buf, "%u\n", !!(val & mask));
 }
 
 static ssize_t type_show(struct device *dev, struct device_attribute *attr,
@@ -235,24 +128,23 @@ static ssize_t type_show(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	u8 mask = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	u32 val;
+	int ret;
 
+	ret = regmap_read(data->regmap, LM95234_REG_REM_MODEL, &val);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, data->sensor_type & mask ? "1\n" : "2\n");
+	return sysfs_emit(buf, "%s\n", val & mask ? "1" : "2");
 }
 
 static ssize_t type_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
-	unsigned long val;
 	u8 mask = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
-
-	if (ret)
-		return ret;
+	unsigned long val;
+	int ret;
 
 	ret = kstrtoul(buf, 10, &val);
 	if (ret < 0)
@@ -261,16 +153,10 @@ static ssize_t type_store(struct device *dev, struct device_attribute *attr,
 	if (val != 1 && val != 2)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	if (val == 1)
-		data->sensor_type |= mask;
-	else
-		data->sensor_type &= ~mask;
-	data->valid = false;
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_REM_MODEL,
-				  data->sensor_type);
-	mutex_unlock(&data->update_lock);
-
+	ret = regmap_update_bits(data->regmap, LM95234_REG_REM_MODEL,
+				 mask, val == 1 ? mask : 0);
+	if (ret)
+		return ret;
 	return count;
 }
 
@@ -279,12 +165,14 @@ static ssize_t tcrit2_show(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	int ret;
+	u32 tcrit2;
 
+	ret = regmap_read(data->regmap, LM95234_REG_TCRIT2(index), &tcrit2);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%u", data->tcrit2[index] * 1000);
+	return sysfs_emit(buf, "%u\n", tcrit2 * 1000);
 }
 
 static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
@@ -293,10 +181,7 @@ static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
 	long val;
-	int ret = lm95234_update_device(data);
-
-	if (ret)
-		return ret;
+	int ret;
 
 	ret = kstrtol(buf, 10, &val);
 	if (ret < 0)
@@ -305,27 +190,38 @@ static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
 	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
 				1000);
 
-	mutex_lock(&data->update_lock);
-	data->tcrit2[index] = val;
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_TCRIT2(index), val);
-	mutex_unlock(&data->update_lock);
-
+	ret = regmap_write(data->regmap, LM95234_REG_TCRIT2(index), val);
+	if (ret)
+		return ret;
 	return count;
 }
 
+static ssize_t tcrit_hyst_show(struct lm95234_data *data, char *buf, int reg)
+{
+	u32 thyst, tcrit;
+	int ret;
+
+	mutex_lock(&data->update_lock);
+	ret = regmap_read(data->regmap, reg, &tcrit);
+	if (ret)
+		goto unlock;
+	ret = regmap_read(data->regmap, LM95234_REG_TCRIT_HYST, &thyst);
+unlock:
+	mutex_unlock(&data->update_lock);
+	if (ret)
+		return ret;
+
+	/* Result can be negative, so be careful with unsigned operands */
+	return sysfs_emit(buf, "%d\n", ((int)tcrit - (int)thyst) * 1000);
+}
+
 static ssize_t tcrit2_hyst_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
 
-	if (ret)
-		return ret;
-
-	/* Result can be negative, so be careful with unsigned operands */
-	return sprintf(buf, "%d",
-		       ((int)data->tcrit2[index] - (int)data->thyst) * 1000);
+	return tcrit_hyst_show(data, buf, LM95234_REG_TCRIT2(index));
 }
 
 static ssize_t tcrit1_show(struct device *dev, struct device_attribute *attr,
@@ -333,8 +229,14 @@ static ssize_t tcrit1_show(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
+	int ret;
+	u32 val;
 
-	return sprintf(buf, "%u", data->tcrit1[index] * 1000);
+	ret = regmap_read(data->regmap, LM95234_REG_TCRIT1(index), &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", val * 1000);
 }
 
 static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
@@ -342,11 +244,8 @@ static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
 	long val;
-
-	if (ret)
-		return ret;
+	int ret;
 
 	ret = kstrtol(buf, 10, &val);
 	if (ret < 0)
@@ -354,10 +253,9 @@ static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
 
 	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
 
-	mutex_lock(&data->update_lock);
-	data->tcrit1[index] = val;
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_TCRIT1(index), val);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_write(data->regmap, LM95234_REG_TCRIT1(index), val);
+	if (ret)
+		return ret;
 
 	return count;
 }
@@ -367,14 +265,8 @@ static ssize_t tcrit1_hyst_show(struct device *dev,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
 
-	if (ret)
-		return ret;
-
-	/* Result can be negative, so be careful with unsigned operands */
-	return sprintf(buf, "%d",
-		       ((int)data->tcrit1[index] - (int)data->thyst) * 1000);
+	return tcrit_hyst_show(data, buf, LM95234_REG_TCRIT1(index));
 }
 
 static ssize_t tcrit1_hyst_store(struct device *dev,
@@ -383,23 +275,28 @@ static ssize_t tcrit1_hyst_store(struct device *dev,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	u32 tcrit;
 	long val;
-
-	if (ret)
-		return ret;
+	int ret;
 
 	ret = kstrtol(buf, 10, &val);
 	if (ret < 0)
 		return ret;
 
-	val = DIV_ROUND_CLOSEST(clamp_val(val, -255000, 255000), 1000);
-	val = clamp_val((int)data->tcrit1[index] - val, 0, 31);
-
 	mutex_lock(&data->update_lock);
-	data->thyst = val;
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_TCRIT_HYST, val);
+
+	ret = regmap_read(data->regmap, LM95234_REG_TCRIT1(index), &tcrit);
+	if (ret)
+		goto unlock;
+
+	val = DIV_ROUND_CLOSEST(clamp_val(val, -255000, 255000), 1000);
+	val = clamp_val((int)tcrit - val, 0, 31);
+
+	ret = regmap_write(data->regmap, LM95234_REG_TCRIT_HYST, val);
+unlock:
 	mutex_unlock(&data->update_lock);
+	if (ret)
+		return ret;
 
 	return count;
 }
@@ -409,12 +306,14 @@ static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
+	u32 offset;
+	int ret;
 
+	ret = regmap_read(data->regmap, LM95234_REG_OFFSET(index), &offset);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%d", data->toffset[index] * 500);
+	return sysfs_emit(buf, "%d\n", sign_extend32(offset, 7) * 500);
 }
 
 static ssize_t offset_store(struct device *dev, struct device_attribute *attr,
@@ -422,11 +321,8 @@ static ssize_t offset_store(struct device *dev, struct device_attribute *attr,
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	int index = to_sensor_dev_attr(attr)->index;
-	int ret = lm95234_update_device(data);
 	long val;
-
-	if (ret)
-		return ret;
+	int ret;
 
 	ret = kstrtol(buf, 10, &val);
 	if (ret < 0)
@@ -435,25 +331,27 @@ static ssize_t offset_store(struct device *dev, struct device_attribute *attr,
 	/* Accuracy is 1/2 degrees C */
 	val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
 
-	mutex_lock(&data->update_lock);
-	data->toffset[index] = val;
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_OFFSET(index), val);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_write(data->regmap, LM95234_REG_OFFSET(index), val);
+	if (ret < 0)
+		return ret;
 
 	return count;
 }
 
+static u16 update_intervals[] = { 143, 364, 1000, 2500 };
+
 static ssize_t update_interval_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
-	int ret = lm95234_update_device(data);
+	u32 convrate;
+	int ret;
 
+	ret = regmap_read(data->regmap, LM95234_REG_CONVRATE, &convrate);
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%lu\n",
-		       DIV_ROUND_CLOSEST(data->interval * 1000, HZ));
+	return sysfs_emit(buf, "%u\n", update_intervals[convrate & 0x03]);
 }
 
 static ssize_t update_interval_store(struct device *dev,
@@ -461,23 +359,17 @@ static ssize_t update_interval_store(struct device *dev,
 				     const char *buf, size_t count)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
-	int ret = lm95234_update_device(data);
 	unsigned long val;
-	u8 regval;
-
-	if (ret)
-		return ret;
+	int ret;
 
 	ret = kstrtoul(buf, 10, &val);
 	if (ret < 0)
 		return ret;
 
-	regval = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
-
-	mutex_lock(&data->update_lock);
-	data->interval = msecs_to_jiffies(update_intervals[regval]);
-	i2c_smbus_write_byte_data(data->client, LM95234_REG_CONVRATE, regval);
-	mutex_unlock(&data->update_lock);
+	val = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
+	ret = regmap_write(data->regmap, LM95234_REG_CONVRATE, val);
+	if (ret)
+		return ret;
 
 	return count;
 }
@@ -488,10 +380,10 @@ static SENSOR_DEVICE_ATTR_RO(temp3_input, temp, 2);
 static SENSOR_DEVICE_ATTR_RO(temp4_input, temp, 3);
 static SENSOR_DEVICE_ATTR_RO(temp5_input, temp, 4);
 
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, alarm, BIT(0) | BIT(1));
-static SENSOR_DEVICE_ATTR_RO(temp3_fault, alarm, BIT(2) | BIT(3));
-static SENSOR_DEVICE_ATTR_RO(temp4_fault, alarm, BIT(4) | BIT(5));
-static SENSOR_DEVICE_ATTR_RO(temp5_fault, alarm, BIT(6) | BIT(7));
+static SENSOR_DEVICE_ATTR_2_RO(temp2_fault, alarm, LM95234_REG_STS_FAULT, BIT(0) | BIT(1));
+static SENSOR_DEVICE_ATTR_2_RO(temp3_fault, alarm, LM95234_REG_STS_FAULT, BIT(2) | BIT(3));
+static SENSOR_DEVICE_ATTR_2_RO(temp4_fault, alarm, LM95234_REG_STS_FAULT, BIT(4) | BIT(5));
+static SENSOR_DEVICE_ATTR_2_RO(temp5_fault, alarm, LM95234_REG_STS_FAULT, BIT(6) | BIT(7));
 
 static SENSOR_DEVICE_ATTR_RW(temp2_type, type, BIT(1));
 static SENSOR_DEVICE_ATTR_RW(temp3_type, type, BIT(2));
@@ -510,11 +402,11 @@ static SENSOR_DEVICE_ATTR_RO(temp3_max_hyst, tcrit2_hyst, 1);
 static SENSOR_DEVICE_ATTR_RO(temp4_max_hyst, tcrit1_hyst, 3);
 static SENSOR_DEVICE_ATTR_RO(temp5_max_hyst, tcrit1_hyst, 4);
 
-static SENSOR_DEVICE_ATTR_RO(temp1_max_alarm, alarm, BIT(0 + 8));
-static SENSOR_DEVICE_ATTR_RO(temp2_max_alarm, alarm, BIT(1 + 16));
-static SENSOR_DEVICE_ATTR_RO(temp3_max_alarm, alarm, BIT(2 + 16));
-static SENSOR_DEVICE_ATTR_RO(temp4_max_alarm, alarm, BIT(3 + 8));
-static SENSOR_DEVICE_ATTR_RO(temp5_max_alarm, alarm, BIT(4 + 8));
+static SENSOR_DEVICE_ATTR_2_RO(temp1_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(0));
+static SENSOR_DEVICE_ATTR_2_RO(temp2_max_alarm, alarm, LM95234_REG_STS_TCRIT2, BIT(1));
+static SENSOR_DEVICE_ATTR_2_RO(temp3_max_alarm, alarm, LM95234_REG_STS_TCRIT2, BIT(2));
+static SENSOR_DEVICE_ATTR_2_RO(temp4_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(3));
+static SENSOR_DEVICE_ATTR_2_RO(temp5_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(4));
 
 static SENSOR_DEVICE_ATTR_RW(temp2_crit, tcrit1, 1);
 static SENSOR_DEVICE_ATTR_RW(temp3_crit, tcrit1, 2);
@@ -522,8 +414,8 @@ static SENSOR_DEVICE_ATTR_RW(temp3_crit, tcrit1, 2);
 static SENSOR_DEVICE_ATTR_RO(temp2_crit_hyst, tcrit1_hyst, 1);
 static SENSOR_DEVICE_ATTR_RO(temp3_crit_hyst, tcrit1_hyst, 2);
 
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_alarm, alarm, BIT(1 + 8));
-static SENSOR_DEVICE_ATTR_RO(temp3_crit_alarm, alarm, BIT(2 + 8));
+static SENSOR_DEVICE_ATTR_2_RO(temp2_crit_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(1));
+static SENSOR_DEVICE_ATTR_2_RO(temp3_crit_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(2));
 
 static SENSOR_DEVICE_ATTR_RW(temp2_offset, offset, 0);
 static SENSOR_DEVICE_ATTR_RW(temp3_offset, offset, 1);
@@ -587,6 +479,45 @@ static const struct attribute_group lm95234_group = {
 	.attrs = lm95234_attrs,
 };
 
+static bool lm95234_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LM95234_REG_TEMPH(0) ... LM95234_REG_TEMPH(4):
+	case LM95234_REG_TEMPL(0) ... LM95234_REG_TEMPL(4):
+	case LM95234_REG_UTEMPH(0) ... LM95234_REG_UTEMPH(3):
+	case LM95234_REG_UTEMPL(0) ... LM95234_REG_UTEMPL(3):
+	case LM95234_REG_STS_FAULT:
+	case LM95234_REG_STS_TCRIT1:
+	case LM95234_REG_STS_TCRIT2:
+	case LM95234_REG_REM_MODEL_STS:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool lm95234_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LM95234_REG_CONFIG ... LM95234_REG_FILTER:
+	case LM95234_REG_REM_MODEL ... LM95234_REG_OFFSET(3):
+	case LM95234_REG_TCRIT1(0) ... LM95234_REG_TCRIT1(4):
+	case LM95234_REG_TCRIT2(0) ... LM95234_REG_TCRIT2(1):
+	case LM95234_REG_TCRIT_HYST:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config lm95234_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.writeable_reg = lm95234_writeable_reg,
+	.volatile_reg = lm95234_volatile_reg,
+	.cache_type = REGCACHE_MAPLE,
+};
+
 static int lm95234_detect(struct i2c_client *client,
 			  struct i2c_board_info *info)
 {
@@ -647,33 +578,30 @@ static int lm95234_detect(struct i2c_client *client,
 	return 0;
 }
 
-static int lm95234_init_client(struct i2c_client *client)
+static int lm95234_init_client(struct device *dev, struct regmap *regmap)
 {
-	int val, model;
+	u32 val, model;
+	int ret;
 
 	/* start conversion if necessary */
-	val = i2c_smbus_read_byte_data(client, LM95234_REG_CONFIG);
-	if (val < 0)
-		return val;
-	if (val & 0x40)
-		i2c_smbus_write_byte_data(client, LM95234_REG_CONFIG,
-					  val & ~0x40);
+	ret = regmap_clear_bits(regmap, LM95234_REG_CONFIG, 0x40);
+	if (ret)
+		return ret;
 
 	/* If diode type status reports an error, try to fix it */
-	val = i2c_smbus_read_byte_data(client, LM95234_REG_REM_MODEL_STS);
-	if (val < 0)
-		return val;
-	model = i2c_smbus_read_byte_data(client, LM95234_REG_REM_MODEL);
-	if (model < 0)
-		return model;
+	ret = regmap_read(regmap, LM95234_REG_REM_MODEL_STS, &val);
+	if (ret < 0)
+		return ret;
+	ret = regmap_read(regmap, LM95234_REG_REM_MODEL, &model);
+	if (ret < 0)
+		return ret;
 	if (model & val) {
-		dev_notice(&client->dev,
+		dev_notice(dev,
 			   "Fixing remote diode type misconfiguration (0x%x)\n",
 			   val);
-		i2c_smbus_write_byte_data(client, LM95234_REG_REM_MODEL,
-					  model & ~val);
+		ret = regmap_write(regmap, LM95234_REG_REM_MODEL, model & ~val);
 	}
-	return 0;
+	return ret;
 }
 
 static int lm95234_probe(struct i2c_client *client)
@@ -682,17 +610,22 @@ static int lm95234_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct lm95234_data *data;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
 	int err;
 
 	data = devm_kzalloc(dev, sizeof(struct lm95234_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->client = client;
+	regmap = devm_regmap_init_i2c(client, &lm95234_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data->regmap = regmap;
 	mutex_init(&data->update_lock);
 
 	/* Initialize the LM95234 chip */
-	err = lm95234_init_client(client);
+	err = lm95234_init_client(dev, regmap);
 	if (err < 0)
 		return err;
 
-- 
2.39.2

Re: [PATCH 3/6] hwmon: (lm95234) Convert to use regmap

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:32PM -0700, Guenter Roeck wrote:
> Use regmap to replace local caching and to be able to use regmap API
> functions.
> 
> No functional change.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

[PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Guenter Roeck]

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

This patch reduces the object file size by about 30%.

No functional change.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm95234.c | 557 ++++++++++++++++------------------------
 1 file changed, 227 insertions(+), 330 deletions(-)

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index 7a3aff1d183a..1a164f47fb3e 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -10,14 +10,12 @@
 
 #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>
 #include <linux/slab.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
-#include <linux/sysfs.h>
 #include <linux/util_macros.h>
 
 #define DRVNAME "lm95234"
@@ -56,11 +54,11 @@ static const unsigned short normal_i2c[] = {
 /* Client data (each client gets its own) */
 struct lm95234_data {
 	struct regmap *regmap;
-	const struct attribute_group *groups[3];
 	struct mutex update_lock;
+	enum chips type;
 };
 
-static int lm95234_read_temp(struct regmap *regmap, int index, int *t)
+static int lm95234_read_temp(struct regmap *regmap, int index, long *t)
 {
 	int temp = 0, ret;
 	u32 val;
@@ -93,110 +91,7 @@ static int lm95234_read_temp(struct regmap *regmap, int index, int *t)
 	return 0;
 }
 
-static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	int ret, temp;
-
-	ret = lm95234_read_temp(data->regmap, index, &temp);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%d\n", temp);
-}
-
-static ssize_t alarm_show(struct device *dev, struct device_attribute *attr,
-			  char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	u8 mask = to_sensor_dev_attr_2(attr)->index;
-	u8 reg = to_sensor_dev_attr_2(attr)->nr;
-	int ret;
-	u32 val;
-
-	ret = regmap_read(data->regmap, reg, &val);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%u\n", !!(val & mask));
-}
-
-static ssize_t type_show(struct device *dev, struct device_attribute *attr,
-			 char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	u8 mask = to_sensor_dev_attr(attr)->index;
-	u32 val;
-	int ret;
-
-	ret = regmap_read(data->regmap, LM95234_REG_REM_MODEL, &val);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%s\n", val & mask ? "1" : "2");
-}
-
-static ssize_t type_store(struct device *dev, struct device_attribute *attr,
-			  const char *buf, size_t count)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	u8 mask = to_sensor_dev_attr(attr)->index;
-	unsigned long val;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
-	if (val != 1 && val != 2)
-		return -EINVAL;
-
-	ret = regmap_update_bits(data->regmap, LM95234_REG_REM_MODEL,
-				 mask, val == 1 ? mask : 0);
-	if (ret)
-		return ret;
-	return count;
-}
-
-static ssize_t tcrit2_show(struct device *dev, struct device_attribute *attr,
-			   char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	int ret;
-	u32 tcrit2;
-
-	ret = regmap_read(data->regmap, LM95234_REG_TCRIT2(index), &tcrit2);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%u\n", tcrit2 * 1000);
-}
-
-static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
-			    const char *buf, size_t count)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	long val;
-	int ret;
-
-	ret = kstrtol(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
-	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
-				1000);
-
-	ret = regmap_write(data->regmap, LM95234_REG_TCRIT2(index), val);
-	if (ret)
-		return ret;
-	return count;
-}
-
-static ssize_t tcrit_hyst_show(struct lm95234_data *data, char *buf, int reg)
+static int lm95234_hyst_get(struct lm95234_data *data, int reg, long *val)
 {
 	u32 thyst, tcrit;
 	int ret;
@@ -212,80 +107,18 @@ static ssize_t tcrit_hyst_show(struct lm95234_data *data, char *buf, int reg)
 		return ret;
 
 	/* Result can be negative, so be careful with unsigned operands */
-	return sysfs_emit(buf, "%d\n", ((int)tcrit - (int)thyst) * 1000);
+	*val = ((int)tcrit - (int)thyst) * 1000;
+	return 0;
 }
 
-static ssize_t tcrit2_hyst_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
+static ssize_t lm95234_hyst_set(struct lm95234_data *data, long val)
 {
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-
-	return tcrit_hyst_show(data, buf, LM95234_REG_TCRIT2(index));
-}
-
-static ssize_t tcrit1_show(struct device *dev, struct device_attribute *attr,
-			   char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	int ret;
-	u32 val;
-
-	ret = regmap_read(data->regmap, LM95234_REG_TCRIT1(index), &val);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%u\n", val * 1000);
-}
-
-static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
-			    const char *buf, size_t count)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	long val;
-	int ret;
-
-	ret = kstrtol(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
-	val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
-
-	ret = regmap_write(data->regmap, LM95234_REG_TCRIT1(index), val);
-	if (ret)
-		return ret;
-
-	return count;
-}
-
-static ssize_t tcrit1_hyst_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-
-	return tcrit_hyst_show(data, buf, LM95234_REG_TCRIT1(index));
-}
-
-static ssize_t tcrit1_hyst_store(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t count)
-{
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
 	u32 tcrit;
-	long val;
 	int ret;
 
-	ret = kstrtol(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
 	mutex_lock(&data->update_lock);
 
-	ret = regmap_read(data->regmap, LM95234_REG_TCRIT1(index), &tcrit);
+	ret = regmap_read(data->regmap, LM95234_REG_TCRIT1(0), &tcrit);
 	if (ret)
 		goto unlock;
 
@@ -295,188 +128,255 @@ static ssize_t tcrit1_hyst_store(struct device *dev,
 	ret = regmap_write(data->regmap, LM95234_REG_TCRIT_HYST, val);
 unlock:
 	mutex_unlock(&data->update_lock);
-	if (ret)
-		return ret;
-
-	return count;
+	return ret;
 }
 
-static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
-			   char *buf)
+static int lm95234_crit_reg(int channel)
 {
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	u32 offset;
-	int ret;
-
-	ret = regmap_read(data->regmap, LM95234_REG_OFFSET(index), &offset);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%d\n", sign_extend32(offset, 7) * 500);
+	if (channel == 1 || channel == 2)
+		return LM95234_REG_TCRIT2(channel - 1);
+	return LM95234_REG_TCRIT1(channel);
 }
 
-static ssize_t offset_store(struct device *dev, struct device_attribute *attr,
-			    const char *buf, size_t count)
+static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
-	int index = to_sensor_dev_attr(attr)->index;
-	long val;
+	struct regmap *regmap = data->regmap;
+
+	switch (attr) {
+	case hwmon_temp_type:
+		if (val != 1 && val != 2)
+			return -EINVAL;
+		return regmap_update_bits(regmap, LM95234_REG_REM_MODEL,
+					  BIT(channel),
+					  val == 1 ? BIT(channel) : 0);
+	case hwmon_temp_offset:
+		val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
+		return regmap_write(regmap, LM95234_REG_OFFSET(channel - 1), val);
+	case hwmon_temp_max:
+		val = clamp_val(val, 0, channel ? 255000 : 127000);
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		return regmap_write(regmap, lm95234_crit_reg(channel), val);
+	case hwmon_temp_max_hyst:
+		return lm95234_hyst_set(data, val);
+	case hwmon_temp_crit:
+		val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
+		return regmap_write(regmap, LM95234_REG_TCRIT1(channel), val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int lm95234_alarm_reg(int channel)
+{
+	if (channel == 1 || channel == 2)
+		return LM95234_REG_STS_TCRIT2;
+	return LM95234_REG_STS_TCRIT1;
+}
+
+static int lm95234_temp_read(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct lm95234_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	u32 regval, mask;
 	int ret;
 
-	ret = kstrtol(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
-	/* Accuracy is 1/2 degrees C */
-	val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
-
-	ret = regmap_write(data->regmap, LM95234_REG_OFFSET(index), val);
-	if (ret < 0)
-		return ret;
-
-	return count;
+	switch (attr) {
+	case hwmon_temp_input:
+		return lm95234_read_temp(regmap, channel, val);
+	case hwmon_temp_max_alarm:
+		ret =  regmap_read(regmap, lm95234_alarm_reg(channel), &regval);
+		if (ret)
+			return ret;
+		*val = !!(regval & BIT(channel));
+		break;
+	case hwmon_temp_crit_alarm:
+		ret =  regmap_read(regmap, LM95234_REG_STS_TCRIT1, &regval);
+		if (ret)
+			return ret;
+		*val = !!(regval & BIT(channel));
+		break;
+	case hwmon_temp_crit_hyst:
+		return lm95234_hyst_get(data, LM95234_REG_TCRIT1(channel), val);
+	case hwmon_temp_type:
+		ret = regmap_read(regmap, LM95234_REG_REM_MODEL, &regval);
+		if (ret)
+			return ret;
+		*val = (regval & BIT(channel)) ? 1 : 2;
+		break;
+	case hwmon_temp_offset:
+		ret = regmap_read(regmap, LM95234_REG_OFFSET(channel - 1), &regval);
+		if (ret)
+			return ret;
+		*val = sign_extend32(regval, 7) * 500;
+		break;
+	case hwmon_temp_fault:
+		ret = regmap_read(regmap, LM95234_REG_STS_FAULT, &regval);
+		if (ret)
+			return ret;
+		mask = (BIT(0) | BIT(1)) << ((channel - 1) << 1);
+		*val = !!(regval & mask);
+		break;
+	case hwmon_temp_max:
+		ret = regmap_read(regmap, lm95234_crit_reg(channel), &regval);
+		if (ret)
+			return ret;
+		*val = regval * 1000;
+		break;
+	case hwmon_temp_max_hyst:
+		return lm95234_hyst_get(data, lm95234_crit_reg(channel), val);
+	case hwmon_temp_crit:
+		ret = regmap_read(regmap, LM95234_REG_TCRIT1(channel), &regval);
+		if (ret)
+			return ret;
+		*val = regval * 1000;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
 }
 
 static u16 update_intervals[] = { 143, 364, 1000, 2500 };
 
-static ssize_t update_interval_show(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static int lm95234_chip_write(struct device *dev, u32 attr, long val)
+{
+	struct lm95234_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		val = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
+		return regmap_write(data->regmap, LM95234_REG_CONVRATE, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int lm95234_chip_read(struct device *dev, u32 attr, long *val)
 {
 	struct lm95234_data *data = dev_get_drvdata(dev);
 	u32 convrate;
 	int ret;
 
-	ret = regmap_read(data->regmap, LM95234_REG_CONVRATE, &convrate);
-	if (ret)
-		return ret;
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		ret = regmap_read(data->regmap, LM95234_REG_CONVRATE, &convrate);
+		if (ret)
+			return ret;
 
-	return sysfs_emit(buf, "%u\n", update_intervals[convrate & 0x03]);
+		*val = update_intervals[convrate & 0x03];
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
 }
 
-static ssize_t update_interval_store(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t count)
+static int lm95234_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
 {
-	struct lm95234_data *data = dev_get_drvdata(dev);
-	unsigned long val;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-
-	val = find_closest(val, update_intervals, ARRAY_SIZE(update_intervals));
-	ret = regmap_write(data->regmap, LM95234_REG_CONVRATE, val);
-	if (ret)
-		return ret;
-
-	return count;
+	switch (type) {
+	case hwmon_chip:
+		return lm95234_chip_write(dev, attr, val);
+	case hwmon_temp:
+		return lm95234_temp_write(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
 }
 
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_input, temp, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_input, temp, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_input, temp, 4);
+static int lm95234_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return lm95234_chip_read(dev, attr, val);
+	case hwmon_temp:
+		return lm95234_temp_read(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
 
-static SENSOR_DEVICE_ATTR_2_RO(temp2_fault, alarm, LM95234_REG_STS_FAULT, BIT(0) | BIT(1));
-static SENSOR_DEVICE_ATTR_2_RO(temp3_fault, alarm, LM95234_REG_STS_FAULT, BIT(2) | BIT(3));
-static SENSOR_DEVICE_ATTR_2_RO(temp4_fault, alarm, LM95234_REG_STS_FAULT, BIT(4) | BIT(5));
-static SENSOR_DEVICE_ATTR_2_RO(temp5_fault, alarm, LM95234_REG_STS_FAULT, BIT(6) | BIT(7));
+static umode_t lm95234_is_visible(const void *_data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	const struct lm95234_data *data = _data;
 
-static SENSOR_DEVICE_ATTR_RW(temp2_type, type, BIT(1));
-static SENSOR_DEVICE_ATTR_RW(temp3_type, type, BIT(2));
-static SENSOR_DEVICE_ATTR_RW(temp4_type, type, BIT(3));
-static SENSOR_DEVICE_ATTR_RW(temp5_type, type, BIT(4));
+	if (data->type == lm95233 && channel > 2)
+		return 0;
 
-static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0);
-static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0);
-static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1);
-static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3);
-static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4);
+	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_max_alarm:
+			return 0444;
+		case hwmon_temp_crit_alarm:
+		case hwmon_temp_crit_hyst:
+			return (channel && channel < 3) ? 0444 : 0;
+		case hwmon_temp_type:
+		case hwmon_temp_offset:
+			return channel ? 0644 : 0;
+		case hwmon_temp_fault:
+			return channel ? 0444 : 0;
+		case hwmon_temp_max:
+			return 0644;
+		case hwmon_temp_max_hyst:
+			return channel ? 0444 : 0644;
+		case hwmon_temp_crit:
+			return (channel && channel < 3) ? 0644 : 0;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
 
-static SENSOR_DEVICE_ATTR_RW(temp1_max_hyst, tcrit1_hyst, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_max_hyst, tcrit2_hyst, 0);
-static SENSOR_DEVICE_ATTR_RO(temp3_max_hyst, tcrit2_hyst, 1);
-static SENSOR_DEVICE_ATTR_RO(temp4_max_hyst, tcrit1_hyst, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_max_hyst, tcrit1_hyst, 4);
-
-static SENSOR_DEVICE_ATTR_2_RO(temp1_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(0));
-static SENSOR_DEVICE_ATTR_2_RO(temp2_max_alarm, alarm, LM95234_REG_STS_TCRIT2, BIT(1));
-static SENSOR_DEVICE_ATTR_2_RO(temp3_max_alarm, alarm, LM95234_REG_STS_TCRIT2, BIT(2));
-static SENSOR_DEVICE_ATTR_2_RO(temp4_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(3));
-static SENSOR_DEVICE_ATTR_2_RO(temp5_max_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(4));
-
-static SENSOR_DEVICE_ATTR_RW(temp2_crit, tcrit1, 1);
-static SENSOR_DEVICE_ATTR_RW(temp3_crit, tcrit1, 2);
-
-static SENSOR_DEVICE_ATTR_RO(temp2_crit_hyst, tcrit1_hyst, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_crit_hyst, tcrit1_hyst, 2);
-
-static SENSOR_DEVICE_ATTR_2_RO(temp2_crit_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(1));
-static SENSOR_DEVICE_ATTR_2_RO(temp3_crit_alarm, alarm, LM95234_REG_STS_TCRIT1, BIT(2));
-
-static SENSOR_DEVICE_ATTR_RW(temp2_offset, offset, 0);
-static SENSOR_DEVICE_ATTR_RW(temp3_offset, offset, 1);
-static SENSOR_DEVICE_ATTR_RW(temp4_offset, offset, 2);
-static SENSOR_DEVICE_ATTR_RW(temp5_offset, offset, 3);
-
-static DEVICE_ATTR_RW(update_interval);
-
-static struct attribute *lm95234_common_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp3_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_fault.dev_attr.attr,
-	&sensor_dev_attr_temp3_fault.dev_attr.attr,
-	&sensor_dev_attr_temp2_type.dev_attr.attr,
-	&sensor_dev_attr_temp3_type.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
-	&sensor_dev_attr_temp2_max.dev_attr.attr,
-	&sensor_dev_attr_temp3_max.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp3_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit.dev_attr.attr,
-	&sensor_dev_attr_temp3_crit.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp2_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp2_offset.dev_attr.attr,
-	&sensor_dev_attr_temp3_offset.dev_attr.attr,
-	&dev_attr_update_interval.attr,
+static const struct hwmon_channel_info * const lm95234_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_MAX_ALARM,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST |
+			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
+			   HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
+			   HWMON_T_OFFSET),
 	NULL
 };
 
-static const struct attribute_group lm95234_common_group = {
-	.attrs = lm95234_common_attrs,
+static const struct hwmon_ops lm95234_hwmon_ops = {
+	.is_visible = lm95234_is_visible,
+	.read = lm95234_read,
+	.write = lm95234_write,
 };
 
-static struct attribute *lm95234_attrs[] = {
-	&sensor_dev_attr_temp4_input.dev_attr.attr,
-	&sensor_dev_attr_temp5_input.dev_attr.attr,
-	&sensor_dev_attr_temp4_fault.dev_attr.attr,
-	&sensor_dev_attr_temp5_fault.dev_attr.attr,
-	&sensor_dev_attr_temp4_type.dev_attr.attr,
-	&sensor_dev_attr_temp5_type.dev_attr.attr,
-	&sensor_dev_attr_temp4_max.dev_attr.attr,
-	&sensor_dev_attr_temp5_max.dev_attr.attr,
-	&sensor_dev_attr_temp4_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp5_max_hyst.dev_attr.attr,
-	&sensor_dev_attr_temp4_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp5_max_alarm.dev_attr.attr,
-	&sensor_dev_attr_temp4_offset.dev_attr.attr,
-	&sensor_dev_attr_temp5_offset.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group lm95234_group = {
-	.attrs = lm95234_attrs,
+static const struct hwmon_chip_info lm95234_chip_info = {
+	.ops = &lm95234_hwmon_ops,
+	.info = lm95234_info,
 };
 
 static bool lm95234_volatile_reg(struct device *dev, unsigned int reg)
@@ -606,7 +506,6 @@ static int lm95234_init_client(struct device *dev, struct regmap *regmap)
 
 static int lm95234_probe(struct i2c_client *client)
 {
-	enum chips type = (uintptr_t)i2c_get_match_data(client);
 	struct device *dev = &client->dev;
 	struct lm95234_data *data;
 	struct device *hwmon_dev;
@@ -617,6 +516,8 @@ static int lm95234_probe(struct i2c_client *client)
 	if (!data)
 		return -ENOMEM;
 
+	data->type = (uintptr_t)i2c_get_match_data(client);
+
 	regmap = devm_regmap_init_i2c(client, &lm95234_regmap_config);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
@@ -629,12 +530,8 @@ static int lm95234_probe(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
-	data->groups[0] = &lm95234_common_group;
-	if (type == lm95234)
-		data->groups[1] = &lm95234_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, &lm95234_chip_info, NULL);
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-- 
2.39.2

Re: [PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:33PM -0700, Guenter Roeck wrote:
> +static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
>  {
[...]
> +	case hwmon_temp_max:
> +		val = clamp_val(val, 0, channel ? 255000 : 127000);

Perhaps I am misunderstanding, but this looks weird to me.  By applying
the patch, the maximum values are:

static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 127000
static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000


However, it was originally:

static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
	        const char *buf, size_t count)
{
[...]
    val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);

static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
	        const char *buf, size_t count)
{
[...]
    val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
		    1000);

static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 127000
static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000

> +		val = DIV_ROUND_CLOSEST(val, 1000);
> +		return regmap_write(regmap, lm95234_crit_reg(channel), val);

Re: [PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Guenter Roeck]

On 7/18/24 09:40, Tzung-Bi Shih wrote:
> On Wed, Jul 17, 2024 at 08:39:33PM -0700, Guenter Roeck wrote:
>> +static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
>>   {
> [...]
>> +	case hwmon_temp_max:
>> +		val = clamp_val(val, 0, channel ? 255000 : 127000);
> 
> Perhaps I am misunderstanding, but this looks weird to me.  By applying
> the patch, the maximum values are:
> 
> static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 127000
> static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> 
> 
> However, it was originally:
> 
> static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
> 	        const char *buf, size_t count)
> {
> [...]
>      val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
> 
> static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
> 	        const char *buf, size_t count)
> {
> [...]
>      val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
> 		    1000);
> 
> static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 127000
> static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> 
>> +		val = DIV_ROUND_CLOSEST(val, 1000);
>> +		return regmap_write(regmap, lm95234_crit_reg(channel), val);
> 

That is indeed a bug. Here is the fix:

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index c3c68c196479..7da6c8f07332 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -150,7 +150,7 @@ static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long va
                 val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
                 return regmap_write(regmap, LM95234_REG_OFFSET(channel - 1), val);
         case hwmon_temp_max:
-               val = clamp_val(val, 0, channel ? 255000 : 127000);
+               val = clamp_val(val, 0, channel == 1 ? 127000 : 255000);
                 val = DIV_ROUND_CLOSEST(val, 1000);
                 return regmap_write(regmap, lm95234_crit_reg(channel), val);
         case hwmon_temp_max_hyst:

Thanks a lot for the detailed review!

Guenter

Re: [PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Tzung-Bi Shih]

On Thu, Jul 18, 2024 at 10:47:55AM -0700, Guenter Roeck wrote:
> On 7/18/24 09:40, Tzung-Bi Shih wrote:
> > On Wed, Jul 17, 2024 at 08:39:33PM -0700, Guenter Roeck wrote:
> > > +static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
> > >   {
> > [...]
> > > +	case hwmon_temp_max:
> > > +		val = clamp_val(val, 0, channel ? 255000 : 127000);
> > 
> > Perhaps I am misunderstanding, but this looks weird to me.  By applying
> > the patch, the maximum values are:
> > 
> > static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 127000
> > static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> > 
> > 
> > However, it was originally:
> > 
> > static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
> > 	        const char *buf, size_t count)
> > {
> > [...]
> >      val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
> > 
> > static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
> > 	        const char *buf, size_t count)
> > {
> > [...]
> >      val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
> > 		    1000);
> > 
> > static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 127000
> > static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
> > static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
> > 
> > > +		val = DIV_ROUND_CLOSEST(val, 1000);
> > > +		return regmap_write(regmap, lm95234_crit_reg(channel), val);
> > 
> 
> That is indeed a bug. Here is the fix:
> 
> diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
> index c3c68c196479..7da6c8f07332 100644
> --- a/drivers/hwmon/lm95234.c
> +++ b/drivers/hwmon/lm95234.c
> @@ -150,7 +150,7 @@ static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long va
>                 val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
>                 return regmap_write(regmap, LM95234_REG_OFFSET(channel - 1), val);
>         case hwmon_temp_max:
> -               val = clamp_val(val, 0, channel ? 255000 : 127000);
> +               val = clamp_val(val, 0, channel == 1 ? 127000 : 255000);
>                 val = DIV_ROUND_CLOSEST(val, 1000);
>                 return regmap_write(regmap, lm95234_crit_reg(channel), val);
>         case hwmon_temp_max_hyst:

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

Re: [PATCH 4/6] hwmon: (lm95234) Convert to with_info hwmon API

[Guenter Roeck]

On 7/18/24 17:52, Tzung-Bi Shih wrote:
> On Thu, Jul 18, 2024 at 10:47:55AM -0700, Guenter Roeck wrote:
>> On 7/18/24 09:40, Tzung-Bi Shih wrote:
>>> On Wed, Jul 17, 2024 at 08:39:33PM -0700, Guenter Roeck wrote:
>>>> +static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long val)
>>>>    {
>>> [...]
>>>> +	case hwmon_temp_max:
>>>> +		val = clamp_val(val, 0, channel ? 255000 : 127000);
>>>
>>> Perhaps I am misunderstanding, but this looks weird to me.  By applying
>>> the patch, the maximum values are:
>>>
>>> static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 127000
>>> static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
>>>
>>>
>>> However, it was originally:
>>>
>>> static ssize_t tcrit1_store(struct device *dev, struct device_attribute *attr,
>>> 	        const char *buf, size_t count)
>>> {
>>> [...]
>>>       val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 255000), 1000);
>>>
>>> static ssize_t tcrit2_store(struct device *dev, struct device_attribute *attr,
>>> 	        const char *buf, size_t count)
>>> {
>>> [...]
>>>       val = DIV_ROUND_CLOSEST(clamp_val(val, 0, (index ? 255 : 127) * 1000),
>>> 		    1000);
>>>
>>> static SENSOR_DEVICE_ATTR_RW(temp1_max, tcrit1, 0); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp2_max, tcrit2, 0); -> 127000
>>> static SENSOR_DEVICE_ATTR_RW(temp3_max, tcrit2, 1); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp4_max, tcrit1, 3); -> 255000
>>> static SENSOR_DEVICE_ATTR_RW(temp5_max, tcrit1, 4); -> 255000
>>>
>>>> +		val = DIV_ROUND_CLOSEST(val, 1000);
>>>> +		return regmap_write(regmap, lm95234_crit_reg(channel), val);
>>>
>>
>> That is indeed a bug. Here is the fix:
>>
>> diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
>> index c3c68c196479..7da6c8f07332 100644
>> --- a/drivers/hwmon/lm95234.c
>> +++ b/drivers/hwmon/lm95234.c
>> @@ -150,7 +150,7 @@ static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long va
>>                  val = DIV_ROUND_CLOSEST(clamp_val(val, -64000, 63500), 500);
>>                  return regmap_write(regmap, LM95234_REG_OFFSET(channel - 1), val);
>>          case hwmon_temp_max:
>> -               val = clamp_val(val, 0, channel ? 255000 : 127000);
>> +               val = clamp_val(val, 0, channel == 1 ? 127000 : 255000);
>>                  val = DIV_ROUND_CLOSEST(val, 1000);
>>                  return regmap_write(regmap, lm95234_crit_reg(channel), val);
>>          case hwmon_temp_max_hyst:
> 
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
Thanks a lot for the reviews!

Guenter

[PATCH 5/6] hwmon: (lm95234) Add support for tempX_enable attribute

[Guenter Roeck]

LM95233/LM95234 support enabling temperature channels one by one.
Add support for tempX_enable attribute to be able to use that
functionality.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm95234.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index 1a164f47fb3e..f0df1134f811 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -144,6 +144,11 @@ static int lm95234_temp_write(struct device *dev, u32 attr, int channel, long va
 	struct regmap *regmap = data->regmap;
 
 	switch (attr) {
+	case hwmon_temp_enable:
+		if (val && val != 1)
+			return -EINVAL;
+		return regmap_update_bits(regmap, LM95234_REG_ENABLE,
+					  BIT(channel), val ? BIT(channel) : 0);
 	case hwmon_temp_type:
 		if (val != 1 && val != 2)
 			return -EINVAL;
@@ -183,6 +188,12 @@ static int lm95234_temp_read(struct device *dev, u32 attr, int channel, long *va
 	int ret;
 
 	switch (attr) {
+	case hwmon_temp_enable:
+		ret = regmap_read(regmap, LM95234_REG_ENABLE, &regval);
+		if (ret)
+			return ret;
+		*val = !!(regval & BIT(channel));
+		break;
 	case hwmon_temp_input:
 		return lm95234_read_temp(regmap, channel, val);
 	case hwmon_temp_max_alarm:
@@ -331,6 +342,7 @@ static umode_t lm95234_is_visible(const void *_data, enum hwmon_sensor_types typ
 		case hwmon_temp_fault:
 			return channel ? 0444 : 0;
 		case hwmon_temp_max:
+		case hwmon_temp_enable:
 			return 0644;
 		case hwmon_temp_max_hyst:
 			return channel ? 0444 : 0644;
@@ -350,21 +362,21 @@ static const struct hwmon_channel_info * const lm95234_info[] = {
 	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
 	HWMON_CHANNEL_INFO(temp,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
-			   HWMON_T_MAX_ALARM,
+			   HWMON_T_MAX_ALARM | HWMON_T_ENABLE,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
 			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
 			   HWMON_T_CRIT | HWMON_T_CRIT_HYST |
-			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET,
+			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET | HWMON_T_ENABLE,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
 			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
 			   HWMON_T_CRIT | HWMON_T_CRIT_HYST |
-			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET,
+			   HWMON_T_CRIT_ALARM | HWMON_T_OFFSET | HWMON_T_ENABLE,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
 			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
-			   HWMON_T_OFFSET,
+			   HWMON_T_OFFSET | HWMON_T_ENABLE,
 			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
 			   HWMON_T_MAX_ALARM | HWMON_T_FAULT | HWMON_T_TYPE |
-			   HWMON_T_OFFSET),
+			   HWMON_T_OFFSET | HWMON_T_ENABLE),
 	NULL
 };
 
-- 
2.39.2

Re: [PATCH 5/6] hwmon: (lm95234) Add support for tempX_enable attribute

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:34PM -0700, Guenter Roeck wrote:
> LM95233/LM95234 support enabling temperature channels one by one.
> Add support for tempX_enable attribute to be able to use that
> functionality.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

[PATCH 6/6] hwmon: (lm95234) Use multi-byte regmap operations

[Guenter Roeck]

Use  multi-byte regmap operations to simplify the code
and to reduce dependency on locking.

No functional change.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm95234.c | 45 +++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/lm95234.c b/drivers/hwmon/lm95234.c
index f0df1134f811..c3c68c196479 100644
--- a/drivers/hwmon/lm95234.c
+++ b/drivers/hwmon/lm95234.c
@@ -60,54 +60,45 @@ struct lm95234_data {
 
 static int lm95234_read_temp(struct regmap *regmap, int index, long *t)
 {
+	unsigned int regs[2];
 	int temp = 0, ret;
-	u32 val;
+	u8 regvals[2];
 
 	if (index) {
-		ret = regmap_read(regmap, LM95234_REG_UTEMPH(index - 1), &val);
+		regs[0] = LM95234_REG_UTEMPH(index - 1);
+		regs[1] = LM95234_REG_UTEMPL(index - 1);
+		ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (ret)
 			return ret;
-		temp = val << 8;
-		ret = regmap_read(regmap, LM95234_REG_UTEMPL(index - 1), &val);
-		if (ret)
-			return ret;
-		temp |= val;
+		temp = (regvals[0] << 8) | regvals[1];
 	}
 	/*
 	 * Read signed temperature if unsigned temperature is 0,
 	 * or if this is the local sensor.
 	 */
 	if (!temp) {
-		ret = regmap_read(regmap, LM95234_REG_TEMPH(index), &val);
+		regs[0] = LM95234_REG_TEMPH(index);
+		regs[1] = LM95234_REG_TEMPL(index);
+		ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (ret)
 			return ret;
-		temp = val << 8;
-		ret = regmap_read(regmap, LM95234_REG_TEMPL(index), &val);
-		if (ret)
-			return ret;
-		temp = sign_extend32(temp | val, 15);
+		temp = (regvals[0] << 8) | regvals[1];
+		temp = sign_extend32(temp, 15);
 	}
 	*t = DIV_ROUND_CLOSEST(temp * 125, 32);
 	return 0;
 }
 
-static int lm95234_hyst_get(struct lm95234_data *data, int reg, long *val)
+static int lm95234_hyst_get(struct regmap *regmap, int reg, long *val)
 {
-	u32 thyst, tcrit;
+	unsigned int regs[2] = {reg, LM95234_REG_TCRIT_HYST};
+	u8 regvals[2];
 	int ret;
 
-	mutex_lock(&data->update_lock);
-	ret = regmap_read(data->regmap, reg, &tcrit);
-	if (ret)
-		goto unlock;
-	ret = regmap_read(data->regmap, LM95234_REG_TCRIT_HYST, &thyst);
-unlock:
-	mutex_unlock(&data->update_lock);
+	ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 	if (ret)
 		return ret;
-
-	/* Result can be negative, so be careful with unsigned operands */
-	*val = ((int)tcrit - (int)thyst) * 1000;
+	*val = (regvals[0] - regvals[1]) * 1000;
 	return 0;
 }
 
@@ -209,7 +200,7 @@ static int lm95234_temp_read(struct device *dev, u32 attr, int channel, long *va
 		*val = !!(regval & BIT(channel));
 		break;
 	case hwmon_temp_crit_hyst:
-		return lm95234_hyst_get(data, LM95234_REG_TCRIT1(channel), val);
+		return lm95234_hyst_get(regmap, LM95234_REG_TCRIT1(channel), val);
 	case hwmon_temp_type:
 		ret = regmap_read(regmap, LM95234_REG_REM_MODEL, &regval);
 		if (ret)
@@ -236,7 +227,7 @@ static int lm95234_temp_read(struct device *dev, u32 attr, int channel, long *va
 		*val = regval * 1000;
 		break;
 	case hwmon_temp_max_hyst:
-		return lm95234_hyst_get(data, lm95234_crit_reg(channel), val);
+		return lm95234_hyst_get(regmap, lm95234_crit_reg(channel), val);
 	case hwmon_temp_crit:
 		ret = regmap_read(regmap, LM95234_REG_TCRIT1(channel), &regval);
 		if (ret)
-- 
2.39.2

Re: [PATCH 6/6] hwmon: (lm95234) Use multi-byte regmap operations

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 08:39:35PM -0700, Guenter Roeck wrote:
> Use  multi-byte regmap operations to simplify the code
> and to reduce dependency on locking.
> 
> No functional change.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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