Re: [rfc] Collie battery status sensing code

[Pavel Machek <pavel@ucw.cz>]

Hi!

> > This is collie battery sensing code. It differs from sharpsl code a
> > bit -- because it is dependend on ucb1x00, not on platform bus.
> > 
> > I guess I should reorganize #include's and remove #if 0-ed
> > code. Anything else
> 
> Basically looks good. Could probably use a
> s/printk/dev_dbg(sharpsl_pm.dev, /. I've made a few other comments
> below. 

I'll just remove most printks before next merge attempt, I guess.

> Just for my interest, can you summarise the status of PM and charging on
> collie with this code?

Well, with heavy ifdefs into sharpsl_pm, it can correctly sense
AC/battery. Voltmeters seem to return *some* values. Temperature is
probably okay, battery level is *extremely* noisy, and outside range
where sharp code expects it, but seems to correlate with actual
battery levels.

Now... something does charge my battery, but it is definitely not my
code. I don't dare enabling charging yet. 

(More detailed reply will come later today, when I'm close to collie).

> > +#include "../drivers/mfd/ucb1x00.h"
> > +#include <asm/mach/sharpsl_param.h>
> > +
> > +#ifdef CONFIG_MCP_UCB1200_TS
> > +#error Battery interferes with touchscreen
> > +#endif
> 
> Is this a case of bad locking or something more serious?

Original sharp code does some magic between TS and battery reading. It
seems AD0 is used by both, or something similary ugly. I'll have to
decipher sharp code and figure out how to do it properly.

> > +static int __init collie_pm_add(struct ucb1x00_dev *pdev)
> > +{
> > +	sharpsl_pm.dev = NULL;
> > +	sharpsl_pm.machinfo = &collie_pm_machinfo;
> > +	ucb = pdev->ucb;
> > +	return sharpsl_pm_init();
> > +}
> 
> I don't understand how this is supposed to work at all. For a start,
> sharpsl_pm.c says "static int __devinit sharpsl_pm_init(void)" so that
> function isn't available. I've just noticed your further patches
> although I still don't like this.
> 
> The correct approach is to register a platform device called
> "sharpsl-pm" in collie_pm_add() which the driver will then see and
> attach to. I'd also not register the platform device if ucb is NULL for
> whatever reason.

I thought about it, and considered it quite ugly. Result would be all
data on the platform bus with half-empty device on ucb1x00 "bus". It
would bring me some problems with registering order: if platform
device is registered too soon, ucb will be NULL and it will crash and
burn. OTOH I already have static *ucb, so it is doable, and I can do
it if you prefer it that way...
-- 
Thanks, Sharp!
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/