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

[Anton Vorontsov <cbouatmailru@gmail.com>]

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