Message-ID: <773d8bea-7ddb-4138-b75c-219a52c82fc6@cherry.de>
Date: Fri, 5 Jul 2024 12:59:45 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH v3 09/11] hwmon: (amc6821) Convert to use regmap

Hi Guenter,

On 7/4/24 7:52 PM, Guenter Roeck wrote:
> Use regmap for register accesses and caching.
> 
> While at it, use sysfs_emit() instead of sprintf() to write sysfs
> attribute data, and remove spurious debug messages which would only
> be seen as result of a bug in the code. Also make sure that error
> codes are propagated and not replaced with -EIO.
> 
> While at it, introduce rounding of written temperature values and for
> internal calculations to reduce deviation from written values and as
> much as possible.
> 
> No functional change intended except for differences introduced by
> rounding.
> 
> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> ---
> v3: Add more details to patch description
>      Cache all attributes
>      Introduce rounding when writing attributes and for some calculations
>      Always return error codes from regmap operations; never replace with
>      -EIO
> 
> v2: Drop another spurious debug message in this patch instead of patch 10
>      Add missing "select REGMAP_I2C" to Kconfig
>      Change misleading variable name from 'mask' to 'mode'.
>      Use sysfs_emit instead of sprintf everywhere
> 
> 
>   drivers/hwmon/Kconfig   |   1 +
>   drivers/hwmon/amc6821.c | 812 ++++++++++++++++++----------------------
>   2 files changed, 373 insertions(+), 440 deletions(-)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e14ae18a973b..a8fa87a96e8f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2127,6 +2127,7 @@ config SENSORS_ADS7871
>   config SENSORS_AMC6821
>   	tristate "Texas Instruments AMC6821"
>   	depends on I2C
> +	select REGMAP_I2C
>   	help
>   	  If you say yes here you get support for the Texas Instruments
>   	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index 295a9148779d..a5abd36a1430 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -8,15 +8,18 @@
>    * Copyright (C) 2007 Hans J. Koch <hjk@...sjkoch.de>
>    */
>   
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
>   #include <linux/bits.h>
>   #include <linux/err.h>
>   #include <linux/hwmon.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/i2c.h>
>   #include <linux/init.h>
> -#include <linux/jiffies.h>
> +#include <linux/minmax.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/regmap.h>
>   #include <linux/slab.h>
>   
>   /*
> @@ -44,6 +47,7 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_CONF4		0x04
>   #define AMC6821_REG_STAT1		0x02
>   #define AMC6821_REG_STAT2		0x03
> +#define AMC6821_REG_TEMP_LO		0x06
>   #define AMC6821_REG_TDATA_LOW		0x08
>   #define AMC6821_REG_TDATA_HI		0x09
>   #define AMC6821_REG_LTEMP_HI		0x0A
> @@ -61,11 +65,8 @@ module_param(init, int, 0444);
>   #define AMC6821_REG_DCY_LOW_TEMP	0x21
>   
>   #define AMC6821_REG_TACH_LLIMITL	0x10
> -#define AMC6821_REG_TACH_LLIMITH	0x11
>   #define AMC6821_REG_TACH_HLIMITL	0x12
> -#define AMC6821_REG_TACH_HLIMITH	0x13
>   #define AMC6821_REG_TACH_SETTINGL	0x1e
> -#define AMC6821_REG_TACH_SETTINGH	0x1f
>   
>   #define AMC6821_CONF1_START		BIT(0)
>   #define AMC6821_CONF1_FAN_INT_EN	BIT(1)
> @@ -108,6 +109,9 @@ module_param(init, int, 0444);
>   #define AMC6821_STAT2_L_THERM		BIT(6)
>   #define AMC6821_STAT2_THERM_IN		BIT(7)
>   
> +#define AMC6821_TEMP_SLOPE_MASK		GENMASK(2, 0)
> +#define AMC6821_TEMP_LIMIT_MASK		GENMASK(7, 3)
> +
>   enum {IDX_TEMP1_INPUT = 0, IDX_TEMP1_MIN, IDX_TEMP1_MAX,
>   	IDX_TEMP1_CRIT, IDX_TEMP2_INPUT, IDX_TEMP2_MIN,
>   	IDX_TEMP2_MAX, IDX_TEMP2_CRIT,
> @@ -130,224 +134,155 @@ static const u8 fan_reg_low[] = {AMC6821_REG_TDATA_LOW,
>   			AMC6821_REG_TACH_HLIMITL,
>   			AMC6821_REG_TACH_SETTINGL, };
>   
> -static const u8 fan_reg_hi[] = {AMC6821_REG_TDATA_HI,
> -			AMC6821_REG_TACH_LLIMITH,
> -			AMC6821_REG_TACH_HLIMITH,
> -			AMC6821_REG_TACH_SETTINGH, };
> -
>   /*
>    * Client data (each client gets its own)
>    */
>   
>   struct amc6821_data {
> -	struct i2c_client *client;
> +	struct regmap *regmap;
>   	struct mutex update_lock;
> -	bool valid; /* false until following fields are valid */
> -	unsigned long last_updated; /* in jiffies */
> -
> -	/* register values */
> -	int temp[TEMP_IDX_LEN];
> -
> -	u16 fan[FAN1_IDX_LEN];
> -	u8 fan1_pulses;
> -
> -	u8 pwm1;
> -	u8 temp1_auto_point_temp[3];
> -	u8 temp2_auto_point_temp[3];
> -	u8 pwm1_auto_point_pwm[3];
> -	u8 pwm1_enable;
> -	u8 pwm1_auto_channels_temp;
> -
> -	u8 stat1;
> -	u8 stat2;
>   };
>   
> -static struct amc6821_data *amc6821_update_device(struct device *dev)
> +/*
> + * Return set of three temperatures:

It actually returns 0 if successful, negative errno otherwise (matches 
regmap_* return values).

But it does write to temps array with those values.

Would be nice to say what we're expecting in channel, i.e. 0 for Remote 
and 1 for Local.

> + * temps[0]: Passive cooling temperature, applies to both channels
> + * temps[1]: Low temperature, start slope calculations
> + * temps[2]: High temperature
> + */

IIUC, we have different units there, >> 3 (/4) °C for 0 and 2, but °C 
for temps[1] ? If I didn't misunderstand, I think it's worth making it 
explicit in the docs (or make them have the same unit).

> +static int amc6821_get_auto_point_temps(struct regmap *regmap, int channel, u8 *temps)
>   {
> -	struct amc6821_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int timeout = HZ;
> -	u8 reg;
> -	int i;
> +	u32 pwm, regval;
> +	int err;
>   
> -	mutex_lock(&data->update_lock);
> +	err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> +	if (err)
> +		return err;
>   
> -	if (time_after(jiffies, data->last_updated + timeout) ||
> -			!data->valid) {
> +	err = regmap_read(regmap, AMC6821_REG_PSV_TEMP, &regval);
> +	if (err)
> +		return err;
> +	temps[0] = regval;
>   
> -		for (i = 0; i < TEMP_IDX_LEN; i++)
> -			data->temp[i] = (int8_t)i2c_smbus_read_byte_data(
> -				client, temp_reg[i]);
> +	err = regmap_read(regmap,
> +			  channel ? AMC6821_REG_RTEMP_FAN_CTRL : AMC6821_REG_LTEMP_FAN_CTRL,
> +			  &regval);
> +	if (err)
> +		return err;
> +	temps[1] = (regval & 0xF8) >> 1;

I think we want to use AMC6821_TEMP_LIMIT_MASK here instead of 0xF8?

I guess we could also use FIELD_GET?

>   
> -		data->stat1 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT1);
> -		data->stat2 = i2c_smbus_read_byte_data(client,
> -			AMC6821_REG_STAT2);
> +	regval &= 0x07;

I think we want to use AMC6821_TEMP_SLOPE_MASK instead of 0x07 here?

I guess we could also use FIELD_GET?

[...]

>   static ssize_t temp_auto_point_temp_store(struct device *dev,
>   					  struct device_attribute *attr,
>   					  const char *buf, size_t count)
>   {
> -	struct amc6821_data *data = amc6821_update_device(dev);
> -	struct i2c_client *client = data->client;
> +	struct amc6821_data *data = dev_get_drvdata(dev);
>   	int ix = to_sensor_dev_attr_2(attr)->index;
>   	int nr = to_sensor_dev_attr_2(attr)->nr;
> -	u8 *ptemp;
> -	u8 reg;
> -	int dpwm;
> +	struct regmap *regmap = data->regmap;
> +	u8 temps[3], otemps[3];
>   	long val;
> -	int ret = kstrtol(buf, 10, &val);
> +	int ret;
> +
> +	ret = kstrtol(buf, 10, &val);
>   	if (ret)
>   		return ret;
>   
> -	switch (nr) {
> -	case 1:
> -		ptemp = data->temp1_auto_point_temp;
> -		reg = AMC6821_REG_LTEMP_FAN_CTRL;
> -		break;
> -	case 2:
> -		ptemp = data->temp2_auto_point_temp;
> -		reg = AMC6821_REG_RTEMP_FAN_CTRL;
> -		break;
> -	default:
> -		dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
> -		return -EINVAL;
> -	}
> -
>   	mutex_lock(&data->update_lock);
> -	data->valid = false;
> +
> +	ret = amc6821_get_auto_point_temps(data->regmap, nr, temps);
> +	if (ret)
> +		goto unlock;
> +	ret = amc6821_get_auto_point_temps(data->regmap, 1 - nr, otemps);
> +	if (ret)
> +		goto unlock;
>   

We could reduce the scope of otemps since it's only used in the ix==0 
case below.

>   	switch (ix) {
>   	case 0:
> -		ptemp[0] = clamp_val(val / 1000, 0,
> -				     data->temp1_auto_point_temp[1]);
> -		ptemp[0] = clamp_val(ptemp[0], 0,
> -				     data->temp2_auto_point_temp[1]);
> -		ptemp[0] = clamp_val(ptemp[0], 0, 63);
> -		if (i2c_smbus_write_byte_data(
> -					client,
> -					AMC6821_REG_PSV_TEMP,
> -					ptemp[0])) {
> -				dev_err(&client->dev,
> -					"Register write error, aborting.\n");
> -				count = -EIO;
> -		}
> -		goto EXIT;
> +		/*
> +		 * Passive cooling temperature. Range limit against low limit
> +		 * of both channels.
> +		 */
> +		val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 63000), 1000);

This was already in the original code, but I think 64°C should be doable 
as well? The datasheet says:

"""
The PSV ranges from 0°C to +64°C.
"""

And there's a PSV8 bit we can write, meaning we can do (1 << 8) with a 
step of 4°C which gives us 64°C? In a separate commit though, to not mix 
too many fixes into one, making it easier for people to identify and 
possibly revert them if necessary.

> +		val = clamp_val(val, 0, min(temps[1], otemps[1]));
> +		ret = regmap_write(regmap, AMC6821_REG_PSV_TEMP, val);
> +		break;
>   	case 1:
> -		ptemp[1] = clamp_val(val / 1000, (ptemp[0] & 0x7C) + 4, 124);
> -		ptemp[1] &= 0x7C;
> -		ptemp[2] = clamp_val(ptemp[2], ptemp[1] + 1, 255);
> +		/*
> +		 * Low limit; must be between passive and high limit,
> +		 * and not exceed 124. Step size is 4 degrees C.
> +		 */
> +		val = clamp_val(val, DIV_ROUND_UP(temps[0], 4) * 4000, 124000);

Oof. I think the issue is that we have different units for temps[0], 
temps[1] and temps[2]?

temps[1] is in °C, while temps[0] is in °C >> 3 (so / 4) because we read 
from PSV-Temp register directly, which only exposes PSV[8:3] and 
PSV[2:0] are 0. I'm wondering if we shouldn't just have the same unit 
when filled by amc6821_get_auto_point_temps?

temps[2] is also °C >> 3 (4°C step in the register). I think we would 
benefit from having the same unit here to make it easier to do maths 
with temps[1] and temps[0/2]. What do you think?

If we didn't have this °C >> 3 formula, we could simply divide by 1000 
to get the value and then do the same maths for writing to the registers 
(except a different offset for temps[0] than temps[1/2]).


> +		temps[1] = DIV_ROUND_CLOSEST(val, 4000) * 4;
> +		val = temps[1] / 4;
> +		/* Auto-adjust high limit if necessary */
> +		temps[2] = clamp_val(temps[2], temps[1] + 1, 255);

Is this why we didn't want 255 for temps[1]? Because then we could have 
256 here?

> +		ret = regmap_update_bits(regmap,
> +					 nr ? AMC6821_REG_RTEMP_FAN_CTRL
> +					    : AMC6821_REG_LTEMP_FAN_CTRL,
> +					 AMC6821_TEMP_LIMIT_MASK,
> +					 FIELD_PREP(AMC6821_TEMP_LIMIT_MASK, val));
> +		if (ret)
> +			break;
> +		ret = set_slope_register(regmap, nr, temps);

I'm wondering if we shouldn't put the writes to the TEMP_LIMIT_MASK and 
AMC6821_TEMP_SLOPE_MASK into the same regmap write, otherwise there's a 
small timeframe during which the slope is not matching the TEMP_LIMIT. I 
guess it's probably not that big of a deal but still bringing this up.

Cheers,
Quentin

Powered by blists - more mailing lists