Re: [PATCH v3 09/11] hwmon: (amc6821) Convert to use regmap

Quentin Schulz
2024-07-05T10:59:52Z

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@roeck-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@hansjkoch.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