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

[Guenter Roeck]

On 7/1/24 09:54, Quentin Schulz wrote:
> 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.

The underlying code does an implicit conversion. For example,  see
regmap_smbus_byte_reg_read(). There is no endianness issue because
the returned data is always in host endianness order. Sure, that
could be big endian, but the sign bit is still in bit 7.

Thanks,
Guenter