Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938643AbcLVHGF (ORCPT ); Thu, 22 Dec 2016 02:06:05 -0500 Received: from mail-oi0-f47.google.com ([209.85.218.47]:36655 "EHLO mail-oi0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S936031AbcLVHGD (ORCPT ); Thu, 22 Dec 2016 02:06:03 -0500 MIME-Version: 1.0 In-Reply-To: <87bmw4rgij.fsf@notabene.neil.brown.name> References: <87k2cttptn.fsf@notabene.neil.brown.name> <87a8dls7yn.fsf@notabene.neil.brown.name> <871sytqrqh.fsf@notabene.neil.brown.name> <87pokms19o.fsf@notabene.neil.brown.name> <878trarlgs.fsf@notabene.neil.brown.name> <87bmw4rgij.fsf@notabene.neil.brown.name> From: Baolin Wang Date: Thu, 22 Dec 2016 15:06:02 +0800 Message-ID: Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation To: NeilBrown Cc: Felipe Balbi , Greg KH , Sebastian Reichel , Dmitry Eremin-Solenikov , David Woodhouse , robh@kernel.org, Jun Li , Marek Szyprowski , Ruslan Bilovol , Peter Chen , Alan Stern , grygorii.strashko@ti.com, Yoshihiro Shimoda , Lee Jones , Mark Brown , John Stultz , Charles Keepax , patches@opensource.wolfsonmicro.com, Linux PM list , USB , device-mainlining@lists.linuxfoundation.org, LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6979 Lines: 164 On 22 December 2016 at 07:47, NeilBrown wrote: > On Wed, Dec 21 2016, Baolin Wang wrote: > >> On 21 December 2016 at 11:48, NeilBrown wrote: >>> On Wed, Dec 21 2016, Baolin Wang wrote: >>> >>>> Hi, >>>> >>>> On 21 December 2016 at 06:07, NeilBrown wrote: >>>>> On Tue, Dec 20 2016, Baolin Wang wrote: >>>>> >>>>>> Hi Neil, >>>>>> >>>>>> On 3 November 2016 at 09:25, NeilBrown 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