On 4/30/24 04:11, Lars Petter Mostad wrote:
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.
We should really do all or nothing.
- 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.
Adding support for the new chip is desirable. Please don't drop it.
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.
No, you should handle the error.
+ 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.
I don't even know if such a system exists, but if it does it won't build a Linux
kernel. It doesn't make sense to even try to support a system with 16-bit integers.
Is it acceptable to handle the low byte for data registers only?
No, as mentioned above.
Should it be kept unsigned only (if dropping emc1438 patch)?
You should not drop the emc1438 patch. What I would suggest, though,
is to consider converting the driver to use the _with_info registration
API. That would simplify the code a lot and make it much easier to add
support for new chips and to add conditional support for signed temperatures.
Thanks,
Guenter
