On Sun, Apr 28, 2024 at 8:07 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Sun, Apr 28, 2024 at 11:00:47AM -0700, Guenter Roeck wrote:
> > On Fri, Apr 26, 2024 at 04:13:22PM +0200, Lars Petter Mostad wrote:
> > > Decode all diode data low byte registers.
> > >
> > All ?
> > What about the following ?
> >
> > 2c -> 2e
> > 2d -> 2f
>
> Also all other limit registers, and for those the write part is missing.
Yes, my intention was only to decode the (already non-zero) data registers,
not the limit registers.
> > > - unsigned int val;
> > > + unsigned int val, val_lowbyte;
> >
> > FWIW, this is wrong. The upper bit of the high byte is a sign bit
> > on emc1438.
Yes, I missed the sign bit in the datasheets. See my comment on patch for
emc1438. If I withdraw the EMC1438 patch, this will work for the current
chips with unsigned registers.
> > retval = regmap_read(data->regmap, sda->index, &val);
> > if (retval < 0)
> > return retval;
> > - return sprintf(buf, "%d000\n", val);
> > +
> > + if (idx_lowbyte) {
> > + retval = regmap_read(data->regmap, idx_lowbyte, &val_lowbyte);
> > + if (retval < 0)
> > + val_lowbyte = 0;
>
> This is an error and should be handled, not ignored.
My idea here was that if for some reason it manages to read the high byte but
not the low byte, I don't break anything. The output will be the same as before
the patch.
> > + return sprintf(buf, "%d\n",
> > + (((val << 8) | val_lowbyte) * (u32)1000) >> 8);
>
> The u32 typecast is unnecessary and would interfer with negative temperatures.
I put the u32 typecast there on the off chance that somebody will compile
this with a compiler with 16-bit ints (uClinux?), as C only guarantees 16 bits
for unsigned int. It would of course have to change if negative values are
to be supported.
Is it acceptable to handle the low byte for data registers only?
Should it be kept unsigned only (if dropping emc1438 patch)?
Regards,
Lars Petter
