* [PATCH] hwmon: (amc6821) sign extension temperature @ 2016-11-16 22:23 Matt Weber 2016-11-16 22:51 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Matt Weber @ 2016-11-16 22:23 UTC (permalink / raw) To: linux-hwmon; +Cc: Jared Bents, Matt Weber From: Jared Bents <jared.bents@rockwellcollins.com> Converts the unsigned temperature values from the i2c read to be sign extended as defined in the datasheet so that negative temperatures are properly read. Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> --- drivers/hwmon/amc6821.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index 12e851a..d7368f7 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) !data->valid) { for (i = 0; i < TEMP_IDX_LEN; i++) - data->temp[i] = i2c_smbus_read_byte_data(client, + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, temp_reg[i]); data->stat1 = i2c_smbus_read_byte_data(client, -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hwmon: (amc6821) sign extension temperature 2016-11-16 22:23 [PATCH] hwmon: (amc6821) sign extension temperature Matt Weber @ 2016-11-16 22:51 ` Guenter Roeck 2016-11-17 20:08 ` Jared Bents 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2016-11-16 22:51 UTC (permalink / raw) To: Matt Weber; +Cc: linux-hwmon, Jared Bents On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote: > From: Jared Bents <jared.bents@rockwellcollins.com> > > Converts the unsigned temperature values from the i2c read > to be sign extended as defined in the datasheet so that > negative temperatures are properly read. > > Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> > Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> > --- > drivers/hwmon/amc6821.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > index 12e851a..d7368f7 100644 > --- a/drivers/hwmon/amc6821.c > +++ b/drivers/hwmon/amc6821.c > @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) > !data->valid) { > > for (i = 0; i < TEMP_IDX_LEN; i++) > - data->temp[i] = i2c_smbus_read_byte_data(client, > + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, How does that fix anything ? The only difference is that errors reported from i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no longer as negative numbers. I don't see how a value of 0xff read from the chip would now be reported as -1 degrees C; it should be reported as 255 degrees C as it was all along. What am I missing here ? A real fix would be to actually check for errors either here or in the show function (temp[] < 0 indicates a transfer error), and to use sign_extend() to convert the 8-bit reading to a signed value. Guenter > temp_reg[i]); > > data->stat1 = i2c_smbus_read_byte_data(client, > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hwmon: (amc6821) sign extension temperature 2016-11-16 22:51 ` Guenter Roeck @ 2016-11-17 20:08 ` Jared Bents 2016-11-17 22:18 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Jared Bents @ 2016-11-17 20:08 UTC (permalink / raw) To: Guenter Roeck; +Cc: Matt Weber, linux-hwmon On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote: >> From: Jared Bents <jared.bents@rockwellcollins.com> >> >> Converts the unsigned temperature values from the i2c read >> to be sign extended as defined in the datasheet so that >> negative temperatures are properly read. >> >> Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> >> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> >> --- >> drivers/hwmon/amc6821.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >> index 12e851a..d7368f7 100644 >> --- a/drivers/hwmon/amc6821.c >> +++ b/drivers/hwmon/amc6821.c >> @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) >> !data->valid) { >> >> for (i = 0; i < TEMP_IDX_LEN; i++) >> - data->temp[i] = i2c_smbus_read_byte_data(client, >> + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, > > How does that fix anything ? The only difference is that errors reported from > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no longer > as negative numbers. I don't see how a value of 0xff read from the chip would > now be reported as -1 degrees C; it should be reported as 255 degrees C as it > was all along. What am I missing here ? > > A real fix would be to actually check for errors either here or in the show > function (temp[] < 0 indicates a transfer error), and to use sign_extend() > to convert the 8-bit reading to a signed value. > > Guenter > As stated in the patch note, the amc6821 uses signed numbers for the temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the temperature range table in the amc6821 datasheet. This change does not break any error checking when reading the temperature over the i2c bus in this driver as this driver does not do any error checking for the temperature reads to begin with. There are error checks in the driver but they are for some i2c writes and a few i2c reads for configuration settings. None of those error checks are affected. Without this patch, the temperatures that are displayed in /sys/class when below 0 Deg C are 255 Deg C to 128 Deg C. With the patch, the temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as expected. Jared >> temp_reg[i]); >> >> data->stat1 = i2c_smbus_read_byte_data(client, >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hwmon: (amc6821) sign extension temperature 2016-11-17 20:08 ` Jared Bents @ 2016-11-17 22:18 ` Guenter Roeck 2016-11-18 21:28 ` Matthew Weber 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2016-11-17 22:18 UTC (permalink / raw) To: Jared Bents; +Cc: Matt Weber, linux-hwmon On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote: > On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote: > >> From: Jared Bents <jared.bents@rockwellcollins.com> > >> > >> Converts the unsigned temperature values from the i2c read > >> to be sign extended as defined in the datasheet so that > >> negative temperatures are properly read. > >> > >> Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> > >> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> > >> --- > >> drivers/hwmon/amc6821.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > >> index 12e851a..d7368f7 100644 > >> --- a/drivers/hwmon/amc6821.c > >> +++ b/drivers/hwmon/amc6821.c > >> @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) > >> !data->valid) { > >> > >> for (i = 0; i < TEMP_IDX_LEN; i++) > >> - data->temp[i] = i2c_smbus_read_byte_data(client, > >> + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, > > > > How does that fix anything ? The only difference is that errors reported from > > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no longer > > as negative numbers. I don't see how a value of 0xff read from the chip would > > now be reported as -1 degrees C; it should be reported as 255 degrees C as it > > was all along. What am I missing here ? > > > > A real fix would be to actually check for errors either here or in the show > > function (temp[] < 0 indicates a transfer error), and to use sign_extend() > > to convert the 8-bit reading to a signed value. > > > > Guenter > > > > As stated in the patch note, the amc6821 uses signed numbers for the > temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the > temperature range table in the amc6821 datasheet. This change does > not break any error checking when reading the temperature over the i2c > bus in this driver as this driver does not do any error checking for > the temperature reads to begin with. There are error checks in the > driver but they are for some i2c writes and a few i2c reads for > configuration settings. None of those error checks are affected. > Without this patch, the temperatures that are displayed in /sys/class > when below 0 Deg C are 255 Deg C to 128 Deg C. With the patch, the > temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as > expected. > Ah, yes, it is "int8_t", which is extended to a negative number. Sorry, I somehow read it as unsigned. Please run your patch through checkpatch --strict, fix what it reports, and resubmit. Thanks, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hwmon: (amc6821) sign extension temperature 2016-11-17 22:18 ` Guenter Roeck @ 2016-11-18 21:28 ` Matthew Weber 2016-11-18 23:13 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Matthew Weber @ 2016-11-18 21:28 UTC (permalink / raw) To: Guenter Roeck; +Cc: Jared Bents, linux-hwmon Guenter, On Thu, Nov 17, 2016 at 4:18 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote: > > On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote: > > >> From: Jared Bents <jared.bents@rockwellcollins.com> > > >> > > >> Converts the unsigned temperature values from the i2c read > > >> to be sign extended as defined in the datasheet so that > > >> negative temperatures are properly read. > > >> > > >> Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> > > >> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> > > >> --- > > >> drivers/hwmon/amc6821.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > > >> index 12e851a..d7368f7 100644 > > >> --- a/drivers/hwmon/amc6821.c > > >> +++ b/drivers/hwmon/amc6821.c > > >> @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) > > >> !data->valid) { > > >> > > >> for (i = 0; i < TEMP_IDX_LEN; i++) > > >> - data->temp[i] = i2c_smbus_read_byte_data(client, > > >> + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, > > > > > > How does that fix anything ? The only difference is that errors reported from > > > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no longer > > > as negative numbers. I don't see how a value of 0xff read from the chip would > > > now be reported as -1 degrees C; it should be reported as 255 degrees C as it > > > was all along. What am I missing here ? > > > > > > A real fix would be to actually check for errors either here or in the show > > > function (temp[] < 0 indicates a transfer error), and to use sign_extend() > > > to convert the 8-bit reading to a signed value. > > > > > > Guenter > > > > > > > As stated in the patch note, the amc6821 uses signed numbers for the > > temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the > > temperature range table in the amc6821 datasheet. This change does > > not break any error checking when reading the temperature over the i2c > > bus in this driver as this driver does not do any error checking for > > the temperature reads to begin with. There are error checks in the > > driver but they are for some i2c writes and a few i2c reads for > > configuration settings. None of those error checks are affected. > > Without this patch, the temperatures that are displayed in /sys/class > > when below 0 Deg C are 255 Deg C to 128 Deg C. With the patch, the > > temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as > > expected. > > > Ah, yes, it is "int8_t", which is extended to a negative number. > Sorry, I somehow read it as unsigned. > > Please run your patch through checkpatch --strict, fix what it reports, > and resubmit. I assume we fix errors but wanted to check on warnings as it looks like this file has a lot. -- Matthew L Weber / Pr Software Engineer Airborne Information Systems / Security Systems and Software / Secure Platforms MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA www.rockwellcollins.com Note: Any Export License Required Information and License Restricted Third Party Intellectual Property (TPIP) content must be encrypted and sent to matthew.weber@corp.rockwellcollins.com. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hwmon: (amc6821) sign extension temperature 2016-11-18 21:28 ` Matthew Weber @ 2016-11-18 23:13 ` Guenter Roeck 0 siblings, 0 replies; 6+ messages in thread From: Guenter Roeck @ 2016-11-18 23:13 UTC (permalink / raw) To: Matthew Weber; +Cc: Jared Bents, linux-hwmon On Fri, Nov 18, 2016 at 03:28:30PM -0600, Matthew Weber wrote: > Guenter, > > On Thu, Nov 17, 2016 at 4:18 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote: > > > On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote: > > > >> From: Jared Bents <jared.bents@rockwellcollins.com> > > > >> > > > >> Converts the unsigned temperature values from the i2c read > > > >> to be sign extended as defined in the datasheet so that > > > >> negative temperatures are properly read. > > > >> > > > >> Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com> > > > >> Signed-off-by: Matt Weber <matthew.weber@rockwellcollins.com> > > > >> --- > > > >> drivers/hwmon/amc6821.c | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > > > >> index 12e851a..d7368f7 100644 > > > >> --- a/drivers/hwmon/amc6821.c > > > >> +++ b/drivers/hwmon/amc6821.c > > > >> @@ -188,7 +188,7 @@ static struct amc6821_data *amc6821_update_device(struct device *dev) > > > >> !data->valid) { > > > >> > > > >> for (i = 0; i < TEMP_IDX_LEN; i++) > > > >> - data->temp[i] = i2c_smbus_read_byte_data(client, > > > >> + data->temp[i] = (int8_t) i2c_smbus_read_byte_data(client, > > > > > > > > How does that fix anything ? The only difference is that errors reported from > > > > i2c_smbus_read_byte_data() are now capped off and stored as 0xff and no longer > > > > as negative numbers. I don't see how a value of 0xff read from the chip would > > > > now be reported as -1 degrees C; it should be reported as 255 degrees C as it > > > > was all along. What am I missing here ? > > > > > > > > A real fix would be to actually check for errors either here or in the show > > > > function (temp[] < 0 indicates a transfer error), and to use sign_extend() > > > > to convert the 8-bit reading to a signed value. > > > > > > > > Guenter > > > > > > > > > > As stated in the patch note, the amc6821 uses signed numbers for the > > > temperature values. 0xff is -1 Deg C and 0x80 is -128 Deg C via the > > > temperature range table in the amc6821 datasheet. This change does > > > not break any error checking when reading the temperature over the i2c > > > bus in this driver as this driver does not do any error checking for > > > the temperature reads to begin with. There are error checks in the > > > driver but they are for some i2c writes and a few i2c reads for > > > configuration settings. None of those error checks are affected. > > > Without this patch, the temperatures that are displayed in /sys/class > > > when below 0 Deg C are 255 Deg C to 128 Deg C. With the patch, the > > > temperatures displayed below 0 Deg C are -1 Deg C to -128 Deg C as > > > expected. > > > > > Ah, yes, it is "int8_t", which is extended to a negative number. > > Sorry, I somehow read it as unsigned. > > > > Please run your patch through checkpatch --strict, fix what it reports, > > and resubmit. > > > I assume we fix errors but wanted to check on warnings as it looks > like this file has a lot. > That is not a reason to introduce new warnings and check messages in a new patch. I did not ask you to fix all the checkpatch problems in this file, only the ones reported in your patch. Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-18 23:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-16 22:23 [PATCH] hwmon: (amc6821) sign extension temperature Matt Weber 2016-11-16 22:51 ` Guenter Roeck 2016-11-17 20:08 ` Jared Bents 2016-11-17 22:18 ` Guenter Roeck 2016-11-18 21:28 ` Matthew Weber 2016-11-18 23:13 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.