[PATCH] battery: max17042: Fix temperature unit to milli centigrade.

[PATCH] battery: max17042: Fix temperature unit to milli centigrade.

[Jonghwa Lee]

Standard temp node represents temperature of power supply class
in milli centigrade. So fix max17042 fuel gauge driver to follow
the standard. (deci-centigrade --> milli-centigrade)

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
---
 drivers/power/max17042_battery.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index d664ef5..892ecfc 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -220,9 +220,9 @@ static int max17042_get_property(struct power_supply *psy,
 			val->intval = (0x7fff & ~val->intval) + 1;
 			val->intval *= -1;
 		}
-		/* The value is converted into deci-centigrade scale */
+		/* The value is converted into milli-centigrade scale */
 		/* Units of LSB = 1 / 256 degree Celsius */
-		val->intval = val->intval * 10 / 256;
+		val->intval = val->intval * 1000 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (chip->pdata->enable_current_sense) {
-- 
1.7.9.5

Re: [PATCH] battery: max17042: Fix temperature unit to milli centigrade.

[Anton Vorontsov]

On Fri, Oct 25, 2013 at 01:53:18PM +0900, Jonghwa Lee wrote:
> Standard temp node represents temperature of power supply class
> in milli centigrade. So fix max17042 fuel gauge driver to follow
> the standard. (deci-centigrade --> milli-centigrade)

Maybe I am confused, but the doc says:

 * All voltages, currents, charges, energies, time and temperatures in uV,      
 * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise          
 * stated

So, the current code seems to be correct.

Thanks,

Anton

> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
> ---
>  drivers/power/max17042_battery.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> index d664ef5..892ecfc 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -220,9 +220,9 @@ static int max17042_get_property(struct power_supply *psy,
>  			val->intval = (0x7fff & ~val->intval) + 1;
>  			val->intval *= -1;
>  		}
> -		/* The value is converted into deci-centigrade scale */
> +		/* The value is converted into milli-centigrade scale */
>  		/* Units of LSB = 1 / 256 degree Celsius */
> -		val->intval = val->intval * 10 / 256;
> +		val->intval = val->intval * 1000 / 256;
>  		break;
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  		if (chip->pdata->enable_current_sense) {
> -- 
> 1.7.9.5

Re: [PATCH] battery: max17042: Fix temperature unit to milli centigrade.

[jonghwa3.lee]

Hi,
On 2013년 10월 26일 08:17, Anton Vorontsov wrote:

> On Fri, Oct 25, 2013 at 01:53:18PM +0900, Jonghwa Lee wrote:
>> Standard temp node represents temperature of power supply class
>> in milli centigrade. So fix max17042 fuel gauge driver to follow
>> the standard. (deci-centigrade --> milli-centigrade)
> 
> Maybe I am confused, but the doc says:
> 
>  * All voltages, currents, charges, energies, time and temperatures in uV,      
>  * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise          
>  * stated
> 
> So, the current code seems to be correct.
> 


Honestly, I missed the above paragraph you showed rather I read following one.

TEMP - temperature of the power supply.
TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
TEMP_AMBIENT - ambient temperature.
TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli
centigrade.
TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli
centigrade.

So, we use different unit for properties related temperature, right?
current temperature is in tenth of centigrade and threshold temperatures and
ambient temperature are in milli centigrade. Wouldn't it have to be in same unit?

Thanks,
Jonghwa

> Thanks,
> 
> Anton
> 
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>> Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>
>> ---
>>  drivers/power/max17042_battery.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
>> index d664ef5..892ecfc 100644
>> --- a/drivers/power/max17042_battery.c
>> +++ b/drivers/power/max17042_battery.c
>> @@ -220,9 +220,9 @@ static int max17042_get_property(struct power_supply *psy,
>>  			val->intval = (0x7fff & ~val->intval) + 1;
>>  			val->intval *= -1;
>>  		}
>> -		/* The value is converted into deci-centigrade scale */
>> +		/* The value is converted into milli-centigrade scale */
>>  		/* Units of LSB = 1 / 256 degree Celsius */
>> -		val->intval = val->intval * 10 / 256;
>> +		val->intval = val->intval * 1000 / 256;
>>  		break;
>>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>>  		if (chip->pdata->enable_current_sense) {
>> -- 
>> 1.7.9.5
> 

[PATCH] power_supply: Fix documentation for TEMP_*ALERT* properties

[Anton Vorontsov]

All temperatures should be in tenth degrees Celsius.

bq24190_charger.c probably should be fixed.

Reported-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Cc: Mark A. Greer <mgreer@animalcreek.com>
Signed-off-by: Anton Vorontsov <anton@enomsg.org>
---

On Mon, Oct 28, 2013 at 11:00:52AM +0900, jonghwa3.lee@samsung.com wrote:
> >  * All voltages, currents, charges, energies, time and temperatures in uV,      
> >  * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise          
> >  * stated
> > 
> > So, the current code seems to be correct.
> 
> Honestly, I missed the above paragraph you showed rather I read following one.
> 
> TEMP - temperature of the power supply.
> TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
> TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
> TEMP_AMBIENT - ambient temperature.
> TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli
> centigrade.
> TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli
> centigrade.
> 
> So, we use different unit for properties related temperature, right?
> current temperature is in tenth of centigrade and threshold temperatures and
> ambient temperature are in milli centigrade. Wouldn't it have to be in same unit?

:( They should. Thanks for spotting.

The patch down below should fix the issue...

Documentation/power/power_supply_class.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
index 3f10b39..89a8816 100644
--- a/Documentation/power/power_supply_class.txt
+++ b/Documentation/power/power_supply_class.txt
@@ -135,11 +135,11 @@ CAPACITY_LEVEL - capacity level. This corresponds to
 POWER_SUPPLY_CAPACITY_LEVEL_*.
 
 TEMP - temperature of the power supply.
-TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
-TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
+TEMP_ALERT_MIN - minimum battery temperature alert.
+TEMP_ALERT_MAX - maximum battery temperature alert.
 TEMP_AMBIENT - ambient temperature.
-TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli centigrade.
-TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli centigrade.
+TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert.
+TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert.
 
 TIME_TO_EMPTY - seconds left for battery to be considered empty (i.e.
 while battery powers a load)
-- 
1.8.3.1

Re: [PATCH] power_supply: Fix documentation for TEMP_*ALERT* properties

[jonghwa3.lee]

On 2013년 10월 28일 14:27, Anton Vorontsov wrote:

> All temperatures should be in tenth degrees Celsius.
> 


Let me tell you one thing that thermal framework uses milli centigrade for
temperature. And also we have some relation with thermal framework already in
power suppply core. So, what do think of using milli centigrade in power supply
class either?

Thanks,
Jonghwa

> bq24190_charger.c probably should be fixed.
> 
> Reported-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Signed-off-by: Anton Vorontsov <anton@enomsg.org>
> ---
> 
> On Mon, Oct 28, 2013 at 11:00:52AM +0900, jonghwa3.lee@samsung.com wrote:
>>>  * All voltages, currents, charges, energies, time and temperatures in uV,      
>>>  * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise          
>>>  * stated
>>>
>>> So, the current code seems to be correct.
>>
>> Honestly, I missed the above paragraph you showed rather I read following one.
>>
>> TEMP - temperature of the power supply.
>> TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
>> TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
>> TEMP_AMBIENT - ambient temperature.
>> TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli
>> centigrade.
>> TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli
>> centigrade.
>>
>> So, we use different unit for properties related temperature, right?
>> current temperature is in tenth of centigrade and threshold temperatures and
>> ambient temperature are in milli centigrade. Wouldn't it have to be in same unit?
> 
> :( They should. Thanks for spotting.
> 
> The patch down below should fix the issue...
> 
> Documentation/power/power_supply_class.txt | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> index 3f10b39..89a8816 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -135,11 +135,11 @@ CAPACITY_LEVEL - capacity level. This corresponds to
>  POWER_SUPPLY_CAPACITY_LEVEL_*.
>  
>  TEMP - temperature of the power supply.
> -TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
> -TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
> +TEMP_ALERT_MIN - minimum battery temperature alert.
> +TEMP_ALERT_MAX - maximum battery temperature alert.
>  TEMP_AMBIENT - ambient temperature.
> -TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli centigrade.
> -TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli centigrade.
> +TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert.
> +TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert.
>  
>  TIME_TO_EMPTY - seconds left for battery to be considered empty (i.e.
>  while battery powers a load)

Re: [PATCH] power_supply: Fix documentation for TEMP_*ALERT* properties

[Anton Vorontsov]

On Mon, Oct 28, 2013 at 03:22:53PM +0900, jonghwa3.lee@samsung.com wrote:
> > All temperatures should be in tenth degrees Celsius.
> 
> Let me tell you one thing that thermal framework uses milli centigrade for
> temperature. And also we have some relation with thermal framework already in
> power suppply core. So, what do think of using milli centigrade in power supply
> class either?

Since tenth degrees stuff has been exposed to the userland for 5+ years we
can't just change it. So, no.

If for any reason you need a higher precision temperatures (do you?), then
you are welcome to introduce a set of new properties with a _MILLI affix.

Anton

Re: [PATCH] power_supply: Fix documentation for TEMP_*ALERT* properties

[Mark A. Greer]

On Sun, Oct 27, 2013 at 10:27:43PM -0700, Anton Vorontsov wrote:
> All temperatures should be in tenth degrees Celsius.

> bq24190_charger.c probably should be fixed.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> 
> Reported-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Signed-off-by: Anton Vorontsov <anton@enomsg.org>
> ---
> 
> On Mon, Oct 28, 2013 at 11:00:52AM +0900, jonghwa3.lee@samsung.com wrote:
> > >  * All voltages, currents, charges, energies, time and temperatures in uV,      
> > >  * µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise          
> > >  * stated
> > > 
> > > So, the current code seems to be correct.
> > 
> > Honestly, I missed the above paragraph you showed rather I read following one.
> > 
> > TEMP - temperature of the power supply.
> > TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
> > TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
> > TEMP_AMBIENT - ambient temperature.
> > TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli
> > centigrade.
> > TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli
> > centigrade.
> > 
> > So, we use different unit for properties related temperature, right?
> > current temperature is in tenth of centigrade and threshold temperatures and
> > ambient temperature are in milli centigrade. Wouldn't it have to be in same unit?
> 
> :( They should. Thanks for spotting.
> 
> The patch down below should fix the issue...
> 
> Documentation/power/power_supply_class.txt | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> index 3f10b39..89a8816 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -135,11 +135,11 @@ CAPACITY_LEVEL - capacity level. This corresponds to
>  POWER_SUPPLY_CAPACITY_LEVEL_*.
>  
>  TEMP - temperature of the power supply.
> -TEMP_ALERT_MIN - minimum battery temperature alert value in milli centigrade.
> -TEMP_ALERT_MAX - maximum battery temperature alert value in milli centigrade.
> +TEMP_ALERT_MIN - minimum battery temperature alert.
> +TEMP_ALERT_MAX - maximum battery temperature alert.
>  TEMP_AMBIENT - ambient temperature.
> -TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert value in milli centigrade.
> -TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert value in milli centigrade.
> +TEMP_AMBIENT_ALERT_MIN - minimum ambient temperature alert.
> +TEMP_AMBIENT_ALERT_MAX - maximum ambient temperature alert.
>  
>  TIME_TO_EMPTY - seconds left for battery to be considered empty (i.e.
>  while battery powers a load)
> -- 
> 1.8.3.1

I missed the beginning of this thread so I may be missing something but
the bq24190_charger driver already uses tenth of degrees Celcius for
POWER_SUPPLY_PROP_TEMP_ALERT_MAX.  IIUC, that's correct so there's is
nothing to fix in the bq24190_charger.c driver, correct?

Mark
--

Re: [PATCH] power_supply: Fix documentation for TEMP_*ALERT* properties

[Anton Vorontsov]

On Mon, Oct 28, 2013 at 10:20:08AM -0700, Mark A. Greer wrote:
> On Sun, Oct 27, 2013 at 10:27:43PM -0700, Anton Vorontsov wrote:
> > All temperatures should be in tenth degrees Celsius.
> 
> > bq24190_charger.c probably should be fixed.
...
> I missed the beginning of this thread so I may be missing something but
> the bq24190_charger driver already uses tenth of degrees Celcius for
> POWER_SUPPLY_PROP_TEMP_ALERT_MAX.  IIUC, that's correct so there's is
> nothing to fix in the bq24190_charger.c driver, correct?

Yup, correct. Thanks a lot for confirming!

Anton