Message-ID: <8c45819c-1738-dc95-35ad-b6ba37f8f418@redhat.com>
Date:   Wed, 17 May 2017 00:24:52 +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>
Subject: Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

Hi,

On 05/16/2017 02:07 PM, Heikki Krogerus wrote:
> Hi Hans,
>
> On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote:

<snip>

>> To me making the combination of the 2 sources the problem
>> of the consumer seems just wrong, as you mentioned
>> above there should really be only one extcon for the
>> connector, likewise their should be only one definitive
>> source on what the input current limit is.
>
> Why? The consumer driver, bq24290 in this case, does not need to
> understand it has multiple sources. It will just react to events from
> *a* source, and the psy framework takes care of everything else. The
> psy framework will need some tuning, but that should not be a problem.
> I'm preparing support for _PCL (power consumer list) ACPI method
> handling for the psy framework, so I have some patches already. The
> idea is that in the future USB ports could have virtual power source
> object with _PCL.
>
> This is in any case separate issue compared to the problem of telling
> the BC1.2 detection to start in case of TYPEC_CC_RP_DEF.
>
>> TL;DR: We need some way to combine the current limit info
>> from TYPE-C and USB-2 BC detection into a single source and
>> to propagate that current to drivers which can actually set
>> the current-limit.
>>
>> Note I'm happy to use something else then extcon for this,
>> but we do need some way to combine + propagate the
>> current-limit.
>
> That must be handled using psy framework. Otherwise we'll just be
> trying to re-invent the wheel. There is no problem for a consumer to
> have multiple sources.

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.

<snip>

> Back to this topic. I can see at least the following problems:
>
> 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.
>
> You are trying to solve everything using extcon, and that is wrong.

As indicated I'm not stuck on using extcon, I started using it
in my paches because it is used to set the current limit in some
other places already, but I'm fine with using something else.

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

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.

For 3. (vbus) we could do something similar using a
"vbus-regulator-id" device-property and the regulator
framework, making the driver which controls Vbus register
itself as a regulator.

I can take a shot at implementing both suggestions, but
first lets try to get some general idea of how we want
to solve this.

Regards,

Hans

Powered by blists - more mailing lists