Message-ID: <20170517114547.GD5322@kuha.fi.intel.com>
Date:   Wed, 17 May 2017 14:45:47 +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>
Subject: Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

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

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.

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

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.

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

But in any case, we should never just pick a power supply and set its
values from outside its driver, even if the current API allows it.
Instead maybe we should try to prevent that by improving the API.

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

The regulator framework does sound reasonable.

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

I hope I was able to explain myself, and make my case.

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.

Then in case of TYPEC_CC_RP_DEF, the PMIC charger driver would always
know that it needs to execute the detection, and otherwise simply
passthough the limits. The driver for fusb302 would not need to do
anything extra in case of TYPEC_CC_RP_DEF.


Thanks,

-- 
heikki

Powered by blists - more mailing lists