Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640765 > unrolled thread

Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede <hdegoede@redhat.com> - 2017-05-12 23:30 +0200
    Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-05-16 14:10 +0200
      Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Hans de Goede <hdegoede@redhat.com> - 2017-05-17 00:30 +0200
        Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2017-05-17 13:50 +0200
          USB TYPE-C support and the power-supply subsys (was Re: [PATCH 2/5]  extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede <hdegoede@redhat.com> - 2017-05-17 16:50 +0200
            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> - 2017-05-18 10:50 +0200
              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> - 2017-05-18 14:00 +0200
              Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH  2/5] extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede <hdegoede@redhat.com> - 2017-05-19 22:20 +0200
                Re: USB TYPE-C support and the power-supply subsys (was Re: [PATCH  2/5] extcon: Add FUSB302 USB TYPE-C controller support) Hans de Goede <hdegoede@redhat.com> - 2017-05-19 22:20 +0200
                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> - 2017-05-22 17:10 +0200
    Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support Guenter Roeck <linux@roeck-us.net> - 2017-05-17 01:00 +0200

#1640765 — Re: [PATCH 2/5] extcon: Add FUSB302 USB TYPE-C controller support

Hi,

On 24-04-17 15:12, Guenter Roeck wrote:
> On 04/24/2017 04:02 AM, Heikki Krogerus wrote:
>> +Guenter
>>
>> On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote:
>>> +Cc: Heikki.
>>>
>>> He might comment on this.
>>
>> Thanks Andy.
>>
>>> On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Add support for USB TYPE-C cable detection on systems using a
>>>> FUSB302 USB TYPE-C controller.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/extcon/Kconfig          |   8 +
>>>>  drivers/extcon/Makefile         |   1 +
>>>>  drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 791 insertions(+)
>>>>  create mode 100644 drivers/extcon/extcon-fusb302.c
>>
>> There is now the typec class in linux-next that really needs to be
>> used with all USB Type-C PHYs like fusb302.
>>
>> Unless I'm mistaken, Guenter has also written a driver for fusb302. I
> 
> Not me; it was Nathan.
> 
>> have not seen it, but if I understood correctly, that driver will
>> register itself with the upcoming tcpm (USB Type-C Port Manager) [1],
>> which in practice would mean we can take properly advantage of the USB
>> PD transceiver on fusb302.
>>
>> Guenter! Can you publish the fusb302 driver?
>>
> 
> Working on it.

Ok, I see this has landed in staging. So lets go with this driver
then and not mine (I did not get around to the PD stuff yet, so
that is fine).

Question, how is this supposed to interface with the rest of the
kernel ? Specifically the commit message for the tcpm frameworks
says:

"This driver only implements the state machine. Lower level drivers are
responsible for
- Reporting VBUS status and activating VBUS
- Setting CC lines and providing CC line status
- Setting line polarity
- Activating and deactivating VCONN
- Setting the current limit
- Activating and deactivating PD message transfers
- Sending and receiving PD messages"

But the FUSB302 cannot set the current limit...

The device I'm working on:
http://www.gpd.hk/gpdwin.asp

Has the following more or less relevant components:

-Intel Cherry Trail Z8700 SoC
-Intel Cherry Trail Whiskey Cove PMIC (which is different from the Broxton Whiskey Cove PMIC),
  this PMIC uses an external charger and fuel-gauge:
-TI bq24292i charger
-Maxim MAX17047 fuel-gauge
-PI3USB30532 USB TYPE-C mux

Currently I only have the USB-C connector on the
device working in USB-2 mode (I did not realize
the USB-3 bits were wired up at first).

The device ships with a charger with an USB-A
receptacle and an USB-2 only USB-A to USB-C
cable.

The way I've hooked up USB-2 charging is like this:
The Whiskey Cove PMIC has built in USB-2 BC detection, and
I've written an extcon driver for this which reports one
of the following cables depended on the BC detection:

         EXTCON_CHG_USB_SDP,
         EXTCON_CHG_USB_CDP,
         EXTCON_CHG_USB_DCP,

And I've modified the bq24290_charger driver to (optionally)
receive an extcon name through device-properties and if
it does, then set the input current based on the detected
charger type (0.5 A for SDP, 1.5A for CDP, 2A for DCP).

Most of the patches for this are upstream. But once we
hookup the FUSB302 driver we need to propagate the current
limit it has negotiated to the bq24290_charger driver.

Which is part of the reason why I wrote the 5th patch of
my original series, let me reply to Heikki's question about
that one here as it is related:

On 24-04-17 16:25, Heikki Krogerus wrote:

 > Doesn't this mean you have several extcon_dev registered for a
 > single port?

Yes this is unfortunate, but the primary consumer of extcon
info is the kernel itself and we can teach it to look at the
right one.

 > I'm not completely sure how extcon framework is meant to
 > be used, but shouldn't a single extcon_dev represent all the types of
 > connectors a port is meant to support?

Ideally, yes. But here we've 2 chips / drivers connected
to the same connector (more even, but 2 which provide extcon
related info).

As explained above the device ships with an USB-2 charger +
USB-A to USB-C cable, that cable correctly gets seen by the
FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input
current to 0.5A, but as the FUSB302 datasheet says:

"The host software is expected to follow the USB Type-C
specification for charging current priority based on
feedback from the FUSB302 detection, external BC1.2 detection
and any USB Power Delivery communication.

The FUSB302 does not integrate BC1.2 charger
detection which is assumed available in the USB
transceiver or USB charger in the system."

So when we see TYPEC_CC_RP_DEF we need to ask the
Cherry Trail PMIC (in my case) to do BC detection
and use the result of that, otherwise we end up
setting input current limit way too low.

 >> Since the Type-C controller info is incomplete and needs to be
 >> supplemented with the BC1.2 detection result in some cases, this
 >> commit makes the intel-cht-wc extcon driver monitor the extcon device
 >> registered by the Type-C controller and report that as our extcon state
 >> except when BC-1.2 detection should be used. This allows extcon
 >> consumers on these boards to keep monitoring only the intel-cht-wc extcon
 >> and then get complete extcon info from that, which combines the Type-C
 >> and BC-1.2 info.
 >
 > I don't really understand why does this need to be done in such a
 > complex manner? Both components should just report the result and be
 > done with it, and there is no need for any coupling between the two.
 > If there is a consumer for the power, the driver for it can decide on
 > its own the limits and maximum power from the two sources.

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.

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.

Likewise we need a way to tell another driver to drive VBus
on the USB-C connector. In my case this is again done by the
bq24290_charger driver, which currently does this based on the
combination of EXTCON_CHG_USB_* not being set on the extcon +
EXTCON_USB_HOST being set.

One possible solution would be for the
drivers/extcon/extcon-intel-cht-wc.c to somehow hook into
the tpmc device and to get notifications of state changes,
then it could reflect the info from the FUSB302 in its
extcon info, not sure if that is the best design though.

What also seems to missing is for a way to hookup a
mux driver to deal with upside-down plug-ins and
alt-mode switching.

Regards,

Hans

[toc] | [next] | [standalone]

#1642469

Hi Hans,

On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote:
> On 24-04-17 16:25, Heikki Krogerus wrote:
> 
> > Doesn't this mean you have several extcon_dev registered for a
> > single port?
> 
> Yes this is unfortunate, but the primary consumer of extcon
> info is the kernel itself and we can teach it to look at the
> right one.
> 
> > I'm not completely sure how extcon framework is meant to
> > be used, but shouldn't a single extcon_dev represent all the types of
> > connectors a port is meant to support?
> 
> Ideally, yes. But here we've 2 chips / drivers connected
> to the same connector (more even, but 2 which provide extcon
> related info).
> 
> As explained above the device ships with an USB-2 charger +
> USB-A to USB-C cable, that cable correctly gets seen by the
> FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input
> current to 0.5A, but as the FUSB302 datasheet says:
> 
> "The host software is expected to follow the USB Type-C
> specification for charging current priority based on
> feedback from the FUSB302 detection, external BC1.2 detection
> and any USB Power Delivery communication.
> 
> The FUSB302 does not integrate BC1.2 charger
> detection which is assumed available in the USB
> transceiver or USB charger in the system."
> 
> So when we see TYPEC_CC_RP_DEF we need to ask the
> Cherry Trail PMIC (in my case) to do BC detection
> and use the result of that, otherwise we end up
> setting input current limit way too low.

I understand that.

> >> Since the Type-C controller info is incomplete and needs to be
> >> supplemented with the BC1.2 detection result in some cases, this
> >> commit makes the intel-cht-wc extcon driver monitor the extcon device
> >> registered by the Type-C controller and report that as our extcon state
> >> except when BC-1.2 detection should be used. This allows extcon
> >> consumers on these boards to keep monitoring only the intel-cht-wc extcon
> >> and then get complete extcon info from that, which combines the Type-C
> >> and BC-1.2 info.
> >
> > I don't really understand why does this need to be done in such a
> > complex manner? Both components should just report the result and be
> > done with it, and there is no need for any coupling between the two.
> > If there is a consumer for the power, the driver for it can decide on
> > its own the limits and maximum power from the two sources.
> 
> 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.

There are several issues here..

> Likewise we need a way to tell another driver to drive VBus
> on the USB-C connector. In my case this is again done by the
> bq24290_charger driver, which currently does this based on the
> combination of EXTCON_CHG_USB_* not being set on the extcon +
> EXTCON_USB_HOST being set.

This is an other..

> One possible solution would be for the
> drivers/extcon/extcon-intel-cht-wc.c to somehow hook into
> the tpmc device and to get notifications of state changes,
> then it could reflect the info from the FUSB302 in its
> extcon info, not sure if that is the best design though.
> 
> What also seems to missing is for a way to hookup a
> mux driver to deal with upside-down plug-ins and
> alt-mode switching.

This is fourth issue, but let's not go into that now. Just in case you
guys are interested, right now I'm looking if we can use device graph
to link all the components related to an alt-mode:
Documentation/devicetree/bindings/graph.txt

The reason for that is because we should be able to use it also with
ACPI: Documentation/acpi/dsd/graph.txt


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.
The second problem definitely needs to be handled using psy framework.
The psy framework provides already everything needed for that.

I don't have suggestions for the other issues at this point. If extcon
can be used with those without any problems, OK. But let's try to
solve one problem at a time.


Thanks,

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1642879

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

[toc] | [prev] | [next] | [standalone]

#1643289

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

[toc] | [prev] | [next] | [standalone]

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

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.

<snip>

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

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

On Thu, May 18, 2017 at 11:37:56AM +0300, Heikki Krogerus wrote:
> 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.

"USB" -> "USB Type-C/PD". Sorry about that.

-- 
heikki

[toc] | [prev] | [next] | [standalone]

#1645890 — 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 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.

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

[toc] | [prev] | [next] | [standalone]

#1645891 — 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

[toc] | [prev] | [next] | [standalone]

#1647024 — 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

[toc] | [prev] | [next] | [standalone]

#1642892

On Fri, May 12, 2017 at 11:22:46PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 24-04-17 15:12, Guenter Roeck wrote:
> >On 04/24/2017 04:02 AM, Heikki Krogerus wrote:
> >>+Guenter
> >>
> >>On Fri, Apr 21, 2017 at 09:51:50PM +0300, Andy Shevchenko wrote:
> >>>+Cc: Heikki.
> >>>
> >>>He might comment on this.
> >>
> >>Thanks Andy.
> >>
> >>>On Fri, Apr 21, 2017 at 4:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>Add support for USB TYPE-C cable detection on systems using a
> >>>>FUSB302 USB TYPE-C controller.
> >>>>
> >>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>---
> >>>> drivers/extcon/Kconfig          |   8 +
> >>>> drivers/extcon/Makefile         |   1 +
> >>>> drivers/extcon/extcon-fusb302.c | 782 ++++++++++++++++++++++++++++++++++++++++
> >>>> 3 files changed, 791 insertions(+)
> >>>> create mode 100644 drivers/extcon/extcon-fusb302.c
> >>
> >>There is now the typec class in linux-next that really needs to be
> >>used with all USB Type-C PHYs like fusb302.
> >>
> >>Unless I'm mistaken, Guenter has also written a driver for fusb302. I
> >
> >Not me; it was Nathan.
> >
> >>have not seen it, but if I understood correctly, that driver will
> >>register itself with the upcoming tcpm (USB Type-C Port Manager) [1],
> >>which in practice would mean we can take properly advantage of the USB
> >>PD transceiver on fusb302.
> >>
> >>Guenter! Can you publish the fusb302 driver?
> >>
> >
> >Working on it.
> 
> Ok, I see this has landed in staging. So lets go with this driver
> then and not mine (I did not get around to the PD stuff yet, so
> that is fine).
> 
> Question, how is this supposed to interface with the rest of the
> kernel ? Specifically the commit message for the tcpm frameworks
> says:
> 
> "This driver only implements the state machine. Lower level drivers are
> responsible for
> - Reporting VBUS status and activating VBUS
> - Setting CC lines and providing CC line status
> - Setting line polarity
> - Activating and deactivating VCONN
> - Setting the current limit
> - Activating and deactivating PD message transfers
> - Sending and receiving PD messages"
> 
> But the FUSB302 cannot set the current limit...
> 

There is an underlying driver which I didn't pay enough attention to.

Badhri, Yueyao, can we publish the pd engine code as well ?
If that doesn't solve the problem, it might at least give a starting
point.

Thanks,
Guenter

> The device I'm working on:
> http://www.gpd.hk/gpdwin.asp
> 
> Has the following more or less relevant components:
> 
> -Intel Cherry Trail Z8700 SoC
> -Intel Cherry Trail Whiskey Cove PMIC (which is different from the Broxton Whiskey Cove PMIC),
>  this PMIC uses an external charger and fuel-gauge:
> -TI bq24292i charger
> -Maxim MAX17047 fuel-gauge
> -PI3USB30532 USB TYPE-C mux
> 
> Currently I only have the USB-C connector on the
> device working in USB-2 mode (I did not realize
> the USB-3 bits were wired up at first).
> 
> The device ships with a charger with an USB-A
> receptacle and an USB-2 only USB-A to USB-C
> cable.
> 
> The way I've hooked up USB-2 charging is like this:
> The Whiskey Cove PMIC has built in USB-2 BC detection, and
> I've written an extcon driver for this which reports one
> of the following cables depended on the BC detection:
> 
>         EXTCON_CHG_USB_SDP,
>         EXTCON_CHG_USB_CDP,
>         EXTCON_CHG_USB_DCP,
> 
> And I've modified the bq24290_charger driver to (optionally)
> receive an extcon name through device-properties and if
> it does, then set the input current based on the detected
> charger type (0.5 A for SDP, 1.5A for CDP, 2A for DCP).
> 
> Most of the patches for this are upstream. But once we
> hookup the FUSB302 driver we need to propagate the current
> limit it has negotiated to the bq24290_charger driver.
> 
> Which is part of the reason why I wrote the 5th patch of
> my original series, let me reply to Heikki's question about
> that one here as it is related:
> 
> On 24-04-17 16:25, Heikki Krogerus wrote:
> 
> > Doesn't this mean you have several extcon_dev registered for a
> > single port?
> 
> Yes this is unfortunate, but the primary consumer of extcon
> info is the kernel itself and we can teach it to look at the
> right one.
> 
> > I'm not completely sure how extcon framework is meant to
> > be used, but shouldn't a single extcon_dev represent all the types of
> > connectors a port is meant to support?
> 
> Ideally, yes. But here we've 2 chips / drivers connected
> to the same connector (more even, but 2 which provide extcon
> related info).
> 
> As explained above the device ships with an USB-2 charger +
> USB-A to USB-C cable, that cable correctly gets seen by the
> FUSB302 as a TYPEC_CC_RP_DEF cable, which means limiting input
> current to 0.5A, but as the FUSB302 datasheet says:
> 
> "The host software is expected to follow the USB Type-C
> specification for charging current priority based on
> feedback from the FUSB302 detection, external BC1.2 detection
> and any USB Power Delivery communication.
> 
> The FUSB302 does not integrate BC1.2 charger
> detection which is assumed available in the USB
> transceiver or USB charger in the system."
> 
> So when we see TYPEC_CC_RP_DEF we need to ask the
> Cherry Trail PMIC (in my case) to do BC detection
> and use the result of that, otherwise we end up
> setting input current limit way too low.
> 
> >> Since the Type-C controller info is incomplete and needs to be
> >> supplemented with the BC1.2 detection result in some cases, this
> >> commit makes the intel-cht-wc extcon driver monitor the extcon device
> >> registered by the Type-C controller and report that as our extcon state
> >> except when BC-1.2 detection should be used. This allows extcon
> >> consumers on these boards to keep monitoring only the intel-cht-wc extcon
> >> and then get complete extcon info from that, which combines the Type-C
> >> and BC-1.2 info.
> >
> > I don't really understand why does this need to be done in such a
> > complex manner? Both components should just report the result and be
> > done with it, and there is no need for any coupling between the two.
> > If there is a consumer for the power, the driver for it can decide on
> > its own the limits and maximum power from the two sources.
> 
> 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.
> 
> 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.
> 
> Likewise we need a way to tell another driver to drive VBus
> on the USB-C connector. In my case this is again done by the
> bq24290_charger driver, which currently does this based on the
> combination of EXTCON_CHG_USB_* not being set on the extcon +
> EXTCON_USB_HOST being set.
> 
> One possible solution would be for the
> drivers/extcon/extcon-intel-cht-wc.c to somehow hook into
> the tpmc device and to get notifications of state changes,
> then it could reflect the info from the FUSB302 in its
> extcon info, not sure if that is the best design though.
> 
> What also seems to missing is for a way to hookup a
> mux driver to deal with upside-down plug-ins and
> alt-mode switching.
> 
> Regards,
> 
> Hans

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web