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: Wed May 17 2017 - 10:47:38 EST


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

On Wed, May 17, 2017 at 12:24:52AM +0200, Hans de Goede wrote:

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.


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

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.


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.