[PATCH V1] fix adc to voltage calculation in da9052 power driver

[PATCH V1] fix adc to voltage calculation in da9052 power driver

[Anthony Olech]

The ADC resolution of the PMIC is 10-bits, this means that the maximum
possible value is 1023 and not the 1024 as in the code.

Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
---
This patch is relative to linux-next repository tag next-20131218

 drivers/power/da9052-battery.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/da9052-battery.c b/drivers/power/da9052-battery.c
index f8f4c0f..8f0f259 100644
--- a/drivers/power/da9052-battery.c
+++ b/drivers/power/da9052-battery.c
@@ -178,7 +178,7 @@ struct da9052_battery {
 
 static inline int volt_reg_to_mV(int value)
 {
-	return ((value * 1000) / 512) + 2500;
+	return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
 }
 
 static inline int ichg_reg_to_mA(int value)
-- 
end-of-patch for fix adc to voltage calculation in da9052 power driver V1

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Jean Delvare]

On Wed, 18 Dec 2013 15:21:13 +0000, Anthony Olech wrote:
> The ADC resolution of the PMIC is 10-bits, this means that the maximum
> possible value is 1023 and not the 1024 as in the code.

The conversion from register value to mV depends on the ADC's LSB, not
its range. So the maximum value which can be represented is irrelevant.

> 
> Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> ---
> This patch is relative to linux-next repository tag next-20131218
> 
>  drivers/power/da9052-battery.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/da9052-battery.c b/drivers/power/da9052-battery.c
> index f8f4c0f..8f0f259 100644
> --- a/drivers/power/da9052-battery.c
> +++ b/drivers/power/da9052-battery.c
> @@ -178,7 +178,7 @@ struct da9052_battery {
>  
>  static inline int volt_reg_to_mV(int value)
>  {
> -	return ((value * 1000) / 512) + 2500;
> +	return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
>  }
>  
>  static inline int ichg_reg_to_mA(int value)


-- 
Jean Delvare

RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 18 December 2013 15:33
> To: Opensource [Anthony Olech]
> Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list; open
> list:HARDWARE MONITORING
> Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> power driver
> On Wed, 18 Dec 2013 15:21:13 +0000, Anthony Olech wrote:
> > The ADC resolution of the PMIC is 10-bits, this means that the maximum
> > possible value is 1023 and not the 1024 as in the code.
> The conversion from register value to mV depends on the ADC's LSB, not its
> range. So the maximum value which can be represented is irrelevant.

thanks for the speedy response, but the converted value returned by the function
does depend on the scaling.

For example take the two cases where value in 0x1F0, then the erroneous previous
calculation yields 3469 and the new corrected calculation yields 3470

So please apply the patch as it truly fixes an error.

many thanks

Tony Olech

> > Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> > ---
> > This patch is relative to linux-next repository tag next-20131218
> >  drivers/power/da9052-battery.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/drivers/power/da9052-battery.c
> > b/drivers/power/da9052-battery.c index f8f4c0f..8f0f259 100644
> > --- a/drivers/power/da9052-battery.c
> > +++ b/drivers/power/da9052-battery.c
> > @@ -178,7 +178,7 @@ struct da9052_battery {
> >  static inline int volt_reg_to_mV(int value)  {
> > -	return ((value * 1000) / 512) + 2500;
> > +	return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
> >  }
> >  static inline int ichg_reg_to_mA(int value)
> --
> Jean Delvare

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Guenter Roeck]

On Wed, Dec 18, 2013 at 03:45:32PM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Jean Delvare [mailto:khali@linux-fr.org]
> > Sent: 18 December 2013 15:33
> > To: Opensource [Anthony Olech]
> > Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list; open
> > list:HARDWARE MONITORING
> > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> > power driver
> > On Wed, 18 Dec 2013 15:21:13 +0000, Anthony Olech wrote:
> > > The ADC resolution of the PMIC is 10-bits, this means that the maximum
> > > possible value is 1023 and not the 1024 as in the code.
> > The conversion from register value to mV depends on the ADC's LSB, not its
> > range. So the maximum value which can be represented is irrelevant.
> 
> thanks for the speedy response, but the converted value returned by the function
> does depend on the scaling.
> 
> For example take the two cases where value in 0x1F0, then the erroneous previous
> calculation yields 3469 and the new corrected calculation yields 3470
> 
> So please apply the patch as it truly fixes an error.
> 
AFAICS the previous calculation should return 3468. Replacing the division by
512 with DIV_ROUND_CLOSEST would return 3469, and the new calculation would
return 3470.

Still, an ADC would typically have an LSB. Question here is what that LSB is
(in mV), or in other words what the maximim voltage represented by 0x3ff is.
Current assumption is that it is 1.9531 mV or 2000/1024, which admittedly
is odd. It might be 2000/1023 = 1.9550 mV, or it might be 2 mV.

Thanks,
Guenter

> many thanks
> 
> Tony Olech
> 
> > > Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
> > > ---
> > > This patch is relative to linux-next repository tag next-20131218
> > >  drivers/power/da9052-battery.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > diff --git a/drivers/power/da9052-battery.c
> > > b/drivers/power/da9052-battery.c index f8f4c0f..8f0f259 100644
> > > --- a/drivers/power/da9052-battery.c
> > > +++ b/drivers/power/da9052-battery.c
> > > @@ -178,7 +178,7 @@ struct da9052_battery {
> > >  static inline int volt_reg_to_mV(int value)  {
> > > -	return ((value * 1000) / 512) + 2500;
> > > +	return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
> > >  }
> > >  static inline int ichg_reg_to_mA(int value)
> > --
> > Jean Delvare
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 

RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 18 December 2013 17:24
> To: Opensource [Anthony Olech]
> Cc: Jean Delvare; open list:HARDWARE MONITORING; Anton Vorontsov;
> David Woodhouse; open list; David Dajun Chen
> Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> power driver
> On Wed, Dec 18, 2013 at 03:45:32PM +0000, Opensource [Anthony Olech]
> wrote:
> > > -----Original Message-----
> > > From: Jean Delvare [mailto:khali@linux-fr.org]
> > > Sent: 18 December 2013 15:33
> > > To: Opensource [Anthony Olech]
> > > Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list;
> > > open list:HARDWARE MONITORING
> > > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation
> > > in da9052 power driver On Wed, 18 Dec 2013 15:21:13 +0000, Anthony
> > > Olech wrote:
> > > > The ADC resolution of the PMIC is 10-bits, this means that the
> > > > maximum possible value is 1023 and not the 1024 as in the code.
> > > The conversion from register value to mV depends on the ADC's LSB,
> > > not its range. So the maximum value which can be represented is
> irrelevant.
> > thanks for the speedy response, but the converted value returned by
> > the function does depend on the scaling.
> > For example take the two cases where value in 0x1F0, then the
> > erroneous previous calculation yields 3469 and the new corrected
> > calculation yields 3470
> > So please apply the patch as it truly fixes an error.
> AFAICS the previous calculation should return 3468. Replacing the division by
> 512 with DIV_ROUND_CLOSEST would return 3469, and the new calculation
> would return 3470.
> Still, an ADC would typically have an LSB. Question here is what that LSB is (in
> mV), or in other words what the maximim voltage represented by 0x3ff is.
> Current assumption is that it is 1.9531 mV or 2000/1024, which admittedly is
> odd. It might be 2000/1023 = 1.9550 mV, or it might be 2 mV.

the maximum voltage represented by 0x3ff is 2000 mV, so the existing code is
incorrect and the patch is to correct the error.

many thanks for your comment,

Tony Olech
 
> Thanks,
> Guenter
> > many thanks
> > Tony Olech
> > > > Signed-off-by: Anthony Olech
> > > > <anthony.olech.opensource@diasemi.com>
> > > > This patch is relative to linux-next repository tag next-20131218
> > > >  drivers/power/da9052-battery.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-) diff --git
> > > > a/drivers/power/da9052-battery.c b/drivers/power/da9052-battery.c
> > > > index f8f4c0f..8f0f259 100644
> > > > --- a/drivers/power/da9052-battery.c
> > > > +++ b/drivers/power/da9052-battery.c
> > > > @@ -178,7 +178,7 @@ struct da9052_battery {  static inline int
> > > > volt_reg_to_mV(int value)  {
> > > > -	return ((value * 1000) / 512) + 2500;
> > > > +	return DIV_ROUND_CLOSEST(value * 2000, 1023) + 2500;
> > > >  }
> > > >  static inline int ichg_reg_to_mA(int value)
> > > Jean Delvare 

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Jean Delvare]

Hi Anthony,

On Thu, 19 Dec 2013 14:13:09 +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> > Roeck
> > Sent: 18 December 2013 17:24
> > To: Opensource [Anthony Olech]
> > Cc: Jean Delvare; open list:HARDWARE MONITORING; Anton Vorontsov;
> > David Woodhouse; open list; David Dajun Chen
> > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> > power driver
> > On Wed, Dec 18, 2013 at 03:45:32PM +0000, Opensource [Anthony Olech]
> > wrote:
> > > > -----Original Message-----
> > > > From: Jean Delvare [mailto:khali@linux-fr.org]
> > > > Sent: 18 December 2013 15:33
> > > > To: Opensource [Anthony Olech]
> > > > Cc: Anton Vorontsov; David Woodhouse; David Dajun Chen; open list;
> > > > open list:HARDWARE MONITORING
> > > > Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation
> > > > in da9052 power driver On Wed, 18 Dec 2013 15:21:13 +0000, Anthony
> > > > Olech wrote:
> > > > > The ADC resolution of the PMIC is 10-bits, this means that the
> > > > > maximum possible value is 1023 and not the 1024 as in the code.
> > > > The conversion from register value to mV depends on the ADC's LSB,
> > > > not its range. So the maximum value which can be represented is
> > irrelevant.
> > > thanks for the speedy response, but the converted value returned by
> > > the function does depend on the scaling.
> > > For example take the two cases where value in 0x1F0, then the
> > > erroneous previous calculation yields 3469 and the new corrected
> > > calculation yields 3470
> > > So please apply the patch as it truly fixes an error.
> > AFAICS the previous calculation should return 3468. Replacing the division by
> > 512 with DIV_ROUND_CLOSEST would return 3469, and the new calculation
> > would return 3470.
> > Still, an ADC would typically have an LSB. Question here is what that LSB is (in
> > mV), or in other words what the maximim voltage represented by 0x3ff is.
> > Current assumption is that it is 1.9531 mV or 2000/1024, which admittedly is
> > odd. It might be 2000/1023 = 1.9550 mV, or it might be 2 mV.
> 
> the maximum voltage represented by 0x3ff is 2000 mV, so the existing code is
> incorrect and the patch is to correct the error.

What makes you think so? The datasheet is not public so unfortunately I
cannot check it myself. Please quote the parts of the datasheet which
you think are relevant to convince us.

I and Guenter are only speaking by experience. Product briefs often
mention the ADC range as an informative value and this is not something
you can rely on. For example 8-bit ADC are often referred to as having
a 2 V, 3 V or 4 V range, while when you look at the datasheet, they
have a 8 mV, 12 mV or 16 mV LSB, which means their actual range is
2.048 V, 3.072 V and 4.096 V respectively.

Additionally, due to the limited resolution, each ADC value represents
a range of input values, rather than an input value. 0x00 doesn't mean
"0 mV", is typically means "less than whatever 1 LSB represents". Some
datasheets are very clear on that, see for example the ADT7490
datasheet, page 16, table 7, "10−Bit ADC Output Code vs. V IN". This
means that what the drivers report (register value * LSB) is off by 0.5
LSB on average. All drivers do that and this is considered OK.

This also means that the top value of the range is never reported.
Consider an 8-bit ADC with a LSB of 16 mV, it's range is 4.096 V but
the top value (0xff) really means "4.080 V <= input value". Which
the driver will report as 4.080 V by convention.

BTW, you (and we) probably shouldn't waste too much energy on this.
ADCs are only accurate to some degree anyway. If you take the
components connected to the input into consideration, the accuracy gets
even worse. For example, if the input voltage must be scaled down to
fit in the ADC's range, you need at least one resistor, which in best
cases will have a 1% accurate value (known as tolerance.) That's ten
times the LSB of your 10-bit ADC, at which point / 1023 or / 1024
really makes no practical difference. Sub-percent tolerant resistors are
expensive and rare in consumer electronics in my experience.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Guenter Roeck]

On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote:
> Hi Anthony,
> 
[ ... ]
> 
> BTW, you (and we) probably shouldn't waste too much energy on this.
> ADCs are only accurate to some degree anyway. If you take the
> components connected to the input into consideration, the accuracy gets
> even worse. For example, if the input voltage must be scaled down to
> fit in the ADC's range, you need at least one resistor, which in best
> cases will have a 1% accurate value (known as tolerance.) That's ten
> times the LSB of your 10-bit ADC, at which point / 1023 or / 1024
> really makes no practical difference. Sub-percent tolerant resistors are
> expensive and rare in consumer electronics in my experience.
> 
True, but on the other side (and after looking into the datasheet)
the driver already calculated the voltage on adc4..6 correctly,
so unless you disagree I would like to apply the hwmon patch to -next
for consistency.

Thanks,
Guenter

RE: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 19 December 2013 18:11
> To: Jean Delvare
> Cc: Opensource [Anthony Olech]; lm-sensors@lm-sensors.org; Anton
> Vorontsov; David Woodhouse; linux-kernel@vger.kernel.org; David Dajun
> Chen
> Subject: Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052
> power driver
> On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote:
> > Hi Anthony,
> >
> [ ... ]
> > BTW, you (and we) probably shouldn't waste too much energy on this.
> > ADCs are only accurate to some degree anyway. If you take the
> > components connected to the input into consideration, the accuracy
> > gets even worse. For example, if the input voltage must be scaled down
> > to fit in the ADC's range, you need at least one resistor, which in
> > best cases will have a 1% accurate value (known as tolerance.) That's
> > ten times the LSB of your 10-bit ADC, at which point / 1023 or / 1024
> > really makes no practical difference. Sub-percent tolerant resistors
> > are expensive and rare in consumer electronics in my experience.
> True, but on the other side (and after looking into the datasheet) the driver
> already calculated the voltage on adc4..6 correctly, so unless you disagree I
> would like to apply the hwmon patch to -next for consistency.
> Thanks,
> Guenter

OK, thanks,
Tony Olech

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Jean Delvare]

Hi Guenter, hi Anthony,

On Thu, 19 Dec 2013 10:10:49 -0800, Guenter Roeck wrote:
> On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote:
> > BTW, you (and we) probably shouldn't waste too much energy on this.
> > ADCs are only accurate to some degree anyway. If you take the
> > components connected to the input into consideration, the accuracy gets
> > even worse. For example, if the input voltage must be scaled down to
> > fit in the ADC's range, you need at least one resistor, which in best
> > cases will have a 1% accurate value (known as tolerance.) That's ten
> > times the LSB of your 10-bit ADC, at which point / 1023 or / 1024
> > really makes no practical difference. Sub-percent tolerant resistors are
> > expensive and rare in consumer electronics in my experience.
>
> True, but on the other side (and after looking into the datasheet)
> the driver already calculated the voltage on adc4..6 correctly,
> so unless you disagree I would like to apply the hwmon patch to -next
> for consistency.

The datasheet isn't the best quality I've seen. The gain values
mentioned to not even match the formulas in the same cell... But I can't
object to implementing conversions the way the datasheet says, no
matter how suspicious.

I don't really know what tree the patch is based on. Anthony said
linux-next but I can't see ichg_reg_to_mA there.

If volt_reg_to_mv is updated then you certainly want to update
vbbat_reg_to_mv the same way, as it diverges from the datasheet just
the same.

Also, please update Documentation/hwmon/da9052 so that the
documentation matches the actual driver code.

I really would like to hear what David Dajun Chen has to say about
that. He must have had a reason to use different conversions formulas
than the datasheet specifies.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH V1] fix adc to voltage calculation in da9052 power driver

[Guenter Roeck]

On 12/21/2013 09:30 AM, Jean Delvare wrote:
> Hi Guenter, hi Anthony,
>
> On Thu, 19 Dec 2013 10:10:49 -0800, Guenter Roeck wrote:
>> On Thu, Dec 19, 2013 at 03:54:54PM +0100, Jean Delvare wrote:
>>> BTW, you (and we) probably shouldn't waste too much energy on this.
>>> ADCs are only accurate to some degree anyway. If you take the
>>> components connected to the input into consideration, the accuracy gets
>>> even worse. For example, if the input voltage must be scaled down to
>>> fit in the ADC's range, you need at least one resistor, which in best
>>> cases will have a 1% accurate value (known as tolerance.) That's ten
>>> times the LSB of your 10-bit ADC, at which point / 1023 or / 1024
>>> really makes no practical difference. Sub-percent tolerant resistors are
>>> expensive and rare in consumer electronics in my experience.
>>
>> True, but on the other side (and after looking into the datasheet)
>> the driver already calculated the voltage on adc4..6 correctly,
>> so unless you disagree I would like to apply the hwmon patch to -next
>> for consistency.
>
> The datasheet isn't the best quality I've seen. The gain values
> mentioned to not even match the formulas in the same cell... But I can't
> object to implementing conversions the way the datasheet says, no
> matter how suspicious.
>
> I don't really know what tree the patch is based on. Anthony said
> linux-next but I can't see ichg_reg_to_mA there.
>
I didn't check the power patch, but the hwmon patch applied cleanly to
the current upstream.

> If volt_reg_to_mv is updated then you certainly want to update
> vbbat_reg_to_mv the same way, as it diverges from the datasheet just
> the same.
>
He is doing that in the hwmon patch. I don't see vbbat_reg_to_mv()
in the power driver, at least not in the upstream version.

> Also, please update Documentation/hwmon/da9052 so that the
> documentation matches the actual driver code.
>
You are right, that needs to be updated as well.

> I really would like to hear what David Dajun Chen has to say about
> that. He must have had a reason to use different conversions formulas
> than the datasheet specifies.
>

Hmm ... if this was done on purpose, I'd like to see a comment somewhere
describing why the formula in the datasheet isn't used, to avoid this kind
of back-and-forth in the future.

Thanks,
Guenter