From: Baolin Wang <baolin.wang@linaro.org>
To: NeilBrown <neilb@suse.com>
Cc: Felipe Balbi <balbi@kernel.org>,
Greg KH <gregkh@linuxfoundation.org>,
Sebastian Reichel <sre@kernel.org>,
Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>,
David Woodhouse <dwmw2@infradead.org>,
robh@kernel.org, Jun Li <jun.li@nxp.com>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Ruslan Bilovol <ruslan.bilovol@gmail.com>,
Peter Chen <peter.chen@freescale.com>,
Alan Stern <stern@rowland.harvard.edu>,
grygorii.strashko@ti.com,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
John Stultz <john.stultz@linaro.org>,
Charles Keepax <ckeepax@opensource.wolfsonmicro.com>,
patches@opensource.wolfsonmicro.com,
Linux PM list <linux-pm@vger.kernel.org>,
USB <linux-usb@vger.kernel.org>,
device-mainlining@lists.linuxfoundation.org,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation
Date: Thu, 22 Dec 2016 15:06:02 +0800 [thread overview]
Message-ID: <CAMz4ku+JOq3mAkJcbda1Pw0m=btuXZtMVaZA_qZxS7M6eL5U=Q@mail.gmail.com> (raw)
In-Reply-To: <87bmw4rgij.fsf@notabene.neil.brown.name>
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
prev parent reply other threads:[~2016-12-22 7:06 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-19 2:37 [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation Baolin Wang
2016-10-19 2:37 ` [PATCH v18 1/4] usb: gadget: Introduce the usb charger framework Baolin Wang
2016-10-19 2:37 ` [PATCH v18 2/4] usb: gadget: Support for " Baolin Wang
2016-10-19 2:37 ` [PATCH v18 3/4] usb: gadget: Integrate with the usb gadget supporting for usb charger Baolin Wang
2016-10-19 2:37 ` [PATCH v18 4/4] power: wm831x_power: Support USB charger current limit management Baolin Wang
2016-10-27 7:33 ` [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation Baolin Wang
2016-10-27 22:00 ` NeilBrown
2016-10-28 12:51 ` Baolin Wang
2016-10-28 17:03 ` Mark Brown
2016-10-31 11:25 ` Baolin Wang
2016-10-31 0:00 ` NeilBrown
2016-11-01 12:54 ` Baolin Wang
2016-11-03 1:25 ` NeilBrown
2016-11-07 8:14 ` Baolin Wang
2016-11-07 20:36 ` NeilBrown
2016-11-10 9:42 ` Baolin Wang
2016-11-14 4:21 ` NeilBrown
2016-11-14 12:04 ` Mark Brown
2016-11-14 21:35 ` NeilBrown
2016-11-15 5:03 ` Peter Chen
2016-11-16 16:16 ` Mark Brown
2016-11-17 6:46 ` NeilBrown
2016-11-21 17:17 ` Mark Brown
2016-11-21 22:40 ` NeilBrown
2016-11-25 13:00 ` Mark Brown
2016-11-26 0:44 ` NeilBrown
2016-11-28 7:15 ` Baolin Wang
2016-11-14 12:36 ` Baolin Wang
2016-11-08 8:41 ` Peter Chen
2016-11-08 20:38 ` NeilBrown
2016-11-09 1:33 ` Peter Chen
2016-12-20 7:05 ` Baolin Wang
2016-12-20 22:07 ` NeilBrown
2016-12-21 2:54 ` Baolin Wang
2016-12-21 3:48 ` NeilBrown
2016-12-21 9:07 ` Baolin Wang
2016-12-21 23:47 ` NeilBrown
2016-12-22 7:06 ` Baolin Wang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMz4ku+JOq3mAkJcbda1Pw0m=btuXZtMVaZA_qZxS7M6eL5U=Q@mail.gmail.com' \
--to=baolin.wang@linaro.org \
--cc=balbi@kernel.org \
--cc=broonie@kernel.org \
--cc=ckeepax@opensource.wolfsonmicro.com \
--cc=dbaryshkov@gmail.com \
--cc=device-mainlining@lists.linuxfoundation.org \
--cc=dwmw2@infradead.org \
--cc=gregkh@linuxfoundation.org \
--cc=grygorii.strashko@ti.com \
--cc=john.stultz@linaro.org \
--cc=jun.li@nxp.com \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=neilb@suse.com \
--cc=patches@opensource.wolfsonmicro.com \
--cc=peter.chen@freescale.com \
--cc=robh@kernel.org \
--cc=ruslan.bilovol@gmail.com \
--cc=sre@kernel.org \
--cc=stern@rowland.harvard.edu \
--cc=yoshihiro.shimoda.uh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).