Message-ID: <20170522145657.GF14626@kuha.fi.intel.com>
Date:   Mon, 22 May 2017 17:56:57 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sebastian Reichel <sre@...nel.org>
Subject: Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH
 2/5] extcon: Add FUSB302 USB TYPE-C controller support)

On Fri, May 19, 2017 at 10:12:14PM +0200, 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.
> 
> 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.

Couldn't we extend the psy framework a little so it would be able to
support classes of devices that are not being presented to the user
space as power supplies as such, but are possibly just part of a chain
that makes up a single power supply? I'm playing with ideas at this
point, I'm not sure if that is feasible, but it does not sound
impossible.

I've been playing with that idea for some time now. Maybe it's time to
really check out what could be done :-).

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

I was thinking that at first stage it would be better to mix the
support into the device drivers, and later move it to tcpm, but I'm
not against it.

Actually, I was hoping that we could at one point consider every USB
port as a potential "power supply", not only Type-C ports, and
therefore handle the support in USB core, but that at least is
something that we probable should not think about at this point.

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

That sounds reasonable to me.


Br,

-- 
heikki

Powered by blists - more mailing lists