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

[Hans de Goede <hdegoede@redhat.com>]

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