hwmon: (amc6821) sign extension temperature

Commit Message

Matt Weber Nov. 16, 2016, 10:23 p.m. UTC

From: Jared Bents <[e-mail]>

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 <[e-mail]>
Signed-off-by: Matt Weber <[e-mail]>
---
 drivers/hwmon/amc6821.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Guenter Roeck Nov. 16, 2016, 10:51 p.m. UTC | #1

On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> From: Jared Bents <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Matt Weber <[e-mail]>
> ---
>  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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jared Bents Nov. 17, 2016, 8:08 p.m. UTC | #2

On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <[e-mail]> wrote:
> On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
>> From: Jared Bents <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Matt Weber <[e-mail]>
>> ---
>>  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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guenter Roeck Nov. 17, 2016, 10:18 p.m. UTC | #3

On Thu, Nov 17, 2016 at 02:08:41PM -0600, Jared Bents wrote:
> On Wed, Nov 16, 2016 at 4:51 PM, Guenter Roeck <[e-mail]> wrote:
> > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> >> From: Jared Bents <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Signed-off-by: Matt Weber <[e-mail]>
> >> ---
> >>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Weber Nov. 18, 2016, 9:28 p.m. UTC | #4

Guenter,

On Thu, Nov 17, 2016 at 4:18 PM, Guenter Roeck <[e-mail]> 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 <[e-mail]> wrote:
> > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> > >> From: Jared Bents <[e-mail]>
> > >>
> > >> 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 <[e-mail]>
> > >> Signed-off-by: Matt Weber <[e-mail]>
> > >> ---
> > >>  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.

Guenter Roeck Nov. 18, 2016, 11:13 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > On Wed, Nov 16, 2016 at 04:23:53PM -0600, Matt Weber wrote:
> > > >> From: Jared Bents <[e-mail]>
> > > >>
> > > >> 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 <[e-mail]>
> > > >> Signed-off-by: Matt Weber <[e-mail]>
> > > >> ---
> > > >>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,