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

[Guenter Roeck <linux@roeck-us.net>]

On 7/3/24 09:19, Quentin Schulz wrote:
> Hi Guenter,
> 
> On 7/1/24 11:23 PM, Guenter Roeck wrote:
>> Use regmap for register accesses and for most 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.
>>
>> No functional change intended.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> 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 | 713 ++++++++++++++++++----------------------
>>   2 files changed, 329 insertions(+), 385 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 028998d3bedf..3fe0bfeac843 100644
>> --- a/drivers/hwmon/amc6821.c
>> +++ b/drivers/hwmon/amc6821.c
>> @@ -8,15 +8,16 @@
>>    * Copyright (C) 2007 Hans J. Koch <hjk@hansjkoch.de>
>>    */
>> +#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/module.h>
>>   #include <linux/mutex.h>
>> +#include <linux/regmap.h>
>>   #include <linux/slab.h>
>>   /*
>> @@ -44,6 +45,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 +63,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)
>> @@ -130,224 +129,169 @@ 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)
>> +static int amc6821_init_auto_point_data(struct amc6821_data *data)
>>   {
>> -    struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int timeout = HZ;
>> -    u8 reg;
>> -    int i;
>> +    struct regmap *regmap = data->regmap;
>> +    u32 pwm, regval;
>> +    int err;
>> -    mutex_lock(&data->update_lock);
>> +    err = regmap_read(regmap, AMC6821_REG_DCY_LOW_TEMP, &pwm);
> 
> I think this is incorrect logic.
> 
> amc6821_init_auto_point_data is only called once in init_client, in probe. While we currently do not write to AMC6821_REG_DCY_LOW_TEMP, we could in the future. But writing to it would desynchronise the auto_point_temp values for the new value of the register.
> 

We do write the register, in pwm1_auto_point_pwm_store().

> I suggest we put the logic into a function and return the value for a given temp_auto_point (1/2 currently) so that we are calling this function instead of a member in the struct so that we are never caching it unknowingly (regmap would do it for us in any case). It's a bit odd anyway to have only **those** values be cached in the struct as members and migrating everything else.

Yes, I know, that is a bit odd. Essentially I was too lazy to clean that up.
But, yes, you are correct, that results in the cached values being wrong
after AMC6821_REG_DCY_LOW_TEMP is updated. Back to the drawing board,
and thanks for finding this.

[ ... ]

>>       default:
>> -        dev_dbg(dev, "Unknown attr->index (%d).\n", ix);
> 
> Was this an intended removal? I think we can afford keeping it in?

Yes, it is intentional. It (and the others below) indicate a coding
error, which would have been found during development. While it does make
sense to keep the default: case to make the compiler happy, it doesn't
make sense to keep the pointless message in the code.

>> @@ -561,10 +538,11 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>                        const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int dpwm;
>> +    struct regmap *regmap = data->regmap;
>>       u8 val;
>> -    int ret = kstrtou8(buf, 10, &val);
>> +    int ret;
>> +
>> +    ret = kstrtou8(buf, 10, &val);
> 
> Not sure this cosmetic change is worth it? Or maybe squash with an earlier commit?
> 

That slipped in, I think from the first patch of the series. I'll move it there.

>>       if (ret)
>>           return ret;
>> @@ -572,27 +550,24 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>           return -EINVAL;
>>       mutex_lock(&data->update_lock);
>> -    data->pwm1_auto_point_pwm[1] = val;
>> -    if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP,
>> -            data->pwm1_auto_point_pwm[1])) {
>> -        dev_err(&client->dev, "Register write error, aborting.\n");
>> -        count = -EIO;
>> -        goto EXIT;
>> +    ret = regmap_write(regmap, AMC6821_REG_DCY_LOW_TEMP, val);
>> +    if (ret)
> 
> I think we're missing a count = something here?
> 
Yes.

>> +        goto unlock;
>> +
>> +    ret = set_slope_register(regmap, AMC6821_REG_LTEMP_FAN_CTRL,
>> +                 data->temp1_auto_point_temp);
>> +    if (ret) {
>> +        count = ret;
> 
> In some places, we replace set_slope_register return code with -EIO (like it was) and sometimes we propagate it, like here. Is there some reason for this or can we have some kind of consistency across the code base here?
> 
Thanks for noticing. The code should be consistent and always propagate the error code.
I'll fix it.

Thanks,
Guenter