Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[NeilBrown]

On Tue, Nov 22 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 17 2016, Mark Brown wrote:
>
>> > To me that's pretty much what's being done here, the code just happens
>> > to sit in USB instead but fundamentally it's just a blob of helper code,
>> > you could replace the notifier with a callback if that's the big concern
>> > here.
>
>> It is a lot more than "just a blob of helper code".  It duplicates
>> existing infrastructure instead of fixing and using the
>> infrastructure.... but I've said all this before.  Repeatedly.
>
> My read on that is that the question of what we want to be responsible
> for aggregating the information about what power the system is allowed
> to draw from a given USB port hasn't been resolved yet and that apart
> from that you're fairly close.  It seems to me like that's really what
> the difference between your two positions is.  Fixing the existing
> notifiers implies that things have to be aggregated in the power supply
> drivers but Baolin is proposing doing that in the USB code instead.  It
> does seem at least worth considering if that's a good idea since the
> current situation doesn't look terribly thought through.

I agree that the question of where the responsibility for information
aggregation lies is open for discussion. If fact all details on how
things should work are always open for discussion.
I don't agree that this is the main different between our positions,
though I can see how you might get that impression.

I don't agree that "Fixing the existing notifiers implies that things
have to be aggregated in the power supply drivers".  That would be the
simplest way to fix them, but not the only way.  You could fix them so
that the usb driver sends notifications instead of the phy, and you
could fix them so that the information contained in the notification
being a power range instead the current ad-hoc inconsistent set of
information.

You could even fix them so they look *exactly* like the notifiers that
Baolin is proposing.  This is my key point.  It is not the end result
that I particularly object to (though I do object to some details).  It
is the process of getting to the end result that I don't like.  If the
current system doesn't work and something different is needed, then the
correct thing to do is to transform the existing system into something
new that works better.  This should be a clear series of steps.  Each
one makes a small, well defined, change.  Maybe it adds a new feature.
Maybe it refactors code without changing functionality.  Maybe it fixes a
bug.  Maybe it moves the responsibility for an action from *here* to
*there*.  Each step is clearly explained and convincingly justified.
This doesn't have to be a long justification.  Sometimes a single
sentence or a short paragraph is plenty.  Sometimes the change is so
obviously an improvement that no explanation is necessary.

Changing one step at a time make it easier to ensure that no
functionality is lost and that each change is actually needed.

If the patch series fixed the existing code and transformed it into
something better and more powerful, then it would be a lot easier to
have sensible discussions about individual patches, about which there
might be disagreement.

>
> There are a whole bunch of things that need to be sorted out whatever
> the decision is like the extcon related cleanups you mentioned in your
> mail the other day (steps 1 and 2), it seems like those could be moved
> forwards independently.

Agreed.

>
> By the way it occurred to me recently that we have a use case for
> multiple USB ports that could supply power - USB C.  Things with more
> than one port like things in a laptop form factor are going to want to
> be able to use all of them interchangably for power support (likely only
> one at a time, at least initially, but still more than one port).

Thanks!  It is so much easier to discusses these sorts of things where
there is a concrete reference point.

I wonder if each port would have its own current regulator, or if there
would be a single central one.  Maybe there would be a switch that
allowed power to flow in only from one port.
Would each "charger" need to get notifications from "its" port, or would
there be a single "charger" which got notifications from the
power-switch, which in-turn got notifications from each port?
Or would the battery-charger driver want to register with every port,
receive notifications, and be able to turn each one on or off?
Without concrete details of actual hardware, we can only guess.

But I think here my key point got lost too, in part because it was hard
to refer to an actual instance.
My point was that in the present patch set, the "usb charger" is given
a name which is dependant on discovery order, and only supports
lookup-by-name.  This cannot work.
If each USB-C port registers a usb_charger, they would be
"usb-charger.0", "usb-charger.1", "usb-charger.2", with no reliable
correlation between name and physical location.
If they supported lookup by phy-name or lookup-by-active (i.e. "find me
any usb-charger which has power available"), or look up by some other
attribute, then discover-order naming could work.  But the only
lookup-mechanism is by-name, and the names aren't reliably stable.  So
the name/lookup system proposed cannot possibly do anything useful
with more than one usb_charger.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature