Message-ID: <6a1b02fe-7d3f-839e-7516-076c91b673f8@redhat.com>
Date:   Fri, 19 May 2017 22:15:37 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.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)

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

Powered by blists - more mailing lists