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

Groups > linux.kernel > #1510118 > unrolled thread

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

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-10-27 09:40 +0200
    Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-10-28 00:10 +0200
      Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-10-28 15:00 +0200
        Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Mark Brown <broonie@kernel.org> - 2016-10-28 19:10 +0200
          Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-10-31 12:30 +0100
        Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-10-31 01:10 +0100
          Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-11-01 14:00 +0100
            Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-11-03 02:30 +0100
              Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-11-07 09:20 +0100
                Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-11-07 22:00 +0100
                  Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Baolin Wang <baolin.wang@linaro.org> - 2016-11-10 10:50 +0100
              Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Peter Chen <hzpeterchen@gmail.com> - 2016-11-08 10:00 +0100
                Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation NeilBrown <neilb@suse.com> - 2016-11-08 21:40 +0100
                  Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the  usb gadget power negotation Peter Chen <hzpeterchen@gmail.com> - 2016-11-09 02:40 +0100

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

Hi Felipe,

On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> 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

[toc] | [next] | [standalone]

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

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

On Thu, Oct 27 2016, Baolin Wang wrote:

> Hi Felipe,
>
> On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> 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.  If you did that it would be much easier
  to see how it works and what the strengths/weaknesses were.


NeilBrown

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

#1511119

Hi,

On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> wrote:
> On Thu, Oct 27 2016, Baolin Wang wrote:
>
>> Hi Felipe,
>>
>> On 19 October 2016 at 10:37, Baolin Wang <baolin.wang@linaro.org> 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 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:

enum usb_charger_type pmic_get_charger_type(struct usb_charger *charger)
{
    enum usb_charger_type type;
    u32 val;

    regmap(reg_map, PMIC_CHARGER_STATUS, &val);

    /* change val to 'enum usb_charger_type' type ... */
    return type;
}

->get_charger_type() = pmic_get_charger_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.

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.

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

I think the kernel doc have explained that, but I like to explain it
again. Since we can detect the charger by software or hardware (like
PMIC), if you need to detect your charger type by software, then you
can implement this callback, you can refer to Jun's patch:
http://www.spinics.net/lists/linux-usb/msg139808.html

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


-- 
Baolin.wang
Best Regards

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

#1511261

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

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> 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.

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

#1512494

On 29 October 2016 at 01:03, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
>> On 28 October 2016 at 06:00, NeilBrown <neilb@suse.com> 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

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

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

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

On 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 the extcon.

>
>>
>>  And I just noticed you have a ->charger_detect() too, which seems
>>  identical to ->get_charger_type().  There is no documentation
>>  explaining the difference.
>
> I think the kernel doc have explained that, but I like to explain it
> again. Since we can detect the charger by software or hardware (like
> PMIC), if you need to detect your charger type by software, then you
> can implement this callback, you can refer to Jun's patch:
> http://www.spinics.net/lists/linux-usb/msg139808.html

The kernel-doc says:

 + * @get_charger_type: User can get charger type by implementing this callback.
 + * @charger_detect: Charger detection method can be implemented if you need to
 + * manually detect the charger type.

To me, that doesn't say anything useful.  What is the difference between
"get charger type" and "manually detect the charger type" ??

I don't want to have to refer to some extra set of patches to guess how
something is supposed to work.  It needs to be clearly and
unambiguously documented.

However, I'm very quickly losing interest in this whole debate, partly
because it misses a key point.  As I have said, I strongly feel that
resolving the current inconsistencies in the use of
usb_register_notifier() is an important preliminary to resolve this
issue, as that is *already* sometimes used to communication available
current.

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

NeilBrown

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

#1513255

Hi,

On 31 October 2016 at 08:00, NeilBrown <neilb@suse.com> 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).
>
> 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 the extcon.
>
>>
>>>
>>>  And I just noticed you have a ->charger_detect() too, which seems
>>>  identical to ->get_charger_type().  There is no documentation
>>>  explaining the difference.
>>
>> I think the kernel doc have explained that, but I like to explain it
>> again. Since we can detect the charger by software or hardware (like
>> PMIC), if you need to detect your charger type by software, then you
>> can implement this callback, you can refer to Jun's patch:
>> http://www.spinics.net/lists/linux-usb/msg139808.html
>
> The kernel-doc says:
>
>  + * @get_charger_type: User can get charger type by implementing this callback.
>  + * @charger_detect: Charger detection method can be implemented if you need to
>  + * manually detect the charger type.
>
> To me, that doesn't say anything useful.  What is the difference between
> "get charger type" and "manually detect the charger type" ??
>
> I don't want to have to refer to some extra set of patches to guess how
> something is supposed to work.  It needs to be clearly and
> unambiguously documented.

OK. I will update the kernel doc to make it clear.

>
> However, I'm very quickly losing interest in this whole debate, partly
> because it misses a key point.  As I have said, I strongly feel that
> resolving the current inconsistencies in the use of
> usb_register_notifier() is an important preliminary to resolve this
> issue, as that is *already* sometimes used to communication available
> current.
>
> 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?

-- 
Baolin.wang
Best Regards

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

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

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

On Tue, 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

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

#1515896

On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> wrote:
> On Tue, Nov 01 2016, Baolin Wang wrote:
>
>
>>> So I won't be responding on this topic any further until I see a genuine
>>> attempt to understand and resolve the inconsistencies with
>>> usb_register_notifier().
>>
>> Any better solution?
>
> I'm not sure exactly what you are asking, so I'll assume you are asking
> the question I want to answer :-)
>
> 1/ Liase with the extcon developers to resolve the inconsistencies
>   with USB connector types.
>   e.g. current there is both "EXTCON_USB" and "EXTCON_CHG_USB_SDP"
>   which both seem to suggest a standard downstream port.  There is no
>   documentation describing how these relate, and no consistent practice
>   to copy.
>   I suspect the intention is that
>     EXTCON_USB and EXTCON_USB_HOST indicated that data capabilities of
>     the cable, while EXTCON_CHG_USB* indicate the power capabilities of
>     the cable.
>     So EXTCON_CHG_USB_SDP should always appear together with EXTCON_USB
>     while EXTCON_CHG_USB_DCP would not, and EXTCON_CHG_USB_ACA
>     would normally appear with EXTCON_USB_HOST (I think).
>   Some drivers follow this model, particularly extcon-max14577.c
>   but it is not consistent.
>
>   This policy should be well documented and possibly existing drivers
>   should be updated to follow it.
>
>   At the same time it would make sense to resolve EXTCON_CHG_USB_SLOW
>   and EXTCON_CHG_USB_FAST.  These names don't mean much.
>   They were recently removed from drivers/power/axp288_charger.c
>   which is good, but are still used in drivers/extcon/extcon-max*
>   Possibly they should be changed to names from the standard, or
>   possibly they should be renamed to identify the current they are
>   expected to provide. e.g. EXTCON_CHG_USB_500MA and EXTCON_CHG_USB_1A
>
> 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 which
is better, and do not need to notify current for power driver in usb
phy driver.

-- 
Baolin.wang
Best Regards

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

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

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

On Mon, Nov 07 2016, Baolin Wang wrote:

> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> 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

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

#1518817

Hi

On 8 November 2016 at 04:36, NeilBrown <neilb@suse.com> wrote:
> On Mon, Nov 07 2016, Baolin Wang wrote:
>
>> On 3 November 2016 at 09:25, NeilBrown <neilb@suse.com> 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

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

#1516967

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

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

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

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

On Tue, 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

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

#1517723

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web