Message-ID: <20240804105937.GA20421@l14.localdomain>
Date: Sun, 4 Aug 2024 12:59:37 +0200
From: Henrik Grimler <henrik@...mler.se>
To: Artur Weber <aweber.kernel@...il.com>
Cc: Krzysztof Kozlowski <krzk@...nel.org>,
	Chanwoo Choi <cw00.choi@...sung.com>,
	Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Alim Akhtar <alim.akhtar@...sung.com>, linux-pm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org,
	~postmarketos/upstreaming@...ts.sr.ht,
	Wolfgang Wiedmeyer <wolfgit@...dmeyer.de>,
	Denis 'GNUtoo' Carikli <GNUtoo@...erdimension.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v3 00/10] power: supply: max77693: Toggle charging/OTG
 based on extcon status

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@...il.com>

Tested-by: Henrik Grimler <henrik@...mler.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@...il.com>
> > 

Powered by blists - more mailing lists