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

[Baolin Wang]

On 21 December 2016 at 11:48, NeilBrown <neilb@xxxxxxxx> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> 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.
>
> It isn't really a question of what the charger can provide.  It is a
> question of what the cable reports to the phy.

Yes, there is no spec to describe fast/slow charger type and how much
current fast/slow charger can provide. Maybe some fast charger can
provide 1A/2A, others can provide 5A, which depends on users'
platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast
charger can provide 1.5A on user's platform, will it report the fast
charger type by EXTCON_CHG_USB_1A on user's platform (but it can
provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as
they were, and maybe fix them in future? (BTW, I've fixed issue 1 and
maintainer has applied them).

>
> Unfortunately I cannot find any datasheets on line to verify how these
> devices work, but the code seems to suggest that they can detect and
> report special charger types that provide 500mA and 1A respectively.
> If the hardware exists that reports the functionality, it make sense to
> use it.  Hopefully new hardware will follow the USB BC spec, so further
> special cases won't be needed.
>
> Thanks,
> NeilBrown



-- 
Baolin.wang
Best Regards