[PATCH 0/6] hwmon: Use multi-byte regmap operations

[PATCH 0/6] hwmon: Use multi-byte regmap operations

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional change.

----------------------------------------------------------------
Guenter Roeck (6):
      hwmon: (lm95245) Use multi-byte regmap operations
      hwmon: (nct7802) Use multi-byte regmap operations
      hwmon: (adt7x10) Use multi-byte regmap operations
      hwmon: (tmp464) Use multi-byte regmap operations
      hwmon: (max6639) Use multi-byte regmap operations
      hwmon: (amc6821) Use multi-byte regmap operations

 drivers/hwmon/adt7x10.c |  18 +++-----
 drivers/hwmon/amc6821.c |  30 ++++++-------
 drivers/hwmon/lm95245.c | 110 ++++++++++++++++++++----------------------------
 drivers/hwmon/max6639.c |  40 +++++-------------
 drivers/hwmon/nct7802.c |  61 ++++++++++-----------------
 drivers/hwmon/tmp464.c  |  33 +++++++--------
 6 files changed, 112 insertions(+), 180 deletions(-)

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional change.

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

diff --git a/drivers/hwmon/lm95245.c b/drivers/hwmon/lm95245.c
index d293b4f15dc1..3bdc30530847 100644
--- a/drivers/hwmon/lm95245.c
+++ b/drivers/hwmon/lm95245.c
@@ -161,18 +161,18 @@ static int lm95245_read_temp(struct device *dev, u32 attr, int channel,
 {
 	struct lm95245_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
-	int ret, regl, regh, regvall, regvalh;
+	unsigned int regs[2];
+	unsigned int regval;
+	u8 regvals[2];
+	int ret;
 
 	switch (attr) {
 	case hwmon_temp_input:
-		regl = channel ? LM95245_REG_R_REMOTE_TEMPL_S :
-				 LM95245_REG_R_LOCAL_TEMPL_S;
-		regh = channel ? LM95245_REG_R_REMOTE_TEMPH_S :
-				 LM95245_REG_R_LOCAL_TEMPH_S;
-		ret = regmap_read(regmap, regl, &regvall);
-		if (ret < 0)
-			return ret;
-		ret = regmap_read(regmap, regh, &regvalh);
+		regs[0] = channel ? LM95245_REG_R_REMOTE_TEMPL_S :
+				    LM95245_REG_R_LOCAL_TEMPL_S;
+		regs[1] = channel ? LM95245_REG_R_REMOTE_TEMPH_S :
+				    LM95245_REG_R_LOCAL_TEMPH_S;
+		ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (ret < 0)
 			return ret;
 		/*
@@ -181,92 +181,77 @@ static int lm95245_read_temp(struct device *dev, u32 attr, int channel,
 		 * Use signed calculation for remote if signed bit is set
 		 * or if reported temperature is below signed limit.
 		 */
-		if (!channel || (regvalh & 0x80) || regvalh < 0x7f) {
-			*val = temp_from_reg_signed(regvalh, regvall);
+		if (!channel || (regvals[1] & 0x80) || regvals[1] < 0x7f) {
+			*val = temp_from_reg_signed(regvals[1], regvals[0]);
 			return 0;
 		}
-		ret = regmap_read(regmap, LM95245_REG_R_REMOTE_TEMPL_U,
-				  &regvall);
-		if (ret < 0)
+		ret = regmap_bulk_read(regmap, LM95245_REG_R_REMOTE_TEMPH_U, regvals, 2);
+		if (ret)
 			return ret;
-		ret = regmap_read(regmap, LM95245_REG_R_REMOTE_TEMPH_U,
-				  &regvalh);
-		if (ret < 0)
-			return ret;
-		*val = temp_from_reg_unsigned(regvalh, regvall);
+		*val = temp_from_reg_unsigned(regvals[0], regvals[1]);
 		return 0;
 	case hwmon_temp_max:
 		ret = regmap_read(regmap, LM95245_REG_RW_REMOTE_OS_LIMIT,
-				  &regvalh);
+				  &regval);
 		if (ret < 0)
 			return ret;
-		*val = regvalh * 1000;
+		*val = regval * 1000;
 		return 0;
 	case hwmon_temp_crit:
-		regh = channel ? LM95245_REG_RW_REMOTE_TCRIT_LIMIT :
-				 LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT;
-		ret = regmap_read(regmap, regh, &regvalh);
+		regs[0] = channel ? LM95245_REG_RW_REMOTE_TCRIT_LIMIT :
+				    LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT;
+		ret = regmap_read(regmap, regs[0], &regval);
 		if (ret < 0)
 			return ret;
-		*val = regvalh * 1000;
+		*val = regval * 1000;
 		return 0;
 	case hwmon_temp_max_hyst:
-		ret = regmap_read(regmap, LM95245_REG_RW_REMOTE_OS_LIMIT,
-				  &regvalh);
+		regs[0] = LM95245_REG_RW_REMOTE_OS_LIMIT;
+		regs[1] = LM95245_REG_RW_COMMON_HYSTERESIS;
+		ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (ret < 0)
 			return ret;
-		ret = regmap_read(regmap, LM95245_REG_RW_COMMON_HYSTERESIS,
-				  &regvall);
-		if (ret < 0)
-			return ret;
-		*val = (regvalh - regvall) * 1000;
+		*val = (regvals[0] - regvals[1]) * 1000;
 		return 0;
 	case hwmon_temp_crit_hyst:
-		regh = channel ? LM95245_REG_RW_REMOTE_TCRIT_LIMIT :
-				 LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT;
-		ret = regmap_read(regmap, regh, &regvalh);
+		regs[0] = channel ? LM95245_REG_RW_REMOTE_TCRIT_LIMIT :
+				    LM95245_REG_RW_LOCAL_OS_TCRIT_LIMIT;
+		regs[1] = LM95245_REG_RW_COMMON_HYSTERESIS;
+
+		ret = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (ret < 0)
 			return ret;
-		ret = regmap_read(regmap, LM95245_REG_RW_COMMON_HYSTERESIS,
-				  &regvall);
-		if (ret < 0)
-			return ret;
-		*val = (regvalh - regvall) * 1000;
+		*val = (regvals[0] - regvals[1]) * 1000;
 		return 0;
 	case hwmon_temp_type:
-		ret = regmap_read(regmap, LM95245_REG_RW_CONFIG2, &regvalh);
+		ret = regmap_read(regmap, LM95245_REG_RW_CONFIG2, &regval);
 		if (ret < 0)
 			return ret;
-		*val = (regvalh & CFG2_REMOTE_TT) ? 1 : 2;
+		*val = (regval & CFG2_REMOTE_TT) ? 1 : 2;
 		return 0;
 	case hwmon_temp_offset:
-		ret = regmap_read(regmap, LM95245_REG_RW_REMOTE_OFFL,
-				  &regvall);
+		ret = regmap_bulk_read(regmap, LM95245_REG_RW_REMOTE_OFFH, regvals, 2);
 		if (ret < 0)
 			return ret;
-		ret = regmap_read(regmap, LM95245_REG_RW_REMOTE_OFFH,
-				  &regvalh);
-		if (ret < 0)
-			return ret;
-		*val = temp_from_reg_signed(regvalh, regvall);
+		*val = temp_from_reg_signed(regvals[0], regvals[1]);
 		return 0;
 	case hwmon_temp_max_alarm:
-		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regvalh);
+		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regval);
 		if (ret < 0)
 			return ret;
-		*val = !!(regvalh & STATUS1_ROS);
+		*val = !!(regval & STATUS1_ROS);
 		return 0;
 	case hwmon_temp_crit_alarm:
-		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regvalh);
+		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regval);
 		if (ret < 0)
 			return ret;
-		*val = !!(regvalh & (channel ? STATUS1_RTCRIT : STATUS1_LOC));
+		*val = !!(regval & (channel ? STATUS1_RTCRIT : STATUS1_LOC));
 		return 0;
 	case hwmon_temp_fault:
-		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regvalh);
+		ret = regmap_read(regmap, LM95245_REG_R_STATUS1, &regval);
 		if (ret < 0)
 			return ret;
-		*val = !!(regvalh & STATUS1_DIODE_FAULT);
+		*val = !!(regval & STATUS1_DIODE_FAULT);
 		return 0;
 	default:
 		return -EOPNOTSUPP;
@@ -279,6 +264,7 @@ static int lm95245_write_temp(struct device *dev, u32 attr, int channel,
 	struct lm95245_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
 	unsigned int regval;
+	u8 regvals[2];
 	int ret, reg;
 
 	switch (attr) {
@@ -311,16 +297,10 @@ static int lm95245_write_temp(struct device *dev, u32 attr, int channel,
 	case hwmon_temp_offset:
 		val = clamp_val(val, -128000, 127875);
 		val = val * 256 / 1000;
-		mutex_lock(&data->update_lock);
-		ret = regmap_write(regmap, LM95245_REG_RW_REMOTE_OFFL,
-				   val & 0xe0);
-		if (ret < 0) {
-			mutex_unlock(&data->update_lock);
-			return ret;
-		}
-		ret = regmap_write(regmap, LM95245_REG_RW_REMOTE_OFFH,
-				   (val >> 8) & 0xff);
-		mutex_unlock(&data->update_lock);
+		regvals[0] = val >> 8;
+		regvals[1] = val & 0xe0;
+
+		ret = regmap_bulk_write(regmap, LM95245_REG_RW_REMOTE_OFFH, regvals, 2);
 		return ret;
 	case hwmon_temp_type:
 		if (val != 1 && val != 2)
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:45PM -0700, Guenter Roeck wrote:
> Use multi-byte regmap operations where possible to reduce code size
> and the need for mutex protection.
> 
> No functional change.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

> @@ -181,92 +181,77 @@ static int lm95245_read_temp(struct device *dev, u32 attr, int channel,
[...]
> -		ret = regmap_read(regmap, LM95245_REG_R_REMOTE_TEMPL_U,
> -				  &regvall);
> -		if (ret < 0)
> +		ret = regmap_bulk_read(regmap, LM95245_REG_R_REMOTE_TEMPH_U, regvals, 2);
> +		if (ret)
>  			return ret;
> -		ret = regmap_read(regmap, LM95245_REG_R_REMOTE_TEMPH_U,
> -				  &regvalh);
> -		if (ret < 0)
> -			return ret;
> -		*val = temp_from_reg_unsigned(regvalh, regvall);
> +		*val = temp_from_reg_unsigned(regvals[0], regvals[1]);

Just a note:
I took a while for realizing that a regmap_bulk_read() is sufficient here as
LM95245_REG_R_REMOTE_TEMPH_U and LM95245_REG_R_REMOTE_TEMPL_U are continuous.

The same logic applies to the rest changes.

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional changes.

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

diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 97e8c6424403..3d6819aebe16 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -229,41 +229,34 @@ static int nct7802_read_temp(struct nct7802_data *data,
 
 static int nct7802_read_fan(struct nct7802_data *data, u8 reg_fan)
 {
-	unsigned int f1, f2;
+	unsigned int regs[2] = {reg_fan, REG_FANCOUNT_LOW};
+	u8 f[2];
 	int ret;
 
-	mutex_lock(&data->access_lock);
-	ret = regmap_read(data->regmap, reg_fan, &f1);
-	if (ret < 0)
-		goto abort;
-	ret = regmap_read(data->regmap, REG_FANCOUNT_LOW, &f2);
-	if (ret < 0)
-		goto abort;
-	ret = (f1 << 5) | (f2 >> 3);
+	ret = regmap_multi_reg_read(data->regmap, regs, f, 2);
+	if (ret)
+		return ret;
+	ret = (f[0] << 5) | (f[1] >> 3);
 	/* convert fan count to rpm */
 	if (ret == 0x1fff)	/* maximum value, assume fan is stopped */
 		ret = 0;
 	else if (ret)
 		ret = DIV_ROUND_CLOSEST(1350000U, ret);
-abort:
-	mutex_unlock(&data->access_lock);
 	return ret;
 }
 
 static int nct7802_read_fan_min(struct nct7802_data *data, u8 reg_fan_low,
 				u8 reg_fan_high)
 {
-	unsigned int f1, f2;
+	unsigned int regs[2] = {reg_fan_low, reg_fan_high};
+	u8 f[2];
 	int ret;
 
-	mutex_lock(&data->access_lock);
-	ret = regmap_read(data->regmap, reg_fan_low, &f1);
+	ret = regmap_multi_reg_read(data->regmap, regs, f, 2);
 	if (ret < 0)
-		goto abort;
-	ret = regmap_read(data->regmap, reg_fan_high, &f2);
-	if (ret < 0)
-		goto abort;
-	ret = f1 | ((f2 & 0xf8) << 5);
+		return ret;
+
+	ret = f[0] | ((f[1] & 0xf8) << 5);
 	/* convert fan count to rpm */
 	if (ret == 0x1fff)	/* maximum value, assume no limit */
 		ret = 0;
@@ -271,8 +264,6 @@ static int nct7802_read_fan_min(struct nct7802_data *data, u8 reg_fan_low,
 		ret = DIV_ROUND_CLOSEST(1350000U, ret);
 	else
 		ret = 1350000U;
-abort:
-	mutex_unlock(&data->access_lock);
 	return ret;
 }
 
@@ -302,33 +293,27 @@ static u8 nct7802_vmul[] = { 4, 2, 2, 2, 2 };
 
 static int nct7802_read_voltage(struct nct7802_data *data, int nr, int index)
 {
-	unsigned int v1, v2;
+	u8 v[2];
 	int ret;
 
 	mutex_lock(&data->access_lock);
 	if (index == 0) {	/* voltage */
-		ret = regmap_read(data->regmap, REG_VOLTAGE[nr], &v1);
+		unsigned int regs[2] = {REG_VOLTAGE[nr], REG_VOLTAGE_LOW};
+
+		ret = regmap_multi_reg_read(data->regmap, regs, v, 2);
 		if (ret < 0)
-			goto abort;
-		ret = regmap_read(data->regmap, REG_VOLTAGE_LOW, &v2);
-		if (ret < 0)
-			goto abort;
-		ret = ((v1 << 2) | (v2 >> 6)) * nct7802_vmul[nr];
+			return ret;
+		ret = ((v[0] << 2) | (v[1] >> 6)) * nct7802_vmul[nr];
 	}  else {	/* limit */
 		int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
+		unsigned int regs[2] = {REG_VOLTAGE_LIMIT_LSB[index - 1][nr],
+					REG_VOLTAGE_LIMIT_MSB[nr]};
 
-		ret = regmap_read(data->regmap,
-				  REG_VOLTAGE_LIMIT_LSB[index - 1][nr], &v1);
+		ret = regmap_multi_reg_read(data->regmap, regs, v, 2);
 		if (ret < 0)
-			goto abort;
-		ret = regmap_read(data->regmap, REG_VOLTAGE_LIMIT_MSB[nr],
-				  &v2);
-		if (ret < 0)
-			goto abort;
-		ret = (v1 | ((v2 << shift) & 0x300)) * nct7802_vmul[nr];
+			return ret;
+		ret = (v[0] | ((v[1] << shift) & 0x300)) * nct7802_vmul[nr];
 	}
-abort:
-	mutex_unlock(&data->access_lock);
 	return ret;
 }
 
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:46PM -0700, Guenter Roeck wrote:
> Use multi-byte regmap operations where possible to reduce code size
> and the need for mutex protection.
> 
> No functional changes.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional changes.

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

diff --git a/drivers/hwmon/adt7x10.c b/drivers/hwmon/adt7x10.c
index 6701920de17f..2d329391ed3f 100644
--- a/drivers/hwmon/adt7x10.c
+++ b/drivers/hwmon/adt7x10.c
@@ -170,21 +170,15 @@ static int adt7x10_temp_write(struct adt7x10_data *data, int index, long temp)
 
 static int adt7x10_hyst_read(struct adt7x10_data *data, int index, long *val)
 {
-	int hyst, temp, ret;
+	unsigned int regs[2] = {ADT7X10_T_HYST, ADT7X10_REG_TEMP[index]};
+	int hyst, ret;
+	u16 regdata[2];
 
-	mutex_lock(&data->update_lock);
-	ret = regmap_read(data->regmap, ADT7X10_T_HYST, &hyst);
-	if (ret) {
-		mutex_unlock(&data->update_lock);
-		return ret;
-	}
-
-	ret = regmap_read(data->regmap, ADT7X10_REG_TEMP[index], &temp);
-	mutex_unlock(&data->update_lock);
+	ret = regmap_multi_reg_read(data->regmap, regs, regdata, 2);
 	if (ret)
 		return ret;
 
-	hyst = (hyst & ADT7X10_T_HYST_MASK) * 1000;
+	hyst = (regdata[0] & ADT7X10_T_HYST_MASK) * 1000;
 
 	/*
 	 * hysteresis is stored as a 4 bit offset in the device, convert it
@@ -194,7 +188,7 @@ static int adt7x10_hyst_read(struct adt7x10_data *data, int index, long *val)
 	if (index == adt7x10_t_alarm_low)
 		hyst = -hyst;
 
-	*val = ADT7X10_REG_TO_TEMP(data, temp) - hyst;
+	*val = ADT7X10_REG_TO_TEMP(data, regdata[1]) - hyst;
 	return 0;
 }
 
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:47PM -0700, Guenter Roeck wrote:
> Use multi-byte regmap operations where possible to reduce code size
> and the need for mutex protection.
> 
> No functional changes.

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

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional changes.

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

diff --git a/drivers/hwmon/tmp464.c b/drivers/hwmon/tmp464.c
index 3ee1137533d6..0a7c0448835b 100644
--- a/drivers/hwmon/tmp464.c
+++ b/drivers/hwmon/tmp464.c
@@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 {
 	struct tmp464_data *data = dev_get_drvdata(dev);
 	struct regmap *regmap = data->regmap;
-	unsigned int regval, regval2;
+	unsigned int regs[2];
+	unsigned int regval;
+	u16 regvals[2];
 	int err = 0;
 
-	mutex_lock(&data->update_lock);
-
 	switch (attr) {
 	case hwmon_temp_max_alarm:
 		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
@@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		 * complete. That means we have to cache the value internally
 		 * for one measurement cycle and report the cached value.
 		 */
+		mutex_lock(&data->update_lock);
 		if (!data->valid || time_after(jiffies, data->last_updated +
 					       msecs_to_jiffies(data->update_interval))) {
 			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
 			if (err < 0)
-				break;
+				goto unlock;
 			data->open_reg = regval;
 			data->last_updated = jiffies;
 			data->valid = true;
 		}
 		*val = !!(data->open_reg & BIT(channel + 7));
+unlock:
+		mutex_unlock(&data->update_lock);
 		break;
 	case hwmon_temp_max_hyst:
-		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
+		regs[0] = TMP464_THERM_LIMIT[channel];
+		regs[1] = TMP464_TEMP_HYST_REG;
+		err = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (err < 0)
 			break;
-		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
-		if (err < 0)
-			break;
-		regval -= regval2;
-		*val = temp_from_reg(regval);
+		*val = temp_from_reg(regvals[0] - regvals[1]);
 		break;
 	case hwmon_temp_max:
 		err = regmap_read(regmap, TMP464_THERM_LIMIT[channel], &regval);
@@ -200,14 +201,12 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		*val = temp_from_reg(regval);
 		break;
 	case hwmon_temp_crit_hyst:
-		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
+		regs[0] = TMP464_THERM2_LIMIT[channel];
+		regs[1] = TMP464_TEMP_HYST_REG;
+		err = regmap_multi_reg_read(regmap, regs, regvals, 2);
 		if (err < 0)
 			break;
-		err = regmap_read(regmap, TMP464_TEMP_HYST_REG, &regval2);
-		if (err < 0)
-			break;
-		regval -= regval2;
-		*val = temp_from_reg(regval);
+		*val = temp_from_reg(regvals[0] - regvals[1]);
 		break;
 	case hwmon_temp_crit:
 		err = regmap_read(regmap, TMP464_THERM2_LIMIT[channel], &regval);
@@ -239,8 +238,6 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
 		break;
 	}
 
-	mutex_unlock(&data->update_lock);
-
 	return err;
 }
 
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
> @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>  {
[...]
> -	mutex_lock(&data->update_lock);
> -
>  	switch (attr) {
>  	case hwmon_temp_max_alarm:
>  		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
> @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>  		 * complete. That means we have to cache the value internally
>  		 * for one measurement cycle and report the cached value.
>  		 */
> +		mutex_lock(&data->update_lock);
>  		if (!data->valid || time_after(jiffies, data->last_updated +
>  					       msecs_to_jiffies(data->update_interval))) {
>  			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
>  			if (err < 0)
> -				break;
> +				goto unlock;
>  			data->open_reg = regval;
>  			data->last_updated = jiffies;
>  			data->valid = true;
>  		}
>  		*val = !!(data->open_reg & BIT(channel + 7));
> +unlock:
> +		mutex_unlock(&data->update_lock);
>  		break;

I think the function can entirely drop the mutex.  Only [1] needs it.

[1]: https://elixir.bootlin.com/linux/v6.10/source/drivers/hwmon/tmp464.c#L313

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

[Guenter Roeck]

On 7/17/24 06:35, Tzung-Bi Shih wrote:
> On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
>> @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>>   {
> [...]
>> -	mutex_lock(&data->update_lock);
>> -
>>   	switch (attr) {
>>   	case hwmon_temp_max_alarm:
>>   		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
>> @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
>>   		 * complete. That means we have to cache the value internally
>>   		 * for one measurement cycle and report the cached value.
>>   		 */
>> +		mutex_lock(&data->update_lock);
>>   		if (!data->valid || time_after(jiffies, data->last_updated +
>>   					       msecs_to_jiffies(data->update_interval))) {
>>   			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
>>   			if (err < 0)
>> -				break;
>> +				goto unlock;
>>   			data->open_reg = regval;
>>   			data->last_updated = jiffies;
>>   			data->valid = true;
>>   		}
>>   		*val = !!(data->open_reg & BIT(channel + 7));
>> +unlock:
>> +		mutex_unlock(&data->update_lock);
>>   		break;
> 
> I think the function can entirely drop the mutex.  Only [1] needs it.
> 

It is needed to protect updating open_reg. Otherwise a second process
could enter the code and read the register again, which would then return
different (cleared) values. As result open_reg might contain the temporarily
"cleared" values.

Process 1			Process 2
err = regmap_read();
data->open_reg = regval;
				err = regmap_read();
				data->open_reg = regval;
data->last_updated = jiffies;
...

Thanks,
Guenter

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

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 07:37:10AM -0700, Guenter Roeck wrote:
> On 7/17/24 06:35, Tzung-Bi Shih wrote:
> > On Tue, Jul 16, 2024 at 04:00:48PM -0700, Guenter Roeck wrote:
> > > @@ -147,11 +147,11 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
> > >   {
> > [...]
> > > -	mutex_lock(&data->update_lock);
> > > -
> > >   	switch (attr) {
> > >   	case hwmon_temp_max_alarm:
> > >   		err = regmap_read(regmap, TMP464_THERM_STATUS_REG, &regval);
> > > @@ -172,26 +172,27 @@ static int tmp464_temp_read(struct device *dev, u32 attr, int channel, long *val
> > >   		 * complete. That means we have to cache the value internally
> > >   		 * for one measurement cycle and report the cached value.
> > >   		 */
> > > +		mutex_lock(&data->update_lock);
> > >   		if (!data->valid || time_after(jiffies, data->last_updated +
> > >   					       msecs_to_jiffies(data->update_interval))) {
> > >   			err = regmap_read(regmap, TMP464_REMOTE_OPEN_REG, &regval);
> > >   			if (err < 0)
> > > -				break;
> > > +				goto unlock;
> > >   			data->open_reg = regval;
> > >   			data->last_updated = jiffies;
> > >   			data->valid = true;
> > >   		}
> > >   		*val = !!(data->open_reg & BIT(channel + 7));
> > > +unlock:
> > > +		mutex_unlock(&data->update_lock);
> > >   		break;
> > 
> > I think the function can entirely drop the mutex.  Only [1] needs it.
> > 
> 
> It is needed to protect updating open_reg. Otherwise a second process
> could enter the code and read the register again, which would then return
> different (cleared) values. As result open_reg might contain the temporarily
> "cleared" values.
> 
> Process 1			Process 2
> err = regmap_read();
> data->open_reg = regval;
> 				err = regmap_read();
> 				data->open_reg = regval;
> data->last_updated = jiffies;
> ...

Ack.

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

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size
and the need for mutex protection.

No functional changes.

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

diff --git a/drivers/hwmon/max6639.c b/drivers/hwmon/max6639.c
index f54720d3d2ce..e877a5520416 100644
--- a/drivers/hwmon/max6639.c
+++ b/drivers/hwmon/max6639.c
@@ -88,25 +88,16 @@ struct max6639_data {
 
 static int max6639_temp_read_input(struct device *dev, int channel, long *temp)
 {
+	u32 regs[2] = {MAX6639_REG_TEMP_EXT(channel), MAX6639_REG_TEMP(channel) };
 	struct max6639_data *data = dev_get_drvdata(dev);
-	unsigned int val;
+	u8 regvals[2];
 	int res;
 
-	/*
-	 * Lock isn't needed as MAX6639_REG_TEMP wpnt change for at least 250ms after reading
-	 * MAX6639_REG_TEMP_EXT
-	 */
-	res = regmap_read(data->regmap, MAX6639_REG_TEMP_EXT(channel), &val);
+	res = regmap_multi_reg_read(data->regmap, regs, regvals, 2);
 	if (res < 0)
 		return res;
 
-	*temp = val >> 5;
-	res = regmap_read(data->regmap, MAX6639_REG_TEMP(channel), &val);
-	if (res < 0)
-		return res;
-
-	*temp |= val << 3;
-	*temp *= 125;
+	*temp = ((regvals[0] >> 5) | (regvals[1] << 3)) * 125;
 
 	return 0;
 }
@@ -290,8 +281,10 @@ static umode_t max6639_fan_is_visible(const void *_data, u32 attr, int channel)
 static int max6639_read_pwm(struct device *dev, u32 attr, int channel,
 			    long *pwm_val)
 {
+	u32 regs[2] = { MAX6639_REG_FAN_CONFIG3(channel), MAX6639_REG_GCONFIG };
 	struct max6639_data *data = dev_get_drvdata(dev);
 	unsigned int val;
+	u8 regvals[2];
 	int res;
 	u8 i;
 
@@ -303,26 +296,13 @@ static int max6639_read_pwm(struct device *dev, u32 attr, int channel,
 		*pwm_val = val * 255 / 120;
 		return 0;
 	case hwmon_pwm_freq:
-		mutex_lock(&data->update_lock);
-		res = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(channel), &val);
-		if (res < 0) {
-			mutex_unlock(&data->update_lock);
+		res = regmap_multi_reg_read(data->regmap, regs, regvals, 2);
+		if (res < 0)
 			return res;
-		}
-		i = val & MAX6639_FAN_CONFIG3_FREQ_MASK;
-
-		res = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &val);
-		if (res < 0) {
-			mutex_unlock(&data->update_lock);
-			return res;
-		}
-
-		if (val & MAX6639_GCONFIG_PWM_FREQ_HI)
+		i = regvals[0] & MAX6639_FAN_CONFIG3_FREQ_MASK;
+		if (regvals[1] & MAX6639_GCONFIG_PWM_FREQ_HI)
 			i |= 0x4;
-		i &= 0x7;
 		*pwm_val = freq_table[i];
-
-		mutex_unlock(&data->update_lock);
 		return 0;
 	default:
 		return -EOPNOTSUPP;
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:49PM -0700, Guenter Roeck wrote:
> @@ -88,25 +88,16 @@ struct max6639_data {
>  
>  static int max6639_temp_read_input(struct device *dev, int channel, long *temp)
>  {
> +	u32 regs[2] = {MAX6639_REG_TEMP_EXT(channel), MAX6639_REG_TEMP(channel) };
                       ^                                                       ^
To be consistent, either drop the space or insert an extra space otherwise.

> @@ -290,8 +281,10 @@ static umode_t max6639_fan_is_visible(const void *_data, u32 attr, int channel)
>  static int max6639_read_pwm(struct device *dev, u32 attr, int channel,
>  			    long *pwm_val)
>  {
> +	u32 regs[2] = { MAX6639_REG_FAN_CONFIG3(channel), MAX6639_REG_GCONFIG };
                       ^                                                     ^
Same here.

> @@ -303,26 +296,13 @@ static int max6639_read_pwm(struct device *dev, u32 attr, int channel,
>  		*pwm_val = val * 255 / 120;
>  		return 0;
>  	case hwmon_pwm_freq:
> -		mutex_lock(&data->update_lock);
> -		res = regmap_read(data->regmap, MAX6639_REG_FAN_CONFIG3(channel), &val);
> -		if (res < 0) {
> -			mutex_unlock(&data->update_lock);
> +		res = regmap_multi_reg_read(data->regmap, regs, regvals, 2);
> +		if (res < 0)
>  			return res;
> -		}
> -		i = val & MAX6639_FAN_CONFIG3_FREQ_MASK;
> -
> -		res = regmap_read(data->regmap, MAX6639_REG_GCONFIG, &val);
> -		if (res < 0) {
> -			mutex_unlock(&data->update_lock);
> -			return res;
> -		}
> -
> -		if (val & MAX6639_GCONFIG_PWM_FREQ_HI)
> +		i = regvals[0] & MAX6639_FAN_CONFIG3_FREQ_MASK;
> +		if (regvals[1] & MAX6639_GCONFIG_PWM_FREQ_HI)
>  			i |= 0x4;
> -		i &= 0x7;

Just a note: this line can be safely dropped.

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

[Guenter Roeck]

On 7/17/24 06:36, Tzung-Bi Shih wrote:
> On Tue, Jul 16, 2024 at 04:00:49PM -0700, Guenter Roeck wrote:
>> @@ -88,25 +88,16 @@ struct max6639_data {
>>   
>>   static int max6639_temp_read_input(struct device *dev, int channel, long *temp)
>>   {
>> +	u32 regs[2] = {MAX6639_REG_TEMP_EXT(channel), MAX6639_REG_TEMP(channel) };
>                         ^                                                       ^
> To be consistent, either drop the space or insert an extra space otherwise.
> 
Good point. I added the space.

Thanks,
Guenter

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

[Tzung-Bi Shih]

On Wed, Jul 17, 2024 at 10:14:49AM -0700, Guenter Roeck wrote:
> On 7/17/24 06:36, Tzung-Bi Shih wrote:
> > On Tue, Jul 16, 2024 at 04:00:49PM -0700, Guenter Roeck wrote:
> > > @@ -88,25 +88,16 @@ struct max6639_data {
> > >   static int max6639_temp_read_input(struct device *dev, int channel, long *temp)
> > >   {
> > > +	u32 regs[2] = {MAX6639_REG_TEMP_EXT(channel), MAX6639_REG_TEMP(channel) };
> >                         ^                                                       ^
> > To be consistent, either drop the space or insert an extra space otherwise.
> > 
> Good point. I added the space.

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

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

[Guenter Roeck]

On 7/17/24 18:28, Tzung-Bi Shih wrote:
> On Wed, Jul 17, 2024 at 10:14:49AM -0700, Guenter Roeck wrote:
>> On 7/17/24 06:36, Tzung-Bi Shih wrote:
>>> On Tue, Jul 16, 2024 at 04:00:49PM -0700, Guenter Roeck wrote:
>>>> @@ -88,25 +88,16 @@ struct max6639_data {
>>>>    static int max6639_temp_read_input(struct device *dev, int channel, long *temp)
>>>>    {
>>>> +	u32 regs[2] = {MAX6639_REG_TEMP_EXT(channel), MAX6639_REG_TEMP(channel) };
>>>                          ^                                                       ^
>>> To be consistent, either drop the space or insert an extra space otherwise.
>>>
>> Good point. I added the space.
> 
> With that,
> Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>


Thanks a lot for the reviews !

Guenter

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

[Guenter Roeck]

Use multi-byte regmap operations where possible to reduce code size.

No functional changes.

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

diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index ec94392fcb65..ac64b407ed0e 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -136,29 +136,25 @@ struct amc6821_data {
  */
 static int amc6821_get_auto_point_temps(struct regmap *regmap, int channel, u8 *temps)
 {
-	u32 pwm, regval;
+	u32 regs[] = {
+		AMC6821_REG_DCY_LOW_TEMP,
+		AMC6821_REG_PSV_TEMP,
+		channel ? AMC6821_REG_RTEMP_FAN_CTRL : AMC6821_REG_LTEMP_FAN_CTRL
+	};
+	u8 regvals[3];
+	int slope;
 	int err;
 
-	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
+	err = regmap_multi_reg_read(regmap, regs, regvals, 3);
 	if (err)
 		return err;
-
-	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
-	if (err)
-		return err;
-	temps[0] = regval;
-
-	err = regmap_read(regmap,
-			  channel ? AMC6821_REG_RTEMP_FAN_CTRL : AMC6821_REG_LTEMP_FAN_CTRL,
-			  &regval);
-	if (err)
-		return err;
-	temps[1] = FIELD_GET(AMC6821_TEMP_LIMIT_MASK, regval) * 4;
+	temps[0] = regvals[1];
+	temps[1] = FIELD_GET(AMC6821_TEMP_LIMIT_MASK, regvals[2]) * 4;
 
 	/* slope is 32 >> <slope bits> in °C */
-	regval = 32 >> FIELD_GET(AMC6821_TEMP_SLOPE_MASK, regval);
-	if (regval)
-		temps[2] = temps[1] + DIV_ROUND_CLOSEST(255 - pwm, regval);
+	slope = 32 >> FIELD_GET(AMC6821_TEMP_SLOPE_MASK, regvals[2]);
+	if (slope)
+		temps[2] = temps[1] + DIV_ROUND_CLOSEST(255 - regvals[0], slope);
 	else
 		temps[2] = 255;
 
-- 
2.39.2

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

[Tzung-Bi Shih]

On Tue, Jul 16, 2024 at 04:00:50PM -0700, Guenter Roeck wrote:
> Use multi-byte regmap operations where possible to reduce code size.
> 
> No functional changes.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

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