Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support)

From: Hans de Goede
Date: Fri May 19 2017 - 16:12:26 EST


Hi,

On 18-05-17 10:37, Heikki Krogerus wrote:
On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote:
Hi,

On 17-05-17 13:45, Heikki Krogerus wrote:

<snip>

In this case the driver for fusb302 registers a power supply that
provides properties like POWER_SUPPLY_PROP_VOLTAGE_MAX, etc. and
simply calls power_supply_changed() when ever needed (when we know the
limits and when a source is attached/de-attached -> changes PRESENT
& ONLINE properties). The fusb302 driver does not need to know if
there are any consumers for it or not. The platform driver that
registers the device for it will simply assign the consumer for it in
this case, but in the future we want to get that kind of detail from
the platform of course, so ACPI or DT.

The PMIC charger driver will similarly register a power supply device
and function pretty much exactly the same way.

The consumer, bq24190, will receive notification from psy framework to
its external_power_changed hook when ever either of the supplies
calls power_supply_changed(). It then needs to check the properties of
the supply (the external power) that are interesting to it in order to
set the limits accordingly (this btw. is where the psy API and the
class driver can be improved without much effort to make things easier
for the consumers). The consumer driver in any case does not need to
know what the supplies actually are, how many of them it has, or are
there any at all, just like the drivers for the supplies don't need to
know the consumers.

I like parts of this idea, but as said I've trouble with exporting 3
power-supply devices to userspace for this.

I must also say that I'm not a fan of making the BC-1.2 charger
a separate power-supply and letting the consumer figure out which
one to use, but lets forget about the BC-1.2 charger for now and
focus on solving this for just setting the TYPE-C determined
input current limit for now.

OK, You have a point. I happy to agree that adding an other psy for
the BC1.2 charger alone is not the correct thing to do.

I'm mainly interested in just considering USB as a power supply with a
board like this, because really, that is what it is, but also by using
the power supply class properly (and possibly improving it a little),
we avoid unnecessary software couplings.

Ok, so although I'm still not 100% sold on having the fusb302 driver
registering a power-supply is a good idea from an userspace API pov, I
from a design pov otherwise. And in a way it does make sense the fusb302
driver is a way to query (and in some case modify) settings of the external
power-supply connected to the USB-C port.

So maybe we need a new "External" power-supply type for this ? Either way
this should not be a real issue since userspace (upower) does not seem to
do much if anything with power-supply class devices with a type of USB.

So lets go with the plan to make the fusb302 driver register a power-supply
for now, which will be a supplier for (for example) the bq24190 charger.

2 questions about implementing this:

1) You said you want the power-supply code (get_property, etc.) to live
in the fusb302 driver, rather then in the tcpm code. I agree that the
fusb302 device should be the parent-device of the power-supply, but I can
see registering a power-supply and querying things like
POWER_SUPPLY_PROP_VOLTAGE_MAX being something which we will want in other
USB TYPE-C drivers too, so wouldn't it be better to have this code in
the generic tcpm bits ?

2) I could modify the bq24190 driver to check and update its
input-current-limit itself from its external_power_changed callback,
but this seems like something which many charger drivers will need
to implement so instead I think that drivers like bq24190 should just
be able to set a "sync_input_current_with_suppliers" flag and then
when a supplier calls power_supply_changed() have the core call
set_property PROP_INPUT_CURRENT_LIMIT. That way all we need to do
to get charger drivers to support this is set that flag rather then
copy and paste code. Or maybe a
power_supply_sync_input_current_with_suppliers() helper which drivers
can call from their external_power_changed callback. Thinking more
about this I think I like the helper function idea better.

Regards,

Hans