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

[Baolin Wang]

Hi,

On 21 December 2016 at 06:07, NeilBrown <neilb@xxxxxxxx> wrote:
> On Tue, Dec 20 2016, Baolin Wang wrote:
>
>> Hi Neil,
>>
>> On 3 November 2016 at 09:25, NeilBrown <neilb@xxxxxxxx> wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>>
>>>>> So I won't be responding on this topic any further until I see a genuine
>>>>> attempt to understand and resolve the inconsistencies with
>>>>> usb_register_notifier().
>>>>
>>>> Any better solution?
>>>
>>> I'm not sure exactly what you are asking, so I'll assume you are asking
>>> the question I want to answer :-)
>>>
>>> 1/ Liase with the extcon developers to resolve the inconsistencies
>>>   with USB connector types.
>>>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>>>   which both seem to suggest a standard downstream port.  There is no
>>>   documentation describing how these relate, and no consistent practice
>>>   to copy.
>>>   I suspect the intention is that
>>>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>>>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>>>     the cable.
>>>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>>>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>>>     would normally appear with EXTCON_USB_HOST (I think).
>>>   Some drivers follow this model, particularly extcon-max14577.c
>>>   but it is not consistent.
>>>
>>>   This policy should be well documented and possibly existing drivers
>>>   should be updated to follow it.
>>>
>>>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>>>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>>>   They were recently removed from drivers/power/axp288_charger.c
>>>   which is good, but are still used in drivers/extcon/extcon-max*
>>>   Possibly they should be changed to names from the standard, or
>>>   possibly they should be renamed to identify the current they are
>>>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>>
>> Now I am creating the new patchset with fixing and converting exist drivers.
>
> Thanks!
>
>>
>> I did some investigation about EXTCON subsystem. From your suggestion:
>> 1. EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB.
>> ---- After checking, now all extcon drivers were following this rule.
>
> what about extcon-axp288.c ?
> axp288_handle_chrg_det_event() sets or clears EXTCON_CHG_USB_SDP but
> never sets EXTCON_USB.
> Similarly phy-rockchip-inno-usb2.c never sets EXTCON_USB.

Ha, sorry, I missed these 2 files, and I will fix them.

>
>>
>> 2. EXTCON_CHG_USB_ACA would normally appear with EXTCON_USB_HOST.
>> ---- Now no extcon drivers used EXTCON_CHG_USB_ACA, then no need to
>> change.
>
> Agreed.
>
>>
>> 3. Change EXTCON_CHG_USB_SLOW/FAST to EXTCON_CHG_USB_500MA/1A.
>> ---- There are no model that shows the slow charger should be 500mA
>> and fast charger is 1A. (In extcon-max77693.c file, the fast charger
>> can be drawn 2A), so changing to EXTCON_CHG_USB_500MA/1A is not useful
>> I think.
>
> Leaving the names a SLOW/FAST is less useful as those names don't *mean*
> anything.
> The only place where the cable types are registered are in
>  extcon-max{14577,77693,77843,8997}.c
>
> In each case, the code strongly suggests that the meaning is that "slow"
> means "500mA" and that "fast" means "1A" (or sometimes 1A-2A).
>
> With names like "fast" and "slow", any common changer framework cannot
> make use of these cable types as the name doesn't mean anything.
> If the names were changed to 500MA/1A, then common code could reasonably
> assume how much current can safely be drawn from each.

As I know, some fast charger can be drawn 5A, then do we need another
macro named 5A? then will introduce more macros in future, I am not
true this is helpful.

>
>>
>> From above investigation, I did not find anything need to be changed
>> now. What do you think? Thanks.
>>
>
> As above, I think changes are needed.
>
> I particularly highlight:
>
>>>   This policy should be well documented and possibly existing drivers
>>>   should be updated to follow it.
>
> The documentation is key.  A usb charger framework needs to depend on
> correct information from extcon, and currently there is no clear
> documentation on how a USB phy should signal the charger type.
> There isn't a lot to say: primarily that both the EXTCON_USB* and
> EXTCON_CGH_USB* types should be provided together.  Each does not imply
> the other in any way.  But it still needs to be documented.

Yes, I will try to add some documentation for this. Thanks.

>
> Thanks,
> NeilBrown
>
>
>>>
>>> 2/ Change all usb phys to register an extcon and to send appropriate
>>>    notifications.  Many already do, but I don't think it is universal.
>>>    It is probable that the extcon should be registered using common code
>>>    instead of each phy driver having its own
>>>    extcon_get_edev_by_phandle()
>>>    or whatever.
>>>    If the usb phy driver needs to look at battery charger registers to
>>>    know what sort of cable was connected (which I believe is the case
>>>    for the chips you are interested in), then it should do that.
>>>
>>> 3/ Currently some USB controllers discover that a cable was connected by
>>>    listening on an extcon, and some by registering for a usb_notifier
>>>    (described below) ... though there seem to only be 2 left which do that.
>>>    Now that all USB phys send connection information via extcon (see 2),
>>>    the USB controllers should be changed to all find out about the cable
>>>    using extcon.
>>>
>>> 4/ struct usb_phy contains:
>>>         /* for notification of usb_phy_events */
>>>         struct atomic_notifier_head     notifier;
>>>
>>>   This is used inconsistently.  Sometimes the argument passed
>>>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>>>   limited negotiated via USB, sometimes it is a pointer the the gadget
>>>   though as far as I can tell, that last one is never used.
>>>
>>>   This should be changed to be consistent.  This notifier is no longer
>>>   needed to tell the USB controller that a cable was connected (extcon
>>>   now does that, see 3) so it is only used to communicate the
>>>   'vbus_draw' information.
>>>   So it should be changed to *only* send a notification when vbus_draw
>>>   is known, and it should carry that information.
>>>   This should probably be done in common code, and removed
>>>   from individual drivers.
>>>
>>> 5/ Now that all cable connection notifications are sent over extcon and
>>>    all vbus_draw notifications are sent over the usb_phy notifier, write
>>>    some support code that a power supply client can use to be told what
>>>    power is available.
>>>    e.g. a battery charger driver would call:
>>>        register_power_client(.....)
>>>    or similar, providing a phandle (or similar) for the usb phy and a
>>>    function to call back when the available current changes (or maybe a
>>>    work_struct containing the function pointer)
>>>
>>>    register_power_client() would then register with extcon and separately
>>>    with the usb_phy notifier.  When the different events arrive it
>>>    calculates what ranges of currents are expected and calls the
>>>    call-back function with those details.
>>>
>>> 6/ Any battery charger that needs to know the available current can now
>>>    call register_power_client() and get the information delivered.
>>>
>>> NeilBrown
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards