Re: [PATCH 10/10] hwmon: (amc6821) Convert to with_info API

Guenter Roeck
2024-07-01T18:24:51Z

Hi uentin,

On 7/1/24 10:46, Quentin Schulz wrote:

>>           return err;
>> -    return sysfs_emit(buf, "%d\n", !!(regval & mask));
>> +    *val = (int8_t)regval * 1000;
>
> The discussed signed_extended32() in another patch would need to make it here too.
>
Yes. Thanks for the reminder, I had missed that one.

>> -    return sprintf(buf, "%d\n", val);
>> +    err = regmap_bulk_read(regmap, reg, regs, 2);
>> +    if (err)
>> +        return err;
>> +
>> +    regval = (regs[1] << 8) | regs[0];
>> +    *val = 6000000 / (regval ? : 1);
>> +
>
> Just putting a "bookmark" here since we're having a discussion about this logic in another thread for another patch.
>
> Also... missing 0 here between the question mark and the colon?
>
It translates to
*val = 6000000 / (regval ? regval : 1);
which is what I had wanted (divide by regval if regval != 0,
otherwise divide by 1 [i.e., return 6000000]).

I have it now as
*val = regval ? 6000000 / regval : 0;

>> +static int amc6821_fan_write(struct device *dev, u32 attr, long val)
>> +{
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>> +    struct regmap *regmap = data->regmap;
>> +    u8 regs[2];
>> +    int reg;
>> +
>> +    if (attr == hwmon_fan_pulses) {
>> +        if (val != 2 && val != 4)
>> +            return -EINVAL;
>> +        return regmap_update_bits(regmap, AMC6821_REG_CONF4,
>> +                     AMC6821_CONF4_PSPR,
>> +                     val == 4 ? AMC6821_CONF4_PSPR : 0);
>> +    }
>> +
>> +    if (val < 0)
>> +        return -EINVAL;
>> +
>> +    val = clamp_val(6000000 / (val ? : 1), 0, 0xffff);
>> +
>
> And another "bookmark" here.
>

That is now
val = val ? 6000000 / clamp_val(val, 1, 6000000) : 0;
val = clamp_val(val, 0, 0xffff);

'val' is checked earlier and must be > 0 for the minimum
and target fan speed.

>> +    switch (attr) {
>> +    case hwmon_fan_min:
>> +        reg = AMC6821_REG_TACH_LLIMITL;
>> +        break;
>> +    case hwmon_fan_max:
>> +        reg = AMC6821_REG_TACH_HLIMITL;
>> +        break;
>> +    case hwmon_fan_target:
>> +        reg = AMC6821_REG_TACH_SETTINGL;
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    regs[0] = val & 0xff;
>> +    regs[1] = val >> 8;
>> +
>> +    return regmap_bulk_write(data->regmap, reg, regs, 2);
>>   }
>>   static ssize_t temp_auto_point_temp_show(struct device *dev,
>>                        struct device_attribute *devattr,
>>                        char *buf)
>>   {
>> +    struct amc6821_data *data = dev_get_drvdata(dev);
>>       int ix = to_sensor_dev_attr_2(devattr)->index;
>>       int nr = to_sensor_dev_attr_2(devattr)->nr;
>> -    struct amc6821_data *data = dev_get_drvdata(dev);
>
> Should be squashed with the appropriate commit?
>

Yes, the previous one.

>>       switch (nr) {
>>       case 1:
>> @@ -423,7 +495,6 @@ static ssize_t temp_auto_point_temp_show(struct device *dev,
>>           return sprintf(buf, "%d\n",
>>               data->temp2_auto_point_temp[ix] * 1000);
>>       default:
>> -        dev_dbg(dev, "Unknown attr->nr (%d).\n", nr);
>
> Not sure this is related? Maybe a separate commit?
>

It should have been part of the previous commit, where it is explained ("remove
spurious debug messages which would only be seen as result of a bug in the code")

>> @@ -872,7 +902,6 @@ static int amc6821_probe(struct i2c_client *client)
>>       if (!data)
>>           return -ENOMEM;
>> -
>
> Should be squashed with the previous commit.
>
Ok.

> Nice clean-up albeit very difficult to review in patch form, not sure there's anything we can do about it though. Maybe migrating one attribute at a time, but I would myself not be very happy if I was asked to do it :)
>

Yes, that is always the problem with the conversion to the with_info API.
I don't think it would make much sense to try to split it further.

Thanks a lot for the detailed review!

Guenter