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

[Heikki Krogerus]

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