Introduce usb charger framework to deal with the usb gadget power negotation

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

Hi Felipe,

Quoted 45 lines · 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.

Quoted 4 lines

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.

Quoted 53 lines · Baolin Wang

Hi Felipe,

Quoted 49 lines · 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.

Quoted 2 lines

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.

Quoted 2 lines · NeilBrown

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

Yes, until now...

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 10 lines · Baolin Wang

Quoted 2 lines · NeilBrown

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

Yes, until now...

Quoted 3 lines

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

Quoted 1 line

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

Quoted 9 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 5 lines

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

Quoted 8 lines

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:

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 7 lines · Baolin Wang

Quoted 5 lines

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.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 1 line

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.

Quoted 15 lines

Quoted 14 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 11 lines

Quoted 5 lines

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

Quoted 3 lines

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?

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 1 line · NeilBrown

On Tue, Nov 01 2016, Baolin Wang wrote:

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.

Quoted 4 lines · Baolin Wang

Quoted 1 line · NeilBrown

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Hi

Quoted 41 lines · NeilBrown

Quoted 4 lines · Baolin Wang

Quoted 1 line · NeilBrown

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 2 lines · Baolin Wang

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.

Quoted 2 lines

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.

Quoted 5 lines · NeilBrown

Quoted 2 lines · Baolin Wang

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

Quoted 7 lines

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.

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.

Quoted 14 lines · Mark Brown

Quoted 5 lines · NeilBrown

Quoted 2 lines · Baolin Wang

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

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 6 lines · Mark Brown

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.

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.

[ Unknown signature status ]

Quoted 14 lines · NeilBrown

Quoted 6 lines · Mark Brown

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.

...

Quoted 8 lines

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

Quoted 5 lines

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.

Quoted 4 lines · Mark Brown

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.

[ Unknown signature status ]

Quoted 10 lines · NeilBrown

Quoted 4 lines · Mark Brown

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

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

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

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

[ Unknown signature status ]

Quoted 5 lines · NeilBrown

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.

Quoted 3 lines

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?

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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?

Quoted 9 lines · NeilBrown

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?

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

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?

Quoted 52 lines · Baolin Wang

Hi

Quoted 48 lines · NeilBrown

Quoted 4 lines · Baolin Wang

Quoted 1 line · NeilBrown

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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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 incremental improvements is much easier and more enjoyable. But that is just a guess.

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 12 lines · 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.

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.

Quoted 14 lines · Peter Chen

Quoted 12 lines · 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.

Quoted 4 lines

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.

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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

From above investigation, I did not find anything need to be changed

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

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 55 lines

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 · 2 lines

Baolin.wang
Best Regards

Quoted 42 lines · Baolin Wang

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 1 line

Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

Quoted 1 line

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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:

Quoted 2 lines

Quoted 2 lines

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

Quoted 61 lines

Quoted 51 lines

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.

Quoted 4 lines

6/ Any battery charger that needs to know the available current can now
call register_power_client() and get the information delivered.

NeilBrown

--
Baolin.wang
Best Regards

Hi,

Quoted 49 lines · NeilBrown

Quoted 42 lines · Baolin Wang

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 1 line

Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

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.

Quoted 23 lines

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 91 lines · Baolin Wang

Hi,

Quoted 49 lines · NeilBrown

Quoted 42 lines · Baolin Wang

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 1 line

Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

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.

Quoted 27 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 93 lines · NeilBrown

Quoted 91 lines · Baolin Wang

Hi,

Quoted 49 lines · NeilBrown

Quoted 42 lines · Baolin Wang

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 1 line

Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

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.

Quoted 27 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 101 lines · Baolin Wang

Quoted 93 lines · NeilBrown

Quoted 91 lines · Baolin Wang

Hi,

Quoted 49 lines · NeilBrown

Quoted 42 lines · Baolin Wang

Hi Neil,

Quoted 31 lines · NeilBrown

Quoted 5 lines · Baolin Wang

Quoted 3 lines

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.

Quoted 7 lines

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

Quoted 1 line

Now I am creating the new patchset with fixing and converting exist drivers.

Thanks!

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

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.

Quoted 27 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Yes, there is no spec to describe fast/slow charger type and how much current fast/slow charger can provide. Maybe some fast charger can provide 1A/2A, others can provide 5A, which depends on users' platform. If we change to EXTCON_CHG_USB_500MA/1A and some fast charger can provide 1.5A on user's platform, will it report the fast charger type by EXTCON_CHG_USB_1A on user's platform (but it can provide 1.5A)? So what I mean, can we keep EXTCON_CHG_USB_SLOW/FAST as they were, and maybe fix them in future? (BTW, I've fixed issue 1 and maintainer has applied them).

I said "It isn't really a question of what the charger can provide." and you are still taking about "and some fast charger can provide 1.5A". So it seems like you didn't read, or you didn't understand what I wrote. I'll try again.

Ignore the chargers. COMPLETELY. This not about chargers AT ALL. This is about cables and the information that the cable reports.

Some how, and I cannot find the details, these MAXIM devices can report things like

        case MAX14577_CHARGER_TYPE_SPECIAL_500MA:
and
        case MAX14577_CHARGER_TYPE_SPECIAL_1A:
and
        case MAX77693_CHARGER_TYPE_APPLE_500MA:
and
        case MAX77693_CHARGER_TYPE_APPLE_1A_2A:

If this information is to be useful to the USB battery charging infrastructure, then it must be communicated unambiguously. Reporting "EXTCON_CHG_USB_SLOW" or "..._FAST" doesn't have an obvious unambiguous meaning.

If it were documented somewhere that
A cable of type EXTCON_CHG_USB_SLOW can provide at least 500mA of current at 5V. A cable of type EXTCON_CHG_USB_FAST can provide at least 1A of current at 5V.
when those cable types could be used by the USB battery chargers. If we just changed the names, then we wouldn't really need to document it and the intention would be obvious. Self-documenting names are better where possible.

So yes, we could leave it as it is a decide not to fix this bug. But then I would then argue strongly against any attempt for the USB battery charging infrastructure to do anything with EXTCON_CHG_USB_SLOW and EXTCON_CHG_USB_FAST. If you are going to fix this thing, you may as well fix it properly.

NeilBrown