linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

      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).