Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935299AbcLUXsf (ORCPT ); Wed, 21 Dec 2016 18:48:35 -0500 Received: from mx2.suse.de ([195.135.220.15]:49164 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1764304AbcLUXsb (ORCPT ); Wed, 21 Dec 2016 18:48:31 -0500 From: NeilBrown To: Baolin Wang Date: Thu, 22 Dec 2016 10:47:48 +1100 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 Subject: Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation In-Reply-To: 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> User-Agent: Notmuch/0.22.1 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-suse-linux-gnu) Message-ID: <87bmw4rgij.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7530 Lines: 177 --=-=-= Content-Type: text/plain 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. 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 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAlhbFKQACgkQOeye3VZi gblPHhAAjEkP33gD1kpPIoP8WXqUXqDm8Bfdo+ZU/OcncFH7cmmLHLgoBy8O1T0T kaDMDV3ftYANugqrXqk0lftsqhs9IJRq8jxh3eXwiIA9mkzcJKhKBH4W85bUZHPe zseLtNSDXkPR8FtioHhxquDZurn1G0AiP0thGEYZ0ysAfYVFL/lfP9c/Rqj0Dx84 FHbHVZhzjbbFvov28XYbjViCbgvdeKYKjByGbBVqdG/5RAXF7YT8tcwj6rFAtzLN JTvBfMRzcNBXEPrfquc72GuhEpKpNq9LR9caLK06I5+jt878MuAb2/Ya5DjwcQr8 Tyicp9mrqu117CTvKXlldLrFBXpCYVgcMjBF+HkSHsYsV+UOzwa3a4x7jD5/p5T+ UQb7KxhLKH8wzTpPVBB+zn/pUW+N7PNiGteSW9t40I7WfrUTv1paAXrry0JChpRd rK1W6e2p0Den6DDPOzOn4UkNb+AcGOXsUPLp1cqg1PKxZwd+3oXRV2qaGlnQv9hK EniKGQ86GnWFfXABMMfECNH/XCt+ahr2ag1wbTMFN8qu2Lj/rEBrJ50PIWJI2Dck vIVK0IHvaKVreRN18/Erayp1k6WyIT3XyzdMhgRcCic1CN0ZfokS7+udaS54AoB9 4oa9flJtJcDqcnTE97foaC7H/SYcqMmVYn+nI9oOicJ2nC96cZs= =sWXd -----END PGP SIGNATURE----- --=-=-=--