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]

Hi,

On 19-05-17 22:12, Hans de Goede wrote:
> 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.

Ugh I somewhat mangled the first sentence of this paragraph,
here is a correct version:

Ok, so although I'm still not 100% sold on having the fusb302 driver
registering a power-supply being a good idea from an userspace API pov, I
do actually like it from a design pov.
> 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