Message-ID: <949d9c32-7722-4afe-a8ae-b2ef57d1cf01@roeck-us.net>
Date: Fri, 5 Jul 2024 07:28:09 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Quentin Schulz <quentin.schulz@...rry.de>, 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

On 7/5/24 03:59, Quentin Schulz wrote:
> 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).
> 
I'll rephrase.

> 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.
> 

1 for remote

>> + * 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).
> 

It should be all in °C.

>> +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?
> 

Yes. The value in the register is in °C * 4, so that is going to be
	temps[1] = FIELD_GET(regval, AMC6821_TEMP_LIMIT_MASK) * 4;
which improves readability and should also clarify the units a bit
better.

Note hat
	(regval & 0xF8) >> 1;
resulted in the temperature in °C (shift right 1 instead of 3).

>> -        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?
> 
Done, making it
	regval = BIT(5) >> FIELD_GET(regval, AMC6821_TEMP_SLOPE_MASK);

> [...]
> 
>>   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.
> 
Done.

>>       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.
> """
> 

Yes, but I am sure the datasheet is wrong here. The register has 6 active bits,
which means the highest possible value is 0x3f or 63.

> 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.
> 
Not sure I understand. Can you clarify ?

Temperature bit assignments in the datasheet are confusing. PSV3
means full degrees C, PSV8 means 32 degrees C. That is all in one register.
On the other side, L-TEMP0 reflects _4_ degrees C.

Am I missing something ?

>> +        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?
> 
No, they are all in °C. I think the confusion arises from L-TEMP[0..4] which is in multiples
of 4 °C. Since L-TEMP needs to be in multiples of 4 degrees C, and temps[0] is in degrees C,
the above sets the lower limit to the next multiple of 4 °C at or above temps[0].
The upper limit is 124 degrees C per datasheet.

> 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;

The DIV_ROUND_CLOSEST() here is to improve rounding to 4 degrees C. The resulting value
in temp[1] is {0, 4, 8, ... 124}.

>> +        val = temps[1] / 4;

This is the register value to be written.

>> +        /* 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?
> 

The highest possible value for temps[1] is 124, so the lower clamp value
would never be 256. The above only ensures that temps[2] is > temps[1].

>> +        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.
> 

Hmm, you mean to let set_slope_register() also update the low temperature limit
based on temps[1] ? Excellent idea. I'll do that; it will save a register write
to the chip.

Thanks,
Guenter

Powered by blists - more mailing lists