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

Baolin Wang
2016-12-22T07:06:05Z

On 22 December 2016 at 07:47, NeilBrown <neilb@suse.com> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> On 21 December 2016 at 11:48, NeilBrown <neilb@suse.com> wrote:
>>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>>
>>>> Hi,
>>>>
>>>> On 21 December 2016 at 06:07, NeilBrown <neilb@suse.com> wrote:
>>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>>
>>>>>> Hi Neil,
>>>>>>
>>>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> 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).
>
> I said "It isn't really a question of what the charger can provide."
> and you are still taking about "and some fast charger can provide 1.5A".
> So it seems like you didn't read, or you didn't understand what I wrote.
> I'll try again.
>
> Ignore the chargers. COMPLETELY. This not about chargers AT ALL.
> This is about cables and the information that the cable reports.
>
> Some how, and I cannot find the details, these MAXIM devices can report
> things like
> case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
> and
> case MAX14577_CHARGER_TYPE_SPECIAL_1A:
> and
> case MAX77693_CHARGER_TYPE_APPLE_500MA:
> and
> case MAX77693_CHARGER_TYPE_APPLE_1A_2A:
>
> If this information is to be useful to the USB battery charging
> infrastructure, then it must be communicated unambiguously. Reporting
> "EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous
> meaning.
>
> If it were documented somewhere that
> A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current
> at 5V. A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of
> current at 5V.
> when those cable types could be used by the USB battery chargers.
> If we just changed the names, then we wouldn't really need to document
> it and the intention would be obvious. Self-documenting names are
> better where possible.

Fine, I will submit one patch to document EXTCON_CHG_USB_SLOW/FAST as
you suggested.

>
> So yes, we could leave it as it is a decide not to fix this bug. But
> then I would then argue strongly against any attempt for the USB battery
> charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and
> EXTCON_CHG_USB_FAST. If you are going to fix this thing, you may as
> well fix it properly.
>
> NeilBrown
>

--
Baolin.wang
Best Regards