[PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Lubomir Rintel]

According to [1] and [2], the temperature values are in tenths of degree
Celsius. Exposing the Celsius value makes the battery appear on fire:

  $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
  ...
      temperature:         236.9 degrees C

Tested on OLPC XO-1 and OLPC XO-1.75 laptops.

[1] include/linux/power_supply.h
[2] Documentation/power/power_supply_class.txt

Cc: stable@vger.kernel.org
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/power/supply/olpc_battery.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
index 6da79ae14860..5a97e42a3547 100644
--- a/drivers/power/supply/olpc_battery.c
+++ b/drivers/power/supply/olpc_battery.c
@@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
 		if (ret)
 			return ret;
 
-		val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
+		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
 		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
 		if (ret)
 			return ret;
 
-		val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
+		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
 		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
-- 
2.19.0

Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
>
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>       temperature:         236.9 degrees C
>
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.

It's interesting that the very author of that code is not included in
so-o long Cc list :)
Cc: David.

David, do you remember if and how you had tested temperature report of
the battery on OLPC?
I guess this kind of error would be appear immediately.

OTOH it might be that power framework had changed requirements (which
would be noticeable change).
If the latter is true, this patch misses Fixes tag. Actually in any
case it misses it.

>
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
>                 if (ret)
>                         return ret;
>
> -               val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> +               val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>                 ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>                 if (ret)
>                         return ret;
>
> -               val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> +               val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
>                 break;
>         case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>                 ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> --
> 2.19.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

Hi,

On Fri, Oct 19, 2018 at 04:00:32PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <lkundrak@v3.sk> wrote:
> >
> > According to [1] and [2], the temperature values are in tenths of degree
> > Celsius. Exposing the Celsius value makes the battery appear on fire:
> >
> >   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
> >   ...
> >       temperature:         236.9 degrees C
> >
> > Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
> 
> It's interesting that the very author of that code is not included in
> so-o long Cc list :)
> Cc: David.
> 
> David, do you remember if and how you had tested temperature report of
> the battery on OLPC? I guess this kind of error would be appear immediately.

It depends on the way of testing. It's not so obvious when you just
do a `cat /sys/class/power_supply/.../temperature`, since you just
get the raw integer.

> OTOH it might be that power framework had changed requirements (which
> would be noticeable change).

As far as I know, power-supply has always used 1/10 °C. People tend
to get units wrong all the time though (i.e. mV instead of uV). I'm
not surprised, that this sneaked into the kernel.

> If the latter is true, this patch misses Fixes tag. Actually in any
> case it misses it.

Yes, this should probably have Fixes: fb972873a767 ("[BATTERY] One Laptop
Per Child power/battery driver").

-- Sebastian

> > [1] include/linux/power_supply.h
> > [2] Documentation/power/power_supply_class.txt
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  drivers/power/supply/olpc_battery.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> > index 6da79ae14860..5a97e42a3547 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
> >                 if (ret)
> >                         return ret;
> >
> > -               val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> > +               val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> >                 break;
> >         case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> >                 ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
> >                 if (ret)
> >                         return ret;
> >
> > -               val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> > +               val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> >                 break;
> >         case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> >                 ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> > --
> > 2.19.0
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]

On Wed 2018-10-10 19:22:46, Lubomir Rintel wrote:
> According to [1] and [2], the temperature values are in tenths of degree
> Celsius. Exposing the Celsius value makes the battery appear on fire:
> 
>   $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
>   ...
>       temperature:         236.9 degrees C
> 
> Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
> 
> [1] include/linux/power_supply.h
> [2] Documentation/power/power_supply_class.txt
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/power/supply/olpc_battery.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> index 6da79ae14860..5a97e42a3547 100644
> --- a/drivers/power/supply/olpc_battery.c
> +++ b/drivers/power/supply/olpc_battery.c
> @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> +		val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_TEMP_AMBIENT:
>  		ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
>  		if (ret)
>  			return ret;
>  
> -		val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> +		val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]