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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Wed, May 17, 2017 at 04:47:14PM +0200, Hans de Goede wrote:
> 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.

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.

> > 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.

I second that.


Thanks Hans,

-- 
heikki