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 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     |   69 ++++
 drivers/usb/gadget/Kconfig       |    8 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c    |   17 +
 include/linux/mfd/wm831x/pdata.h |    3 +
 include/linux/usb/charger.h      |  186 +++++++++
 include/linux/usb/gadget.h       |    3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1098 insertions(+)
 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 6 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.

Quoted 3 lines

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.

Could you please apply this patchset into your 'next' branch if you have no comments about it? Thank you.

Quoted 48 lines

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     |   69 ++++
 drivers/usb/gadget/Kconfig       |    8 +
 drivers/usb/gadget/udc/Makefile  |    1 +
 drivers/usb/gadget/udc/charger.c |  780 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c    |   17 +
 include/linux/mfd/wm831x/pdata.h |    3 +
 include/linux/usb/charger.h      |  186 +++++++++
 include/linux/usb/gadget.h       |    3 +
 include/uapi/linux/usb/charger.h |   31 ++
 9 files changed, 1098 insertions(+)
 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

Quoted 3 lines

--
1.7.9.5

--
Baolin.wang
Best Regards

Hi Felipe,

Quoted 13 lines · Baolin Wang

Hi Felipe,

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

Quoted 2 lines

Could you please apply this patchset into your 'next' branch if you have no comments about it? Thank you.

Since there are no other comments about this patchset for a long time, could you please apply this patchset? Thanks.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type). This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are. There is no reason for those not to be hard coded.

Secondly, you treat each charger type as having a single "cur_limit" and utilize that limit by telling the PMIC to draw that much current. Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current

level.
The minimum should always be available. Attempting to draw more than
that (but less that the max) might work or might cause the charger to shut down, but you can be sure that the charger will respond to the increased load by first reducing the voltage, and will not shut down until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger, there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
draws, and if it can monitor the voltage on the charger, then it could gradually increase the power being requested until the voltage drops below some threshold (e.g. 4.75V), and then (probably) back off a little. It should only increase at most up to the maximum, even if the voltage remains high. It should probably start at zero rather than at the minimum. This allows for lossage elsewhere.

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
control of the current requested, then it should request only the minimum available current. Any more is not safe.

Quoted 16 lines · Baolin Wang

Hi Felipe,

Quoted 13 lines · Baolin Wang

Hi Felipe,

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

Quoted 5 lines

Quoted 2 lines

Could you please apply this patchset into your 'next' branch if you have no comments about it? Thank you.

Since there are no other comments about this patchset for a long time, could you please apply this patchset? Thanks.

Sorry, I should have replied earlier. Tim Bird mentioned on the ksummit-discuss list that there was a frustration with this not making progress so I decided to contribute what I could now.

I think this patch set is attempting to address an important problem that needs solving. However I think it gets some key aspects wrong. Maybe they can get fixed up after the patchset is upstream, maybe they should be fixed first - I have no strong opinion on that.

My main complaints involve the detection and handling of the different charger types - DCP, CDP, ACA etc.
The big-picture requirement here that the PHY will detect the physical properties of the cable (e.g. resistance to ground on ID) and determine the type of charger expected. This information must be communicated to the PMIC "power_supply" device so it can regulate the power being drawn through the cable.

The first problem is that there are two different ways that the distinction between DCP, CDP, ACA etc can be represented in Linux. They are cable types in the 'extcon' subsystem, and they are power_supply types in the 'power_supply' subsystem. This duplication is confusing. It is not caused by your patch set, but I believe your patchset needs to work with the duplication and I think it does so poorly.

In my mind, the power_supply should *not* know about this distinction at all (except possibly as an advisor attribute simiarly to the current battery technology attribute). The other types it knows of are "AC", "USB", and "BATTERY". The contrast between these is quite different From the contrast between DCP, CDP, ACA, which, from the perspective of the power supply, are almost irrelevant. Your patchset effectively examines the power_supply_type of one power_supply, and communicates it to another. It isn't clear to me how the first power_supply gets the information, or what the relationship between the two power_supplies is meant to be.

It makes much more sense, to me, to utilized the knowledge of this distinction that extcon provides. A usb PHY can register an extcon, declare the sorts of cables that it can detect, and tell the extcon as cables appear or disappear. The PMIC power_supply can then register with that extcon for events and can find out when a cable is attached, and what sort of cable.
Your usb-charging framework would be well placed to help the power_supply to find the correct extcon, and possibly even to handle the registration for events.

Your framework does currently register with extcon, but only listens for EXTCON_USB cables. I don't think that cable type is (reliably) reported when a DCP (for example) is plugged in.

Here there is another problem that is not of your making, but still needs fixing. Extcon declares a number of cable types like:

/* USB external connector */
#define EXTCON_USB 1
#define EXTCON_USB_HOST 2

/* Charging external connector */

#define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
#define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
#define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
#define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
#define EXTCON_CHG_USB_FAST     9
#define EXTCON_CHG_USB_SLOW     10

However it doesn't define what those mean, so we are left to guess. They each correspond to bits in a bitmap, so a cable can have multiple types. I think the best interpretation is that:

EXTCON_USB means that the cable carries USB data from a host. EXTCON_USB_HOST means that that cable carries USB data to a host. EXTCON_CHG_* means that power is available as described in the standards.
(what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all clear).

There is some support for this in the code, but it is not universally acknowledged. For a USB charging framework to be genuinely useful, it must (I think) make sure this issue gets clarified, and the various cable types used properly.

Once the cable/charger type has been determined, the PMIC power_supply needs to use this information. Your current patches make use of this information in ways that are wrong in two respects.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type). This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are. There is no reason for those not to be hard coded.

Secondly, you treat each charger type as having a single "cur_limit" and utilize that limit by telling the PMIC to draw that much current.

Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current level.
The minimum should always be available. Attempting to draw more than
that (but less that the max) might work or might cause the charger to shut down, but you can be sure that the charger will respond to the increased load by first reducing the voltage, and will not shut down until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger, there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
draws, and if it can monitor the voltage on the charger, then it could gradually increase the power being requested until the voltage drops below some threshold (e.g. 4.75V), and then (probably) back off a little. It should only increase at most up to the maximum, even if the voltage remains high. It should probably start at zero rather than at the minimum. This allows for lossage elsewhere.

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
control of the current requested, then it should request only the minimum available current. Any more is not safe.

So your USB charging framework should communicate the MIN and MAX as specified in the standard, and allow the PMIC to use that information as appropriate.
A library routine to support the incremental increase in current drain would probably be appreciated by PMIC driver writers.

A more details discussion of this problem-space can be found at
https://lwn.net/Articles/693027/
and an analysis of the functionality currently available in the kernel (which adds a number of specifics to the above) can be found at
https://lwn.net/Articles/694062/

NeilBrown

Hi Neil,

Quoted 18 lines · NeilBrown

Quoted 16 lines · Baolin Wang

Hi Felipe,

Quoted 13 lines · Baolin Wang

Hi Felipe,

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

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Could you please apply this patchset into your 'next' branch if you have no comments about it? Thank you.

Since there are no other comments about this patchset for a long time, could you please apply this patchset? Thanks.

Sorry for the late reply.

Quoted 17 lines

Sorry, I should have replied earlier. Tim Bird mentioned on the ksummit-discuss list that there was a frustration with this not making progress so I decided to contribute what I could now.

I think this patch set is attempting to address an important problem that needs solving. However I think it gets some key aspects wrong. Maybe they can get fixed up after the patchset is upstream, maybe they should be fixed first - I have no strong opinion on that.

My main complaints involve the detection and handling of the different charger types - DCP, CDP, ACA etc.
The big-picture requirement here that the PHY will detect the physical properties of the cable (e.g. resistance to ground on ID) and determine the type of charger expected. This information must be communicated to the PMIC "power_supply" device so it can regulate the power being drawn through the cable.

Quoted 6 lines

The first problem is that there are two different ways that the distinction between DCP, CDP, ACA etc can be represented in Linux. They are cable types in the 'extcon' subsystem, and they are power_supply types in the 'power_supply' subsystem. This duplication is confusing. It is not caused by your patch set, but I believe your patchset needs to work with the duplication and I think it does so poorly.

Now the usb charger will not get charger type from 'extcon' subsystem, we get the charger type from 'power_supply' and calllback 'get_charger_type' for users.

Quoted 12 lines

In my mind, the power_supply should *not* know about this distinction at all (except possibly as an advisor attribute simiarly to the current battery technology attribute). The other types it knows of are "AC", "USB", and "BATTERY". The contrast between these is quite different From the contrast between DCP, CDP, ACA, which, from the perspective of the power supply, are almost irrelevant. Your patchset effectively examines the power_supply_type of one power_supply, and communicates it to another. It isn't clear to me how the first power_supply gets the information, or what the relationship between the two power_supplies is meant to be.

We just get the charger type from the power supply which can get the charger type from register or something else,

and the usb charger did
nothing for this power supply. In some platform, the charger type is get in power supply driver, thus we should link this power supply to get the charger type when USB cable is plugin. If you don't get charger type from power supply driver, then it does not need to link this power supply phandle.

Quoted 11 lines

It makes much more sense, to me, to utilized the knowledge of this distinction that extcon provides. A usb PHY can register an extcon, declare the sorts of cables that it can detect, and tell the extcon as cables appear or disappear. The PMIC power_supply can then register with that extcon for events and can find out when a cable is attached, and what sort of cable.
Your usb-charging framework would be well placed to help the power_supply to find the correct extcon, and possibly even to handle the registration for events.

Quoted 3 lines

Your framework does currently register with extcon, but only listens for EXTCON_USB cables. I don't think that cable type is (reliably) reported when a DCP (for example) is plugged in.

Here we just listen the plugin/out events from extcon, if one cable plugin it will report to usb charger.

Quoted 28 lines

Here there is another problem that is not of your making, but still needs fixing. Extcon declares a number of cable types like:

/* USB external connector */
#define EXTCON_USB 1
#define EXTCON_USB_HOST 2

/* Charging external connector */

#define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
#define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
#define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
#define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
#define EXTCON_CHG_USB_FAST     9
#define EXTCON_CHG_USB_SLOW     10

However it doesn't define what those mean, so we are left to guess. They each correspond to bits in a bitmap, so a cable can have multiple types. I think the best interpretation is that:

EXTCON_USB means that the cable carries USB data from a host. EXTCON_USB_HOST means that that cable carries USB data to a host. EXTCON_CHG_* means that power is available as described in the standards.
(what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all clear).

Quoted 4 lines

There is some support for this in the code, but it is not universally acknowledged. For a USB charging framework to be genuinely useful, it must (I think) make sure this issue gets clarified, and the various cable types used properly.

Here I agree with you, now the usb charger does not listen the charger type from extcon which need to be fixed.

Quoted 9 lines

Once the cable/charger type has been determined, the PMIC power_supply needs to use this information. Your current patches make use of this information in ways that are wrong in two respects.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type). This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are. There is no reason for those not to be hard coded.

Yes, but you must think about some special cases on some platforms. Users may need to change the current in some situations, thus we should export one API for users to change the current. (I think you misunderstand the current limit here, that is the current for power driver to draw).

Quoted 4 lines

Secondly, you treat each charger type as having a single "cur_limit" and utilize that limit by telling the PMIC to draw that much current.

This "cur_limit" is set by user or USB configuration what they want, usb charger just tells the power driver how much current need to draw.

Quoted 22 lines

Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current level.
The minimum should always be available. Attempting to draw more than
that (but less that the max) might work or might cause the charger to shut down, but you can be sure that the charger will respond to the increased load by first reducing the voltage, and will not shut down until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger, there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
draws, and if it can monitor the voltage on the charger, then it could gradually increase the power being requested until the voltage drops below some threshold (e.g. 4.75V), and then (probably) back off a little. It should only increase at most up to the maximum, even if the voltage remains high. It should probably start at zero rather than at the minimum. This allows for lossage elsewhere.

Quoted 3 lines

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
control of the current requested, then it should request only the minimum available current. Any more is not safe.

Make sense.

Quoted 6 lines

So your USB charging framework should communicate the MIN and MAX as specified in the standard, and allow the PMIC to use that information as appropriate.
A library routine to support the incremental increase in current drain would probably be appreciated by PMIC driver writers.

I should say again, the current is set by user or USB configuration what they want, USB charger just tells the power driver how much current need to draw. But we can add some checking if the setting current is in the right range.

Thanks for your comments.

Quoted 11 lines

A more details discussion of this problem-space can be found at
https://lwn.net/Articles/693027/
and an analysis of the functionality currently available in the kernel (which adds a number of specifics to the above) can be found at
https://lwn.net/Articles/694062/

NeilBrown

Signature · 2 lines

Baolin.wang
Best Regards

Quoted 54 lines · Baolin Wang

Hi Neil,

Quoted 18 lines · NeilBrown

Quoted 16 lines · Baolin Wang

Hi Felipe,

Quoted 13 lines · Baolin Wang

Hi Felipe,

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

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Could you please apply this patchset into your 'next' branch if you have no comments about it? Thank you.

Since there are no other comments about this patchset for a long time, could you please apply this patchset? Thanks.

Sorry for the late reply.

Quoted 23 lines

Sorry, I should have replied earlier. Tim Bird mentioned on the ksummit-discuss list that there was a frustration with this not making progress so I decided to contribute what I could now.

I think this patch set is attempting to address an important problem that needs solving. However I think it gets some key aspects wrong. Maybe they can get fixed up after the patchset is upstream, maybe they should be fixed first - I have no strong opinion on that.

My main complaints involve the detection and handling of the different charger types - DCP, CDP, ACA etc.
The big-picture requirement here that the PHY will detect the physical properties of the cable (e.g. resistance to ground on ID) and determine the type of charger expected. This information must be communicated to the PMIC "power_supply" device so it can regulate the power being drawn through the cable.

The first problem is that there are two different ways that the distinction between DCP, CDP, ACA etc can be represented in Linux. They are cable types in the 'extcon' subsystem, and they are power_supply types in the 'power_supply' subsystem. This duplication is confusing. It is not caused by your patch set, but I believe your patchset needs to work with the duplication and I think it does so poorly.

Quoted 3 lines

Now the usb charger will not get charger type from 'extcon' subsystem, we get the charger type from 'power_supply' and calllback 'get_charger_type' for users.

I understand this. I think it is wrong because, in general, the power_supply doesn't know what the charger_type is (it might know it is USB, but most don't know which sort of USB). The PHY knows that, not the power_supply.

Quoted 13 lines

Quoted 12 lines

In my mind, the power_supply should *not* know about this distinction at all (except possibly as an advisor attribute simiarly to the current battery technology attribute). The other types it knows of are "AC", "USB", and "BATTERY". The contrast between these is quite different From the contrast between DCP, CDP, ACA, which, from the perspective of the power supply, are almost irrelevant. Your patchset effectively examines the power_supply_type of one power_supply, and communicates it to another. It isn't clear to me how the first power_supply gets the information, or what the relationship between the two power_supplies is meant to be.

Quoted 2 lines

We just get the charger type from the power supply which can get the charger type from register or something else,

But that register would be part of the PHY, not part of the charger. Having the power_supply driver reading the PHY register might work for some hardware, but is not a general solution.

Quoted 22 lines

and the usb charger did
nothing for this power supply. In some platform, the charger type is get in power supply driver, thus we should link this power supply to get the charger type when USB cable is plugin. If you don't get charger type from power supply driver, then it does not need to link this power supply phandle.

Quoted 14 lines

It makes much more sense, to me, to utilized the knowledge of this distinction that extcon provides. A usb PHY can register an extcon, declare the sorts of cables that it can detect, and tell the extcon as cables appear or disappear. The PMIC power_supply can then register with that extcon for events and can find out when a cable is attached, and what sort of cable.
Your usb-charging framework would be well placed to help the power_supply to find the correct extcon, and possibly even to handle the registration for events.

Your framework does currently register with extcon, but only listens for EXTCON_USB cables. I don't think that cable type is (reliably) reported when a DCP (for example) is plugged in.

Quoted 2 lines

Here we just listen the plugin/out events from extcon, if one cable plugin it will report to usb charger.

As far as I can tell there are no generic "plugin/out events from extcon". There are only events for a specific cable being plugged or unplugged.

So you need to listed for each different type of cable that you are interested in.

If it detects a "EXTCON_CHG_USB_ACA plugged" event, then it makes sense to use that event to tell the power supply that the current profile for an ACA is available.

Quoted 46 lines

Quoted 28 lines

Here there is another problem that is not of your making, but still needs fixing. Extcon declares a number of cable types like:

/* USB external connector */
#define EXTCON_USB 1
#define EXTCON_USB_HOST 2

/* Charging external connector */

#define EXTCON_CHG_USB_SDP      5       /* Standard Downstream Port */
#define EXTCON_CHG_USB_DCP      6       /* Dedicated Charging Port */
#define EXTCON_CHG_USB_CDP      7       /* Charging Downstream Port */
#define EXTCON_CHG_USB_ACA      8       /* Accessory Charger Adapter */
#define EXTCON_CHG_USB_FAST     9
#define EXTCON_CHG_USB_SLOW     10

However it doesn't define what those mean, so we are left to guess. They each correspond to bits in a bitmap, so a cable can have multiple types. I think the best interpretation is that:

EXTCON_USB means that the cable carries USB data from a host. EXTCON_USB_HOST means that that cable carries USB data to a host. EXTCON_CHG_* means that power is available as described in the standards.
(what EXTCON_CHG_USB_FAST and EXTCON_CHG_USB_SLOW mean is not at all clear).

Quoted 4 lines

There is some support for this in the code, but it is not universally acknowledged. For a USB charging framework to be genuinely useful, it must (I think) make sure this issue gets clarified, and the various cable types used properly.

Here I agree with you, now the usb charger does not listen the charger type from extcon which need to be fixed.

Quoted 9 lines

Once the cable/charger type has been determined, the PMIC power_supply needs to use this information. Your current patches make use of this information in ways that are wrong in two respects.

Firstly, you have made the current limit associated with each cable type
configurable (__usb_charger_set_cur_limit_by_type). This is nonsense.
The standard (e.g. BC-1.2) declares what the current limits are. There is no reason for those not to be hard coded.

Quoted 5 lines

Yes, but you must think about some special cases on some platforms. Users may need to change the current in some situations, thus we should export one API for users to change the current. (I think you misunderstand the current limit here, that is the current for power driver to draw).

Can you be specific about these "special cases" please? I cannot think of any.

And what sort of "user" might use that API? I don't think having an app on your phone that allows you to set the max-current would be a good idea, so you must be thinking of some other sort of user.

Quoted 7 lines

Quoted 4 lines

Secondly, you treat each charger type as having a single "cur_limit" and utilize that limit by telling the PMIC to draw that much current.

This "cur_limit" is set by user or USB configuration what they want, usb charger just tells the power driver how much current need to draw.

You don't tell the power driver "how much current it needs to draw". You tell it "how much current it is safe to draw".

A power driver cannot draw current that isn't available, and it cannot draw current that the system as a whole has no use for.
The task of the power driver is to make sure the system doesn't draw more power than is available, else the voltage will drop and the power supply might shut off.

Quoted 36 lines

Quoted 23 lines

Again, this is inconsistent with the specification.
BC-1.2 defines, for each charger type, a minimum and maximum current level.
The minimum should always be available. Attempting to draw more than
that (but less that the max) might work or might cause the charger to shut down, but you can be sure that the charger will respond to the increased load by first reducing the voltage, and will not shut down until the voltage has dropped below 2V.
If you try to draw more current than the maximum, then the charger might shut down before the voltage drops below 2V.

Given this understanding of the current available from the charger, there are two approaches the PMIC might take.
1/ if the PMIC is able to exercise fine control over the current it
draws, and if it can monitor the voltage on the charger, then it could gradually increase the power being requested until the voltage drops below some threshold (e.g. 4.75V), and then (probably) back off a little. It should only increase at most up to the maximum, even if the voltage remains high. It should probably start at zero rather than at the minimum. This allows for lossage elsewhere.

Quoted 3 lines

2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
control of the current requested, then it should request only the minimum available current. Any more is not safe.

Make sense.

Quoted 6 lines

So your USB charging framework should communicate the MIN and MAX as specified in the standard, and allow the PMIC to use that information as appropriate.
A library routine to support the incremental increase in current drain would probably be appreciated by PMIC driver writers.

Quoted 4 lines

I should say again, the current is set by user or USB configuration what they want, USB charger just tells the power driver how much current need to draw. But we can add some checking if the setting current is in the right range.

The USB configuration tells the Host how much power it needs. If the host enables the configuration, that implicitly say that much power is available, so the power_driver can safely request it. I'm mostly happy wit that aspect of your patch set.

For a dedicated charger there is no USB configuration. The electrical properties of the cable provide some information about how the charger will behave, but they only guarantee a minimum. If you want to use more than the minimum, you have to do so carefully and watch the voltage level. You cannot sensible let the user tell you how much current the charger can provide, because they'll probably get it wrong and the charger will just shut down.

Thanks,
NeilBrown

Quoted 20 lines

Thanks for your comments.

Quoted 11 lines

A more details discussion of this problem-space can be found at
https://lwn.net/Articles/693027/
and an analysis of the functionality currently available in the kernel (which adds a number of specifics to the above) can be found at
https://lwn.net/Articles/694062/

NeilBrown

--
Baolin.wang
Best Regards

Quoted 10 lines · NeilBrown

Quoted 4 lines · Baolin Wang

Now the usb charger will not get charger type from 'extcon' subsystem, we get the charger type from 'power_supply' and calllback 'get_charger_type' for users.

I understand this. I think it is wrong because, in general, the power_supply doesn't know what the charger_type is (it might know it is USB, but most don't know which sort of USB). The PHY knows that, not the power_supply.

I don't think so. Now many platforms will detect the charger type by PMIC hardware, and we can get the charger type by PMIC hardware register. Then power supply driver can access the PMIC register to get the charger type. Here USB charger just considers if the accessing the PMIC register to get charger type is implemented in power supply, it is optional depending on what your platform designed.

Quoted 7 lines

Quoted 6 lines

Yes, but you must think about some special cases on some platforms. Users may need to change the current in some situations, thus we should export one API for users to change the current. (I think you misunderstand the current limit here, that is the current for power driver to draw).

Quoted 2 lines

Can you be specific about these "special cases" please? I cannot think of any.

Suppose the USB configuration requests 100mA, then we should set the USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() funtion, then notify this to power driver.

Quoted 12 lines · Baolin Wang

Quoted 10 lines · NeilBrown

Quoted 4 lines · Baolin Wang

Now the usb charger will not get charger type from 'extcon' subsystem, we get the charger type from 'power_supply' and calllback 'get_charger_type' for users.

I understand this. I think it is wrong because, in general, the power_supply doesn't know what the charger_type is (it might know it is USB, but most don't know which sort of USB). The PHY knows that, not the power_supply.

Quoted 7 lines

I don't think so. Now many platforms will detect the charger type by PMIC hardware, and we can get the charger type by PMIC hardware register. Then power supply driver can access the PMIC register to get the charger type. Here USB charger just considers if the accessing the PMIC register to get charger type is implemented in power supply, it is optional depending on what your platform designed.

In practice, the USB PHY and the Power manager will often be in the same IC (the PMIC) so the driver for one could look at the registers for the other.
But there is no guarantee that the hardware works like that. It is best to create a generally design.

Conceptually, the PHY is separate from the power manager and a solution which recognises that will be more universal.

If the power manager can always just look at that phy registers to know what sort of charger is connected, why does you framework need to work with charger types at all?

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Yes, but you must think about some special cases on some platforms. Users may need to change the current in some situations, thus we should export one API for users to change the current. (I think you misunderstand the current limit here, that is the current for power driver to draw).

Quoted 6 lines

Quoted 2 lines

Can you be specific about these "special cases" please? I cannot think of any.

Suppose the USB configuration requests 100mA, then we should set the USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() funtion, then notify this to power driver.

ahh.... I had missed something there. It's a while since I looked closely at these patches.

Only.... this usage of usb_charger_set_cur_limit_by_type() is really nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then the number negotiated with the USB configuration is not relevant and should be ignored. There is a guaranteed minimum which is at least the maximum that *can* be negotiated.

It is only when the cable appears to be a SDP (standard downstream port) that the usb-config negotiation is relevant. That is because the minimum guaranteed for SDP is only 100mA.

NeilBrown

Quoted 6 lines

In practice, the USB PHY and the Power manager will often be in the same IC (the PMIC) so the driver for one could look at the registers for the other.
But there is no guarantee that the hardware works like that. It is best to create a generally design.

Yes, we hope to create one generally design, so we need to consider this situation: the power supply getting the charger type by accessing PMIC registers. The registers which save the charger type are not always belong to the USB PHY, may be just some registers on PMIC.

Now in mainline kernel, there are 3 methods can get the charger type which need to integrate with USB charger framework:
1. power supply
2. extcon (need to add as you suggested)
3. others (by 'get_charger_type' callback of USB charger)

Quoted 8 lines

Quoted 4 lines

Suppose the USB configuration requests 100mA, then we should set the USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() funtion, then notify this to power driver.

ahh.... I had missed something there. It's a while since I looked closely at these patches.

Quoted 7 lines

Only.... this usage of usb_charger_set_cur_limit_by_type() is really nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then the number negotiated with the USB configuration is not relevant and should be ignored. There is a guaranteed minimum which is at least the maximum that *can* be negotiated.

Yes. If it is not relevant, we will no't set the current from USB configuration. Just when your charger type is SDP and the USB enumeration is done, we can get the USB configuration from host to set current.

Quoted 11 lines · Baolin Wang

Quoted 6 lines

In practice, the USB PHY and the Power manager will often be in the same IC (the PMIC) so the driver for one could look at the registers for the other.
But there is no guarantee that the hardware works like that. It is best to create a generally design.

Yes, we hope to create one generally design, so we need to consider this situation: the power supply getting the charger type by accessing PMIC registers. The registers which save the charger type are not always belong to the USB PHY, may be just some registers on PMIC.

If the power_supply can directly detect the type of charger, then it doesn't need any usb-charger-infrastructure to tell it. It can handle current selection entirely internally.
Surely the only interesting case for a framework to address is the one where the power_supply cannot directly detect the charger type.

Quoted 7 lines

Now in mainline kernel, there are 3 methods can get the charger type which need to integrate with USB charger framework:
1. power supply
2. extcon (need to add as you suggested)
3. others (by 'get_charger_type' callback of USB charger)

There is no "get_charger_type" now in the mainline kernel.

Quoted 17 lines

Quoted 9 lines

Quoted 5 lines

Suppose the USB configuration requests 100mA, then we should set the USB charger current is 100mA by __usb_charger_set_cur_limit_by_type() funtion, then notify this to power driver.

ahh.... I had missed something there. It's a while since I looked closely at these patches.

Quoted 7 lines

Only.... this usage of usb_charger_set_cur_limit_by_type() is really nonsensical.

If the cable is detected as being DCP or CDP or ACA (or ACA/DOCK) then the number negotiated with the USB configuration is not relevant and should be ignored. There is a guaranteed minimum which is at least the maximum that *can* be negotiated.

Quoted 4 lines

Yes. If it is not relevant, we will no't set the current from USB configuration. Just when your charger type is SDP and the USB enumeration is done, we can get the USB configuration from host to set current.

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

Quoted 6 lines

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

Quoted 1 line

So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number from the USB configuration ), it will not do the USB enumeration, then no USB configuration from host to set current.

From the talking, there are some issues (thanks for Neil's comments)

--
Baolin.wang
Best Regards

Quoted 11 lines

Quoted 7 lines

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number from the USB configuration ), it will not do the USB enumeration, then no USB configuration from host to set current.

From the talking, there are some issues (thanks for Neil's comments) need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum current or minimum current to power driver.

Now the current v16 patchset can work well on my Spreadtrum platform and Jun's NXP platform, if you like to apply this patchset then I can send out new patches to fix above issues. If you don't like that, I can send out new version patchset to fix above issues. Could you give me some suggestions what should I do next step? Thanks.

Hi Baolin,

Quoted 19 lines · Baolin Wang

Quoted 11 lines

Quoted 7 lines

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number from the USB configuration ), it will not do the USB enumeration, then no USB configuration from host to set current.

From the talking, there are some issues (thanks for Neil's comments) need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum current or minimum current to power driver.

Quoted 5 lines

Now the current v16 patchset can work well on my Spreadtrum platform and Jun's NXP platform, if you like to apply this patchset then I can send out new patches to fix above issues. If you don't like that, I can send out new version patchset to fix above issues. Could you give me some suggestions what should I do next step? Thanks.

Merge window just opened, nothing will happen for about 2 weeks. How about you send a new version after merge window closes and we go from there? Fixing 1 and 2 is needed. 3 we need to consider more

carefully. Perhaps report both minimum and maximum somehow?

Neil, comments?

--
balbi

Hi Baolin,

Quoted 21 lines · Baolin Wang

Quoted 11 lines

Quoted 7 lines

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number from the USB configuration ), it will not do the USB enumeration, then no USB configuration from host to set current.

From the talking, there are some issues (thanks for Neil's comments) need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum current or minimum current to power driver.

Now the current v16 patchset can work well on my Spreadtrum platform and Jun's NXP platform, if you like to apply this patchset then I can

Quoted 3 lines

send out new patches to fix above issues. If you don't like that, I can send out new version patchset to fix above issues. Could you give me some suggestions what should I do next step? Thanks.

Merge window just opened, nothing will happen for about 2 weeks. How about you send a new version after merge window closes and we go from there? Fixing 1 and 2 is needed. 3 we need to consider more carefully. Perhaps report both minimum and maximum somehow?

Neil, comments?

Quoted 1 line

1. Need to add the method getting charger type from extcon subsystem.

Quoted 1 line

2. Need to remove the method getting charger type from power supply.

Quoted 2 lines

3. There are still some different views about reporting the maximum current or minimum current to power driver.

Signature · 2 lines

-- 
balbi

Quoted 22 lines · Felipe Balbi

Hi Baolin,

Quoted 19 lines · Baolin Wang

Quoted 11 lines

Quoted 7 lines

But you do!
The mA number from the USB configuration is passed to usb_gadget_vbus_draw. Your patch passes that to usb_charger_set_cur_limit_by_type() which calls __usb_charger_set_cur_limit_by_type() which will set the cur_limit for whichever type uchger->type currently is.

So when it is not relevant, your code *does* set some current limit.

Suppose the charger type is DCP(it is not relevant to the mA number from the USB configuration ), it will not do the USB enumeration, then no USB configuration from host to set current.

From the talking, there are some issues (thanks for Neil's comments) need to be fixed as below:
1. Need to add the method getting charger type from extcon subsystem.
2. Need to remove the method getting charger type from power supply.
3. There are still some different views about reporting the maximum current or minimum current to power driver.

Quoted 2 lines

Quoted 2 lines

Now the current v16 patchset can work well on my Spreadtrum platform and Jun's NXP platform, if you like to apply this patchset then I can

I'm really curious how much testing this has had. Have you actually plugged in different cable types (SDP DCP DCP ACA) and has each one been detected correctly? Because I cannot see how that could happen with the code you have posted.

Quoted 10 lines

Quoted 4 lines

send out new patches to fix above issues. If you don't like that, I can send out new version patchset to fix above issues. Could you give me some suggestions what should I do next step? Thanks.

Merge window just opened, nothing will happen for about 2 weeks. How about you send a new version after merge window closes and we go from there? Fixing 1 and 2 is needed. 3 we need to consider more carefully. Perhaps report both minimum and maximum somehow?

Quoted 1 line

Neil, comments?

This probably seems a bit harsh, but I really think the current patchset should be discarded and the the project started again with a clear vision of what is required. What we currently have is too confused.

To respond to the points:

Quoted 1 line

Quoted 1 line

1. Need to add the method getting charger type from extcon subsystem.

Yes. This should be the only way to get the charger type.

Quoted 2 lines

Quoted 2 lines

2. Need to remove the method getting charger type from power supply.

Also need to remove the ->get_charger_type() method as there is no credible use-case for this.

Quoted 3 lines

Quoted 3 lines

3. There are still some different views about reporting the maximum current or minimum current to power driver.

I think those were resolved. There was some confusion over whether a particular power manager wanted to be told the maximum or the minimum, but I think both have a clear use case in different hardware.

Also: We don't want another notifier_chain. The usb_notifier combined with the extcon notifier are sufficient. Possibly it would be sensible to replace the usb notifier with a new new notifier chain, but don't add something without first cleaning up what is there.

Also: resolve the question of whether it could ever make sense to have
more than one "usb_charger" in a system. If it doesn't, make it an obvious singleton. If it does, make it clear how the correct usb_charger is chosen.

Also: think very carefully before exposing any details through sysfs.
Some of the details are already visible, either in sys/class/extcon or sys/class/power_supply. Don't duplicate without good reason.

Conceptually, the PHY is separate from the power manager and a solution which recognises that will be more universal.

[ Unknown signature status ]

Quoted 2 lines · NeilBrown

Conceptually, the PHY is separate from the power manager and a solution which recognises that will be more universal.

The wm831x driver in the patch series is an example of such hardware - it is purely a power manager, it has no USB PHY hardware at all. It's a current limiter intended to sit in line with the USB power lines to ensure the system doesn't go over the maximum current draw (and also integrates with the power source selection logic the chip has to pick the best available power source for the system).

Quoted 2 lines · Mark Brown

The wm831x driver in the patch series is an example of such hardware - it is purely a power manager, it has no USB PHY hardware at all. It's a

The "probe" routine calls

+ usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Presumably wm831x_pdata is initialised by a board file?

I strongly suspect it is initialized to "usb-charger.0" because the names given to usb chargers are "usb-charger.%d" in discovery order. I don't see this being at all useful if there is ever more than one usb-charger.
Do you?

So how can this wm831x driver actually find out what sort of charger is connected and so set the power limit? I just don't see this working *at* *all*.

[ Unknown signature status ]

Quoted 10 lines · NeilBrown

Quoted 2 lines · Mark Brown

The wm831x driver in the patch series is an example of such hardware - it is purely a power manager, it has no USB PHY hardware at all. It's a

The "probe" routine calls

+ usb_charger_find_by_name(wm831x_pdata->usb_gadget);

Presumably wm831x_pdata is initialised by a board file?

Yes.

Quoted 5 lines

I strongly suspect it is initialized to "usb-charger.0" because the names given to usb chargers are "usb-charger.%d" in discovery order. I don't see this being at all useful if there is ever more than one usb-charger.
Do you?

It's no worse than any other board file situation - if someone has that problem they get to fix it.

Quoted 4 lines

So how can this wm831x driver actually find out what sort of charger is connected and so set the power limit? I just don't see this working *at* *all*.

The whole point from the point of view of the wm831x driver is that it just wants something to tell it how much current it's allowed to draw, I appreciate that doesn't change your analysis of the bit in the middle but the consumer driver bit seems fine here.

Quoted 2 lines · Mark Brown

It's no worse than any other board file situation - if someone has that problem they get to fix it.

My point is that the present design does not appear to scale beyond a single USB power supply (as if there were two, they would be named in discovery order, which is not reliably stable).

Your point is, I think, that when someone actually cares about that lack of scaling, they can fix it.

I am perfectly happy with that approach. However if the code doesn't scale beyond one charger, it shouldn't pretend that it does. i.e.
Don't have "usb_charger_find_by_name()", just a global "usb_charger" (or similar).
The first charger to register gets to be the "usb_charger". The second one gets an error.
I could be quite happy with that sort of interface.

Quoted 4 lines

The whole point from the point of view of the wm831x driver is that it just wants something to tell it how much current it's allowed to draw, I appreciate that doesn't change your analysis of the bit in the middle but the consumer driver bit seems fine here.

Yes, the wm831x driver probably does the right thing.
Other drivers might want to know not only the minimum they are allowed to draw, but also the maximum they should try even if they are carefully monitoring the voltage.
So wm831x is doing the right thing with the wrong interface. Maybe you can describe that as "fine".

[ Unknown signature status ]

Quoted 8 lines · NeilBrown

Quoted 2 lines · Mark Brown

It's no worse than any other board file situation - if someone has that problem they get to fix it.

My point is that the present design does not appear to scale beyond a single USB power supply (as if there were two, they would be named in discovery order, which is not reliably stable).

For the practical purposes of people making systems (as opposed to upstream where this is likely to get most use) it pretty much is. Though quite how many systems have multiple chargers is itself also a question.

Quoted 2 lines

Your point is, I think, that when someone actually cares about that lack of scaling, they can fix it.

Yes.

Quoted 8 lines

I am perfectly happy with that approach. However if the code doesn't scale beyond one charger, it shouldn't pretend that it does. i.e.
Don't have "usb_charger_find_by_name()", just a global "usb_charger" (or similar).
The first charger to register gets to be the "usb_charger". The second one gets an error.
I could be quite happy with that sort of interface.

Well, a fairly standard way of extending would be to allow the explicit assignment of names to chargers so this'd avoid such churn.

Quoted 6 lines

Quoted 5 lines

The whole point from the point of view of the wm831x driver is that it just wants something to tell it how much current it's allowed to draw, I appreciate that doesn't change your analysis of the bit in the middle but the consumer driver bit seems fine here.

Quoted 6 lines

Yes, the wm831x driver probably does the right thing.
Other drivers might want to know not only the minimum they are allowed to draw, but also the maximum they should try even if they are carefully monitoring the voltage.
So wm831x is doing the right thing with the wrong interface. Maybe you can describe that as "fine".

That's not actually 100% clear to me - for what the wm831x is doing it probably *does* want the higher limit. This is a system inflow limit (as it should be for this), at least the charger will adapt to voltage variations though other users in the system are much less likely to do so.

Quoted 5 lines · Mark Brown

That's not actually 100% clear to me - for what the wm831x is doing it probably *does* want the higher limit. This is a system inflow limit (as it should be for this), at least the charger will adapt to voltage variations though other users in the system are much less likely to do so.

Not having very much electrical engineering background, I cannot say for sure what will happen, but it seems likely that once the voltage drops much below 4.75V, the charger won't be operating at peak efficiency, which would be a waste.
I can easily imagine that the hardware would switch off at some voltage level, rather than just making do with what is there.
So I'm skeptical of this approach, but I'm open to being corrected by someone more knowledgeable than I.

Looking at it from a different perspective, according to the patch set, the limits that wm831x is able to impose are:

+ 0,
+ 2,
+ 100,
+ 500,
+ 900,
+ 1500,
+ 1800,
+ 550,

These are, from the battery charger spec, minimums rather than maximums. e.g. a CDP provides at least 1500, and as much as 5000. So it seems that the wm831x was designed to be told the minimum guaranteed available. But that is circumstantial evidence and might be misleading.

[ Unknown signature status ]

Quoted 8 lines · NeilBrown

Quoted 5 lines · Mark Brown

That's not actually 100% clear to me - for what the wm831x is doing it probably *does* want the higher limit. This is a system inflow limit (as it should be for this), at least the charger will adapt to voltage variations though other users in the system are much less likely to do so.

Quoted 8 lines

Not having very much electrical engineering background, I cannot say for sure what will happen, but it seems likely that once the voltage drops much below 4.75V, the charger won't be operating at peak efficiency, which would be a waste.
I can easily imagine that the hardware would switch off at some voltage level, rather than just making do with what is there.
So I'm skeptical of this approach, but I'm open to being corrected by someone more knowledgeable than I.

Yes, the idea is that the charger will back off charging and stop entirely if the rest of the system is consuming too much power to allow it to continue effectively. The same thing happens with wall power, if a wall supply isn't able to power the charger (eg, because the rest of the system is running flat out) it'll have to cope with that.

Quoted 13 lines

Looking at it from a different perspective, according to the patch set, the limits that wm831x is able to impose are:

+ 0,
+ 2,
+ 100,
+ 500,
+ 900,
+ 1500,
+ 1800,
+ 550,

Quoted 4 lines

These are, from the battery charger spec, minimums rather than maximums. e.g. a CDP provides at least 1500, and as much as 5000. So it seems that the wm831x was designed to be told the minimum guaranteed available. But that is circumstantial evidence and might be misleading.

AIUI this is conservatisim in the system design - another way of reading a spec with a range like this is that the consumer should aim for the lower limit, the provider should aim for the upper limit and that way if either of them has issues with tolerances things will still work out. It predates wide availability of CDP so I wouldn't be surprised if that bit were just an oversight. Like I say this bit of hardware is totally separate to the battery charger.

Quoted 5 lines · Mark Brown

Yes, the idea is that the charger will back off charging and stop entirely if the rest of the system is consuming too much power to allow it to continue effectively. The same thing happens with wall power, if a wall supply isn't able to power the charger (eg, because the rest of the system is running flat out) it'll have to cope with that.

Maybe you are correct. I don't find your argument convincing, but maybe that is because I don't want to...

1/ I had a report once from someone whose device stopped charging because it was pulling more current than the charger could supply. The voltage dropped below the 3.5V (I think) that the battery charging hardware needed, so it switched off. It wouldn't switch back on again until explicitly told too. It would then overload the charger again and switch off.

Which seems to say the maximum is just for safety, implying that the minimum is the important value.

3/ Felipe Balbi <[email protected]> appears to agree with my
perspective.

      http://www.mail-archive.com/[email protected]/msg1224904.html

does argument-by-authority work?

TI do a lot of the more software managed chargers (which I suspect are the main thing Felipe will have looked at) if that's what you're referring to here? The device is implementing pretty much the algorithm you're describing in that e-mail so I'm a bit confused as to what you're saying here.

It's a
current limiter intended to sit in line with the USB power lines to ensure the system doesn't go over the maximum current draw (and also integrates with the power source selection logic the chip has to pick the best available power source for the system).

On Wed, Sep 14 2016, Mark Brown wrote:

Ah.... my mistake, sorry.
When earlier you said:

Quoted 2 lines

It's a
current limiter intended to sit in line with the USB power lines to

I assumed that all it did was limit the current to number given. If it also limits the current to ensure that voltage doesn't drop unduly, then I agree with your assertion that it just needs to be told the upper limit.

I hope you'll agree that other drivers might need to know the lower limit, so reporting both to all drivers is sensible.

Quoted 5 lines

That's not actually 100% clear to me - for what the wm831x is doing it probably *does* want the higher limit. This is a system inflow limit (as it should be for this), at least the charger will adapt to voltage variations though other users in the system are much less likely to do so.

Interesting ... I hadn't considered that possibility.

As long as the current remains below the maximum, the charger will reduce the voltage towards 2V as load increases. Somewhere before it gets there, the system will not be able to make use of the power as the voltage will be too low to be usable. So that will naturally limit the current being drawn.

Not having very much electrical engineering background, I cannot say for sure what will happen, but it seems likely that once the voltage drops much below 4.75V, the charger won't be operating at peak efficiency, which would be a waste.
I can easily imagine that the hardware would switch off at some voltage level, rather than just making do with what is there.
So I'm skeptical of this approach, but I'm open to being corrected by someone more knowledgeable than I.