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 17-05-17 13:45, Heikki Krogerus wrote:
> Hi,
>
> On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 05/16/2017 02:07 PM, Heikki Krogerus wrote:
> > But we don't really have multiple sources here, we have only
> > one source, the USB-C cable hooked to an external power-supply.
> > Just because 2 different pieces of hardware may be involved in
> > determining the current limit does not mean that we should
> > model this as 2 different sources. Just as you rightfully
> > pointed out that me using 2 extcon devices for the single
> > Type-C connector is wrong, modelling it as 2 sources is
> > wrong too.
>
> The power supply devices don't represent the port like the extcon
> device would. The power supply devices represent the two types
> of external chargers we support. So BC1.2 and Type-C/PD source.

Which are both USB chargers, and the TI bq24290 driver itself
also registers itself as a USB charger, continued below ...

> > > 1. Tell the BC1.2 detection to start from fusb302 driver
> > > 2. Deliver the power limits to the discrete charger ic or battery driver
> > > 3. Tell what ever regulates VBUS to start driving VBUS.

<snip>

> > > The second problem definitely needs to be handled using psy framework.
> > > The psy framework provides already everything needed for that.
> >
> > So above you're talking about sources to the bq24190, which if
> > I understand you correctly suggest that you want the tcpm code to
> > register a power-supply device per Type-C port ?
>
> No, the underlying device driver (so fusb302) needs to register the
> power supply at this point. We just notify the psy framework if the
> limits we get from tcpm to the set_current_limit hook change.
>
> > I'm not sure that
> > is a good idea, any registered power-supply devices will show up
> > under /sys/class/power_supply, currently on a typical system
> > there will be 2 entries under /sys/class/power_supply one for
> > the "Mains" or USB power input and one for the battery, adding
> > more entries there is likely to confuse userspace.
>
> The userspace uses the type attribute to differentiate the supplies.
> Otherwise it would not be able to tell even which is the Battery and
> which is Mains. Having more power supplies is not a problem.

I disagree yes power-supply devices have a type, so if we do
as you suggest we end up with 3 power-supplies with type USB under
/sys/class/power_supply suggesting to userspace that the device
may be supplied with power through 3 different USB connectors.

This is simply just wrong. I understand that you want to decouple
the different drivers from each-other and I agree with that as
a goal.

But using the power-supply subsys in the way you suggests means
that the way we end up solving a problem which is purely
about different pieces of code in the kernel talking to each
other gets leaked to userspace and once we've done this userspace
may start depending on this and we cannot change things later.

TL;DR: I'm against the FUSB302 and the BC1.2 drivers each
registering a power-supply because:

1) There should be only one power-supply device registered
not 3, since there is only one power-input to the board, not 3.

2) Ideally the in kernel interface should not be visible to
userspace at all, we are still figuring all of this out and
we may need to change things later leaking internal kernel
details to userspace in something which will become an ABI
is not a good idea.

I've added Sebastian Reichel, the power-supply subsys
maintainer to the Cc so that he can weigh in on this.

> > More in general having 2 power-supply devices for what is
> > in essence one power-input feels wrong.
> >
> > We can still use the power-supply framework, but I would
> > envision this working like this:
> >
> > The platform code which enumerates the type-c controller
> > sets a "input-current-power-supply-name" string device-property
> > on the tcpm's (parent)device. When this is set the tpcm code
> > will do a power_supply_get_by_name and set the input
> > current on the returned power_supply by setting the
> > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT property.
>
> The psy framework is probable a bit messy at them moment. It
> definitely does not do much protecting and in many cases one driver
> registers a power supply and an other just takes it over, but that
> should be avoided as it makes things difficult (painful) to maintain.
> It also poses risks IMO. There certainly almost never seems to be a
> real need for that.
>
> 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.

<snip>

> I hope I was able to explain myself, and make my case.

Explain yes, but I'm really worried about the consequences of exporting
extra API/ABI to userspace for this, while we are purely trying to
solve in an kernel communication problem.

> One more thing. I think we could actually build a little bit of
> hierarchy and make the fusb302 the supply of, not bq24190, but the
> PMIC instead. The PMIC would then be the only supply for the bq24190.

Actually if you look at the schematic (I don't have a schematic for
my device, but I can deduce one) then the bq24190 is supplying power
to the pmic not the other-way around and the fusb302 is not supplying
power in anyway, it purely is negotiating things, but from an electrical
pov it is not supplying anything. Anyways as said lets forgot about
the PMIC doing BC1.2 detection for now and first focus on how we
can get the current-limit info from the fusb302 driver to the
power-supply device registered by the bq24190 driver.

Regards,

Hans