| Message ID | 20240426141322.609642-1-lars.petter.mostad@appear.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (emc1403) Decode fractional temperatures. | expand |
On Fri, Apr 26, 2024 at 04:13:22PM +0200, Lars Petter Mostad wrote: > Decode all diode data low byte registers. > All ? > Signed-off-by: Lars Petter Mostad <lars.petter.mostad@appear.net> > --- > drivers/hwmon/emc1403.c | 34 ++++++++++++++++++++++++++++++++-- > 1 file changed, 32 insertions(+), 2 deletions(-) > > > base-commit: e723f6ca39fb54ae31f79b5af74fe8496308d4de > > diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c > index d370efd6f986..2b14db802f96 100644 > --- a/drivers/hwmon/emc1403.c > +++ b/drivers/hwmon/emc1403.c > @@ -37,13 +37,43 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr, > { > struct sensor_device_attribute *sda = to_sensor_dev_attr(attr); > struct thermal_data *data = dev_get_drvdata(dev); > - 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. > int retval; > + int idx_lowbyte; > + > + switch (sda->index) { > + case 0x00: > + idx_lowbyte = 0x29; > + break; > + case 0x01: > + idx_lowbyte = 0x10; > + break; > + case 0x23: > + case 0x2a: > + case 0x41: > + case 0x43: > + case 0x45: > + case 0x47: > + idx_lowbyte = sda->index + 1; > + break; What about the following ? 2c -> 2e 2d -> 2f > + default: > + idx_lowbyte = 0; > + } > > 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. > + } else { > + val_lowbyte = 0; > + } > + > + return sprintf(buf, "%d\n", > + (((val << 8) | val_lowbyte) * (u32)1000) >> 8); The u32 typecast is unnecessary and would interfer with negative temperatures. Guenter
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 ? > > > Signed-off-by: Lars Petter Mostad <lars.petter.mostad@appear.net> > > --- > > drivers/hwmon/emc1403.c | 34 ++++++++++++++++++++++++++++++++-- > > 1 file changed, 32 insertions(+), 2 deletions(-) > > > > > > base-commit: e723f6ca39fb54ae31f79b5af74fe8496308d4de > > > > diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c > > index d370efd6f986..2b14db802f96 100644 > > --- a/drivers/hwmon/emc1403.c > > +++ b/drivers/hwmon/emc1403.c > > @@ -37,13 +37,43 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr, > > { > > struct sensor_device_attribute *sda = to_sensor_dev_attr(attr); > > struct thermal_data *data = dev_get_drvdata(dev); > > - 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. > > > int retval; > > + int idx_lowbyte; > > + > > + switch (sda->index) { > > + case 0x00: > > + idx_lowbyte = 0x29; > > + break; > > + case 0x01: > > + idx_lowbyte = 0x10; > > + break; > > + case 0x23: > > + case 0x2a: > > + case 0x41: > > + case 0x43: > > + case 0x45: > > + case 0x47: > > + idx_lowbyte = sda->index + 1; > > + break; > > What about the following ? > > 2c -> 2e > 2d -> 2f Also all other limit registers, and for those the write part is missing. Guenter
On Sun, Apr 28, 2024 at 8:07 PM Guenter Roeck <linux@roeck-us.net> 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
On 4/30/24 04:11, Lars Petter Mostad wrote: > On Sun, Apr 28, 2024 at 8:07 PM Guenter Roeck <linux@roeck-us.net> 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
diff --git a/drivers/hwmon/emc1403.c b/drivers/hwmon/emc1403.c index d370efd6f986..2b14db802f96 100644 --- a/drivers/hwmon/emc1403.c +++ b/drivers/hwmon/emc1403.c @@ -37,13 +37,43 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr, { struct sensor_device_attribute *sda = to_sensor_dev_attr(attr); struct thermal_data *data = dev_get_drvdata(dev); - unsigned int val; + unsigned int val, val_lowbyte; int retval; + int idx_lowbyte; + + switch (sda->index) { + case 0x00: + idx_lowbyte = 0x29; + break; + case 0x01: + idx_lowbyte = 0x10; + break; + case 0x23: + case 0x2a: + case 0x41: + case 0x43: + case 0x45: + case 0x47: + idx_lowbyte = sda->index + 1; + break; + default: + idx_lowbyte = 0; + } 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; + } else { + val_lowbyte = 0; + } + + return sprintf(buf, "%d\n", + (((val << 8) | val_lowbyte) * (u32)1000) >> 8); } static ssize_t bit_show(struct device *dev, struct device_attribute *attr,
Decode all diode data low byte registers. Signed-off-by: Lars Petter Mostad <lars.petter.mostad@appear.net> --- drivers/hwmon/emc1403.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) base-commit: e723f6ca39fb54ae31f79b5af74fe8496308d4de