mbox series

[v3,00/10] power: supply: max77693: Toggle charging/OTG based on extcon status

Message ID 20240729-max77693-charger-extcon-v3-0-02315a6869d4@gmail.com
Headers show
Series power: supply: max77693: Toggle charging/OTG based on extcon status | expand

Message

Artur Weber July 29, 2024, 5:47 p.m. UTC
This patchset does the following:

- Add CURRENT_MAX and INPUT_CURRENT_MAX power supply properties to
  expose the "fast charge current" (maximum current from charger to
  battery) and "CHGIN input current limit" (maximum current from
  external supply to charger).

- Add functions for toggling charging and OTG modes.

- Add an extcon-based handler that enables charging or OTG depending
  on the cable type plugged in. The extcon device to use for cable
  detection can be specified in the device tree, and is entirely
  optional.

The extcon listener implementation is inspired by the rt5033 charger
driver (commit 8242336dc8a8 ("power: supply: rt5033_charger: Add cable
detection and USB OTG supply")).

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
v3 no longer uses the CHARGER regulator to manage the power status, and
that's for two reasons:

- Regulator enable/disable behavior was interfering with how the power
  supply driver worked (we occasionally got "unbalanced disables"
  errors when switching charging state, despite checking for the
  regulator status with regulator_is_enabled() - the CHARGER reg would
  report as enabled despite the enable count being 0).
  This broke OTG insertion if the OTG cable was plugged in first, and
  sometimes caused warnings on unsuspend.

- Changing the charging values directly in the power supply driver is
  less opaque and lets us avoid bringing in a dependency on regulators.

It also splits the current limits back into two properties:
INPUT_CURRENT_LIMIT and CONSTANT_CHARGE_CURRENT_MAX. Again, there are
two reasons for this split:

- They are two separate current controls, one for USB->charger and one
  for charger->battery, and they have different limits (0-2.1A for CC
  vs 60mA-2.58A for input). Given that the power supply core has the
  properties for both values separately, it's more logical to present
  them as such.

- It's safer to keep these separate; CONSTANT_CHARGE_CURRENT_MAX is
  pretty explicitly only set *once* - at probe time with a safe value
  specified in the DT. This way, INPUT_CURRENT_LIMIT is safer to modify
  since in the event of an invalid value the CC current will hold back
  the extra current thus preventing damage to the battery.

The latter is relevant as I'm working on a follow-up patchset that
allows for controlling the charging parameters using power supply
properties/sysfs properties rather than the CHARGER regulator.

Note that the CHARGER regulator gets disabled automatically if it's
not used, which will disable charging if it was auto-enabled by the
extcon code. This can be worked around by re-attaching the cable, or
more properly by removing the CHARGER regulator from DT for devices
that use the extcon-based charger management, as has been done in the
Galaxy Tab 3 8.0 DTSI.

See v1 for old description:

https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com
---
Changes in v3:
- Drop uses of CHARGER regulator, manage registers directly in power
  supply driver instead
- Link to v2: https://lore.kernel.org/r/20240715-max77693-charger-extcon-v2-0-0838ffbb18c3@gmail.com

Changes in v2:
- Changed to use monitored-battery for charge current value
- Both current limit variables are now set by the CHARGER regulator
- Link to v1: https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com

---
Artur Weber (10):
      dt-bindings: power: supply: max77693: Add monitored-battery property
      dt-bindings: power: supply: max77693: Add maxim,usb-connector property
      power: supply: max77693: Expose input current limit and CC current properties
      power: supply: max77693: Set charge current limits during init
      power: supply: max77693: Add USB extcon detection for enabling charging
      power: supply: max77693: Add support for detecting and enabling OTG
      power: supply: max77693: Set up charge/input current according to cable type
      ARM: dts: samsung: exynos4212-tab3: Add battery node with charge current value
      ARM: dts: samsung: exynos4212-tab3: Add USB connector node
      ARM: dts: exynos4212-tab3: Drop CHARGER regulator

 .../bindings/power/supply/maxim,max77693.yaml      |  15 +
 arch/arm/boot/dts/samsung/exynos4212-tab3.dtsi     |  22 +-
 drivers/power/supply/Kconfig                       |   1 +
 drivers/power/supply/max77693_charger.c            | 302 ++++++++++++++++++++-
 include/linux/mfd/max77693-private.h               |  12 +
 5 files changed, 337 insertions(+), 15 deletions(-)
---
base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
change-id: 20240525-max77693-charger-extcon-9ebb7bad83ce

Best regards,

Comments

Henrik Grimler Aug. 1, 2024, 6:22 a.m. UTC | #1
Hi Artur,

On Mon, Jul 29, 2024 at 07:47:34PM +0200, Artur Weber wrote:
> This patchset does the following:
> 
> - Add CURRENT_MAX and INPUT_CURRENT_MAX power supply properties to
>   expose the "fast charge current" (maximum current from charger to
>   battery) and "CHGIN input current limit" (maximum current from
>   external supply to charger).
> 
> - Add functions for toggling charging and OTG modes.
> 
> - Add an extcon-based handler that enables charging or OTG depending
>   on the cable type plugged in. The extcon device to use for cable
>   detection can be specified in the device tree, and is entirely
>   optional.
> 
> The extcon listener implementation is inspired by the rt5033 charger
> driver (commit 8242336dc8a8 ("power: supply: rt5033_charger: Add cable
> detection and USB OTG supply")).

Tested on exynos4412-i9305 (after applying the changes in patch 8 - 10
to exynos4412-midas.dtsi).  It works well, device correctly identifies
a usb cable connected to charger or a usb cable connected to computer,
and sets a limit of 1.8 A and 0.5 A in the two cases.

I did notice that device does not always detect cable insertion, so I
can occassionally get two de-attach events in a row.  Cable was
inserted between 428 and 462 in below log snippet:

[  389.458399] max77693-muic max77693-muic: external connector is attached(chg_type:0x3, prev_chg_type:0x3)
[  389.469765] max77693-charger max77693-charger: fast charging. connector type: 6
[  428.151857] max77693-muic max77693-muic: external connector is detached(chg_type:0x3, prev_chg_type:0x0)
[  428.160319] max77693-charger max77693-charger: not charging. connector type: 13
[  462.156048] max77693-muic max77693-muic: external connector is detached(chg_type:0x0, prev_chg_type:0x0)
[  469.881925] max77693-muic max77693-muic: external connector is attached(chg_type:0x3, prev_chg_type:0x3)
[  469.890049] max77693-charger max77693-charger: fast charging. connector type: 6

but this is probably an issue in extcon driver though rather than
charger.

I have not tested so that MHL still works, as I do not have access to
that cable at the moment, will try it in a few days.

Best regards
Henrik Grimler

> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> 
> v3 no longer uses the CHARGER regulator to manage the power status, and
> that's for two reasons:
> 
> - Regulator enable/disable behavior was interfering with how the power
>   supply driver worked (we occasionally got "unbalanced disables"
>   errors when switching charging state, despite checking for the
>   regulator status with regulator_is_enabled() - the CHARGER reg would
>   report as enabled despite the enable count being 0).
>   This broke OTG insertion if the OTG cable was plugged in first, and
>   sometimes caused warnings on unsuspend.
> 
> - Changing the charging values directly in the power supply driver is
>   less opaque and lets us avoid bringing in a dependency on regulators.
> 
> It also splits the current limits back into two properties:
> INPUT_CURRENT_LIMIT and CONSTANT_CHARGE_CURRENT_MAX. Again, there are
> two reasons for this split:
> 
> - They are two separate current controls, one for USB->charger and one
>   for charger->battery, and they have different limits (0-2.1A for CC
>   vs 60mA-2.58A for input). Given that the power supply core has the
>   properties for both values separately, it's more logical to present
>   them as such.
> 
> - It's safer to keep these separate; CONSTANT_CHARGE_CURRENT_MAX is
>   pretty explicitly only set *once* - at probe time with a safe value
>   specified in the DT. This way, INPUT_CURRENT_LIMIT is safer to modify
>   since in the event of an invalid value the CC current will hold back
>   the extra current thus preventing damage to the battery.
> 
> The latter is relevant as I'm working on a follow-up patchset that
> allows for controlling the charging parameters using power supply
> properties/sysfs properties rather than the CHARGER regulator.
> 
> Note that the CHARGER regulator gets disabled automatically if it's
> not used, which will disable charging if it was auto-enabled by the
> extcon code. This can be worked around by re-attaching the cable, or
> more properly by removing the CHARGER regulator from DT for devices
> that use the extcon-based charger management, as has been done in the
> Galaxy Tab 3 8.0 DTSI.
> 
> See v1 for old description:
> 
> https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com
> ---
> Changes in v3:
> - Drop uses of CHARGER regulator, manage registers directly in power
>   supply driver instead
> - Link to v2: https://lore.kernel.org/r/20240715-max77693-charger-extcon-v2-0-0838ffbb18c3@gmail.com
> 
> Changes in v2:
> - Changed to use monitored-battery for charge current value
> - Both current limit variables are now set by the CHARGER regulator
> - Link to v1: https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com
> 
> ---
> Artur Weber (10):
>       dt-bindings: power: supply: max77693: Add monitored-battery property
>       dt-bindings: power: supply: max77693: Add maxim,usb-connector property
>       power: supply: max77693: Expose input current limit and CC current properties
>       power: supply: max77693: Set charge current limits during init
>       power: supply: max77693: Add USB extcon detection for enabling charging
>       power: supply: max77693: Add support for detecting and enabling OTG
>       power: supply: max77693: Set up charge/input current according to cable type
>       ARM: dts: samsung: exynos4212-tab3: Add battery node with charge current value
>       ARM: dts: samsung: exynos4212-tab3: Add USB connector node
>       ARM: dts: exynos4212-tab3: Drop CHARGER regulator
> 
>  .../bindings/power/supply/maxim,max77693.yaml      |  15 +
>  arch/arm/boot/dts/samsung/exynos4212-tab3.dtsi     |  22 +-
>  drivers/power/supply/Kconfig                       |   1 +
>  drivers/power/supply/max77693_charger.c            | 302 ++++++++++++++++++++-
>  include/linux/mfd/max77693-private.h               |  12 +
>  5 files changed, 337 insertions(+), 15 deletions(-)
> ---
> base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
> change-id: 20240525-max77693-charger-extcon-9ebb7bad83ce
> 
> Best regards,
> -- 
> Artur Weber <aweber.kernel@gmail.com>
>
Henrik Grimler Aug. 4, 2024, 10:59 a.m. UTC | #2
Hi again Artur,

On Thu, Aug 01, 2024 at 08:23:26AM +0200, Henrik Grimler wrote:
> Hi Artur,
> 
> On Mon, Jul 29, 2024 at 07:47:34PM +0200, Artur Weber wrote:
> > This patchset does the following:
> > 
> > - Add CURRENT_MAX and INPUT_CURRENT_MAX power supply properties to
> >   expose the "fast charge current" (maximum current from charger to
> >   battery) and "CHGIN input current limit" (maximum current from
> >   external supply to charger).
> > 
> > - Add functions for toggling charging and OTG modes.
> > 
> > - Add an extcon-based handler that enables charging or OTG depending
> >   on the cable type plugged in. The extcon device to use for cable
> >   detection can be specified in the device tree, and is entirely
> >   optional.
> > 
> > The extcon listener implementation is inspired by the rt5033 charger
> > driver (commit 8242336dc8a8 ("power: supply: rt5033_charger: Add cable
> > detection and USB OTG supply")).
> 
> Tested on exynos4412-i9305 (after applying the changes in patch 8 - 10
> to exynos4412-midas.dtsi).  It works well, device correctly identifies
> a usb cable connected to charger or a usb cable connected to computer,
> and sets a limit of 1.8 A and 0.5 A in the two cases.
> 
> I did notice that device does not always detect cable insertion, so I
> can occassionally get two de-attach events in a row.  Cable was
> inserted between 428 and 462 in below log snippet:
> 
> [  389.458399] max77693-muic max77693-muic: external connector is attached(chg_type:0x3, prev_chg_type:0x3)
> [  389.469765] max77693-charger max77693-charger: fast charging. connector type: 6
> [  428.151857] max77693-muic max77693-muic: external connector is detached(chg_type:0x3, prev_chg_type:0x0)
> [  428.160319] max77693-charger max77693-charger: not charging. connector type: 13
> [  462.156048] max77693-muic max77693-muic: external connector is detached(chg_type:0x0, prev_chg_type:0x0)
> [  469.881925] max77693-muic max77693-muic: external connector is attached(chg_type:0x3, prev_chg_type:0x3)
> [  469.890049] max77693-charger max77693-charger: fast charging. connector type: 6
> 
> but this is probably an issue in extcon driver though rather than
> charger.
> 
> I have not tested so that MHL still works, as I do not have access to
> that cable at the moment, will try it in a few days.

MHL now tested on exynos4412-i9300 as well.  It works, and the series
fixes so that we can hotplug the cable (with a few patches to make
sii9324 use extcon as well), before we had to connect cable before
boot and rely on bootloader to setup everything.  Thanks!

> > Signed-off-by: Artur Weber <aweber.kernel@gmail.com>

Tested-by: Henrik Grimler <henrik@grimler.se>

Best regards,
Henrik Grimler

> > v3 no longer uses the CHARGER regulator to manage the power status, and
> > that's for two reasons:
> > 
> > - Regulator enable/disable behavior was interfering with how the power
> >   supply driver worked (we occasionally got "unbalanced disables"
> >   errors when switching charging state, despite checking for the
> >   regulator status with regulator_is_enabled() - the CHARGER reg would
> >   report as enabled despite the enable count being 0).
> >   This broke OTG insertion if the OTG cable was plugged in first, and
> >   sometimes caused warnings on unsuspend.
> > 
> > - Changing the charging values directly in the power supply driver is
> >   less opaque and lets us avoid bringing in a dependency on regulators.
> > 
> > It also splits the current limits back into two properties:
> > INPUT_CURRENT_LIMIT and CONSTANT_CHARGE_CURRENT_MAX. Again, there are
> > two reasons for this split:
> > 
> > - They are two separate current controls, one for USB->charger and one
> >   for charger->battery, and they have different limits (0-2.1A for CC
> >   vs 60mA-2.58A for input). Given that the power supply core has the
> >   properties for both values separately, it's more logical to present
> >   them as such.
> > 
> > - It's safer to keep these separate; CONSTANT_CHARGE_CURRENT_MAX is
> >   pretty explicitly only set *once* - at probe time with a safe value
> >   specified in the DT. This way, INPUT_CURRENT_LIMIT is safer to modify
> >   since in the event of an invalid value the CC current will hold back
> >   the extra current thus preventing damage to the battery.
> > 
> > The latter is relevant as I'm working on a follow-up patchset that
> > allows for controlling the charging parameters using power supply
> > properties/sysfs properties rather than the CHARGER regulator.
> > 
> > Note that the CHARGER regulator gets disabled automatically if it's
> > not used, which will disable charging if it was auto-enabled by the
> > extcon code. This can be worked around by re-attaching the cable, or
> > more properly by removing the CHARGER regulator from DT for devices
> > that use the extcon-based charger management, as has been done in the
> > Galaxy Tab 3 8.0 DTSI.
> > 
> > See v1 for old description:
> > 
> > https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com
> > ---
> > Changes in v3:
> > - Drop uses of CHARGER regulator, manage registers directly in power
> >   supply driver instead
> > - Link to v2: https://lore.kernel.org/r/20240715-max77693-charger-extcon-v2-0-0838ffbb18c3@gmail.com
> > 
> > Changes in v2:
> > - Changed to use monitored-battery for charge current value
> > - Both current limit variables are now set by the CHARGER regulator
> > - Link to v1: https://lore.kernel.org/r/20240530-max77693-charger-extcon-v1-0-dc2a9e5bdf30@gmail.com
> > 
> > ---
> > Artur Weber (10):
> >       dt-bindings: power: supply: max77693: Add monitored-battery property
> >       dt-bindings: power: supply: max77693: Add maxim,usb-connector property
> >       power: supply: max77693: Expose input current limit and CC current properties
> >       power: supply: max77693: Set charge current limits during init
> >       power: supply: max77693: Add USB extcon detection for enabling charging
> >       power: supply: max77693: Add support for detecting and enabling OTG
> >       power: supply: max77693: Set up charge/input current according to cable type
> >       ARM: dts: samsung: exynos4212-tab3: Add battery node with charge current value
> >       ARM: dts: samsung: exynos4212-tab3: Add USB connector node
> >       ARM: dts: exynos4212-tab3: Drop CHARGER regulator
> > 
> >  .../bindings/power/supply/maxim,max77693.yaml      |  15 +
> >  arch/arm/boot/dts/samsung/exynos4212-tab3.dtsi     |  22 +-
> >  drivers/power/supply/Kconfig                       |   1 +
> >  drivers/power/supply/max77693_charger.c            | 302 ++++++++++++++++++++-
> >  include/linux/mfd/max77693-private.h               |  12 +
> >  5 files changed, 337 insertions(+), 15 deletions(-)
> > ---
> > base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
> > change-id: 20240525-max77693-charger-extcon-9ebb7bad83ce
> > 
> > Best regards,
> > -- 
> > Artur Weber <aweber.kernel@gmail.com>
> >