RE: [PATCH v4 6/7] typec: tcpm: Represent source supply through power_supply class

[Adam Thomson]

On 08 February 2018 10:45, Heikki Krogerus wrote:

Hi Heikki,

Comments in-line as usual. Bit verbose, and may have stated the obvious in
places, but trying to build a picture and aim for something sensible.

> Hi Adam,
>
> On Tue, Feb 06, 2018 at 03:51:26PM +0000, Adam Thomson wrote:
> > Right now there is no documentation for the generic psy class. The stuff in
> > sysfs-class-power is device specific property information, and the same goes for
> > sysfs-class-power-twl4030. The property usage can vary depending on driver
> > implementation, an example being the 'online' property which can differ between
> > drivers, so the usage I have here is very much tcpm related. Also, the ability
> > to write to certain properties varies depending on the driver and HW, so here
> > where we configure 'voltage_now' and 'current_now', the likelihood is that most
> > other psy driver implementations won't allow for this.
>
> The power supply class is missing documentation, YES! That is what I
> have been saying! The fact that even an attribute like "online" can
> mean different things depending on the driver is absolutely horrible.
> The ABI documentation FOR THE POWER SUPPLY CLASS needs to provide
> clear meaning for the attributes. It needs to also point out which
> attributes can be hidden, and it should also give some hints for
> things like which attributes can be expected to be visible for example
> in case of USB type of psy and so on.

There is some coverage under Documenation/power/power_supply_class.txt but it's
not necessarily extensive, at least not to the level you're suggesting. With
regards to the psy class though, I believe this is meant to cover a range of
supply types so I guess the original intention was to make it flexible to cover
those. Maybe along the way that's helped muddy the waters somewhat though.

As an example, a 'Battery' type supply would represent say voltage_now
and current_now, and those values would relate to VBAT and IBAT. However a
Charger IC reporting the USB side information (probably of type 'USB', but could
be AC, Mains) would report voltage_now and current_now, and those values would
represent VIN/VBUS and IIN/IBUS. This example assumes that both sets of HW would
be able to provide instantaneous readings. Some only give averaged readings.
Just from this you can already start to see why that framework has evolved in
the way it has.

Covering the 'online' property, for Charger ICs this would indicate whether
they were externally powered or not (i.e. external charger attached/detached),
so a positive value would indicate external charger presence and 0 would mean
not present. In drivers/power/supply/abx500_chargalg.c it goes further with this
and the positive online value can differ based on what kind of charger was
attached (i.e. Mains, USB). Not sure if anything in user-space would behave
differently based on different values though and suspect a positive or 0 value
will be the only differentiator. For the Battery type scenario 'online', if
provided, is usually a repeat of battery presence information. Examples of this
exist although it's really just redundant information, so a guideline would help
to avoid this.

So right now the only sensible method seems to be to describe property usage
based on the main type, which brings me round to what you're suggesting I think.
I can start something, at least for common USB property usage scenarios, and
maybe Battery as well, but I do think this needs additional attention as there
are so far 66 (including my new addition) properties to document, some of which
may require multiple definitions based on type specific usage. With regards to
presence of attributes, that's going to be solely related to HW and what is
implemented for that device, I think. We can give preferred, sunny day examples,
but I think you'll be hard pressed to make one size fit all. Or do you disagree
with that sentiment?

>
> We are talking about user space ABI for power supplies here. The user
> space does not know that its dealing with tcpm in this case, or some
> other driver in an other case, AND, the user space _must_ not be
> expected to know that kind of details. The behaviour and meaning of an
> individual attribute file quite simply has to be the same, always,
> regardless of the platform, HW, driver or whatever. Otherwise this
> whole ABI is completely useless.

Not technically true as decisions could be based on psy naming. Generic TCPM
usage of the power_supply framework would mean the naming of the psy sysfs entry
(or entries) are well known, just like using /dev. I believe 'type' though is
specifically used in Android to determine which properties it uses for specific
purposes. Personally I go along with this and think 'type' should be the major
determining factor on expectation/usage. User-space needs to know the type of
supply it's dealing with to be able to act accordingly.

> To summarize: We can't just accept chaos. Instead we should organize
> the places without structure, in this case the user space ABI for
> power supplies.
>
> On top of ABI documentation, we will need driver API documentation as
> well. I'm not expecting that you would also propose something for the
> API too, but I just wanted to bring that up here. I would like to have
> some guidelines on how the power supplies should be used also in
> kernel.
>
> Right now it is possible for one driver to create the power supply and
> an other to take over the control of it. It is super easy to gain
> access to a power supply. You can request it with just the name
> without any control, and after gaining access, you have full control
> over it. That makes it really easy to have race condition where both
> the psy device driver and some other driver try to control the same
> things of the same psy.
>
> I guess the whole design of the psy class could use a little bit of
> re-designing. So IMO, access to the psy should be more strict then it
> is now, and also, even after gaining access to a psy handle, drivers
> that are not the actual psy device drivers should have more controlled
> access to it. So possibly separate API for them... OK, this is
> definitely a separate topic. Sorry, I'll stop here.

Actually being able to use psy's in other drivers is an approach that's very
useful, especially for monitoring & controlling charging using PPS. You are
right in that it's very easy to gain access and have control, but have not
personally experienced race issues. I guess that's down to the individual
driver's own locking implementation right now?

Anyway, based on all of this, and assuming I get no major disagreement, I'll
try and come up with something as a decent starting point to better document
and fix property usage, based on supply type. It won't be every property, but
hopefully will be a decent first stab at this.