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

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

On 7/1/24 06:01, Quentin Schulz wrote:

>> -#define AMC6821_CONF1_FDRC1        BIT(7)
>> +#define AMC6821_CONF1_FDRC1        BIT(6)
> 
> This should have been squashed with a previous commit.
> 

Yes. I had found the bug but then fixed it in the wrong patch of the series.

...
>>   static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
>>                char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> +    u32 regval;
> 
> Why not a u8 directly here? We know single reads are going to return a u8 so no need to store more?
> 

The parameter of regmap_read is a pointer to unsigned int.

> I'm not too fluent in type conversion, but maybe even an s8 since this is actually what's stored in the register?
> 
>> +    int err;
>> -    return sprintf(buf, "%d\n", data->temp[ix] * 1000);
>> +    err = regmap_read(data->regmap, temp_reg[ix], &regval);
>> +    if (err)
>> +        return err;
>> +
>> +    return sysfs_emit(buf, "%d\n", (int8_t)regval * 1000);
> 
> The type casting choice is odd here. Considering we'll be printing an int and that 1000 cannot be stored in an int8_t, maybe just cast it to an int directly?
> 

This is a trick which actually originates from the original code, only
there it is hidden in amc6821_update_device(). This is equivalent to
sign_extend32(regval, 7) which is more expensive, so I decided to stick
with it. On the other side, you are correct, it makes the code more
difficult to understand. I'll change it to use sign_extend32().


>>   static ssize_t temp_alarm_show(struct device *dev,
>>                      struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> -    u8 flag;
>> +    u32 regval, mask, reg;
> 
> Why not u8 for regval?
> 

Same as above. regmap() expects a pointer to unsigned int.


>>   static ssize_t temp2_fault_show(struct device *dev,
>>                   struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> -    if (data->stat1 & AMC6821_STAT1_RTF)
>> -        return sprintf(buf, "1");
>> -    else
>> -        return sprintf(buf, "0");
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    u32 regval;
> 
> Ditto.
> 

Same here and everywhere else.

>>   static ssize_t pwm1_enable_store(struct device *dev,
>> @@ -383,49 +346,37 @@ static ssize_t pwm1_enable_store(struct device *dev,
>>                    const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>>       long val;
>> -    int config = kstrtol(buf, 10, &val);
>> -    if (config)
>> -        return config;
>> +    u32 mask;
> 
> Please rename to something else, e.g. val, as this is NOT used as a mask but rather the value to write in the masked bitfield (which is hardcoded to AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1.
> 

I'll rename it to mode.

>> +
>> +    err = regmap_update_bits(data->regmap, AMC6821_REG_CONF1,
>> +                 AMC6821_CONF1_FDRC0 | AMC6821_CONF1_FDRC1,
>> +                 mask);
>> +    if (err < 0)
> 
> Shouldn't we check for err != 0 (so err) instead to be consistent with the rest of the code base in the driver?
> 

Ok.

>> @@ -564,38 +544,37 @@ 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;
>> -    unsigned long val;
>> -    int ret = kstrtoul(buf, 10, &val);
>> +    struct regmap *regmap = data->regmap;
>> +    long val;
>> +    int ret;
>> +
>> +    ret = kstrtoul(buf, 10, &val);
>>       if (ret)
>>           return ret;
>> -    if (val > 255)
>> +    if (val > 254)
> 
> I think this should have been squashed with an earlier commit?
> 
Yes, another fix applied to the wrong patch.

>> @@ -603,58 +582,72 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev,
>>   static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
>>               char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr(devattr)->index;
>> -    if (0 == data->fan[ix])
>> -        return sprintf(buf, "6000000");
>> -    return sprintf(buf, "%d\n", (int)(6000000 / data->fan[ix]));
>> +    u32 regval;
>> +    u8 regs[2];
> 
> Can't this be a u16 directly.......
> 
>> +    int err;
>> +
>> +    err = regmap_bulk_read(data->regmap, fan_reg_low[ix], regs, 2);
>> +    if (err)
>> +        return err;
>> +    regval = (regs[1] << 8) | regs[0];
>> +
> 
> 
> ..... to avoid doing this here?
> 

Then it would need an endianness conversion instead. That might save a couple
of cycles, but I think it would make the code more difficult to read.

>> +    return sysfs_emit(buf, "%d\n", 6000000 / (regval ? : 1));
>>   }
>>   static ssize_t fan1_fault_show(struct device *dev,
>>                      struct device_attribute *devattr, char *buf)
>>   {
>> -    struct amc6821_data *data = amc6821_update_device(dev);
>> -    if (data->stat1 & AMC6821_STAT1_FANS)
>> -        return sprintf(buf, "1");
>> -    else
>> -        return sprintf(buf, "0");
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    u32 regval;
>> +    int err;
>> +
>> +    err = regmap_read(data->regmap, AMC6821_REG_STAT1, &regval);
>> +    if (err)
>> +        return err;
>> +
>> +    return sysfs_emit(buf, "%d\n", !!(regval & AMC6821_STAT1_FANS));
>>   }
>>   static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>>                const char *buf, size_t count)
>>   {
>>       struct amc6821_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    long val;
>>       int ix = to_sensor_dev_attr(attr)->index;
>> -    int ret = kstrtol(buf, 10, &val);
>> -    if (ret)
>> -        return ret;
>> -    val = val < 1 ? 0xFFFF : 6000000 / val;
>> +    u8 regs[2];
> 
> Ditto.
> 
>> +    long val;
>> +    int err;
>> +
>> +    err = kstrtol(buf, 10, &val);
>> +    if (err)
>> +        return err;
>> +
>> +    val = val < 1 ? 0xFFFF : 6000000 / val;
>> +    val = clamp_val(val, 0, 0xFFFF);
>> +
>> +    regs[0] = val & 0xff;
>> +    regs[1] = val >> 8;
>> +
> 
> to avoid this here.
> Again, this would require an endianness conversion. Sure, that would save a couple
of cycles, but it would make the code more difficult to read.

>>   static int amc6821_probe(struct i2c_client *client)
>>   {
>>       struct device *dev = &client->dev;
>>       struct amc6821_data *data;
>>       struct device *hwmon_dev;
>> +    struct regmap *regmap;
> 
> Save this variable by using data->client directly?
> 

Then I'd have to dereference it twice to check for error.
In such situations I prefer to use a separate variable.

Thanks,
Guenter