Message-ID: <CAMz4ku+JOq3mAkJcbda1Pw0m=btuXZtMVaZA_qZxS7M6eL5U=Q@mail.gmail.com>
Date:   Thu, 22 Dec 2016 15:06:02 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     NeilBrown <neilb@...e.com>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sebastian Reichel <sre@...nel.org>,
        Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
        David Woodhouse <dwmw2@...radead.org>, robh@...nel.org,
        Jun Li <jun.li@....com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Ruslan Bilovol <ruslan.bilovol@...il.com>,
        Peter Chen <peter.chen@...escale.com>,
        Alan Stern <stern@...land.harvard.edu>,
        grygorii.strashko@...com,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Lee Jones <lee.jones@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        John Stultz <john.stultz@...aro.org>,
        Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>,
        patches@...nsource.wolfsonmicro.com,
        Linux PM list <linux-pm@...r.kernel.org>,
        USB <linux-usb@...r.kernel.org>,
        device-mainlining@...ts.linuxfoundation.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the
 usb gadget power negotation

On 22 December 2016 at 07:47, NeilBrown <neilb@...e.com> wrote:
> On Wed, Dec 21 2016, Baolin Wang wrote:
>
>> On 21 December 2016 at 11:48, NeilBrown <neilb@...e.com> wrote:
>>> On Wed, Dec 21 2016, Baolin Wang wrote:
>>>
>>>> Hi,
>>>>
>>>> On 21 December 2016 at 06:07, NeilBrown <neilb@...e.com> wrote:
>>>>> On Tue, Dec 20 2016, Baolin Wang wrote:
>>>>>
>>>>>> Hi Neil,
>>>>>>
>>>>>> On 3 November 2016 at 09:25, NeilBrown <neilb@...e.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

Powered by blists - more mailing lists