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

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Guenter,

On 7/1/24 3:47 PM, Guenter Roeck wrote:
> 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.
> 

Eventually through the many indirections, because our regmap_config sets 
the size of values in registers to 8b, it's a u8. But it's not worth our 
time to optimize this.

EDIT: coming back to this after reading the rest... Wouldn't we have a 
possible endianness issue here? Especially with the int8_t cast or the 
sign_extend32 (wouldn't the sign bit be at a different index on LE/BE 
?). Sorry for the question, endianness really isn't my cup of tea.

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

Indeed, I completely missed the implications of having a "real" s8 
stored in a u32, thanks for the explanation. It does make more sense to 
use sign_extend32() indeed.

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

Ah dang it, I always forget Aarch64 (thus LE) is not the only 
architecture that exists in the world :D Endianness conversion being my 
nemesis, I wholeheartedly agree with you.

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

Fair enough. One's taste after all :)

Cheers,
Quentin