Re: [Ksummit-discuss] [LTSI-dev] [Stable kernel] feature backporting collaboration

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3910 bytes --]

On Fri, Sep 09 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Fri, Sep 09, 2016 at 08:38:07AM +1000, NeilBrown wrote:
>
>> There is a usb notifier which allows other drivers, such as power
>> managers, to be told when a cable is attached to a USB port.  That is
>> enough to get basic charger functionality working.  But not all phys
>> send a notification and those that do, don't do it in a consistent way.
>> There is also extcon, which some phys use to report a cable, but not
>> all.  It provides more details of the cable, but cannot report anything
>> about a current limit negotiated with a host.
>
> Right, we did look at these early on but they're all working with
> subsets of the functionality - for example physical presence isn't
> enough to kick off high current charging.  Part of the thinking was to
> create something which could aggregate the various bits of information
> that individual subsystems are detecting to produce a coherent view.
>
>> So you certainly can make this work with mainline (I've done it)
>> without any extra infrastructure.
>
> You can support low rate charging (and some systems will just do chunks
> of this autonomously in the hardware with no OS intervention) but I'm
> not clear how high rate is going to work.

Get the phy to register an extcon, and report the charger type.
Get the power supply to register with that extcon and, when an
EXTCON_CHG_USB_* cable is reported, start using more current.

>
>> The current patchset isn't exactly adding a third way to do things, but

Actually I got that wrong.  It *does* exactly add a third way of doing
things.
->uchgr_nh  is a third, separate, notifier chain.

>> it is adding stuff without cleaning up what is currently there.  This
>> won't make the code less messy.
>
> It wasn't clear that the messiness wasn't just because nothing is taking
> a top level view of what's going on.

That is no excuse for leaving it there.  If you want to fix something,
take the top level view, tidy up the mess, and fix it.

The first step should be to do an audit of the current code.  See how
things are used, ask how they could be used more consistently and maybe
could be modified to meet your needs.  Part of this is making sure you
have a clear understanding of the need.  The current patchset doesn't show
that clear understand at all.  This is particularly obvious in the way it
has incorrect defaults for the various charger types.

With a little bit of fixing, extcon is perfectly placed to report
charger types to the power supply.  I think that should be fixed up and
used.  No extra framework needed, just fix what we have.

For communicating the current negotiated in the USB config I see two
options.  One is to fix up the current usb_register_notifier() framework
so that it is used consistently, and have it communicate the allowed
current level.
The other is to remove usb_register_notifier() completely and replace it
with something like the uchgr_nh in the current patchset.
I think usb_register_notifier() is used for two different things.
One is to report a USB or a USB-OTG connection to the usb driver, so it
can configure as 'host' or 'gadget'.
The other is to report the current limit.
Some drivers use extcon for the USB-OTG notification.
Deciding on, and standardizing, a single way to notify which sort of USB
data cable has been plugged, would be very valuable.
The charger-type and negotiated-current notification could use the same
mechanism, or could use a separate mechanism, or could use two different
mechanisms.
I have no strong opinion on what mechanism should be used for each
(well ... I do, but I'll probably have a different opinion tomorrow).
But I *do* strongly believe that each particular type of communication
should be done just one way, and part of creating a new "framework" is
to make sure that all current use-cases fit neatly into the framework.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]