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

[Baolin Wang]

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 

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

[Baolin Wang]

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 

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

[NeilBrown]

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 

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

[NeilBrown]

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 

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

[Baolin Wang]

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

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

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

[Baolin Wang]

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

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

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

[NeilBrown]

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.

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


signature.asc
Description: PGP signature

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

[NeilBrown]

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.

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


signature.asc
Description: PGP signature

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

[Baolin Wang]

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.

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

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

[Baolin Wang]

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.

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

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

[NeilBrown]

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.

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

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

[NeilBrown]

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.

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

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

[Baolin Wang]

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.

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

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

[Baolin Wang]

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.

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 

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

[Baolin Wang]

On 25 November 2016 at 21:00, Mark Brown  wrote:
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?
>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.
>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

OK. I will think about how to incorporate conversion of the existing
users according to Neil's suggestion.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

Yes, I agree. But 'usb charger' is one virtual device and we described
nothing in DT about 'usb charger'. But as you and Neil said, we need
one usful method to look up the USB charger. I will think about that.
Thanks.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 25 November 2016 at 21:00, Mark Brown  wrote:
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?
>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.
>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

OK. I will think about how to incorporate conversion of the existing
users according to Neil's suggestion.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

Yes, I agree. But 'usb charger' is one virtual device and we described
nothing in DT about 'usb charger'. But as you and Neil said, we need
one usful method to look up the USB charger. I will think about that.
Thanks.

-- 
Baolin.wang
Best Regards

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

[NeilBrown]

On Sat, Nov 26 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?

I don't really like the idea of a separate "usb charger" object.  It
looks too much like a "midlayer" and they tend to get in the way.  But
if a convincing case could be made that changing from the current design
to that aspect of the proposed design brings measurable benefits, then I
would certainly assess that case on its merits.  No such case was made,
and the patchset didn't seem to even acknowledge the existing design.

When I said "I do object to some details" it was details of the end
result, not details of what took responsibility of information
aggregation (in case that wasn't clear).  Those details were everything
that duplicated existing functionality, or ignored existing
functionality, or was simply unworkable.  e.g. the lack of proper
integration with extcon, the new sysfs attributes, the name-lookup
mechanism.  Probably others.

>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.

Yes, you need a clear vision of the goal.  You also need a clear vision
of the starting point. There was no evidence of the latter.
Yes, sometimes you need to create a new thing and transition users over,
then discard the old.  There was no discarding of the old.

>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

I would be very encouraged to see those simple things done first!
Seeing the series grow isn't much fun, but seeing preliminary work land
certainly is.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

We already have a lookup mechanism for a battery charger to find the phy
that it gets current from: devm_usb_get_phy_by_phandle() (or even
devm_usb_get_phy() if there is known to only be one phy).  We would need
a case to be made that the existing mechanism cannot be used before we
consider "adding a DT binding and lookup mechanism".

Thanks,
NeilBrown


signature.asc
Description: PGP signature

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

[NeilBrown]

On Sat, Nov 26 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:
>
>> I agree that the question of where the responsibility for information
>> aggregation lies is open for discussion. If fact all details on how
>> things should work are always open for discussion.
>> I don't agree that this is the main different between our positions,
>> though I can see how you might get that impression.
>
>> You could even fix them so they look *exactly* like the notifiers that
>> Baolin is proposing.  This is my key point.  It is not the end result
>> that I particularly object to (though I do object to some details).  It
>
> Ah, OK.  This really hadn't been at all clear - both Baolin and I had
> the impression that the it was both that were blockers for you.  What
> were the details here?

I don't really like the idea of a separate "usb charger" object.  It
looks too much like a "midlayer" and they tend to get in the way.  But
if a convincing case could be made that changing from the current design
to that aspect of the proposed design brings measurable benefits, then I
would certainly assess that case on its merits.  No such case was made,
and the patchset didn't seem to even acknowledge the existing design.

When I said "I do object to some details" it was details of the end
result, not details of what took responsibility of information
aggregation (in case that wasn't clear).  Those details were everything
that duplicated existing functionality, or ignored existing
functionality, or was simply unworkable.  e.g. the lack of proper
integration with extcon, the new sysfs attributes, the name-lookup
mechanism.  Probably others.

>
>> is the process of getting to the end result that I don't like.  If the
>> current system doesn't work and something different is needed, then the
>> correct thing to do is to transform the existing system into something
>> new that works better.  This should be a clear series of steps.  Each
>
> Sometimes there's something to be said for working out what we want
> things to look like before setting out to make these gradual
> refactorings and sometimes the refactorings are just more pain than
> they're worth, especially when they go across subsystems.  In this case
> I do worry about the cross subsystem aspect causing hassle, it may be
> more practical to do anything that represents an interface change by
> adding the new interface, converting the users to it and then removing
> the old interface.

Yes, you need a clear vision of the goal.  You also need a clear vision
of the starting point. There was no evidence of the latter.
Yes, sometimes you need to create a new thing and transition users over,
then discard the old.  There was no discarding of the old.

>
> At the very least the series should grow to incorporate conversion of
> the existing users though.  Baolin, I think this does need adding to the
> series but probably best to think about how to do it - some of Neil's
> suggestions for incremental steps do seem like they should be useful
> for organizing things here, probably we can get some things that can be
> done internally within individual drivers merged while everything else
> is under discussion.

I would be very encouraged to see those simple things done first!
Seeing the series grow isn't much fun, but seeing preliminary work land
certainly is.

>
>> But I think here my key point got lost too, in part because it was hard
>> to refer to an actual instance.
>> My point was that in the present patch set, the "usb charger" is given
>> a name which is dependant on discovery order, and only supports
>> lookup-by-name.  This cannot work.
>
> There's two bits here: one is the way names are assigned and the other
> is the lookup by name.  I agree that the lookup by name isn't
> particularly useful as things stand, that could just be dropped until
> some naming mechanism is added.  We'd be more likely to use phandles in
> DT systems, I don't know what ACPI systems would look like but I guess
> it'd be something similar.
>
>> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
>> any usb-charger which has power available"), or look up by some other
>> attribute, then discover-order naming could work.  But the only
>> lookup-mechanism is by-name, and the names aren't reliably stable.  So
>> the name/lookup system proposed cannot possibly do anything useful
>> with more than one usb_charger.
>
> Baolin, I think adding a DT binding and lookup mechanism makes sense
> here - do you agree?

We already have a lookup mechanism for a battery charger to find the phy
that it gets current from: devm_usb_get_phy_by_phandle() (or even
devm_usb_get_phy() if there is known to only be one phy).  We would need
a case to be made that the existing mechanism cannot be used before we
consider "adding a DT binding and lookup mechanism".

Thanks,
NeilBrown


signature.asc
Description: PGP signature

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

[Mark Brown]

On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:

> I agree that the question of where the responsibility for information
> aggregation lies is open for discussion. If fact all details on how
> things should work are always open for discussion.
> I don't agree that this is the main different between our positions,
> though I can see how you might get that impression.

> You could even fix them so they look *exactly* like the notifiers that
> Baolin is proposing.  This is my key point.  It is not the end result
> that I particularly object to (though I do object to some details).  It

Ah, OK.  This really hadn't been at all clear - both Baolin and I had
the impression that the it was both that were blockers for you.  What
were the details here?

> is the process of getting to the end result that I don't like.  If the
> current system doesn't work and something different is needed, then the
> correct thing to do is to transform the existing system into something
> new that works better.  This should be a clear series of steps.  Each

Sometimes there's something to be said for working out what we want
things to look like before setting out to make these gradual
refactorings and sometimes the refactorings are just more pain than
they're worth, especially when they go across subsystems.  In this case
I do worry about the cross subsystem aspect causing hassle, it may be
more practical to do anything that represents an interface change by
adding the new interface, converting the users to it and then removing
the old interface.

At the very least the series should grow to incorporate conversion of
the existing users though.  Baolin, I think this does need adding to the
series but probably best to think about how to do it - some of Neil's
suggestions for incremental steps do seem like they should be useful
for organizing things here, probably we can get some things that can be
done internally within individual drivers merged while everything else
is under discussion.

> But I think here my key point got lost too, in part because it was hard
> to refer to an actual instance.
> My point was that in the present patch set, the "usb charger" is given
> a name which is dependant on discovery order, and only supports
> lookup-by-name.  This cannot work.

There's two bits here: one is the way names are assigned and the other
is the lookup by name.  I agree that the lookup by name isn't
particularly useful as things stand, that could just be dropped until
some naming mechanism is added.  We'd be more likely to use phandles in
DT systems, I don't know what ACPI systems would look like but I guess
it'd be something similar.

> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
> any usb-charger which has power available"), or look up by some other
> attribute, then discover-order naming could work.  But the only
> lookup-mechanism is by-name, and the names aren't reliably stable.  So
> the name/lookup system proposed cannot possibly do anything useful
> with more than one usb_charger.

Baolin, I think adding a DT binding and lookup mechanism makes sense
here - do you agree?


signature.asc
Description: PGP signature

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

[Mark Brown]

On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:

> I agree that the question of where the responsibility for information
> aggregation lies is open for discussion. If fact all details on how
> things should work are always open for discussion.
> I don't agree that this is the main different between our positions,
> though I can see how you might get that impression.

> You could even fix them so they look *exactly* like the notifiers that
> Baolin is proposing.  This is my key point.  It is not the end result
> that I particularly object to (though I do object to some details).  It

Ah, OK.  This really hadn't been at all clear - both Baolin and I had
the impression that the it was both that were blockers for you.  What
were the details here?

> is the process of getting to the end result that I don't like.  If the
> current system doesn't work and something different is needed, then the
> correct thing to do is to transform the existing system into something
> new that works better.  This should be a clear series of steps.  Each

Sometimes there's something to be said for working out what we want
things to look like before setting out to make these gradual
refactorings and sometimes the refactorings are just more pain than
they're worth, especially when they go across subsystems.  In this case
I do worry about the cross subsystem aspect causing hassle, it may be
more practical to do anything that represents an interface change by
adding the new interface, converting the users to it and then removing
the old interface.

At the very least the series should grow to incorporate conversion of
the existing users though.  Baolin, I think this does need adding to the
series but probably best to think about how to do it - some of Neil's
suggestions for incremental steps do seem like they should be useful
for organizing things here, probably we can get some things that can be
done internally within individual drivers merged while everything else
is under discussion.

> But I think here my key point got lost too, in part because it was hard
> to refer to an actual instance.
> My point was that in the present patch set, the "usb charger" is given
> a name which is dependant on discovery order, and only supports
> lookup-by-name.  This cannot work.

There's two bits here: one is the way names are assigned and the other
is the lookup by name.  I agree that the lookup by name isn't
particularly useful as things stand, that could just be dropped until
some naming mechanism is added.  We'd be more likely to use phandles in
DT systems, I don't know what ACPI systems would look like but I guess
it'd be something similar.

> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
> any usb-charger which has power available"), or look up by some other
> attribute, then discover-order naming could work.  But the only
> lookup-mechanism is by-name, and the names aren't reliably stable.  So
> the name/lookup system proposed cannot possibly do anything useful
> with more than one usb_charger.

Baolin, I think adding a DT binding and lookup mechanism makes sense
here - do you agree?


signature.asc
Description: PGP signature

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

[NeilBrown]

On Tue, Nov 22 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 17 2016, Mark Brown wrote:
>
>> > To me that's pretty much what's being done here, the code just happens
>> > to sit in USB instead but fundamentally it's just a blob of helper code,
>> > you could replace the notifier with a callback if that's the big concern
>> > here.
>
>> It is a lot more than "just a blob of helper code".  It duplicates
>> existing infrastructure instead of fixing and using the
>> infrastructure but I've said all this before.  Repeatedly.
>
> My read on that is that the question of what we want to be responsible
> for aggregating the information about what power the system is allowed
> to draw from a given USB port hasn't been resolved yet and that apart
> from that you're fairly close.  It seems to me like that's really what
> the difference between your two positions is.  Fixing the existing
> notifiers implies that things have to be aggregated in the power supply
> drivers but Baolin is proposing doing that in the USB code instead.  It
> does seem at least worth considering if that's a good idea since the
> current situation doesn't look terribly thought through.

I agree that the question of where the responsibility for information
aggregation lies is open for discussion. If fact all details on how
things should work are always open for discussion.
I don't agree that this is the main different between our positions,
though I can see how you might get that impression.

I don't agree that "Fixing the existing notifiers implies that things
have to be aggregated in the power supply drivers".  That would be the
simplest way to fix them, but not the only way.  You could fix them so
that the usb driver sends notifications instead of the phy, and you
could fix them so that the information contained in the notification
being a power range instead the current ad-hoc inconsistent set of
information.

You could even fix them so they look *exactly* like the notifiers that
Baolin is proposing.  This is my key point.  It is not the end result
that I particularly object to (though I do object to some details).  It
is the process of getting to the end result that I don't like.  If the
current system doesn't work and something different is needed, then the
correct thing to do is to transform the existing system into something
new that works better.  This should be a clear series of steps.  Each
one makes a small, well defined, change.  Maybe it adds a new feature.
Maybe it refactors code without changing functionality.  Maybe it fixes a
bug.  Maybe it moves the responsibility for an action from *here* to
*there*.  Each step is clearly explained and convincingly justified.
This doesn't have to be a long justification.  Sometimes a single
sentence or a short paragraph is plenty.  Sometimes the change is so
obviously an improvement that no explanation is necessary.

Changing one step at a time make it easier to ensure that no
functionality is lost and that each change is actually needed.

If the patch series fixed the existing code and transformed it into
something better and more powerful, then it would be a lot easier to
have sensible discussions about individual patches, about which there
might be disagreement.

>
> There are a whole bunch of things that need to be sorted out whatever
> the decision is like the extcon related cleanups you mentioned in your
> mail the other day (steps 1 and 2), it seems like those could be moved
> forwards independently.

Agreed.

>
> By the way it occurred to me recently that we have a use case for
> multiple USB ports that could supply power - USB C.  Things with more
> than one port like things in a laptop form factor are going to want to
> be able to use all of them interchangably for power support (likely only
> one at a time, at least initially, but still more than one port).

Thanks!  It is so much easier to discusses these sorts of things where
there is a concrete reference point.

I wonder if each port would have its own current regulator, or if there
would be a single central one.  Maybe there would be a switch that
allowed power to flow in only from one port.
Would each "charger" need to get notifications from "its" port, or would
there be a single "charger" which got notifications from the
power-switch, which in-turn got notifications from each port?
Or would the battery-charger driver want to register with every port,
receive notifications, and be able to turn each one on or off?
Without concrete details of actual hardware, we can only guess.

But I think here my key point got lost too, in part because it was hard
to refer to an actual instance.
My point was that in the present patch set, the "usb charger" is given
a name which is dependant on discovery order, and only supports
lookup-by-name.  This cannot work.
If each USB-C port registers a usb_charger, they would be
"usb-charger.0", "usb-charger.1", 

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

[NeilBrown]

On Tue, Nov 22 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 17 2016, Mark Brown wrote:
>
>> > To me that's pretty much what's being done here, the code just happens
>> > to sit in USB instead but fundamentally it's just a blob of helper code,
>> > you could replace the notifier with a callback if that's the big concern
>> > here.
>
>> It is a lot more than "just a blob of helper code".  It duplicates
>> existing infrastructure instead of fixing and using the
>> infrastructure but I've said all this before.  Repeatedly.
>
> My read on that is that the question of what we want to be responsible
> for aggregating the information about what power the system is allowed
> to draw from a given USB port hasn't been resolved yet and that apart
> from that you're fairly close.  It seems to me like that's really what
> the difference between your two positions is.  Fixing the existing
> notifiers implies that things have to be aggregated in the power supply
> drivers but Baolin is proposing doing that in the USB code instead.  It
> does seem at least worth considering if that's a good idea since the
> current situation doesn't look terribly thought through.

I agree that the question of where the responsibility for information
aggregation lies is open for discussion. If fact all details on how
things should work are always open for discussion.
I don't agree that this is the main different between our positions,
though I can see how you might get that impression.

I don't agree that "Fixing the existing notifiers implies that things
have to be aggregated in the power supply drivers".  That would be the
simplest way to fix them, but not the only way.  You could fix them so
that the usb driver sends notifications instead of the phy, and you
could fix them so that the information contained in the notification
being a power range instead the current ad-hoc inconsistent set of
information.

You could even fix them so they look *exactly* like the notifiers that
Baolin is proposing.  This is my key point.  It is not the end result
that I particularly object to (though I do object to some details).  It
is the process of getting to the end result that I don't like.  If the
current system doesn't work and something different is needed, then the
correct thing to do is to transform the existing system into something
new that works better.  This should be a clear series of steps.  Each
one makes a small, well defined, change.  Maybe it adds a new feature.
Maybe it refactors code without changing functionality.  Maybe it fixes a
bug.  Maybe it moves the responsibility for an action from *here* to
*there*.  Each step is clearly explained and convincingly justified.
This doesn't have to be a long justification.  Sometimes a single
sentence or a short paragraph is plenty.  Sometimes the change is so
obviously an improvement that no explanation is necessary.

Changing one step at a time make it easier to ensure that no
functionality is lost and that each change is actually needed.

If the patch series fixed the existing code and transformed it into
something better and more powerful, then it would be a lot easier to
have sensible discussions about individual patches, about which there
might be disagreement.

>
> There are a whole bunch of things that need to be sorted out whatever
> the decision is like the extcon related cleanups you mentioned in your
> mail the other day (steps 1 and 2), it seems like those could be moved
> forwards independently.

Agreed.

>
> By the way it occurred to me recently that we have a use case for
> multiple USB ports that could supply power - USB C.  Things with more
> than one port like things in a laptop form factor are going to want to
> be able to use all of them interchangably for power support (likely only
> one at a time, at least initially, but still more than one port).

Thanks!  It is so much easier to discusses these sorts of things where
there is a concrete reference point.

I wonder if each port would have its own current regulator, or if there
would be a single central one.  Maybe there would be a switch that
allowed power to flow in only from one port.
Would each "charger" need to get notifications from "its" port, or would
there be a single "charger" which got notifications from the
power-switch, which in-turn got notifications from each port?
Or would the battery-charger driver want to register with every port,
receive notifications, and be able to turn each one on or off?
Without concrete details of actual hardware, we can only guess.

But I think here my key point got lost too, in part because it was hard
to refer to an actual instance.
My point was that in the present patch set, the "usb charger" is given
a name which is dependant on discovery order, and only supports
lookup-by-name.  This cannot work.
If each USB-C port registers a usb_charger, they would be
"usb-charger.0", "usb-charger.1", 

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

[Mark Brown]

On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
> On Thu, Nov 17 2016, Mark Brown wrote:

> > To me that's pretty much what's being done here, the code just happens
> > to sit in USB instead but fundamentally it's just a blob of helper code,
> > you could replace the notifier with a callback if that's the big concern
> > here.

> It is a lot more than "just a blob of helper code".  It duplicates
> existing infrastructure instead of fixing and using the
> infrastructure but I've said all this before.  Repeatedly.

My read on that is that the question of what we want to be responsible
for aggregating the information about what power the system is allowed
to draw from a given USB port hasn't been resolved yet and that apart
from that you're fairly close.  It seems to me like that's really what
the difference between your two positions is.  Fixing the existing
notifiers implies that things have to be aggregated in the power supply
drivers but Baolin is proposing doing that in the USB code instead.  It
does seem at least worth considering if that's a good idea since the
current situation doesn't look terribly thought through.

There are a whole bunch of things that need to be sorted out whatever
the decision is like the extcon related cleanups you mentioned in your
mail the other day (steps 1 and 2), it seems like those could be moved
forwards independently.

By the way it occurred to me recently that we have a use case for
multiple USB ports that could supply power - USB C.  Things with more
than one port like things in a laptop form factor are going to want to
be able to use all of them interchangably for power support (likely only
one at a time, at least initially, but still more than one port).


signature.asc
Description: PGP signature

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

[Mark Brown]

On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
> On Thu, Nov 17 2016, Mark Brown wrote:

> > To me that's pretty much what's being done here, the code just happens
> > to sit in USB instead but fundamentally it's just a blob of helper code,
> > you could replace the notifier with a callback if that's the big concern
> > here.

> It is a lot more than "just a blob of helper code".  It duplicates
> existing infrastructure instead of fixing and using the
> infrastructure but I've said all this before.  Repeatedly.

My read on that is that the question of what we want to be responsible
for aggregating the information about what power the system is allowed
to draw from a given USB port hasn't been resolved yet and that apart
from that you're fairly close.  It seems to me like that's really what
the difference between your two positions is.  Fixing the existing
notifiers implies that things have to be aggregated in the power supply
drivers but Baolin is proposing doing that in the USB code instead.  It
does seem at least worth considering if that's a good idea since the
current situation doesn't look terribly thought through.

There are a whole bunch of things that need to be sorted out whatever
the decision is like the extcon related cleanups you mentioned in your
mail the other day (steps 1 and 2), it seems like those could be moved
forwards independently.

By the way it occurred to me recently that we have a use case for
multiple USB ports that could supply power - USB C.  Things with more
than one port like things in a laptop form factor are going to want to
be able to use all of them interchangably for power support (likely only
one at a time, at least initially, but still more than one port).


signature.asc
Description: PGP signature

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

[NeilBrown]

On Thu, Nov 17 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
>> On Mon, Nov 14 2016, Mark Brown wrote:
>
>> > Conflating the two seems like the whole point here.  We're looking for
>> > something that sits between the power supply code and the USB code and
>> > tells the power supply code what it's allowed to do which is the result
>> > of a combination of physical cable detection and USB protocol.  It seems
>> > reasonable that extcon drivers ought to be part of this but it doesn't
>> > seem like they are the whole story.
>
>> I don't think "between the power supply code and the USB code" is where
>> this thing sits. I think it sits inside the power-supply driver.
>> We already have extcon which sits between the phy and the power_supply
>> code, and the usb_notifier which sits between the USB code and the
>> power supply code.  We don't need another go-between.
>
> ...
>
>> correct determinations and set the current limits itself.  All this
>> could be done entirely internally, without the help of any new
>> subsystem.
>> Do you agree?
>
>> Clearly doing it that way would result in lots of code duplication and
>> would mean that each driver probably gets its own private set of bugs,
>> but it would be possible.  Just undesirable.
>
> I think this is the key here - the fact that it's technically possible
> to implement doesn't really matter if it's sufficiently fiddly and non
> obvious that nobody is actually joining everything up (bits are done
> intermittently but not as a whole as far as I can see).
>
>> So yes, it makes perfect to provide common code which handles the
>> registrations, and captures the events, and translates the different
>> events into current levels and feeds those back to the driver.  This
>> isn't some new subsystem, this is just a resource, provided by a
>> library, that power drivers can allocate and initialize if the want to.
>
> To me that's pretty much what's being done here, the code just happens
> to sit in USB instead but fundamentally it's just a blob of helper code,
> you could replace the notifier with a callback if that's the big concern
> here.

It is a lot more than "just a blob of helper code".  It duplicates
existing infrastructure instead of fixing and using the
infrastructure but I've said all this before.  Repeatedly.

NeilBrown


signature.asc
Description: PGP signature

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

[NeilBrown]

On Thu, Nov 17 2016, Mark Brown wrote:

> [ Unknown signature status ]
> On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
>> On Mon, Nov 14 2016, Mark Brown wrote:
>
>> > Conflating the two seems like the whole point here.  We're looking for
>> > something that sits between the power supply code and the USB code and
>> > tells the power supply code what it's allowed to do which is the result
>> > of a combination of physical cable detection and USB protocol.  It seems
>> > reasonable that extcon drivers ought to be part of this but it doesn't
>> > seem like they are the whole story.
>
>> I don't think "between the power supply code and the USB code" is where
>> this thing sits. I think it sits inside the power-supply driver.
>> We already have extcon which sits between the phy and the power_supply
>> code, and the usb_notifier which sits between the USB code and the
>> power supply code.  We don't need another go-between.
>
> ...
>
>> correct determinations and set the current limits itself.  All this
>> could be done entirely internally, without the help of any new
>> subsystem.
>> Do you agree?
>
>> Clearly doing it that way would result in lots of code duplication and
>> would mean that each driver probably gets its own private set of bugs,
>> but it would be possible.  Just undesirable.
>
> I think this is the key here - the fact that it's technically possible
> to implement doesn't really matter if it's sufficiently fiddly and non
> obvious that nobody is actually joining everything up (bits are done
> intermittently but not as a whole as far as I can see).
>
>> So yes, it makes perfect to provide common code which handles the
>> registrations, and captures the events, and translates the different
>> events into current levels and feeds those back to the driver.  This
>> isn't some new subsystem, this is just a resource, provided by a
>> library, that power drivers can allocate and initialize if the want to.
>
> To me that's pretty much what's being done here, the code just happens
> to sit in USB instead but fundamentally it's just a blob of helper code,
> you could replace the notifier with a callback if that's the big concern
> here.

It is a lot more than "just a blob of helper code".  It duplicates
existing infrastructure instead of fixing and using the
infrastructure but I've said all this before.  Repeatedly.

NeilBrown


signature.asc
Description: PGP signature

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

[Mark Brown]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:

> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.

> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.

...

> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

> Clearly doing it that way would result in lots of code duplication and
> would mean that each driver probably gets its own private set of bugs,
> but it would be possible.  Just undesirable.

I think this is the key here - the fact that it's technically possible
to implement doesn't really matter if it's sufficiently fiddly and non
obvious that nobody is actually joining everything up (bits are done
intermittently but not as a whole as far as I can see).

> So yes, it makes perfect to provide common code which handles the
> registrations, and captures the events, and translates the different
> events into current levels and feeds those back to the driver.  This
> isn't some new subsystem, this is just a resource, provided by a
> library, that power drivers can allocate and initialize if the want to.

To me that's pretty much what's being done here, the code just happens
to sit in USB instead but fundamentally it's just a blob of helper code,
you could replace the notifier with a callback if that's the big concern
here.


signature.asc
Description: PGP signature

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

[Mark Brown]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:

> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.

> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.

...

> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

> Clearly doing it that way would result in lots of code duplication and
> would mean that each driver probably gets its own private set of bugs,
> but it would be possible.  Just undesirable.

I think this is the key here - the fact that it's technically possible
to implement doesn't really matter if it's sufficiently fiddly and non
obvious that nobody is actually joining everything up (bits are done
intermittently but not as a whole as far as I can see).

> So yes, it makes perfect to provide common code which handles the
> registrations, and captures the events, and translates the different
> events into current levels and feeds those back to the driver.  This
> isn't some new subsystem, this is just a resource, provided by a
> library, that power drivers can allocate and initialize if the want to.

To me that's pretty much what's being done here, the code just happens
to sit in USB instead but fundamentally it's just a blob of helper code,
you could replace the notifier with a callback if that's the big concern
here.


signature.asc
Description: PGP signature

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

[Peter Chen]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:
> 
> > On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> >> On Thu, Nov 10 2016, Baolin Wang wrote:
> >
> >> > Fourth, we need integrate all charger plugin/out
> >> > event in one framework, not from extcon, maybe type-c in future.
> >
> >> Why not extcon?  Given that a charger is connected by an external
> >> connector, extcon seems like exactly the right thing to use.
> >
> >> Obviously extcon doesn't report the current that was negotiated, but
> >> that is best kept separate.  The battery charger can be advised of the
> >> available current either via extcon or separately via the usb
> >> subsystem.  Don't conflate the two.
> >
> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.
> 
> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.
> 
> If we have extcon able to deliver reliable information about cable type,
> and if with have the usb notifier able to deliver reliable information
> about negotiated current, and if the power supply manager is able to
> register with the correct extcon and the correct usb notifier, then the
> power supply manager *could* handle all the notifications and make the
> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

Through the USB gadget/phy framework (usb_gadget.vbus_draw->usb_phy.set_power)
we can get the USB bus information when the device connects SDP, but the
enum usb_phy_events lacks some events like bus suspend (2mA), and bus
speed (high/super speed, 500mA vs 900mA). Besides many USB PHYs use
generic PHY driver now, it is lack of above event and related notifier.

About getting cable type, the key points are detect vbus and negotiate
the charger type, these two stuffs are much different among platforms.
Extcon has charger type definition, it is good, we can use it.
But it needs the device which has charger detection function as extcon
device too, and at meanwhile, this device needs to have vbus detect
function, most pmic devices are suitable for that, but not for USB PHY.

Asssume wm831x as a power client, according your suggestion, does its
design like below?
At dts, it needs to be described like below:
 {
...
phy-dev = <_phy>;
extcon-dev = <>;
...
}
And at wm831x driver, it gets information through extcon-dev and phy-dev
notifier, and it needs knowledge about current limit for specific
cable type, but these information are from USB (Charger) specification.

Your suggestion is trying use current notifications to get the
information for power client, this patch set is trying to keep
these two notifications at an new framework, and power client
gets refined notification from this new framework.

The biggest problem I concern about your solution is extcon device, it may
not be an universal solution, does current frameworks have a way to
get cable type (usb charger type)? If not, we may need to have a new
framework.

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:
> 
> > On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> >> On Thu, Nov 10 2016, Baolin Wang wrote:
> >
> >> > Fourth, we need integrate all charger plugin/out
> >> > event in one framework, not from extcon, maybe type-c in future.
> >
> >> Why not extcon?  Given that a charger is connected by an external
> >> connector, extcon seems like exactly the right thing to use.
> >
> >> Obviously extcon doesn't report the current that was negotiated, but
> >> that is best kept separate.  The battery charger can be advised of the
> >> available current either via extcon or separately via the usb
> >> subsystem.  Don't conflate the two.
> >
> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.
> 
> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.
> 
> If we have extcon able to deliver reliable information about cable type,
> and if with have the usb notifier able to deliver reliable information
> about negotiated current, and if the power supply manager is able to
> register with the correct extcon and the correct usb notifier, then the
> power supply manager *could* handle all the notifications and make the
> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

Through the USB gadget/phy framework (usb_gadget.vbus_draw->usb_phy.set_power)
we can get the USB bus information when the device connects SDP, but the
enum usb_phy_events lacks some events like bus suspend (2mA), and bus
speed (high/super speed, 500mA vs 900mA). Besides many USB PHYs use
generic PHY driver now, it is lack of above event and related notifier.

About getting cable type, the key points are detect vbus and negotiate
the charger type, these two stuffs are much different among platforms.
Extcon has charger type definition, it is good, we can use it.
But it needs the device which has charger detection function as extcon
device too, and at meanwhile, this device needs to have vbus detect
function, most pmic devices are suitable for that, but not for USB PHY.

Asssume wm831x as a power client, according your suggestion, does its
design like below?
At dts, it needs to be described like below:
 {
...
phy-dev = <_phy>;
extcon-dev = <>;
...
}
And at wm831x driver, it gets information through extcon-dev and phy-dev
notifier, and it needs knowledge about current limit for specific
cable type, but these information are from USB (Charger) specification.

Your suggestion is trying use current notifications to get the
information for power client, this patch set is trying to keep
these two notifications at an new framework, and power client
gets refined notification from this new framework.

The biggest problem I concern about your solution is extcon device, it may
not be an universal solution, does current frameworks have a way to
get cable type (usb charger type)? If not, we may need to have a new
framework.

-- 

Best Regards,
Peter Chen

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

[NeilBrown]

On Mon, Nov 14 2016, Mark Brown wrote:

> On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> > Fourth, we need integrate all charger plugin/out
>> > event in one framework, not from extcon, maybe type-c in future.
>
>> Why not extcon?  Given that a charger is connected by an external
>> connector, extcon seems like exactly the right thing to use.
>
>> Obviously extcon doesn't report the current that was negotiated, but
>> that is best kept separate.  The battery charger can be advised of the
>> available current either via extcon or separately via the usb
>> subsystem.  Don't conflate the two.
>
> Conflating the two seems like the whole point here.  We're looking for
> something that sits between the power supply code and the USB code and
> tells the power supply code what it's allowed to do which is the result
> of a combination of physical cable detection and USB protocol.  It seems
> reasonable that extcon drivers ought to be part of this but it doesn't
> seem like they are the whole story.

I don't think "between the power supply code and the USB code" is where
this thing sits. I think it sits inside the power-supply driver.
We already have extcon which sits between the phy and the power_supply
code, and the usb_notifier which sits between the USB code and the
power supply code.  We don't need another go-between.

If we have extcon able to deliver reliable information about cable type,
and if with have the usb notifier able to deliver reliable information
about negotiated current, and if the power supply manager is able to
register with the correct extcon and the correct usb notifier, then the
power supply manager *could* handle all the notifications and make the
correct determinations and set the current limits itself.  All this
could be done entirely internally, without the help of any new
subsystem.
Do you agree?

Clearly doing it that way would result in lots of code duplication and
would mean that each driver probably gets its own private set of bugs,
but it would be possible.  Just undesirable.

So yes, it makes perfect to provide common code which handles the
registrations, and captures the events, and translates the different
events into current levels and feeds those back to the driver.  This
isn't some new subsystem, this is just a resource, provided by a
library, that power drivers can allocate and initialize if the want to.

To quote myself:

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

NeilBrown


signature.asc
Description: PGP signature

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

[NeilBrown]

On Mon, Nov 14 2016, Mark Brown wrote:

> On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
>> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> > Fourth, we need integrate all charger plugin/out
>> > event in one framework, not from extcon, maybe type-c in future.
>
>> Why not extcon?  Given that a charger is connected by an external
>> connector, extcon seems like exactly the right thing to use.
>
>> Obviously extcon doesn't report the current that was negotiated, but
>> that is best kept separate.  The battery charger can be advised of the
>> available current either via extcon or separately via the usb
>> subsystem.  Don't conflate the two.
>
> Conflating the two seems like the whole point here.  We're looking for
> something that sits between the power supply code and the USB code and
> tells the power supply code what it's allowed to do which is the result
> of a combination of physical cable detection and USB protocol.  It seems
> reasonable that extcon drivers ought to be part of this but it doesn't
> seem like they are the whole story.

I don't think "between the power supply code and the USB code" is where
this thing sits. I think it sits inside the power-supply driver.
We already have extcon which sits between the phy and the power_supply
code, and the usb_notifier which sits between the USB code and the
power supply code.  We don't need another go-between.

If we have extcon able to deliver reliable information about cable type,
and if with have the usb notifier able to deliver reliable information
about negotiated current, and if the power supply manager is able to
register with the correct extcon and the correct usb notifier, then the
power supply manager *could* handle all the notifications and make the
correct determinations and set the current limits itself.  All this
could be done entirely internally, without the help of any new
subsystem.
Do you agree?

Clearly doing it that way would result in lots of code duplication and
would mean that each driver probably gets its own private set of bugs,
but it would be possible.  Just undesirable.

So yes, it makes perfect to provide common code which handles the
registrations, and captures the events, and translates the different
events into current levels and feeds those back to the driver.  This
isn't some new subsystem, this is just a resource, provided by a
library, that power drivers can allocate and initialize if the want to.

To quote myself:

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

NeilBrown


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 14 November 2016 at 12:21, NeilBrown  wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> Hi
>>
>> On 8 November 2016 at 04:36, NeilBrown  wrote:
>>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>>
 On 3 November 2016 at 09:25, NeilBrown  wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:

 I agree with your most opinions, but these are optimization.
>>>
>>> I see them as bug fixes, not optimizations.
>>>
  Firstly I
 think we should upstream the USB charger driver.
>>>
>>> I think you missed the point.  The point is that we don't *need* your
>>> "USB charger driver" because all the infrastructure we need is *already*
>>> present in the kernel.  It is buggy and not used uniformly, and could
>>> usefully be polished and improved.  But the structure is already
>>> present.
>>>
>>> If everyone just added new infrastructure when they didn't like, or
>>> didn't understand, what was already present, the kernel would become
>>> like the Mad Hatter's tea party, full of dirty dishes.
>>>
  What I want to ask is
 how can we notify power driver if we don't set the
 usb_register_notifier(), then I think you give the answer is: power
 driver can register by 'struct usb_phy->notifier'. But why usb phy
 should notify the power driver how much current should be drawn, and I
 still think we should notify the current in usb charger driver which
 is better, and do not need to notify current for power driver in usb
 phy driver.
>>>
>>> I accept that it isn't clear that the phy *should* be involved in
>>> communicating the negotiated power availability, but nor is it clear
>>> that it shouldn't.  The power does travel through the physical
>>> interface, so physically it plays a role.
>>>
>>> But more importantly, it already *does* get told (in some cases).
>>> There is an interface "usb_phy_set_power()" which exists explicitly to
>>> tell the phy what power has been negotiated.  Given that infrastructure
>>> exists and works, it make sense to use it.
>>>
>>> If you think it is a broken design and should be removed, then fine:
>>> make a case for that.  Examine the history.  Make sure you know why it
>>> is there (or make sure that information cannot be found), and then
>>> present a case as to why it should be removed and replaced with
>>> something else.  But don't just leave it there and pretend it doesn't
>>> exist and create something similar-but-different and hope people will
>>> know why yours is better.  That way lies madness.
>>
>> Like Peter said, it is not only PHY can detect the USB charger type,
>> which means there are other places can detect the charger type.
>
> If I understand Peter's example correctly, it shows a configuration
> where the USB PHysical interface is partly implemented in the SOC and
> partly in the PMIC.  I appreciate that would make it more challenging to
> implement a PHY driver, but there is no reason it should impact anything
> outside of the PHY.

Like Peter's example, it need to use controller register to pull up dp
to begin the secondary detection, which is not belonged to phy driver
and I don't think it is suitable we implemented these in phy driver.

>
>> Second, some controller need to detect the charger type manually which
>> USB phy did not support.
>
> "manually" is an odd term to use in this context.

Sorry for the confusing.

> I think you mean that to detect the charger type you need to issue some
> command to the hardware and wait for it to respond, then assess the
> response.

Yes.

> That isn't at all surprising.  The charger type is detected by measuring
> resistance between ID and GND, which may require setting up a potential
> and activating ADCs to measure the voltage.  This can all be done
> internally to the phy driver.
> Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
> twl4030, though it never got upstream).
> The code for the imx7d does look more complex, but not intrinsically
> different.

But you should implement these in every phy driver, why not one
standard framework?

>
>> Third, it did not handle what current should
>> be drawn in USB phy.
>
> The standards define that.  The extcon just reports the cable type.
> Certainly it would be sensible to provide a library function to
> translate from cable type to current range.  You don't need a new
> subsystem to do that, just a library function.

I don't think the extcon should handle current things. For example,
the extcon can not know the gadget speed, which is used to change the
default current values for super speed gadget.

>
>> Fourth, we need integrate all charger plugin/out
>> event in one framework, not from extcon, maybe type-c in future.
>
> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

My 

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

[Baolin Wang]

On 14 November 2016 at 12:21, NeilBrown  wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:
>
>> Hi
>>
>> On 8 November 2016 at 04:36, NeilBrown  wrote:
>>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>>
 On 3 November 2016 at 09:25, NeilBrown  wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:

 I agree with your most opinions, but these are optimization.
>>>
>>> I see them as bug fixes, not optimizations.
>>>
  Firstly I
 think we should upstream the USB charger driver.
>>>
>>> I think you missed the point.  The point is that we don't *need* your
>>> "USB charger driver" because all the infrastructure we need is *already*
>>> present in the kernel.  It is buggy and not used uniformly, and could
>>> usefully be polished and improved.  But the structure is already
>>> present.
>>>
>>> If everyone just added new infrastructure when they didn't like, or
>>> didn't understand, what was already present, the kernel would become
>>> like the Mad Hatter's tea party, full of dirty dishes.
>>>
  What I want to ask is
 how can we notify power driver if we don't set the
 usb_register_notifier(), then I think you give the answer is: power
 driver can register by 'struct usb_phy->notifier'. But why usb phy
 should notify the power driver how much current should be drawn, and I
 still think we should notify the current in usb charger driver which
 is better, and do not need to notify current for power driver in usb
 phy driver.
>>>
>>> I accept that it isn't clear that the phy *should* be involved in
>>> communicating the negotiated power availability, but nor is it clear
>>> that it shouldn't.  The power does travel through the physical
>>> interface, so physically it plays a role.
>>>
>>> But more importantly, it already *does* get told (in some cases).
>>> There is an interface "usb_phy_set_power()" which exists explicitly to
>>> tell the phy what power has been negotiated.  Given that infrastructure
>>> exists and works, it make sense to use it.
>>>
>>> If you think it is a broken design and should be removed, then fine:
>>> make a case for that.  Examine the history.  Make sure you know why it
>>> is there (or make sure that information cannot be found), and then
>>> present a case as to why it should be removed and replaced with
>>> something else.  But don't just leave it there and pretend it doesn't
>>> exist and create something similar-but-different and hope people will
>>> know why yours is better.  That way lies madness.
>>
>> Like Peter said, it is not only PHY can detect the USB charger type,
>> which means there are other places can detect the charger type.
>
> If I understand Peter's example correctly, it shows a configuration
> where the USB PHysical interface is partly implemented in the SOC and
> partly in the PMIC.  I appreciate that would make it more challenging to
> implement a PHY driver, but there is no reason it should impact anything
> outside of the PHY.

Like Peter's example, it need to use controller register to pull up dp
to begin the secondary detection, which is not belonged to phy driver
and I don't think it is suitable we implemented these in phy driver.

>
>> Second, some controller need to detect the charger type manually which
>> USB phy did not support.
>
> "manually" is an odd term to use in this context.

Sorry for the confusing.

> I think you mean that to detect the charger type you need to issue some
> command to the hardware and wait for it to respond, then assess the
> response.

Yes.

> That isn't at all surprising.  The charger type is detected by measuring
> resistance between ID and GND, which may require setting up a potential
> and activating ADCs to measure the voltage.  This can all be done
> internally to the phy driver.
> Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
> twl4030, though it never got upstream).
> The code for the imx7d does look more complex, but not intrinsically
> different.

But you should implement these in every phy driver, why not one
standard framework?

>
>> Third, it did not handle what current should
>> be drawn in USB phy.
>
> The standards define that.  The extcon just reports the cable type.
> Certainly it would be sensible to provide a library function to
> translate from cable type to current range.  You don't need a new
> subsystem to do that, just a library function.

I don't think the extcon should handle current things. For example,
the extcon can not know the gadget speed, which is used to change the
default current values for super speed gadget.

>
>> Fourth, we need integrate all charger plugin/out
>> event in one framework, not from extcon, maybe type-c in future.
>
> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

My mistake, what I mean is not only from extcon, maybe 

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

[Mark Brown]

On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:

> > Fourth, we need integrate all charger plugin/out
> > event in one framework, not from extcon, maybe type-c in future.

> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

> Obviously extcon doesn't report the current that was negotiated, but
> that is best kept separate.  The battery charger can be advised of the
> available current either via extcon or separately via the usb
> subsystem.  Don't conflate the two.

Conflating the two seems like the whole point here.  We're looking for
something that sits between the power supply code and the USB code and
tells the power supply code what it's allowed to do which is the result
of a combination of physical cable detection and USB protocol.  It seems
reasonable that extcon drivers ought to be part of this but it doesn't
seem like they are the whole story.

> > word, we need one standard integration of this feature we need, though
> > like you said we should do some clean up or fix to make it better.

> But really, I'm not the person you need to convince.  I'm just a vaguely
> interested bystander who is rapidly losing interest.  You need to
> convince a maintainer, but they have so far shown remarkably little
> interest.  I don't know why, but I'd guess that reviewing a complex new
> subsystem isn't much fun.  Reviewing and applying clear bugfixes and
> incremental improvements is much easier and more enjoyable.  But that is
> just a guess.

OTOH having someone else having reviewed might help them apply something
they don't care about.


signature.asc
Description: PGP signature

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

[Mark Brown]

On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:

> > Fourth, we need integrate all charger plugin/out
> > event in one framework, not from extcon, maybe type-c in future.

> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

> Obviously extcon doesn't report the current that was negotiated, but
> that is best kept separate.  The battery charger can be advised of the
> available current either via extcon or separately via the usb
> subsystem.  Don't conflate the two.

Conflating the two seems like the whole point here.  We're looking for
something that sits between the power supply code and the USB code and
tells the power supply code what it's allowed to do which is the result
of a combination of physical cable detection and USB protocol.  It seems
reasonable that extcon drivers ought to be part of this but it doesn't
seem like they are the whole story.

> > word, we need one standard integration of this feature we need, though
> > like you said we should do some clean up or fix to make it better.

> But really, I'm not the person you need to convince.  I'm just a vaguely
> interested bystander who is rapidly losing interest.  You need to
> convince a maintainer, but they have so far shown remarkably little
> interest.  I don't know why, but I'd guess that reviewing a complex new
> subsystem isn't much fun.  Reviewing and applying clear bugfixes and
> incremental improvements is much easier and more enjoyable.  But that is
> just a guess.

OTOH having someone else having reviewed might help them apply something
they don't care about.


signature.asc
Description: PGP signature

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

[NeilBrown]

On Thu, Nov 10 2016, Baolin Wang wrote:

> Hi
>
> On 8 November 2016 at 04:36, NeilBrown  wrote:
>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>
>>> On 3 November 2016 at 09:25, NeilBrown  wrote:
 On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>> I agree with your most opinions, but these are optimization.
>>
>> I see them as bug fixes, not optimizations.
>>
>>>  Firstly I
>>> think we should upstream the USB charger driver.
>>
>> I think you missed the point.  The point is that we don't *need* your
>> "USB charger driver" because all the infrastructure we need is *already*
>> present in the kernel.  It is buggy and not used uniformly, and could
>> usefully be polished and improved.  But the structure is already
>> present.
>>
>> If everyone just added new infrastructure when they didn't like, or
>> didn't understand, what was already present, the kernel would become
>> like the Mad Hatter's tea party, full of dirty dishes.
>>
>>>  What I want to ask is
>>> how can we notify power driver if we don't set the
>>> usb_register_notifier(), then I think you give the answer is: power
>>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>>> should notify the power driver how much current should be drawn, and I
>>> still think we should notify the current in usb charger driver which
>>> is better, and do not need to notify current for power driver in usb
>>> phy driver.
>>
>> I accept that it isn't clear that the phy *should* be involved in
>> communicating the negotiated power availability, but nor is it clear
>> that it shouldn't.  The power does travel through the physical
>> interface, so physically it plays a role.
>>
>> But more importantly, it already *does* get told (in some cases).
>> There is an interface "usb_phy_set_power()" which exists explicitly to
>> tell the phy what power has been negotiated.  Given that infrastructure
>> exists and works, it make sense to use it.
>>
>> If you think it is a broken design and should be removed, then fine:
>> make a case for that.  Examine the history.  Make sure you know why it
>> is there (or make sure that information cannot be found), and then
>> present a case as to why it should be removed and replaced with
>> something else.  But don't just leave it there and pretend it doesn't
>> exist and create something similar-but-different and hope people will
>> know why yours is better.  That way lies madness.
>
> Like Peter said, it is not only PHY can detect the USB charger type,
> which means there are other places can detect the charger type.

If I understand Peter's example correctly, it shows a configuration
where the USB PHysical interface is partly implemented in the SOC and
partly in the PMIC.  I appreciate that would make it more challenging to
implement a PHY driver, but there is no reason it should impact anything
outside of the PHY.


> Second, some controller need to detect the charger type manually which
> USB phy did not support.

"manually" is an odd term to use in this context.
I think you mean that to detect the charger type you need to issue some
command to the hardware and wait for it to respond, then assess the
response.
That isn't at all surprising.  The charger type is detected by measuring
resistance between ID and GND, which may require setting up a potential
and activating ADCs to measure the voltage.  This can all be done
internally to the phy driver.
Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
twl4030, though it never got upstream).
The code for the imx7d does look more complex, but not intrinsically
different.

> Third, it did not handle what current should
> be drawn in USB phy.

The standards define that.  The extcon just reports the cable type.
Certainly it would be sensible to provide a library function to
translate from cable type to current range.  You don't need a new
subsystem to do that, just a library function.

> Fourth, we need integrate all charger plugin/out
> event in one framework, not from extcon, maybe type-c in future.

Why not extcon?  Given that a charger is connected by an external
connector, extcon seems like exactly the right thing to use.

Obviously extcon doesn't report the current that was negotiated, but
that is best kept separate.  The battery charger can be advised of the
available current either via extcon or separately via the usb
subsystem.  Don't conflate the two.


>  In a
> word, we need one standard integration of this feature we need, though
> like you said we should do some clean up or fix to make it better.

But really, I'm not the person you need to convince.  I'm just a vaguely
interested bystander who is rapidly losing interest.  You need to
convince a maintainer, but they have so far shown remarkably little
interest.  I don't know why, but I'd guess that reviewing a complex new
subsystem isn't much fun.  Reviewing 

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

[NeilBrown]

On Thu, Nov 10 2016, Baolin Wang wrote:

> Hi
>
> On 8 November 2016 at 04:36, NeilBrown  wrote:
>> On Mon, Nov 07 2016, Baolin Wang wrote:
>>
>>> On 3 November 2016 at 09:25, NeilBrown  wrote:
 On Tue, Nov 01 2016, Baolin Wang wrote:
>>>
>>> I agree with your most opinions, but these are optimization.
>>
>> I see them as bug fixes, not optimizations.
>>
>>>  Firstly I
>>> think we should upstream the USB charger driver.
>>
>> I think you missed the point.  The point is that we don't *need* your
>> "USB charger driver" because all the infrastructure we need is *already*
>> present in the kernel.  It is buggy and not used uniformly, and could
>> usefully be polished and improved.  But the structure is already
>> present.
>>
>> If everyone just added new infrastructure when they didn't like, or
>> didn't understand, what was already present, the kernel would become
>> like the Mad Hatter's tea party, full of dirty dishes.
>>
>>>  What I want to ask is
>>> how can we notify power driver if we don't set the
>>> usb_register_notifier(), then I think you give the answer is: power
>>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>>> should notify the power driver how much current should be drawn, and I
>>> still think we should notify the current in usb charger driver which
>>> is better, and do not need to notify current for power driver in usb
>>> phy driver.
>>
>> I accept that it isn't clear that the phy *should* be involved in
>> communicating the negotiated power availability, but nor is it clear
>> that it shouldn't.  The power does travel through the physical
>> interface, so physically it plays a role.
>>
>> But more importantly, it already *does* get told (in some cases).
>> There is an interface "usb_phy_set_power()" which exists explicitly to
>> tell the phy what power has been negotiated.  Given that infrastructure
>> exists and works, it make sense to use it.
>>
>> If you think it is a broken design and should be removed, then fine:
>> make a case for that.  Examine the history.  Make sure you know why it
>> is there (or make sure that information cannot be found), and then
>> present a case as to why it should be removed and replaced with
>> something else.  But don't just leave it there and pretend it doesn't
>> exist and create something similar-but-different and hope people will
>> know why yours is better.  That way lies madness.
>
> Like Peter said, it is not only PHY can detect the USB charger type,
> which means there are other places can detect the charger type.

If I understand Peter's example correctly, it shows a configuration
where the USB PHysical interface is partly implemented in the SOC and
partly in the PMIC.  I appreciate that would make it more challenging to
implement a PHY driver, but there is no reason it should impact anything
outside of the PHY.


> Second, some controller need to detect the charger type manually which
> USB phy did not support.

"manually" is an odd term to use in this context.
I think you mean that to detect the charger type you need to issue some
command to the hardware and wait for it to respond, then assess the
response.
That isn't at all surprising.  The charger type is detected by measuring
resistance between ID and GND, which may require setting up a potential
and activating ADCs to measure the voltage.  This can all be done
internally to the phy driver.
Sometimes it is easy (I did https://lkml.org/lkml/2015/2/23/746 for
twl4030, though it never got upstream).
The code for the imx7d does look more complex, but not intrinsically
different.

> Third, it did not handle what current should
> be drawn in USB phy.

The standards define that.  The extcon just reports the cable type.
Certainly it would be sensible to provide a library function to
translate from cable type to current range.  You don't need a new
subsystem to do that, just a library function.

> Fourth, we need integrate all charger plugin/out
> event in one framework, not from extcon, maybe type-c in future.

Why not extcon?  Given that a charger is connected by an external
connector, extcon seems like exactly the right thing to use.

Obviously extcon doesn't report the current that was negotiated, but
that is best kept separate.  The battery charger can be advised of the
available current either via extcon or separately via the usb
subsystem.  Don't conflate the two.


>  In a
> word, we need one standard integration of this feature we need, though
> like you said we should do some clean up or fix to make it better.

But really, I'm not the person you need to convince.  I'm just a vaguely
interested bystander who is rapidly losing interest.  You need to
convince a maintainer, but they have so far shown remarkably little
interest.  I don't know why, but I'd guess that reviewing a complex new
subsystem isn't much fun.  Reviewing and applying clear bugfixes and

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

[Baolin Wang]

Hi

On 8 November 2016 at 04:36, NeilBrown  wrote:
> On Mon, Nov 07 2016, Baolin Wang wrote:
>
>> On 3 November 2016 at 09:25, NeilBrown  wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>> I agree with your most opinions, but these are optimization.
>
> I see them as bug fixes, not optimizations.
>
>>  Firstly I
>> think we should upstream the USB charger driver.
>
> I think you missed the point.  The point is that we don't *need* your
> "USB charger driver" because all the infrastructure we need is *already*
> present in the kernel.  It is buggy and not used uniformly, and could
> usefully be polished and improved.  But the structure is already
> present.
>
> If everyone just added new infrastructure when they didn't like, or
> didn't understand, what was already present, the kernel would become
> like the Mad Hatter's tea party, full of dirty dishes.
>
>>  What I want to ask is
>> how can we notify power driver if we don't set the
>> usb_register_notifier(), then I think you give the answer is: power
>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>> should notify the power driver how much current should be drawn, and I
>> still think we should notify the current in usb charger driver which
>> is better, and do not need to notify current for power driver in usb
>> phy driver.
>
> I accept that it isn't clear that the phy *should* be involved in
> communicating the negotiated power availability, but nor is it clear
> that it shouldn't.  The power does travel through the physical
> interface, so physically it plays a role.
>
> But more importantly, it already *does* get told (in some cases).
> There is an interface "usb_phy_set_power()" which exists explicitly to
> tell the phy what power has been negotiated.  Given that infrastructure
> exists and works, it make sense to use it.
>
> If you think it is a broken design and should be removed, then fine:
> make a case for that.  Examine the history.  Make sure you know why it
> is there (or make sure that information cannot be found), and then
> present a case as to why it should be removed and replaced with
> something else.  But don't just leave it there and pretend it doesn't
> exist and create something similar-but-different and hope people will
> know why yours is better.  That way lies madness.

Like Peter said, it is not only PHY can detect the USB charger type,
which means there are other places can detect the charger type.
Second, some controller need to detect the charger type manually which
USB phy did not support. Third, it did not handle what current should
be drawn in USB phy. Fourth, we need integrate all charger plugin/out
event in one framework, not from extcon, maybe type-c in future. In a
word, we need one standard integration of this feature we need, though
like you said we should do some clean up or fix to make it better.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi

On 8 November 2016 at 04:36, NeilBrown  wrote:
> On Mon, Nov 07 2016, Baolin Wang wrote:
>
>> On 3 November 2016 at 09:25, NeilBrown  wrote:
>>> On Tue, Nov 01 2016, Baolin Wang wrote:
>>
>> I agree with your most opinions, but these are optimization.
>
> I see them as bug fixes, not optimizations.
>
>>  Firstly I
>> think we should upstream the USB charger driver.
>
> I think you missed the point.  The point is that we don't *need* your
> "USB charger driver" because all the infrastructure we need is *already*
> present in the kernel.  It is buggy and not used uniformly, and could
> usefully be polished and improved.  But the structure is already
> present.
>
> If everyone just added new infrastructure when they didn't like, or
> didn't understand, what was already present, the kernel would become
> like the Mad Hatter's tea party, full of dirty dishes.
>
>>  What I want to ask is
>> how can we notify power driver if we don't set the
>> usb_register_notifier(), then I think you give the answer is: power
>> driver can register by 'struct usb_phy->notifier'. But why usb phy
>> should notify the power driver how much current should be drawn, and I
>> still think we should notify the current in usb charger driver which
>> is better, and do not need to notify current for power driver in usb
>> phy driver.
>
> I accept that it isn't clear that the phy *should* be involved in
> communicating the negotiated power availability, but nor is it clear
> that it shouldn't.  The power does travel through the physical
> interface, so physically it plays a role.
>
> But more importantly, it already *does* get told (in some cases).
> There is an interface "usb_phy_set_power()" which exists explicitly to
> tell the phy what power has been negotiated.  Given that infrastructure
> exists and works, it make sense to use it.
>
> If you think it is a broken design and should be removed, then fine:
> make a case for that.  Examine the history.  Make sure you know why it
> is there (or make sure that information cannot be found), and then
> present a case as to why it should be removed and replaced with
> something else.  But don't just leave it there and pretend it doesn't
> exist and create something similar-but-different and hope people will
> know why yours is better.  That way lies madness.

Like Peter said, it is not only PHY can detect the USB charger type,
which means there are other places can detect the charger type.
Second, some controller need to detect the charger type manually which
USB phy did not support. Third, it did not handle what current should
be drawn in USB phy. Fourth, we need integrate all charger plugin/out
event in one framework, not from extcon, maybe type-c in future. In a
word, we need one standard integration of this feature we need, though
like you said we should do some clean up or fix to make it better.

-- 
Baolin.wang
Best Regards

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

[Peter Chen]

On Wed, Nov 09, 2016 at 07:38:36AM +1100, NeilBrown wrote:
> On Tue, Nov 08 2016, Peter Chen wrote:
> 
> > On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
> >> 
> >> 
> >> 
> >> 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.
> >
> > Not only USB PHY to register an extcon, but also for the drivers which
> > can detect USB charger type, it may be USB controller driver, USB type-c
> > driver, pmic driver, and these drivers may not have an extcon device
> > since the internal part can finish the vbus detect.
> 
> Can you point to an example of the sort of hardware/driver you are
> referring to, preferably in the mainline kernel.  Concrete examples make
> this sort of thing much easier to understand.
> 

Eg, the nxp/fsl USB charger detector part, the register used to begin
the detection is in USB controller register region, and we also need to
use controller register to pull up dp to begin the secondary detect.

Since there is no USB charger framework, the code is not in mainline.
It finished an upstream version based on patch set in this thread [1]

Some SoCs uses chipidea IP doesn't need to use extcon to detect plugging
in/out, since the vbus pin connects to SoC's vbus pin.

The pmic has abilities to charger detection, eg, drivers/mfd/axp20x.c
and drivers/extcon/extcon-axp288.c.

> I think I would argue that if some piece of hardware can detect the USB
> charger type, then that piece of hardware is part of the "USB PHY", even
> if the hardware manufacturer has chosen to partition the register set in
> a way which doesn't make that obvious.

I agree with that the charger detect hardware is in the USB PHY part,
but some USB PHY's control is integrated in USB controller (eg, some
controller uses drivers/usb/phy/phy-generic.c as its PHY driver), so
we have to use controller register to finish some PHY function, eg
charger detection.

[1] https://www.spinics.net/lists/linux-usb/msg139425.html

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

On Wed, Nov 09, 2016 at 07:38:36AM +1100, NeilBrown wrote:
> On Tue, Nov 08 2016, Peter Chen wrote:
> 
> > On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
> >> 
> >> 
> >> 
> >> 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.
> >
> > Not only USB PHY to register an extcon, but also for the drivers which
> > can detect USB charger type, it may be USB controller driver, USB type-c
> > driver, pmic driver, and these drivers may not have an extcon device
> > since the internal part can finish the vbus detect.
> 
> Can you point to an example of the sort of hardware/driver you are
> referring to, preferably in the mainline kernel.  Concrete examples make
> this sort of thing much easier to understand.
> 

Eg, the nxp/fsl USB charger detector part, the register used to begin
the detection is in USB controller register region, and we also need to
use controller register to pull up dp to begin the secondary detect.

Since there is no USB charger framework, the code is not in mainline.
It finished an upstream version based on patch set in this thread [1]

Some SoCs uses chipidea IP doesn't need to use extcon to detect plugging
in/out, since the vbus pin connects to SoC's vbus pin.

The pmic has abilities to charger detection, eg, drivers/mfd/axp20x.c
and drivers/extcon/extcon-axp288.c.

> I think I would argue that if some piece of hardware can detect the USB
> charger type, then that piece of hardware is part of the "USB PHY", even
> if the hardware manufacturer has chosen to partition the register set in
> a way which doesn't make that obvious.

I agree with that the charger detect hardware is in the USB PHY part,
but some USB PHY's control is integrated in USB controller (eg, some
controller uses drivers/usb/phy/phy-generic.c as its PHY driver), so
we have to use controller register to finish some PHY function, eg
charger detection.

[1] https://www.spinics.net/lists/linux-usb/msg139425.html

-- 

Best Regards,
Peter Chen

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

[NeilBrown]

On Tue, Nov 08 2016, Peter Chen wrote:

> On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
>> 
>> 
>> 
>> 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.
>
> Not only USB PHY to register an extcon, but also for the drivers which
> can detect USB charger type, it may be USB controller driver, USB type-c
> driver, pmic driver, and these drivers may not have an extcon device
> since the internal part can finish the vbus detect.

Can you point to an example of the sort of hardware/driver you are
referring to, preferably in the mainline kernel.  Concrete examples make
this sort of thing much easier to understand.

I think I would argue that if some piece of hardware can detect the USB
charger type, then that piece of hardware is part of the "USB PHY", even
if the hardware manufacturer has chosen to partition the register set in
a way which doesn't make that obvious.

NeilBrown


signature.asc
Description: PGP signature

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

[NeilBrown]

On Tue, Nov 08 2016, Peter Chen wrote:

> On Thu, Nov 03, 2016 at 12:25:42PM +1100, NeilBrown wrote:
>> 
>> 
>> 
>> 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.
>
> Not only USB PHY to register an extcon, but also for the drivers which
> can detect USB charger type, it may be USB controller driver, USB type-c
> driver, pmic driver, and these drivers may not have an extcon device
> since the internal part can finish the vbus detect.

Can you point to an example of the sort of hardware/driver you are
referring to, preferably in the mainline kernel.  Concrete examples make
this sort of thing much easier to understand.

I think I would argue that if some piece of hardware can detect the USB
charger type, then that piece of hardware is part of the "USB PHY", even
if the hardware manufacturer has chosen to partition the register set in
a way which doesn't make that obvious.

NeilBrown


signature.asc
Description: PGP signature

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

[Peter Chen]

On Thu, Nov 03, 2016 at 12:25:42PM +1100, 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
> 
> 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.

Not only USB PHY to register an extcon, but also for the drivers which
can detect USB charger type, it may be USB controller driver, USB type-c
driver, pmic driver, and these drivers may not have an extcon device
since the internal part can finish the vbus detect.

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

On Thu, Nov 03, 2016 at 12:25:42PM +1100, 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
> 
> 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.

Not only USB PHY to register an extcon, but also for the drivers which
can detect USB charger type, it may be USB controller driver, USB type-c
driver, pmic driver, and these drivers may not have an extcon device
since the internal part can finish the vbus detect.

-- 

Best Regards,
Peter Chen

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

[NeilBrown]

On Mon, Nov 07 2016, Baolin Wang wrote:

> On 3 November 2016 at 09:25, NeilBrown  wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>
> I agree with your most opinions, but these are optimization. 

I see them as bug fixes, not optimizations.

>  Firstly I
> think we should upstream the USB charger driver.

I think you missed the point.  The point is that we don't *need* your
"USB charger driver" because all the infrastructure we need is *already*
present in the kernel.  It is buggy and not used uniformly, and could
usefully be polished and improved.  But the structure is already
present.

If everyone just added new infrastructure when they didn't like, or
didn't understand, what was already present, the kernel would become
like the Mad Hatter's tea party, full of dirty dishes.

>  What I want to ask is
> how can we notify power driver if we don't set the
> usb_register_notifier(), then I think you give the answer is: power
> driver can register by 'struct usb_phy->notifier'. But why usb phy
> should notify the power driver how much current should be drawn, and I
> still think we should notify the current in usb charger driver which
> is better, and do not need to notify current for power driver in usb
> phy driver.

I accept that it isn't clear that the phy *should* be involved in
communicating the negotiated power availability, but nor is it clear
that it shouldn't.  The power does travel through the physical
interface, so physically it plays a role.

But more importantly, it already *does* get told (in some cases).
There is an interface "usb_phy_set_power()" which exists explicitly to
tell the phy what power has been negotiated.  Given that infrastructure
exists and works, it make sense to use it.

If you think it is a broken design and should be removed, then fine:
make a case for that.  Examine the history.  Make sure you know why it
is there (or make sure that information cannot be found), and then
present a case as to why it should be removed and replaced with
something else.  But don't just leave it there and pretend it doesn't
exist and create something similar-but-different and hope people will
know why yours is better.  That way lies madness.

NeilBrown


signature.asc
Description: PGP signature

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

[NeilBrown]

On Mon, Nov 07 2016, Baolin Wang wrote:

> On 3 November 2016 at 09:25, NeilBrown  wrote:
>> On Tue, Nov 01 2016, Baolin Wang wrote:
>
> I agree with your most opinions, but these are optimization. 

I see them as bug fixes, not optimizations.

>  Firstly I
> think we should upstream the USB charger driver.

I think you missed the point.  The point is that we don't *need* your
"USB charger driver" because all the infrastructure we need is *already*
present in the kernel.  It is buggy and not used uniformly, and could
usefully be polished and improved.  But the structure is already
present.

If everyone just added new infrastructure when they didn't like, or
didn't understand, what was already present, the kernel would become
like the Mad Hatter's tea party, full of dirty dishes.

>  What I want to ask is
> how can we notify power driver if we don't set the
> usb_register_notifier(), then I think you give the answer is: power
> driver can register by 'struct usb_phy->notifier'. But why usb phy
> should notify the power driver how much current should be drawn, and I
> still think we should notify the current in usb charger driver which
> is better, and do not need to notify current for power driver in usb
> phy driver.

I accept that it isn't clear that the phy *should* be involved in
communicating the negotiated power availability, but nor is it clear
that it shouldn't.  The power does travel through the physical
interface, so physically it plays a role.

But more importantly, it already *does* get told (in some cases).
There is an interface "usb_phy_set_power()" which exists explicitly to
tell the phy what power has been negotiated.  Given that infrastructure
exists and works, it make sense to use it.

If you think it is a broken design and should be removed, then fine:
make a case for that.  Examine the history.  Make sure you know why it
is there (or make sure that information cannot be found), and then
present a case as to why it should be removed and replaced with
something else.  But don't just leave it there and pretend it doesn't
exist and create something similar-but-different and hope people will
know why yours is better.  That way lies madness.

NeilBrown


signature.asc
Description: PGP signature

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

[Baolin Wang]

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

I agree with your most opinions, but these are optimization. Firstly I
think we should upstream the USB charger driver. What I want to ask is
how can we notify power driver if we don't set the
usb_register_notifier(), then I think you give the answer is: power
driver can register by 'struct usb_phy->notifier'. But why usb phy
should notify the power driver how much current should be drawn, and I
still think we should notify the current in usb 

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

[Baolin Wang]

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

I agree with your most opinions, but these are optimization. Firstly I
think we should upstream the USB charger driver. What I want to ask is
how can we notify power driver if we don't set the
usb_register_notifier(), then I think you give the answer is: power
driver can register by 'struct usb_phy->notifier'. But why usb phy
should notify the power driver how much current should be drawn, and I
still think we should notify the current in usb charger driver 

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

[NeilBrown]

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

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


signature.asc
Description: PGP signature

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

[NeilBrown]

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

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


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi,

On 31 October 2016 at 08:00, NeilBrown  wrote:
> On Fri, Oct 28 2016, Baolin Wang wrote:
>
>>>
>>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>>   When the extcon detects an SDP, it will be called to set the state
>>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>>   it happened to be before, which is probably wrong.
>>
>> Sorry, I did not get your points here, could you please explain it 
>> explicitly?
>
> usb_charger_get_current() is used to get the min/max current that is
> supported.
> In the case that an SDP (non-super-speed) has been detected it will
> report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
> sdp_max is set
>  - to DEFAULT_SDP_CUR_MAX (500) at initializaion
>  - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
>  which happens after USB negotiation, once an allowed vbus_draw is
>  negotiated.
>
> This means that the first time you plug in an SDP cable, the reported
> max will be 500, even though nothing has been negotiated.   The maximum
> before negotiation is much less than that  - I don't remember exactly
> how much.
>
> If negotiation completes, the sdp_max will be set to whatever was
> negotiated.  Maybe 200mA.
> If you unplug, and then plug another SDP cable in, the sdp_max will
> still be 200mA - different from the first time, but still not correct.
> It will remain incorrect until (and unless) USB negotiation completes.

Yes. I need some modification to reset current to default values when
cable unplugged.

>
>>
>>>   When after USB negotiation completes,
>>>   usb_charger_set_cur_limit_by_gadget()
>>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>>   again, but with a new current.  This will be ignored, as the state is
>>>   already USB_CHARGER_PRESENT.
>>
>> No, we will notify the user the current has been changed by one work.
>
> I can see no evidence in the code to justify this assertion, and you
> didn't even try to provide any.

We have one work to notify the user the current has been changed,
please see usb_charger_notify_work().

>
>>
>>>
>>> 4/ I still strongly object to the ->get_charger_type() interface.
>>>  You previously said:
>>>
>>>  No. User can implement the get_charger_type() method to access the
>>>  PMIC registers to get the charger type, which is one very common 
>>> method.
>>>
>>>  I suggest that if the PMIC registers report the charger type, then the
>>>  PMIC driver should register an EXTCON and report the charger type
>>>  through that.  Then the information would be directly available to
>>>  user-space, and the usb-charger framework would have a single uniform
>>>  mechanism for being told the cable type.
>>
>> We just access only one PMIC register to get the charger type, which
>> is no need add one driver for that and there are no any events for
>> extcon. Some sample code in power driver can be like below:
>
> If there are no events, then how do you know when a charger has been
> plugged in?  Do you poll?

We just monitor the plug-in and plug-out events by extcon,  and get
the charger type by accessing PMIC registers when one cable is
plugged.

>
> In any case, one of the major values provided by using an OS like Linux
> is uniform interfaces.  If a device can detect what sort of cable is
> inserted, then that information should be presented as an EXTCON.

Fine. I can remove this callback. We can add it if we need it in future.

>
>>>
>>>  Related:  I don't like charger_type_show().  I don't think
>>>  the usb-charger should export that information to user-space because
>>>  extcon already does that, and duplication is confusing and pointless.
>>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> Yes and no.
>
> Certainly a uniform consistent interface should be presented.
> "a usb charger" is not the right abstraction.  It is not a thing that
> should have independent existence.  To everybody else in the world, a
> "usb charger" in a box that you plug into the wall, and which has a
> cable to plug into your device.  It is not part of the device itself.
> In general, you cannot point to any component in a device that is the
> "usb charger" so it isn't clear that Linux should even know about a "usb
> charger".

Yes,  we agree that 'usb charger' is not one actual device, and 'usb
charger' is depended on gadget device. Moreover these charger
information is associated with actual gadget device, not virtual usb
charger device.

>
> There is a battery-charger which can take whatever current is available
> and feed it to the battery.  It may well be appropriate for user-space
> to have fine control of the current that this uses quite independently
> of whatever is plugged in (I have a board which can get the current via
> USB or via a more 

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

[Baolin Wang]

Hi,

On 31 October 2016 at 08:00, NeilBrown  wrote:
> On Fri, Oct 28 2016, Baolin Wang wrote:
>
>>>
>>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>>   When the extcon detects an SDP, it will be called to set the state
>>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>>   it happened to be before, which is probably wrong.
>>
>> Sorry, I did not get your points here, could you please explain it 
>> explicitly?
>
> usb_charger_get_current() is used to get the min/max current that is
> supported.
> In the case that an SDP (non-super-speed) has been detected it will
> report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
> sdp_max is set
>  - to DEFAULT_SDP_CUR_MAX (500) at initializaion
>  - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
>  which happens after USB negotiation, once an allowed vbus_draw is
>  negotiated.
>
> This means that the first time you plug in an SDP cable, the reported
> max will be 500, even though nothing has been negotiated.   The maximum
> before negotiation is much less than that  - I don't remember exactly
> how much.
>
> If negotiation completes, the sdp_max will be set to whatever was
> negotiated.  Maybe 200mA.
> If you unplug, and then plug another SDP cable in, the sdp_max will
> still be 200mA - different from the first time, but still not correct.
> It will remain incorrect until (and unless) USB negotiation completes.

Yes. I need some modification to reset current to default values when
cable unplugged.

>
>>
>>>   When after USB negotiation completes,
>>>   usb_charger_set_cur_limit_by_gadget()
>>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>>   again, but with a new current.  This will be ignored, as the state is
>>>   already USB_CHARGER_PRESENT.
>>
>> No, we will notify the user the current has been changed by one work.
>
> I can see no evidence in the code to justify this assertion, and you
> didn't even try to provide any.

We have one work to notify the user the current has been changed,
please see usb_charger_notify_work().

>
>>
>>>
>>> 4/ I still strongly object to the ->get_charger_type() interface.
>>>  You previously said:
>>>
>>>  No. User can implement the get_charger_type() method to access the
>>>  PMIC registers to get the charger type, which is one very common 
>>> method.
>>>
>>>  I suggest that if the PMIC registers report the charger type, then the
>>>  PMIC driver should register an EXTCON and report the charger type
>>>  through that.  Then the information would be directly available to
>>>  user-space, and the usb-charger framework would have a single uniform
>>>  mechanism for being told the cable type.
>>
>> We just access only one PMIC register to get the charger type, which
>> is no need add one driver for that and there are no any events for
>> extcon. Some sample code in power driver can be like below:
>
> If there are no events, then how do you know when a charger has been
> plugged in?  Do you poll?

We just monitor the plug-in and plug-out events by extcon,  and get
the charger type by accessing PMIC registers when one cable is
plugged.

>
> In any case, one of the major values provided by using an OS like Linux
> is uniform interfaces.  If a device can detect what sort of cable is
> inserted, then that information should be presented as an EXTCON.

Fine. I can remove this callback. We can add it if we need it in future.

>
>>>
>>>  Related:  I don't like charger_type_show().  I don't think
>>>  the usb-charger should export that information to user-space because
>>>  extcon already does that, and duplication is confusing and pointless.
>>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> Yes and no.
>
> Certainly a uniform consistent interface should be presented.
> "a usb charger" is not the right abstraction.  It is not a thing that
> should have independent existence.  To everybody else in the world, a
> "usb charger" in a box that you plug into the wall, and which has a
> cable to plug into your device.  It is not part of the device itself.
> In general, you cannot point to any component in a device that is the
> "usb charger" so it isn't clear that Linux should even know about a "usb
> charger".

Yes,  we agree that 'usb charger' is not one actual device, and 'usb
charger' is depended on gadget device. Moreover these charger
information is associated with actual gadget device, not virtual usb
charger device.

>
> There is a battery-charger which can take whatever current is available
> and feed it to the battery.  It may well be appropriate for user-space
> to have fine control of the current that this uses quite independently
> of whatever is plugged in (I have a board which can get the current via
> USB or via a more direct connection).
>

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

[Baolin Wang]

On 29 October 2016 at 01:03, Mark Brown  wrote:
> On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
>> On 28 October 2016 at 06:00, NeilBrown  wrote:
>
>> > 1/ I think we agreed that it doesn't make sense for there to be
>> >  two chargers registered in a system.
>
>> Yes, until now...
>
>> >  However usb_charger_register() still allows that, and assigns
>> >  and arbitrary name to each based on discovery order.
>> >  This *cannot* make sense.
>
>> Fine, I can change that to allow only one charger to register.
>
> Yeah, it's a reasonable change.  I'm not sure the prior discussion was
> 100% conclusive on the issue (I remember there being some debate about
> leaving things there to avoid any need for future refactoring to touch
> the interface).

I think we should leave these things to avoid refactoring in future.

>
>> > 2/ Why do you have usb_charger_set_current()??
>> >   No code ever calls it.
>> >   This updates the min and max current which are defined in a
>> >   standard.  It never makes sense to change the min and max
>> >   for a particular cable type.
>
>> Mark, do we have some scenarios which want to change the current
>> limitation? If not, okay, I agree with you to remove this function.
>
> I'm not aware of any, we can always add it back if the need arises.

OK.

>
>> >  Related:  I don't like charger_type_show().  I don't think
>> >  the usb-charger should export that information to user-space because
>> >  extcon already does that, and duplication is confusing and pointless.
>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> I had also thought there was some software negotation as well as the
> physical charger in cases where the device is plugged into an active
> host?  I could be wrong.
>
>> > 5/ There is no convincing example usage of this framework.
>> >   wm8931x_power.c just scratches the surface.
>> >   If it is so good, it should be easy to convert a lot of other
>> >   drivers over to it.  If you did that it would be much easier
>> >   to see how it works and what the strengths/weaknesses were.
>
>> Jun have send out one patchset[1] based on my patchset, and he tested
>> mypatchset. Thanks for your comments.
>> [1]http://www.spinics.net/lists/linux-usb/msg139809.html
>
> I think it's a good idea to pick up Jun's patches into your patch set,
> that way Jun doesn't need to rebase and it might help with review of
> your patches too.

Yes, I think so. I will ask for Jun's help.


-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

On 29 October 2016 at 01:03, Mark Brown  wrote:
> On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
>> On 28 October 2016 at 06:00, NeilBrown  wrote:
>
>> > 1/ I think we agreed that it doesn't make sense for there to be
>> >  two chargers registered in a system.
>
>> Yes, until now...
>
>> >  However usb_charger_register() still allows that, and assigns
>> >  and arbitrary name to each based on discovery order.
>> >  This *cannot* make sense.
>
>> Fine, I can change that to allow only one charger to register.
>
> Yeah, it's a reasonable change.  I'm not sure the prior discussion was
> 100% conclusive on the issue (I remember there being some debate about
> leaving things there to avoid any need for future refactoring to touch
> the interface).

I think we should leave these things to avoid refactoring in future.

>
>> > 2/ Why do you have usb_charger_set_current()??
>> >   No code ever calls it.
>> >   This updates the min and max current which are defined in a
>> >   standard.  It never makes sense to change the min and max
>> >   for a particular cable type.
>
>> Mark, do we have some scenarios which want to change the current
>> limitation? If not, okay, I agree with you to remove this function.
>
> I'm not aware of any, we can always add it back if the need arises.

OK.

>
>> >  Related:  I don't like charger_type_show().  I don't think
>> >  the usb-charger should export that information to user-space because
>> >  extcon already does that, and duplication is confusing and pointless.
>
>> I think we should combine all charger related information into one
>> place for user. Moreover if we don't get charger type from extcon, we
>> should also need one place to export the charger type.
>
> I had also thought there was some software negotation as well as the
> physical charger in cases where the device is plugged into an active
> host?  I could be wrong.
>
>> > 5/ There is no convincing example usage of this framework.
>> >   wm8931x_power.c just scratches the surface.
>> >   If it is so good, it should be easy to convert a lot of other
>> >   drivers over to it.  If you did that it would be much easier
>> >   to see how it works and what the strengths/weaknesses were.
>
>> Jun have send out one patchset[1] based on my patchset, and he tested
>> mypatchset. Thanks for your comments.
>> [1]http://www.spinics.net/lists/linux-usb/msg139809.html
>
> I think it's a good idea to pick up Jun's patches into your patch set,
> that way Jun doesn't need to rebase and it might help with review of
> your patches too.

Yes, I think so. I will ask for Jun's help.


-- 
Baolin.wang
Best Regards

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

[NeilBrown]

On Fri, Oct 28 2016, Baolin Wang wrote:

>>
>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>   When the extcon detects an SDP, it will be called to set the state
>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>   it happened to be before, which is probably wrong.
>
> Sorry, I did not get your points here, could you please explain it explicitly?

usb_charger_get_current() is used to get the min/max current that is
supported.
In the case that an SDP (non-super-speed) has been detected it will
report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
sdp_max is set
 - to DEFAULT_SDP_CUR_MAX (500) at initializaion
 - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
 which happens after USB negotiation, once an allowed vbus_draw is
 negotiated.

This means that the first time you plug in an SDP cable, the reported
max will be 500, even though nothing has been negotiated.   The maximum
before negotiation is much less than that  - I don't remember exactly
how much.

If negotiation completes, the sdp_max will be set to whatever was
negotiated.  Maybe 200mA.
If you unplug, and then plug another SDP cable in, the sdp_max will
still be 200mA - different from the first time, but still not correct.
It will remain incorrect until (and unless) USB negotiation completes.

>
>>   When after USB negotiation completes,
>>   usb_charger_set_cur_limit_by_gadget()
>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>   again, but with a new current.  This will be ignored, as the state is
>>   already USB_CHARGER_PRESENT.
>
> No, we will notify the user the current has been changed by one work.

I can see no evidence in the code to justify this assertion, and you
didn't even try to provide any.

>
>>
>> 4/ I still strongly object to the ->get_charger_type() interface.
>>  You previously said:
>>
>>  No. User can implement the get_charger_type() method to access the
>>  PMIC registers to get the charger type, which is one very common method.
>>
>>  I suggest that if the PMIC registers report the charger type, then the
>>  PMIC driver should register an EXTCON and report the charger type
>>  through that.  Then the information would be directly available to
>>  user-space, and the usb-charger framework would have a single uniform
>>  mechanism for being told the cable type.
>
> We just access only one PMIC register to get the charger type, which
> is no need add one driver for that and there are no any events for
> extcon. Some sample code in power driver can be like below:

If there are no events, then how do you know when a charger has been
plugged in?  Do you poll?

In any case, one of the major values provided by using an OS like Linux
is uniform interfaces.  If a device can detect what sort of cable is
inserted, then that information should be presented as an EXTCON.

>>
>>  Related:  I don't like charger_type_show().  I don't think
>>  the usb-charger should export that information to user-space because
>>  extcon already does that, and duplication is confusing and pointless.
>
> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

Yes and no.

Certainly a uniform consistent interface should be presented.
"a usb charger" is not the right abstraction.  It is not a thing that
should have independent existence.  To everybody else in the world, a
"usb charger" in a box that you plug into the wall, and which has a
cable to plug into your device.  It is not part of the device itself.
In general, you cannot point to any component in a device that is the
"usb charger" so it isn't clear that Linux should even know about a "usb
charger".

There is a battery-charger which can take whatever current is available
and feed it to the battery.  It may well be appropriate for user-space
to have fine control of the current that this uses quite independently
of whatever is plugged in (I have a board which can get the current via
USB or via a more direct connection).

There is also a USB PHY which can detect when a cable is plugged in
(possibly just because 5V appears on VBUS) and can usually detect some
details of the cable.  It should report, via the EXTCON interface, the
presence and type of the cable.

Maybe these are all in the one integrated circuit, maybe not. On the
board I have, the one IC includes the USB phy, the battery charger, the
audio codec, some regulators, some GPIOs and other stuff.  We have
separate drivers for each logical component, unified by an "mfd" driver.

From the interface design perspective, the number of ICs doesn't matter
at all.  The interface presented, both within the kernel and out to
user-space, should be consistent.  Every USB port should present with an
EXTCON, and if it can detect types of cables, those cable types should
be visible in 

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

[NeilBrown]

On Fri, Oct 28 2016, Baolin Wang wrote:

>>
>> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>>   When the extcon detects an SDP, it will be called to set the state
>>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>>   it happened to be before, which is probably wrong.
>
> Sorry, I did not get your points here, could you please explain it explicitly?

usb_charger_get_current() is used to get the min/max current that is
supported.
In the case that an SDP (non-super-speed) has been detected it will
report the values sdp_min and sdp_max.  Ignoring usb_charger_set_current(),
sdp_max is set
 - to DEFAULT_SDP_CUR_MAX (500) at initializaion
 - to cur_limit when usb_charger_set_cur_limit_by_gadget() is called
 which happens after USB negotiation, once an allowed vbus_draw is
 negotiated.

This means that the first time you plug in an SDP cable, the reported
max will be 500, even though nothing has been negotiated.   The maximum
before negotiation is much less than that  - I don't remember exactly
how much.

If negotiation completes, the sdp_max will be set to whatever was
negotiated.  Maybe 200mA.
If you unplug, and then plug another SDP cable in, the sdp_max will
still be 200mA - different from the first time, but still not correct.
It will remain incorrect until (and unless) USB negotiation completes.

>
>>   When after USB negotiation completes,
>>   usb_charger_set_cur_limit_by_gadget()
>>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>>   again, but with a new current.  This will be ignored, as the state is
>>   already USB_CHARGER_PRESENT.
>
> No, we will notify the user the current has been changed by one work.

I can see no evidence in the code to justify this assertion, and you
didn't even try to provide any.

>
>>
>> 4/ I still strongly object to the ->get_charger_type() interface.
>>  You previously said:
>>
>>  No. User can implement the get_charger_type() method to access the
>>  PMIC registers to get the charger type, which is one very common method.
>>
>>  I suggest that if the PMIC registers report the charger type, then the
>>  PMIC driver should register an EXTCON and report the charger type
>>  through that.  Then the information would be directly available to
>>  user-space, and the usb-charger framework would have a single uniform
>>  mechanism for being told the cable type.
>
> We just access only one PMIC register to get the charger type, which
> is no need add one driver for that and there are no any events for
> extcon. Some sample code in power driver can be like below:

If there are no events, then how do you know when a charger has been
plugged in?  Do you poll?

In any case, one of the major values provided by using an OS like Linux
is uniform interfaces.  If a device can detect what sort of cable is
inserted, then that information should be presented as an EXTCON.

>>
>>  Related:  I don't like charger_type_show().  I don't think
>>  the usb-charger should export that information to user-space because
>>  extcon already does that, and duplication is confusing and pointless.
>
> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

Yes and no.

Certainly a uniform consistent interface should be presented.
"a usb charger" is not the right abstraction.  It is not a thing that
should have independent existence.  To everybody else in the world, a
"usb charger" in a box that you plug into the wall, and which has a
cable to plug into your device.  It is not part of the device itself.
In general, you cannot point to any component in a device that is the
"usb charger" so it isn't clear that Linux should even know about a "usb
charger".

There is a battery-charger which can take whatever current is available
and feed it to the battery.  It may well be appropriate for user-space
to have fine control of the current that this uses quite independently
of whatever is plugged in (I have a board which can get the current via
USB or via a more direct connection).

There is also a USB PHY which can detect when a cable is plugged in
(possibly just because 5V appears on VBUS) and can usually detect some
details of the cable.  It should report, via the EXTCON interface, the
presence and type of the cable.

Maybe these are all in the one integrated circuit, maybe not. On the
board I have, the one IC includes the USB phy, the battery charger, the
audio codec, some regulators, some GPIOs and other stuff.  We have
separate drivers for each logical component, unified by an "mfd" driver.

From the interface design perspective, the number of ICs doesn't matter
at all.  The interface presented, both within the kernel and out to
user-space, should be consistent.  Every USB port should present with an
EXTCON, and if it can detect types of cables, those cable types should
be visible in 

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

[Mark Brown]

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown  wrote:

> > 1/ I think we agreed that it doesn't make sense for there to be
> >  two chargers registered in a system.

> Yes, until now...

> >  However usb_charger_register() still allows that, and assigns
> >  and arbitrary name to each based on discovery order.
> >  This *cannot* make sense.

> Fine, I can change that to allow only one charger to register.

Yeah, it's a reasonable change.  I'm not sure the prior discussion was
100% conclusive on the issue (I remember there being some debate about
leaving things there to avoid any need for future refactoring to touch
the interface).

> > 2/ Why do you have usb_charger_set_current()??
> >   No code ever calls it.
> >   This updates the min and max current which are defined in a
> >   standard.  It never makes sense to change the min and max
> >   for a particular cable type.

> Mark, do we have some scenarios which want to change the current
> limitation? If not, okay, I agree with you to remove this function.

I'm not aware of any, we can always add it back if the need arises.

> >  Related:  I don't like charger_type_show().  I don't think
> >  the usb-charger should export that information to user-space because
> >  extcon already does that, and duplication is confusing and pointless.

> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

I had also thought there was some software negotation as well as the
physical charger in cases where the device is plugged into an active
host?  I could be wrong.

> > 5/ There is no convincing example usage of this framework.
> >   wm8931x_power.c just scratches the surface.
> >   If it is so good, it should be easy to convert a lot of other
> >   drivers over to it.  If you did that it would be much easier
> >   to see how it works and what the strengths/weaknesses were.

> Jun have send out one patchset[1] based on my patchset, and he tested
> mypatchset. Thanks for your comments.
> [1]http://www.spinics.net/lists/linux-usb/msg139809.html

I think it's a good idea to pick up Jun's patches into your patch set,
that way Jun doesn't need to rebase and it might help with review of
your patches too.


signature.asc
Description: PGP signature

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

[Mark Brown]

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown  wrote:

> > 1/ I think we agreed that it doesn't make sense for there to be
> >  two chargers registered in a system.

> Yes, until now...

> >  However usb_charger_register() still allows that, and assigns
> >  and arbitrary name to each based on discovery order.
> >  This *cannot* make sense.

> Fine, I can change that to allow only one charger to register.

Yeah, it's a reasonable change.  I'm not sure the prior discussion was
100% conclusive on the issue (I remember there being some debate about
leaving things there to avoid any need for future refactoring to touch
the interface).

> > 2/ Why do you have usb_charger_set_current()??
> >   No code ever calls it.
> >   This updates the min and max current which are defined in a
> >   standard.  It never makes sense to change the min and max
> >   for a particular cable type.

> Mark, do we have some scenarios which want to change the current
> limitation? If not, okay, I agree with you to remove this function.

I'm not aware of any, we can always add it back if the need arises.

> >  Related:  I don't like charger_type_show().  I don't think
> >  the usb-charger should export that information to user-space because
> >  extcon already does that, and duplication is confusing and pointless.

> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

I had also thought there was some software negotation as well as the
physical charger in cases where the device is plugged into an active
host?  I could be wrong.

> > 5/ There is no convincing example usage of this framework.
> >   wm8931x_power.c just scratches the surface.
> >   If it is so good, it should be easy to convert a lot of other
> >   drivers over to it.  If you did that it would be much easier
> >   to see how it works and what the strengths/weaknesses were.

> Jun have send out one patchset[1] based on my patchset, and he tested
> mypatchset. Thanks for your comments.
> [1]http://www.spinics.net/lists/linux-usb/msg139809.html

I think it's a good idea to pick up Jun's patches into your patch set,
that way Jun doesn't need to rebase and it might help with review of
your patches too.


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi,

On 28 October 2016 at 06:00, NeilBrown  wrote:
> On Thu, Oct 27 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 19 October 2016 at 10:37, Baolin Wang  wrote:
>>> Currently the Linux kernel does not provide any standard integration of this
>>> feature that integrates the USB subsystem with the system power regulation
>>> provided by PMICs meaning that either vendors must add this in their kernels
>>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>>> as they should. Thus provide a standard framework for doing this in kernel.
>>>
>>> Now introduce one user with wm831x_power to support and test the usb 
>>> charger,
>>> which is pending testing. Moreover there may be other potential users will 
>>> use
>>> it in future.
>>>
>>> Changes since v17:
>>>  - Remove goto section in usb_charger_register() function.
>>>  - Remove 'extern' in charger.h file.
>>>  - Move the kfree() to usb_charger_exit() function.
>>>
>>> Changes since v16:
>>>  - Modify the charger current range with introducing the maximum and minimum
>>>  current.
>>>  - Remove the getting charger type method from power supply.
>>>  - Add the getting charger type method from extcon system.
>>>  - Introduce new usb_charger_get_current() API for users to get the maximum 
>>> and
>>>  minimum current.
>>>  - Rename some APIs and other optimization.
>>>
>>> Changes since v15:
>>>  - Add charger state checking to avoid sending out duplicate notifies to 
>>> users.
>>>  - Add one work to notify power users the current has been changed.
>>>
>>> Changes since v14:
>>>  - Add kernel documentation for struct usb_cahrger.
>>>  - Remove some redundant WARN() functions.
>>>
>>> Changes since v13:
>>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>>  - Rename some functions in charger.c file.
>>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
>>> tags/usb-for-v4.8
>>>
>>> Changes since v12:
>>>  - Remove the class and device things.
>>>  - Link usb charger to udc-core.ko.
>>>  - Create one "charger" subdirectory which holds all charger-related 
>>> attributes.
>>>
>>> Changes since v11:
>>>  - Reviewed and tested by Li Jun.
>>>
>>> Changes since v10:
>>>  - Introduce usb_charger_get_state() function to check charger state.
>>>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>>>  in case will be issued in atomic context.
>>
>> Could you apply this patchset into your branch if there are no other
>> comments? Thanks.
>
> Some of my previous comments are still outstanding.  You seem to have
> just brushed them off without apparently understanding.

I am very appreciate for your comments, and I've explained your
comments but you did not reply me..

> And no-one else seems to care enough to try to bridge the gap...
>
> Let me try again.
>
> 1/ I think we agreed that it doesn't make sense for there to be
>  two chargers registered in a system.

Yes, until now...

>  However usb_charger_register() still allows that, and assigns
>  and arbitrary name to each based on discovery order.
>  This *cannot* make sense.

Fine, I can change that to allow only one charger to register.

>
> 2/ Why do you have usb_charger_set_current()??
>   No code ever calls it.
>   This updates the min and max current which are defined in a
>   standard.  It never makes sense to change the min and max
>   for a particular cable type.

Mark, do we have some scenarios which want to change the current
limitation? If not, okay, I agree with you to remove this function.

>
> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>   When the extcon detects an SDP, it will be called to set the state
>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>   it happened to be before, which is probably wrong.

Sorry, I did not get your points here, could you please explain it explicitly?

>   When after USB negotiation completes,
>   usb_charger_set_cur_limit_by_gadget()
>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>   again, but with a new current.  This will be ignored, as the state is
>   already USB_CHARGER_PRESENT.

No, we will notify the user the current has been changed by one work.

>
>  (as an aside
>  +enum usb_charger_state {
>  +  USB_CHARGER_DEFAULT,
>  +  USB_CHARGER_PRESENT,
>  +  USB_CHARGER_REMOVE,
>  +};
>
>  looks odd.  It should probably by
> USB_CHARGER_UNKNOWN
> USB_CHARGER_PRESENT
> USB_CHARGER_ABSENT
>
>  "REMOVE" isn't a state.  "REMOVED" might be.
>  )

Sure.

>
> 4/ I still strongly object to the ->get_charger_type() interface.
>  You previously said:
>
>  No. User can implement the get_charger_type() method to access the
>  PMIC registers to get the charger type, which is one very common method.
>
>  I suggest that if the PMIC registers report the charger type, then the
>  PMIC driver should register an EXTCON and report the charger 

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

[Baolin Wang]

Hi,

On 28 October 2016 at 06:00, NeilBrown  wrote:
> On Thu, Oct 27 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 19 October 2016 at 10:37, Baolin Wang  wrote:
>>> Currently the Linux kernel does not provide any standard integration of this
>>> feature that integrates the USB subsystem with the system power regulation
>>> provided by PMICs meaning that either vendors must add this in their kernels
>>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>>> as they should. Thus provide a standard framework for doing this in kernel.
>>>
>>> Now introduce one user with wm831x_power to support and test the usb 
>>> charger,
>>> which is pending testing. Moreover there may be other potential users will 
>>> use
>>> it in future.
>>>
>>> Changes since v17:
>>>  - Remove goto section in usb_charger_register() function.
>>>  - Remove 'extern' in charger.h file.
>>>  - Move the kfree() to usb_charger_exit() function.
>>>
>>> Changes since v16:
>>>  - Modify the charger current range with introducing the maximum and minimum
>>>  current.
>>>  - Remove the getting charger type method from power supply.
>>>  - Add the getting charger type method from extcon system.
>>>  - Introduce new usb_charger_get_current() API for users to get the maximum 
>>> and
>>>  minimum current.
>>>  - Rename some APIs and other optimization.
>>>
>>> Changes since v15:
>>>  - Add charger state checking to avoid sending out duplicate notifies to 
>>> users.
>>>  - Add one work to notify power users the current has been changed.
>>>
>>> Changes since v14:
>>>  - Add kernel documentation for struct usb_cahrger.
>>>  - Remove some redundant WARN() functions.
>>>
>>> Changes since v13:
>>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>>  - Rename some functions in charger.c file.
>>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
>>> tags/usb-for-v4.8
>>>
>>> Changes since v12:
>>>  - Remove the class and device things.
>>>  - Link usb charger to udc-core.ko.
>>>  - Create one "charger" subdirectory which holds all charger-related 
>>> attributes.
>>>
>>> Changes since v11:
>>>  - Reviewed and tested by Li Jun.
>>>
>>> Changes since v10:
>>>  - Introduce usb_charger_get_state() function to check charger state.
>>>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>>>  in case will be issued in atomic context.
>>
>> Could you apply this patchset into your branch if there are no other
>> comments? Thanks.
>
> Some of my previous comments are still outstanding.  You seem to have
> just brushed them off without apparently understanding.

I am very appreciate for your comments, and I've explained your
comments but you did not reply me..

> And no-one else seems to care enough to try to bridge the gap...
>
> Let me try again.
>
> 1/ I think we agreed that it doesn't make sense for there to be
>  two chargers registered in a system.

Yes, until now...

>  However usb_charger_register() still allows that, and assigns
>  and arbitrary name to each based on discovery order.
>  This *cannot* make sense.

Fine, I can change that to allow only one charger to register.

>
> 2/ Why do you have usb_charger_set_current()??
>   No code ever calls it.
>   This updates the min and max current which are defined in a
>   standard.  It never makes sense to change the min and max
>   for a particular cable type.

Mark, do we have some scenarios which want to change the current
limitation? If not, okay, I agree with you to remove this function.

>
> 3/ usb_charger_notify_state() does nothing if the state doesn't change.
>   When the extcon detects an SDP, it will be called to set the state
>   to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
>   it happened to be before, which is probably wrong.

Sorry, I did not get your points here, could you please explain it explicitly?

>   When after USB negotiation completes,
>   usb_charger_set_cur_limit_by_gadget()
>   will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
>   again, but with a new current.  This will be ignored, as the state is
>   already USB_CHARGER_PRESENT.

No, we will notify the user the current has been changed by one work.

>
>  (as an aside
>  +enum usb_charger_state {
>  +  USB_CHARGER_DEFAULT,
>  +  USB_CHARGER_PRESENT,
>  +  USB_CHARGER_REMOVE,
>  +};
>
>  looks odd.  It should probably by
> USB_CHARGER_UNKNOWN
> USB_CHARGER_PRESENT
> USB_CHARGER_ABSENT
>
>  "REMOVE" isn't a state.  "REMOVED" might be.
>  )

Sure.

>
> 4/ I still strongly object to the ->get_charger_type() interface.
>  You previously said:
>
>  No. User can implement the get_charger_type() method to access the
>  PMIC registers to get the charger type, which is one very common method.
>
>  I suggest that if the PMIC registers report the charger type, then the
>  PMIC driver should register an EXTCON and report the charger type
>  through that.  Then the 

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

[NeilBrown]

On Thu, Oct 27 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 19 October 2016 at 10:37, Baolin Wang  wrote:
>> Currently the Linux kernel does not provide any standard integration of this
>> feature that integrates the USB subsystem with the system power regulation
>> provided by PMICs meaning that either vendors must add this in their kernels
>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>> as they should. Thus provide a standard framework for doing this in kernel.
>>
>> Now introduce one user with wm831x_power to support and test the usb charger,
>> which is pending testing. Moreover there may be other potential users will 
>> use
>> it in future.
>>
>> Changes since v17:
>>  - Remove goto section in usb_charger_register() function.
>>  - Remove 'extern' in charger.h file.
>>  - Move the kfree() to usb_charger_exit() function.
>>
>> Changes since v16:
>>  - Modify the charger current range with introducing the maximum and minimum
>>  current.
>>  - Remove the getting charger type method from power supply.
>>  - Add the getting charger type method from extcon system.
>>  - Introduce new usb_charger_get_current() API for users to get the maximum 
>> and
>>  minimum current.
>>  - Rename some APIs and other optimization.
>>
>> Changes since v15:
>>  - Add charger state checking to avoid sending out duplicate notifies to 
>> users.
>>  - Add one work to notify power users the current has been changed.
>>
>> Changes since v14:
>>  - Add kernel documentation for struct usb_cahrger.
>>  - Remove some redundant WARN() functions.
>>
>> Changes since v13:
>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>  - Rename some functions in charger.c file.
>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
>> tags/usb-for-v4.8
>>
>> Changes since v12:
>>  - Remove the class and device things.
>>  - Link usb charger to udc-core.ko.
>>  - Create one "charger" subdirectory which holds all charger-related 
>> attributes.
>>
>> Changes since v11:
>>  - Reviewed and tested by Li Jun.
>>
>> Changes since v10:
>>  - Introduce usb_charger_get_state() function to check charger state.
>>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>>  in case will be issued in atomic context.
>
> Could you apply this patchset into your branch if there are no other
> comments? Thanks.

Some of my previous comments are still outstanding.  You seem to have
just brushed them off without apparently understanding.
And no-one else seems to care enough to try to bridge the gap...

Let me try again.

1/ I think we agreed that it doesn't make sense for there to be
 two chargers registered in a system.
 However usb_charger_register() still allows that, and assigns
 and arbitrary name to each based on discovery order.
 This *cannot* make sense.

2/ Why do you have usb_charger_set_current()??
  No code ever calls it.
  This updates the min and max current which are defined in a
  standard.  It never makes sense to change the min and max
  for a particular cable type.

3/ usb_charger_notify_state() does nothing if the state doesn't change.
  When the extcon detects an SDP, it will be called to set the state
  to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
  it happened to be before, which is probably wrong.
  When after USB negotiation completes,
  usb_charger_set_cur_limit_by_gadget()
  will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
  again, but with a new current.  This will be ignored, as the state is
  already USB_CHARGER_PRESENT.

 (as an aside
 +enum usb_charger_state {
 +  USB_CHARGER_DEFAULT,
 +  USB_CHARGER_PRESENT,
 +  USB_CHARGER_REMOVE,
 +};

 looks odd.  It should probably by
USB_CHARGER_UNKNOWN
USB_CHARGER_PRESENT
USB_CHARGER_ABSENT

 "REMOVE" isn't a state.  "REMOVED" might be.
 )

4/ I still strongly object to the ->get_charger_type() interface.
 You previously said:

 No. User can implement the get_charger_type() method to access the
 PMIC registers to get the charger type, which is one very common method.

 I suggest that if the PMIC registers report the charger type, then the
 PMIC driver should register an EXTCON and report the charger type
 through that.  Then the information would be directly available to
 user-space, and the usb-charger framework would have a single uniform
 mechanism for being told the cable type.

 Related:  I don't like charger_type_show().  I don't think
 the usb-charger should export that information to user-space because
 extcon already does that, and duplication is confusing and pointless.

 And I just noticed you have a ->charger_detect() too, which seems
 identical to ->get_charger_type().  There is no documentation
 explaining the difference.

5/ There is no convincing example usage of this framework.
  wm8931x_power.c just scratches the surface.
  If it is so good, it should be easy to convert a lot of other
 

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

[NeilBrown]

On Thu, Oct 27 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 19 October 2016 at 10:37, Baolin Wang  wrote:
>> Currently the Linux kernel does not provide any standard integration of this
>> feature that integrates the USB subsystem with the system power regulation
>> provided by PMICs meaning that either vendors must add this in their kernels
>> or USB gadget devices based on Linux (such as mobile phones) may not behave
>> as they should. Thus provide a standard framework for doing this in kernel.
>>
>> Now introduce one user with wm831x_power to support and test the usb charger,
>> which is pending testing. Moreover there may be other potential users will 
>> use
>> it in future.
>>
>> Changes since v17:
>>  - Remove goto section in usb_charger_register() function.
>>  - Remove 'extern' in charger.h file.
>>  - Move the kfree() to usb_charger_exit() function.
>>
>> Changes since v16:
>>  - Modify the charger current range with introducing the maximum and minimum
>>  current.
>>  - Remove the getting charger type method from power supply.
>>  - Add the getting charger type method from extcon system.
>>  - Introduce new usb_charger_get_current() API for users to get the maximum 
>> and
>>  minimum current.
>>  - Rename some APIs and other optimization.
>>
>> Changes since v15:
>>  - Add charger state checking to avoid sending out duplicate notifies to 
>> users.
>>  - Add one work to notify power users the current has been changed.
>>
>> Changes since v14:
>>  - Add kernel documentation for struct usb_cahrger.
>>  - Remove some redundant WARN() functions.
>>
>> Changes since v13:
>>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>>  - Rename some functions in charger.c file.
>>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
>> tags/usb-for-v4.8
>>
>> Changes since v12:
>>  - Remove the class and device things.
>>  - Link usb charger to udc-core.ko.
>>  - Create one "charger" subdirectory which holds all charger-related 
>> attributes.
>>
>> Changes since v11:
>>  - Reviewed and tested by Li Jun.
>>
>> Changes since v10:
>>  - Introduce usb_charger_get_state() function to check charger state.
>>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>>  in case will be issued in atomic context.
>
> Could you apply this patchset into your branch if there are no other
> comments? Thanks.

Some of my previous comments are still outstanding.  You seem to have
just brushed them off without apparently understanding.
And no-one else seems to care enough to try to bridge the gap...

Let me try again.

1/ I think we agreed that it doesn't make sense for there to be
 two chargers registered in a system.
 However usb_charger_register() still allows that, and assigns
 and arbitrary name to each based on discovery order.
 This *cannot* make sense.

2/ Why do you have usb_charger_set_current()??
  No code ever calls it.
  This updates the min and max current which are defined in a
  standard.  It never makes sense to change the min and max
  for a particular cable type.

3/ usb_charger_notify_state() does nothing if the state doesn't change.
  When the extcon detects an SDP, it will be called to set the state
  to USB_CHARGER_PRESENT.  The value of cur.sdp_max will be whatever
  it happened to be before, which is probably wrong.
  When after USB negotiation completes,
  usb_charger_set_cur_limit_by_gadget()
  will call into usb_charger_notify_state() to set USB_CHARGER_PRESENT
  again, but with a new current.  This will be ignored, as the state is
  already USB_CHARGER_PRESENT.

 (as an aside
 +enum usb_charger_state {
 +  USB_CHARGER_DEFAULT,
 +  USB_CHARGER_PRESENT,
 +  USB_CHARGER_REMOVE,
 +};

 looks odd.  It should probably by
USB_CHARGER_UNKNOWN
USB_CHARGER_PRESENT
USB_CHARGER_ABSENT

 "REMOVE" isn't a state.  "REMOVED" might be.
 )

4/ I still strongly object to the ->get_charger_type() interface.
 You previously said:

 No. User can implement the get_charger_type() method to access the
 PMIC registers to get the charger type, which is one very common method.

 I suggest that if the PMIC registers report the charger type, then the
 PMIC driver should register an EXTCON and report the charger type
 through that.  Then the information would be directly available to
 user-space, and the usb-charger framework would have a single uniform
 mechanism for being told the cable type.

 Related:  I don't like charger_type_show().  I don't think
 the usb-charger should export that information to user-space because
 extcon already does that, and duplication is confusing and pointless.

 And I just noticed you have a ->charger_detect() too, which seems
 identical to ->get_charger_type().  There is no documentation
 explaining the difference.

5/ There is no convincing example usage of this framework.
  wm8931x_power.c just scratches the surface.
  If it is so good, it should be easy to convert a lot of other
  drivers over to it.  

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

[Baolin Wang]

Hi Felipe,

On 19 October 2016 at 10:37, Baolin Wang  wrote:
> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should. Thus provide a standard framework for doing this in kernel.
>
> Now introduce one user with wm831x_power to support and test the usb charger,
> which is pending testing. Moreover there may be other potential users will use
> it in future.
>
> Changes since v17:
>  - Remove goto section in usb_charger_register() function.
>  - Remove 'extern' in charger.h file.
>  - Move the kfree() to usb_charger_exit() function.
>
> Changes since v16:
>  - Modify the charger current range with introducing the maximum and minimum
>  current.
>  - Remove the getting charger type method from power supply.
>  - Add the getting charger type method from extcon system.
>  - Introduce new usb_charger_get_current() API for users to get the maximum 
> and
>  minimum current.
>  - Rename some APIs and other optimization.
>
> Changes since v15:
>  - Add charger state checking to avoid sending out duplicate notifies to 
> users.
>  - Add one work to notify power users the current has been changed.
>
> Changes since v14:
>  - Add kernel documentation for struct usb_cahrger.
>  - Remove some redundant WARN() functions.
>
> Changes since v13:
>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>  - Rename some functions in charger.c file.
>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
> tags/usb-for-v4.8
>
> Changes since v12:
>  - Remove the class and device things.
>  - Link usb charger to udc-core.ko.
>  - Create one "charger" subdirectory which holds all charger-related 
> attributes.
>
> Changes since v11:
>  - Reviewed and tested by Li Jun.
>
> Changes since v10:
>  - Introduce usb_charger_get_state() function to check charger state.
>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>  in case will be issued in atomic context.

Could you apply this patchset into your branch if there are no other
comments? Thanks.

>
> Baolin Wang (4):
>   usb: gadget: Introduce the usb charger framework
>   usb: gadget: Support for the usb charger framework
>   usb: gadget: Integrate with the usb gadget supporting for usb charger
>   power: wm831x_power: Support USB charger current limit management
>
>  drivers/power/wm831x_power.c |   75 
>  drivers/usb/gadget/Kconfig   |8 +
>  drivers/usb/gadget/udc/Makefile  |1 +
>  drivers/usb/gadget/udc/charger.c |  877 
> ++
>  drivers/usb/gadget/udc/core.c|   19 +-
>  include/linux/mfd/wm831x/pdata.h |3 +
>  include/linux/usb/charger.h  |  185 
>  include/linux/usb/gadget.h   |3 +
>  include/uapi/linux/usb/charger.h |   31 ++
>  9 files changed, 1201 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> --
> 1.7.9.5
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

On 19 October 2016 at 10:37, Baolin Wang  wrote:
> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should. Thus provide a standard framework for doing this in kernel.
>
> Now introduce one user with wm831x_power to support and test the usb charger,
> which is pending testing. Moreover there may be other potential users will use
> it in future.
>
> Changes since v17:
>  - Remove goto section in usb_charger_register() function.
>  - Remove 'extern' in charger.h file.
>  - Move the kfree() to usb_charger_exit() function.
>
> Changes since v16:
>  - Modify the charger current range with introducing the maximum and minimum
>  current.
>  - Remove the getting charger type method from power supply.
>  - Add the getting charger type method from extcon system.
>  - Introduce new usb_charger_get_current() API for users to get the maximum 
> and
>  minimum current.
>  - Rename some APIs and other optimization.
>
> Changes since v15:
>  - Add charger state checking to avoid sending out duplicate notifies to 
> users.
>  - Add one work to notify power users the current has been changed.
>
> Changes since v14:
>  - Add kernel documentation for struct usb_cahrger.
>  - Remove some redundant WARN() functions.
>
> Changes since v13:
>  - Remove the charger checking in usb_gadget_vbus_draw() function.
>  - Rename some functions in charger.c file.
>  - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
> tags/usb-for-v4.8
>
> Changes since v12:
>  - Remove the class and device things.
>  - Link usb charger to udc-core.ko.
>  - Create one "charger" subdirectory which holds all charger-related 
> attributes.
>
> Changes since v11:
>  - Reviewed and tested by Li Jun.
>
> Changes since v10:
>  - Introduce usb_charger_get_state() function to check charger state.
>  - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
>  in case will be issued in atomic context.

Could you apply this patchset into your branch if there are no other
comments? Thanks.

>
> Baolin Wang (4):
>   usb: gadget: Introduce the usb charger framework
>   usb: gadget: Support for the usb charger framework
>   usb: gadget: Integrate with the usb gadget supporting for usb charger
>   power: wm831x_power: Support USB charger current limit management
>
>  drivers/power/wm831x_power.c |   75 
>  drivers/usb/gadget/Kconfig   |8 +
>  drivers/usb/gadget/udc/Makefile  |1 +
>  drivers/usb/gadget/udc/charger.c |  877 
> ++
>  drivers/usb/gadget/udc/core.c|   19 +-
>  include/linux/mfd/wm831x/pdata.h |3 +
>  include/linux/usb/charger.h  |  185 
>  include/linux/usb/gadget.h   |3 +
>  include/uapi/linux/usb/charger.h |   31 ++
>  9 files changed, 1201 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/gadget/udc/charger.c
>  create mode 100644 include/linux/usb/charger.h
>  create mode 100644 include/uapi/linux/usb/charger.h
>
> --
> 1.7.9.5
>



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger,
which is pending testing. Moreover there may be other potential users will use
it in future.

Changes since v17:
 - Remove goto section in usb_charger_register() function.
 - Remove 'extern' in charger.h file.
 - Move the kfree() to usb_charger_exit() function.

Changes since v16:
 - Modify the charger current range with introducing the maximum and minimum
 current.
 - Remove the getting charger type method from power supply.
 - Add the getting charger type method from extcon system.
 - Introduce new usb_charger_get_current() API for users to get the maximum and
 minimum current.
 - Rename some APIs and other optimization.

Changes since v15:
 - Add charger state checking to avoid sending out duplicate notifies to users.
 - Add one work to notify power users the current has been changed.

Changes since v14:
 - Add kernel documentation for struct usb_cahrger.
 - Remove some redundant WARN() functions.

Changes since v13:
 - Remove the charger checking in usb_gadget_vbus_draw() function.
 - Rename some functions in charger.c file.
 - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
tags/usb-for-v4.8

Changes since v12:
 - Remove the class and device things.
 - Link usb charger to udc-core.ko.
 - Create one "charger" subdirectory which holds all charger-related attributes.

Changes since v11:
 - Reviewed and tested by Li Jun.

Changes since v10:
 - Introduce usb_charger_get_state() function to check charger state.
 - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
 in case will be issued in atomic context.

Baolin Wang (4):
  usb: gadget: Introduce the usb charger framework
  usb: gadget: Support for the usb charger framework
  usb: gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c |   75 
 drivers/usb/gadget/Kconfig   |8 +
 drivers/usb/gadget/udc/Makefile  |1 +
 drivers/usb/gadget/udc/charger.c |  877 ++
 drivers/usb/gadget/udc/core.c|   19 +-
 include/linux/mfd/wm831x/pdata.h |3 +
 include/linux/usb/charger.h  |  185 
 include/linux/usb/gadget.h   |3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1201 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

-- 
1.7.9.5

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

[Baolin Wang]

Currently the Linux kernel does not provide any standard integration of this
feature that integrates the USB subsystem with the system power regulation
provided by PMICs meaning that either vendors must add this in their kernels
or USB gadget devices based on Linux (such as mobile phones) may not behave
as they should. Thus provide a standard framework for doing this in kernel.

Now introduce one user with wm831x_power to support and test the usb charger,
which is pending testing. Moreover there may be other potential users will use
it in future.

Changes since v17:
 - Remove goto section in usb_charger_register() function.
 - Remove 'extern' in charger.h file.
 - Move the kfree() to usb_charger_exit() function.

Changes since v16:
 - Modify the charger current range with introducing the maximum and minimum
 current.
 - Remove the getting charger type method from power supply.
 - Add the getting charger type method from extcon system.
 - Introduce new usb_charger_get_current() API for users to get the maximum and
 minimum current.
 - Rename some APIs and other optimization.

Changes since v15:
 - Add charger state checking to avoid sending out duplicate notifies to users.
 - Add one work to notify power users the current has been changed.

Changes since v14:
 - Add kernel documentation for struct usb_cahrger.
 - Remove some redundant WARN() functions.

Changes since v13:
 - Remove the charger checking in usb_gadget_vbus_draw() function.
 - Rename some functions in charger.c file.
 - Rebase on git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
tags/usb-for-v4.8

Changes since v12:
 - Remove the class and device things.
 - Link usb charger to udc-core.ko.
 - Create one "charger" subdirectory which holds all charger-related attributes.

Changes since v11:
 - Reviewed and tested by Li Jun.

Changes since v10:
 - Introduce usb_charger_get_state() function to check charger state.
 - Remove the mutex lock in usb_charger_set_cur_limit_by_type() function
 in case will be issued in atomic context.

Baolin Wang (4):
  usb: gadget: Introduce the usb charger framework
  usb: gadget: Support for the usb charger framework
  usb: gadget: Integrate with the usb gadget supporting for usb charger
  power: wm831x_power: Support USB charger current limit management

 drivers/power/wm831x_power.c |   75 
 drivers/usb/gadget/Kconfig   |8 +
 drivers/usb/gadget/udc/Makefile  |1 +
 drivers/usb/gadget/udc/charger.c |  877 ++
 drivers/usb/gadget/udc/core.c|   19 +-
 include/linux/mfd/wm831x/pdata.h |3 +
 include/linux/usb/charger.h  |  185 
 include/linux/usb/gadget.h   |3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1201 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/udc/charger.c
 create mode 100644 include/linux/usb/charger.h
 create mode 100644 include/uapi/linux/usb/charger.h

-- 
1.7.9.5