[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7a1f2916-3e2b-45ee-9681-7327d7b1798a@cherry.de>
Date: Mon, 1 Jul 2024 18:54:57 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH 09/10] hwmon: (amc6821) Convert to use regmap
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], ®val);
>>> + 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
Powered by blists - more mailing lists