[PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[Paul Parsons]

Add rated capacity of the HP iPAQ hx4700 3.7V 3600mAh (359114-001) battery.
For this battery the value of the rated capacity EEPROM register at 0x32 is 14;
thus rated_capacities[14] = 3600.

Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
---
diff -uprN clean-3.2-rc2/drivers/power/ds2760_battery.c linux-3.2-rc2/drivers/power/ds2760_battery.c
--- clean-3.2-rc2/drivers/power/ds2760_battery.c	2011-11-15 17:02:59.000000000 +0000
+++ linux-3.2-rc2/drivers/power/ds2760_battery.c	2011-11-23 22:06:46.816841888 +0000
@@ -95,7 +95,11 @@ static int rated_capacities[] = {
 	2880,	/* Samsung */
 	2880,	/* BYD */
 	2880,	/* Lishen */
-	2880	/* NEC */
+	2880,	/* NEC */
+#ifdef CONFIG_MACH_H4700
+	0,
+	3600,	/* HP iPAQ hx4700 3.7V 3600mAh (359114-001) */
+#endif
 };
 
 /* array is level at temps 0°C, 10°C, 20°C, 30°C, 40°C

Re: [PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[Philipp Zabel]

On Wed, Nov 23, 2011 at 11:37 PM, Paul Parsons <lost.distance@yahoo.com> wrote:
> Add rated capacity of the HP iPAQ hx4700 3.7V 3600mAh (359114-001) battery.
> For this battery the value of the rated capacity EEPROM register at 0x32 is 14;
> thus rated_capacities[14] = 3600.
>
> Signed-off-by: Paul Parsons <lost.distance@yahoo.com>
> ---
> diff -uprN clean-3.2-rc2/drivers/power/ds2760_battery.c linux-3.2-rc2/drivers/power/ds2760_battery.c
> --- clean-3.2-rc2/drivers/power/ds2760_battery.c        2011-11-15 17:02:59.000000000 +0000
> +++ linux-3.2-rc2/drivers/power/ds2760_battery.c        2011-11-23 22:06:46.816841888 +0000
> @@ -95,7 +95,11 @@ static int rated_capacities[] = {
>        2880,   /* Samsung */
>        2880,   /* BYD */
>        2880,   /* Lishen */
> -       2880    /* NEC */
> +       2880,   /* NEC */
> +#ifdef CONFIG_MACH_H4700
> +       0,
> +       3600,   /* HP iPAQ hx4700 3.7V 3600mAh (359114-001) */
> +#endif

Is that #ifdef needed? I know there's another one a few lines up, but
I think that one is already unfortunate and there is no conflict here.

regards
Philipp

Re: [PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[Paul Parsons]

--- On Thu, 24/11/11, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> > +#ifdef CONFIG_MACH_H4700
> > +       0,
> > +       3600,   /* HP iPAQ hx4700 3.7V 3600mAh
> (359114-001) */
> > +#endif
> 
> Is that #ifdef needed? I know there's another one a few
> lines up, but
> I think that one is already unfortunate and there is no
> conflict here.

Yes it is needed, because the rated_capacities[] table must contain different values for different platforms. The #ifdefs preserve the original table for other platforms.

Agreed it's untidy. Ideally the table would be replaced by platform specific data. However it's not obvious which other platforms use the table, nor whether the original table is correct for those other platforms. I presume that the table was originally believed to be common for all ds2760 based platforms, but the hx4700 demonstrates this is not the case.

Re: [PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[Anton Vorontsov]

On Thu, Nov 24, 2011 at 11:52:56PM +0000, Paul Parsons wrote:
> --- On Thu, 24/11/11, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> > > +#ifdef CONFIG_MACH_H4700
> > > +       0,
> > > +       3600,   /* HP iPAQ hx4700 3.7V 3600mAh
> > (359114-001) */
> > > +#endif
> > 
> > Is that #ifdef needed? I know there's another one a few
> > lines up, but
> > I think that one is already unfortunate and there is no
> > conflict here.
> 
> Yes it is needed, because the rated_capacities[] table must
> contain different values for different platforms. The #ifdefs
> preserve the original table for other platforms.
> 
> Agreed it's untidy. Ideally the table would be replaced by
> platform specific data. However it's not obvious which other
> platforms use the table, nor whether the original table is
> correct for those other platforms. I presume that the table
> was originally believed to be common for all ds2760 based
> platforms, but the hx4700 demonstrates this is not the case.

Well, the really bad thing is that ds2760 is on the hot-pluggable
bus, so basically there is no such thing as 'platform_data'. You
can basically attach any battery, with any value in the
rated_capacity 'register'. :-(

The only thing we can do is some machine-specific fixup. The
universal way is ds2760_battery.rate_capacity module (and kernel
command line) parameter that you can use to fixup the value.

But, having the MACH_HX4700 fixup is just a heuristic that
helps the driver to work on the particular machine 'out of
the box'.

The really bad thing about that patch is that it uses build-time
way for the heuristic. We should really call something like
machine_is(). But that would be architecture-specific today
(i.e. will only work on ARM). Heh.

So... I'm applying the patch as it is not a big deal, and we
have a similar machine fixup already.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH] power/ds2760_battery: Add rated capacity of the hx4700 3600mAh battery

[Anton Vorontsov]

On Sat, Nov 26, 2011 at 03:56:13AM +0400, Anton Vorontsov wrote:
[...]
> The only thing we can do is some machine-specific fixup. The
> universal way is ds2760_battery.rate_capacity module (and kernel
> command line) parameter that you can use to fixup the value.

Oh, btw, nowadays there is support for writable properties in the
power supply class. So, in addition to the module parameter, for
convenience you might also implement write-function for the
rated_capacity value.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com