| Message ID | 20170414125919.25771-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote: > The temp alert values are 8-bit 2's complement, so sign-extend them > before reporting them back to the caller. Are you sure that these are reported with sign bit? I couldn't find confirmation of this in datasheet. Best regards, Krzysztof > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/max17042_battery.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c > index 790dfa9..a51b296 100644 > --- a/drivers/power/supply/max17042_battery.c > +++ b/drivers/power/supply/max17042_battery.c > @@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy, > if (ret < 0) > return ret; > /* LSB is Alert Minimum. In deci-centigrade */ > - val->intval = (data & 0xff) * 10; > + val->intval = sign_extend32(data & 0xff, 7) * 10; > break; > case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > ret = regmap_read(map, MAX17042_TALRT_Th, &data); > if (ret < 0) > return ret; > /* MSB is Alert Maximum. In deci-centigrade */ > - val->intval = (data >> 8) * 10; > + val->intval = sign_extend32(data >> 8, 7) * 10; > break; > case POWER_SUPPLY_PROP_TEMP_MIN: > val->intval = chip->pdata->temp_min; > -- > 2.9.3 >
Hi, On 14-04-17 17:09, Krzysztof Kozlowski wrote: > On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote: >> The temp alert values are 8-bit 2's complement, so sign-extend them >> before reporting them back to the caller. > > Are you sure that these are reported with sign bit? I couldn't find > confirmation of this in datasheet. From: MAX17047-MAX17050.pdf "T ALRT Threshold Register (02h) The T ALRT Threshold register sets upper and lower limits that generate an ALRT pin interrupt if exceeded by the Temperature register value. The upper 8 bits set the maxi- mum value and the lower 8 bits set the minimum value. Interrupt threshold limits are stored in two’s-complement format" And the reset default of 7F80h also hints at this, as it is +127 for max -128 for min (aka temp based alerts disabled). Regards, Hans > > Best regards, > Krzysztof > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/power/supply/max17042_battery.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c >> index 790dfa9..a51b296 100644 >> --- a/drivers/power/supply/max17042_battery.c >> +++ b/drivers/power/supply/max17042_battery.c >> @@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy, >> if (ret < 0) >> return ret; >> /* LSB is Alert Minimum. In deci-centigrade */ >> - val->intval = (data & 0xff) * 10; >> + val->intval = sign_extend32(data & 0xff, 7) * 10; >> break; >> case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: >> ret = regmap_read(map, MAX17042_TALRT_Th, &data); >> if (ret < 0) >> return ret; >> /* MSB is Alert Maximum. In deci-centigrade */ >> - val->intval = (data >> 8) * 10; >> + val->intval = sign_extend32(data >> 8, 7) * 10; >> break; >> case POWER_SUPPLY_PROP_TEMP_MIN: >> val->intval = chip->pdata->temp_min; >> -- >> 2.9.3 >>
On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote: > Hi, > > On 14-04-17 17:09, Krzysztof Kozlowski wrote: > > On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote: > > > The temp alert values are 8-bit 2's complement, so sign-extend them > > > before reporting them back to the caller. > > > > Are you sure that these are reported with sign bit? I couldn't find > > confirmation of this in datasheet. > > From: MAX17047-MAX17050.pdf > > "T ALRT Threshold Register (02h) > The T ALRT Threshold register sets upper and lower limits > that generate an ALRT pin interrupt if exceeded by the > Temperature register value. The upper 8 bits set the maxi- > mum value and the lower 8 bits set the minimum value. > Interrupt threshold limits are stored in two’s-complement > format" That does not say it is signed. It might be still from 0 to 255. > And the reset default of 7F80h also hints at this, > as it is +127 for max -128 for min (aka temp based > alerts disabled). In this case looks correct... but max77693 fuel gauge datasheet says about TALRT_Th (0x02): Reset value: 0x00FF "...At power up, the thresholds default to their maximum settings- 7F80h (disabled)." which as you see contains to different reset values... Ehhh... Max17047max17050 datasheet looks a little bit more chaotic/organized so I guess it should be used as a source. Best regards, Krzysztof
Hi, On 14-04-17 17:26, Krzysztof Kozlowski wrote: > On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote: >> Hi, >> >> On 14-04-17 17:09, Krzysztof Kozlowski wrote: >>> On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote: >>>> The temp alert values are 8-bit 2's complement, so sign-extend them >>>> before reporting them back to the caller. >>> >>> Are you sure that these are reported with sign bit? I couldn't find >>> confirmation of this in datasheet. >> >> From: MAX17047-MAX17050.pdf >> >> "T ALRT Threshold Register (02h) >> The T ALRT Threshold register sets upper and lower limits >> that generate an ALRT pin interrupt if exceeded by the >> Temperature register value. The upper 8 bits set the maxi- >> mum value and the lower 8 bits set the minimum value. >> Interrupt threshold limits are stored in two’s-complement >> format" > > That does not say it is signed. It might be still from 0 to 255. Two's complement format is always signed that is its whole purpose, from: https://en.wikipedia.org/wiki/Two's_complement "Two's complement is a ... binary signed number representation" >> And the reset default of 7F80h also hints at this, >> as it is +127 for max -128 for min (aka temp based >> alerts disabled). > > In this case looks correct... but max77693 fuel gauge datasheet says > about TALRT_Th (0x02): > Reset value: 0x00FF > "...At power up, the thresholds default to their maximum settings- 7F80h > (disabled)." > which as you see contains to different reset values... Notice that that datasheet is contradicting itself, it says: "Reset value: 0x00FF" but also "at power up, the thresholds default to their maximum settings- 7F80h" I can confirm on my max17047 that the latter is correct. > Ehhh... Max17047max17050 datasheet looks a little bit more > chaotic/organized so I guess it should be used as a source. Ack. Regards, Hans
On Fri, Apr 14, 2017 at 05:36:57PM +0200, Hans de Goede wrote: > Hi, > > On 14-04-17 17:26, Krzysztof Kozlowski wrote: > > On Fri, Apr 14, 2017 at 05:16:32PM +0200, Hans de Goede wrote: > > > Hi, > > > > > > On 14-04-17 17:09, Krzysztof Kozlowski wrote: > > > > On Fri, Apr 14, 2017 at 02:59:09PM +0200, Hans de Goede wrote: > > > > > The temp alert values are 8-bit 2's complement, so sign-extend them > > > > > before reporting them back to the caller. > > > > > > > > Are you sure that these are reported with sign bit? I couldn't find > > > > confirmation of this in datasheet. > > > > > > From: MAX17047-MAX17050.pdf > > > > > > "T ALRT Threshold Register (02h) > > > The T ALRT Threshold register sets upper and lower limits > > > that generate an ALRT pin interrupt if exceeded by the > > > Temperature register value. The upper 8 bits set the maxi- > > > mum value and the lower 8 bits set the minimum value. > > > Interrupt threshold limits are stored in two’s-complement > > > format" > > > > That does not say it is signed. It might be still from 0 to 255. > > Two's complement format is always signed that is its whole > purpose, from: https://en.wikipedia.org/wiki/Two's_complement > Ahh, yes. Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c index 790dfa9..a51b296 100644 --- a/drivers/power/supply/max17042_battery.c +++ b/drivers/power/supply/max17042_battery.c @@ -270,14 +270,14 @@ static int max17042_get_property(struct power_supply *psy, if (ret < 0) return ret; /* LSB is Alert Minimum. In deci-centigrade */ - val->intval = (data & 0xff) * 10; + val->intval = sign_extend32(data & 0xff, 7) * 10; break; case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: ret = regmap_read(map, MAX17042_TALRT_Th, &data); if (ret < 0) return ret; /* MSB is Alert Maximum. In deci-centigrade */ - val->intval = (data >> 8) * 10; + val->intval = sign_extend32(data >> 8, 7) * 10; break; case POWER_SUPPLY_PROP_TEMP_MIN: val->intval = chip->pdata->temp_min;
The temp alert values are 8-bit 2's complement, so sign-extend them before reporting them back to the caller. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/max17042_battery.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)