lkml.org 
[lkml]   [2016]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@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

    \
     
     \ /
      Last update: 2016-12-22 08:06    [W:2.511 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site