Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1544953 > unrolled thread

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-12-20 08:10 +0100
    Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-12-20 23:10 +0100
      Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-12-21 04:00 +0100
        Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-12-21 04:50 +0100
          Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-12-21 10:10 +0100
            Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-12-22 00:50 +0100
              Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-12-22 08:10 +0100

#1544953 — Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

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.

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.

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.

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.

From above investigation, I did not find anything need to be changed
now. What do you think? Thanks.

>
> 2/ Change all usb phys to register an extcon and to send appropriate
>    notifications.  Many already do, but I don't think it is universal.
>    It is probable that the extcon should be registered using common code
>    instead of each phy driver having its own
>    extcon_get_edev_by_phandle()
>    or whatever.
>    If the usb phy driver needs to look at battery charger registers to
>    know what sort of cable was connected (which I believe is the case
>    for the chips you are interested in), then it should do that.
>
> 3/ Currently some USB controllers discover that a cable was connected by
>    listening on an extcon, and some by registering for a usb_notifier
>    (described below) ... though there seem to only be 2 left which do that.
>    Now that all USB phys send connection information via extcon (see 2),
>    the USB controllers should be changed to all find out about the cable
>    using extcon.
>
> 4/ struct usb_phy contains:
>         /* for notification of usb_phy_events */
>         struct atomic_notifier_head     notifier;
>
>   This is used inconsistently.  Sometimes the argument passed
>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>   limited negotiated via USB, sometimes it is a pointer the the gadget
>   though as far as I can tell, that last one is never used.
>
>   This should be changed to be consistent.  This notifier is no longer
>   needed to tell the USB controller that a cable was connected (extcon
>   now does that, see 3) so it is only used to communicate the
>   'vbus_draw' information.
>   So it should be changed to *only* send a notification when vbus_draw
>   is known, and it should carry that information.
>   This should probably be done in common code, and removed
>   from individual drivers.
>
> 5/ Now that all cable connection notifications are sent over extcon and
>    all vbus_draw notifications are sent over the usb_phy notifier, write
>    some support code that a power supply client can use to be told what
>    power is available.
>    e.g. a battery charger driver would call:
>        register_power_client(.....)
>    or similar, providing a phandle (or similar) for the usb phy and a
>    function to call back when the available current changes (or maybe a
>    work_struct containing the function pointer)
>
>    register_power_client() would then register with extcon and separately
>    with the usb_phy notifier.  When the different events arrive it
>    calculates what ranges of currents are expected and calls the
>    call-back function with those details.
>
> 6/ Any battery charger that needs to know the available current can now
>    call register_power_client() and get the information delivered.
>
> NeilBrown



-- 
Baolin.wang
Best Regards

[toc] | [next] | [standalone]

#1545492 — Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Multipart message — attachments visible in raw view] — view raw

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.

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

>
> From above investigation, I did not find anything need to be changed
> now. What do you think? Thanks.
>

As above, I think changes are needed.

I particularly highlight:

>>   This policy should be well documented and possibly existing drivers
>>   should be updated to follow it.

The documentation is key.  A usb charger framework needs to depend on
correct information from extcon, and currently there is no clear
documentation on how a USB phy should signal the charger type.
There isn't a lot to say: primarily that both the EXTCON_USB* and
EXTCON_CGH_USB* types should be provided together.  Each does not imply
the other in any way.  But it still needs to be documented.

Thanks,
NeilBrown


>>
>> 2/ Change all usb phys to register an extcon and to send appropriate
>>    notifications.  Many already do, but I don't think it is universal.
>>    It is probable that the extcon should be registered using common code
>>    instead of each phy driver having its own
>>    extcon_get_edev_by_phandle()
>>    or whatever.
>>    If the usb phy driver needs to look at battery charger registers to
>>    know what sort of cable was connected (which I believe is the case
>>    for the chips you are interested in), then it should do that.
>>
>> 3/ Currently some USB controllers discover that a cable was connected by
>>    listening on an extcon, and some by registering for a usb_notifier
>>    (described below) ... though there seem to only be 2 left which do that.
>>    Now that all USB phys send connection information via extcon (see 2),
>>    the USB controllers should be changed to all find out about the cable
>>    using extcon.
>>
>> 4/ struct usb_phy contains:
>>         /* for notification of usb_phy_events */
>>         struct atomic_notifier_head     notifier;
>>
>>   This is used inconsistently.  Sometimes the argument passed
>>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>>   limited negotiated via USB, sometimes it is a pointer the the gadget
>>   though as far as I can tell, that last one is never used.
>>
>>   This should be changed to be consistent.  This notifier is no longer
>>   needed to tell the USB controller that a cable was connected (extcon
>>   now does that, see 3) so it is only used to communicate the
>>   'vbus_draw' information.
>>   So it should be changed to *only* send a notification when vbus_draw
>>   is known, and it should carry that information.
>>   This should probably be done in common code, and removed
>>   from individual drivers.
>>
>> 5/ Now that all cable connection notifications are sent over extcon and
>>    all vbus_draw notifications are sent over the usb_phy notifier, write
>>    some support code that a power supply client can use to be told what
>>    power is available.
>>    e.g. a battery charger driver would call:
>>        register_power_client(.....)
>>    or similar, providing a phandle (or similar) for the usb phy and a
>>    function to call back when the available current changes (or maybe a
>>    work_struct containing the function pointer)
>>
>>    register_power_client() would then register with extcon and separately
>>    with the usb_phy notifier.  When the different events arrive it
>>    calculates what ranges of currents are expected and calls the
>>    call-back function with those details.
>>
>> 6/ Any battery charger that needs to know the available current can now
>>    call register_power_client() and get the information delivered.
>>
>> NeilBrown
>
>
>
> -- 
> Baolin.wang
> Best Regards

[toc] | [prev] | [next] | [standalone]

#1545560

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.

>
>>
>> From above investigation, I did not find anything need to be changed
>> now. What do you think? Thanks.
>>
>
> As above, I think changes are needed.
>
> I particularly highlight:
>
>>>   This policy should be well documented and possibly existing drivers
>>>   should be updated to follow it.
>
> The documentation is key.  A usb charger framework needs to depend on
> correct information from extcon, and currently there is no clear
> documentation on how a USB phy should signal the charger type.
> There isn't a lot to say: primarily that both the EXTCON_USB* and
> EXTCON_CGH_USB* types should be provided together.  Each does not imply
> the other in any way.  But it still needs to be documented.

Yes, I will try to add some documentation for this. Thanks.

>
> Thanks,
> NeilBrown
>
>
>>>
>>> 2/ Change all usb phys to register an extcon and to send appropriate
>>>    notifications.  Many already do, but I don't think it is universal.
>>>    It is probable that the extcon should be registered using common code
>>>    instead of each phy driver having its own
>>>    extcon_get_edev_by_phandle()
>>>    or whatever.
>>>    If the usb phy driver needs to look at battery charger registers to
>>>    know what sort of cable was connected (which I believe is the case
>>>    for the chips you are interested in), then it should do that.
>>>
>>> 3/ Currently some USB controllers discover that a cable was connected by
>>>    listening on an extcon, and some by registering for a usb_notifier
>>>    (described below) ... though there seem to only be 2 left which do that.
>>>    Now that all USB phys send connection information via extcon (see 2),
>>>    the USB controllers should be changed to all find out about the cable
>>>    using extcon.
>>>
>>> 4/ struct usb_phy contains:
>>>         /* for notification of usb_phy_events */
>>>         struct atomic_notifier_head     notifier;
>>>
>>>   This is used inconsistently.  Sometimes the argument passed
>>>   is NULL, sometimes it is a pointer to 'vbus_draw' - the current
>>>   limited negotiated via USB, sometimes it is a pointer the the gadget
>>>   though as far as I can tell, that last one is never used.
>>>
>>>   This should be changed to be consistent.  This notifier is no longer
>>>   needed to tell the USB controller that a cable was connected (extcon
>>>   now does that, see 3) so it is only used to communicate the
>>>   'vbus_draw' information.
>>>   So it should be changed to *only* send a notification when vbus_draw
>>>   is known, and it should carry that information.
>>>   This should probably be done in common code, and removed
>>>   from individual drivers.
>>>
>>> 5/ Now that all cable connection notifications are sent over extcon and
>>>    all vbus_draw notifications are sent over the usb_phy notifier, write
>>>    some support code that a power supply client can use to be told what
>>>    power is available.
>>>    e.g. a battery charger driver would call:
>>>        register_power_client(.....)
>>>    or similar, providing a phandle (or similar) for the usb phy and a
>>>    function to call back when the available current changes (or maybe a
>>>    work_struct containing the function pointer)
>>>
>>>    register_power_client() would then register with extcon and separately
>>>    with the usb_phy notifier.  When the different events arrive it
>>>    calculates what ranges of currents are expected and calls the
>>>    call-back function with those details.
>>>
>>> 6/ Any battery charger that needs to know the available current can now
>>>    call register_power_client() and get the information delivered.
>>>
>>> NeilBrown
>>
>>
>>
>> --
>> Baolin.wang
>> Best Regards



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1545573 — Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Multipart message — attachments visible in raw view] — view raw

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.

Unfortunately I cannot find any datasheets on line to verify how these
devices work, but the code seems to suggest that they can detect and
report special charger types that provide 500mA and 1A respectively.
If the hardware exists that reports the functionality, it make sense to
use it.  Hopefully new hardware will follow the USB BC spec, so further
special cases won't be needed.

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1545676

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

>
> Unfortunately I cannot find any datasheets on line to verify how these
> devices work, but the code seems to suggest that they can detect and
> report special charger types that provide 500mA and 1A respectively.
> If the hardware exists that reports the functionality, it make sense to
> use it.  Hopefully new hardware will follow the USB BC spec, so further
> special cases won't be needed.
>
> Thanks,
> NeilBrown



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1546064 — Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Multipart message — attachments visible in raw view] — view raw

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.

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

[toc] | [prev] | [next] | [standalone]

#1546203

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web