Message-ID: <20131029155718.717250be@notabene.brown>
Date:	Tue, 29 Oct 2013 15:57:18 +1100
From:	NeilBrown <neilb@...e.de>
To:	Anton Vorontsov <anton@...msg.org>
Cc:	"Tc, Jenny" <jenny.tc@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Kim, Milo" <Milo.Kim@...com>, Lee Jones <lee.jones@...aro.org>,
	Jingoo Han <jg1.han@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Sachin Kamat <sachin.kamat@...aro.org>,
	Rupesh Kumar <rupesh.kumar@...ricsson.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Pali Rohár <pali.rohar@...il.com>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Rhyland Klein <rklein@...dia.com>
Subject: Re: [PATCH 1/7] power_supply: Add charger control properties

On Sun, 27 Oct 2013 23:18:08 -0700 Anton Vorontsov <anton@...msg.org> wrote:

> On Mon, Oct 28, 2013 at 03:36:36AM +0000, Tc, Jenny wrote:
> > > But do we really want to control the chargers through the power_supply's user-visible
> > > interface? It makes the whole power supply thing so complicated that I'm already losing
> > > track of it. Right now I think I would prefer to move all the charger logic out of the psy
> > > class.
> > >
> > 
> > I think exposing properties make the logic generic, otherwise it may end up in having callback
> > functions.
> >
> > Also there are some scenarios where the charging algorithm has to be in the
> > user space.
> 
> Which scenarios?
> 
> Plus, I am more questioning if the power supply framework is the right
> thing to control the *chargers*. Chargers are not the power supply to the
> system or any device (well, except for the batteries themselves).

I'm not sure this is (always) true.
On my device (gta04.org) the battery, the USB OTG port, and a separate 5VDC
input can each be the power source of the whole device.
The USB and 5VDC cannot both be active concurrently, but either can be active
together with the battery.

The device can function without the battery, so the charger plugged into the
USB-OTG must be supplying power to the system (not just the battery).

The "charger" functionality sits between the battery and the external power
supply monitoring the voltage on the battery and the current from the
external supply.  Based on these values (and some timers and a state machine)
it enabled or disables the external supply and possibly imposes a
current-limit on it.

The three power sources all have "power_supply" devices registered (though
the battery only does because it contains a bq27000 charger counter).
I've been wondering where to put sysfs attributes to control the charging.
I currently place them in the power_supply device for each external power
source.
That makes some sense for the 'current limit' value, but not for the
'battery volts at which to re-start charging' value.
There is also a setting which affects  whether the external source is
switched off if the voltage drops below 4.4V.  In some circumstances I want
to leave the charger enabled then, as it could just mean the cyclist is
taking a break and there should be current again soon.

I think the sensible place for these tune-ables is with the battery.  i.e. the
power_supply that corresponds to the battery could register "min voltage" and
"min current".
The charger driver needs to know about this battery and about any power
sources that can charge it, and uses the state of the battery to decide how
to tune the state of the charger.

I note that there is already something a lot like this between 
   88pm860x_battery.c
and
   88pm860x_charger.c

They manage to locate each other and the charger call get_property and
set_property on the battery.

Maybe formalising this might be a useful way forward.

I'm not sure that we really need a new driver-class for chargers.  Maybe just
a way to link external power supplies to battery power supplies, and maybe
some agreement on what they are allowed to say to each other.

Jenny: would something like that work for you??

Thanks,
NeilBrown



> 
> > Using the patch https://lkml.org/lkml/2013/7/25/204,
> > the power supply change notification can be broadcasted. We can add notifier events
> > for power_supply_register and thermal throttling. This way power_supply_charger.c can
> > be a separate driver and it can listen to psy notifications to take actions.
> 
> If you ever need this particular notifier, I am OK with it (but I'll
> consider applying it only together with some its users).
> 
> Basically, I am more against these three patches:
> 
> [PATCH 3/7] power_supply: add throttle state
> [PATCH 2/7] power_supply: add charger cable properties
> [PATCH 1/7] power_supply: Add charger control properties (enable_charger part)
> 
> These three add too much "charger" specifics to the power_supply stuff. I
> think they deserve their own subsystem/class/whatever.
> 
> Also, the battid framework is written without any notion of device/driver
> separation, uses global variables, and I suspect it should not exist at
> all (psy_get_batt_prop function makes me think that you should just
> register the i2c/spi/w1 battery with the power_supply and not use the
> ad-hoc stuff).
> 
> Anton
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)

Powered by blists - more mailing lists