[PATCH 00/10] power: supply: bq25890: Fixes for 6.2 + further work for 6.3

[PATCH 00/10] power: supply: bq25890: Fixes for 6.2 + further work for 6.3

[Hans de Goede]

Hi Sebastian, Marek,

I have been working on getting a Lenovo Yoga Tab 3 Pro (YT3-X90F) to
work with the mainline kernel. This tablet has 2 batteries with
2 bq25892 chargers both connected to a single Micro-USB connector.

Supporting the 2 charger board also requires the recent HiZ mode patches
from Marek, to avoid merging order problems / conflicts I have included
a copy of Marek's series here so this series obsoletes the:

[PATCH 1/2] power: supply: bq25890: Factor out chip state update
[PATCH 2/2] power: supply: bq25890: Add HiZ mode support
[PATCH v2 1/2] power: supply: bq25890: Factor out chip state update
[PATCH v2 2/2] power: supply: bq25890: Add HiZ mode support

patches from Marek.

While working on adding support for this I also noticed some generic issues
with the bq25890 driver currently in linux-power-supply/for-next and I also
have some fixes to the HiZ support on top of Marek's support.

So this entire series consist of 4 parts:

1. Patches 1-3:

Generic bugfixes for the bq25890 charger in its current state
in linux-power-supply/for-next. Patch 1/10 fixes an actual regression on
some boards with for-next so that one definitely needs to go to 6.2.
The other 2 fixes can go to either 6.2 or 6.3

2. Patches 4-5:

Marek's HiZ support work, thank you Marek.

3. Patches 6-7:

Some fixes / improvements from me to Marek's HiZ support.

4. Patch 8-10:

The actual support for boards with 2 chargers.

Stating the obvious here: given where we are in the cycle I expect
parts 2-4 / patches 4-10 to all be 6.3 material. 

Regards,

Hans



Hans de Goede (8):
  power: supply: bq25890: Only use pdata->regulator_init_data for vbus
  power: supply: bq25890: Ensure pump_express_work is cancelled on
    remove
  power: supply: bq25890: Fix usb-notifier probe and remove races
  power: supply: bq25890: Fix setting of F_CONV_RATE rate when disabling
    HiZ mode
  power: supply: bq25890: Always take HiZ mode into account for ADC rate
  power: supply: bq25890: Support boards with more then one charger IC
  power: supply: bq25890: Add support for having a secondary charger IC
  power: supply: bq25890: Add new linux,iinlim-percentage property

Marek Vasut (2):
  power: supply: bq25890: Factor out chip state update
  power: supply: bq25890: Add HiZ mode support

 drivers/platform/x86/x86-android-tablets.c |   2 +-
 drivers/power/supply/bq25890_charger.c     | 213 ++++++++++++++++-----
 2 files changed, 169 insertions(+), 46 deletions(-)

-- 
2.38.1

[PATCH 01/10] power: supply: bq25890: Only use pdata->regulator_init_data for vbus

[Hans de Goede]

bq25890_platform_data.regulator_init_data is intended to only provide
regulator init_data for the vbus regulator.

Remove this from the regulator_config before registering the vsys
regulator. Otherwise the regulator_register() call for vsys will fail
because it tries to register duplicate consumer_dev_name + supply
names from init_data->consumer_supplies[], leading to the entire
probe of the bq25890 driver failing:

[   32.017501] bq25890-charger i2c-bq25892_main: Failed to set supply vbus
[   32.017525] bq25890-charger i2c-bq25892_main: error -EBUSY: registering vsys regulator
[   32.124978] bq25890-charger: probe of i2c-bq25892_main failed with error -16

Fixes: 14a3d159abf8 ("power: supply: bq25890: Add Vsys regulator")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index bfdd2213ba69..512c81662eea 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1161,6 +1161,8 @@ static int bq25890_register_regulator(struct bq25890_device *bq)
 				     "registering vbus regulator");
 	}
 
+	/* pdata->regulator_init_data is for vbus only */
+	cfg.init_data = NULL;
 	reg = devm_regulator_register(bq->dev, &bq25890_vsys_desc, &cfg);
 	if (IS_ERR(reg)) {
 		return dev_err_probe(bq->dev, PTR_ERR(reg),
-- 
2.38.1

[PATCH 02/10] power: supply: bq25890: Ensure pump_express_work is cancelled on remove

[Hans de Goede]

The pump_express_work which gets queued from an external_power_changed
callback might be pending / running on remove() (or on probe failure).

Add a devm action cancelling the work, to ensure that it is cancelled.

Note the devm action is added before devm_power_supply_register(), making
it run after devm unregisters the power_supply, so that the work cannot
be queued anymore (this is also why a devm action is used for this).

Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 512c81662eea..30d77afab839 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	return 0;
 }
 
+static void bq25890_non_devm_cleanup(void *data)
+{
+	struct bq25890_device *bq = data;
+
+	cancel_delayed_work_sync(&bq->pump_express_work);
+}
+
 static int bq25890_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client)
 	/* OTG reporting */
 	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
 
+	ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq);
+	if (ret)
+		return ret;
+
 	ret = bq25890_register_regulator(bq);
 	if (ret)
 		return ret;
-- 
2.38.1

[PATCH 03/10] power: supply: bq25890: Fix usb-notifier probe and remove races

[Hans de Goede]

There are 2 races surrounding the usb-notifier:

1. The notifier, and thus usb_work, may run before the bq->charger
   power_supply class device is registered. But usb_work may call
   power_supply_changed() which relies on the psy device being registered.

2. usb_work may be pending/running at remove() time, so it needs to be
   cancelled on remove after unregistering the usb-notifier.

Fix 1. by moving usb-notifier registration to after the power_supply
registration.

Fix 2. by adding a cancel_work_sync() call directly after the usb-notifier
unregistration.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 30d77afab839..032a10a3877b 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -1387,16 +1387,10 @@ static int bq25890_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
-		INIT_WORK(&bq->usb_work, bq25890_usb_work);
-		bq->usb_nb.notifier_call = bq25890_usb_notifier;
-		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
-	}
-
 	ret = bq25890_power_supply_init(bq);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register power supply\n");
-		goto err_unregister_usb_notifier;
+		return ret;
 	}
 
 	ret = devm_request_threaded_irq(dev, client->irq, NULL,
@@ -1404,23 +1398,25 @@ static int bq25890_probe(struct i2c_client *client)
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					BQ25890_IRQ_PIN, bq);
 	if (ret)
-		goto err_unregister_usb_notifier;
-
-	return 0;
+		return ret;
 
-err_unregister_usb_notifier:
-	if (!IS_ERR_OR_NULL(bq->usb_phy))
-		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
+	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
+		INIT_WORK(&bq->usb_work, bq25890_usb_work);
+		bq->usb_nb.notifier_call = bq25890_usb_notifier;
+		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
+	}
 
-	return ret;
+	return 0;
 }
 
 static void bq25890_remove(struct i2c_client *client)
 {
 	struct bq25890_device *bq = i2c_get_clientdata(client);
 
-	if (!IS_ERR_OR_NULL(bq->usb_phy))
+	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
 		usb_unregister_notifier(bq->usb_phy, &bq->usb_nb);
+		cancel_work_sync(&bq->usb_work);
+	}
 
 	if (!bq->skip_reset) {
 		/* reset all registers to default values */
-- 
2.38.1

[PATCH 04/10] power: supply: bq25890: Factor out chip state update

[Hans de Goede]

From: Marek Vasut <marex@denx.de>

Pull the chip state and ADC conversion update functionality out into
separate function, so it can be reused elsewhere in the driver. This
is a preparatory patch, no functional change.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 032a10a3877b..d0a7a1c11ad5 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -454,20 +454,18 @@ static int bq25890_get_vbus_voltage(struct bq25890_device *bq)
 	return bq25890_find_val(ret, TBL_VBUSV);
 }
 
-static int bq25890_power_supply_get_property(struct power_supply *psy,
-					     enum power_supply_property psp,
-					     union power_supply_propval *val)
+static void bq25890_update_state(struct bq25890_device *bq,
+				 enum power_supply_property psp,
+				 struct bq25890_state *state)
 {
-	struct bq25890_device *bq = power_supply_get_drvdata(psy);
-	struct bq25890_state state;
 	bool do_adc_conv;
 	int ret;
 
 	mutex_lock(&bq->lock);
 	/* update state in case we lost an interrupt */
 	__bq25890_handle_irq(bq);
-	state = bq->state;
-	do_adc_conv = !state.online && bq25890_is_adc_property(psp);
+	*state = bq->state;
+	do_adc_conv = !state->online && bq25890_is_adc_property(psp);
 	if (do_adc_conv)
 		bq25890_field_write(bq, F_CONV_START, 1);
 	mutex_unlock(&bq->lock);
@@ -475,6 +473,17 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 	if (do_adc_conv)
 		regmap_field_read_poll_timeout(bq->rmap_fields[F_CONV_START],
 			ret, !ret, 25000, 1000000);
+}
+
+static int bq25890_power_supply_get_property(struct power_supply *psy,
+					     enum power_supply_property psp,
+					     union power_supply_propval *val)
+{
+	struct bq25890_device *bq = power_supply_get_drvdata(psy);
+	struct bq25890_state state;
+	int ret;
+
+	bq25890_update_state(bq, psp, &state);
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-- 
2.38.1

[PATCH 05/10] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

From: Marek Vasut <marex@denx.de>

The bq25890 is capable of disconnecting itself from the external supply,
in which case the system is supplied only from the battery. This can be
useful e.g. to test the pure battery operation, or draw no power from
USB port.

Implement support for this mode, which can be toggled by writing 0 or
non-zero to sysfs 'online' attribute, to select either offline or online
mode.

The IRQ handler has to be triggered to update chip state, as switching
to and from HiZ mode does not generate an interrupt automatically.

The IRQ handler reinstates the HiZ mode in case a cable is replugged by
the user, the chip itself clears the HiZ mode bit when cable is plugged
in by the user and the chip detects PG bad-to-good transition.

Signed-off-by: Marek Vasut <marex@denx.de>
[hdegoede@redhat.com: Replace "&" with "&&" in a boolean check]
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 58 +++++++++++++++++++-------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index d0a7a1c11ad5..2dffc5df0969 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -95,6 +95,7 @@ struct bq25890_init_data {
 
 struct bq25890_state {
 	u8 online;
+	u8 hiz;
 	u8 chrg_status;
 	u8 chrg_fault;
 	u8 vsys_status;
@@ -119,6 +120,7 @@ struct bq25890_device {
 
 	bool skip_reset;
 	bool read_back_init_data;
+	bool force_hiz;
 	u32 pump_express_vbus_max;
 	enum bq25890_chip_version chip_version;
 	struct bq25890_init_data init_data;
@@ -487,7 +489,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
-		if (!state.online)
+		if (!state.online || state.hiz)
 			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
 		else if (state.chrg_status == STATUS_NOT_CHARGING)
 			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
@@ -502,7 +504,8 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		if (!state.online || state.chrg_status == STATUS_NOT_CHARGING ||
+		if (!state.online || state.hiz ||
+		    state.chrg_status == STATUS_NOT_CHARGING ||
 		    state.chrg_status == STATUS_TERMINATION_DONE)
 			val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
 		else if (state.chrg_status == STATUS_PRE_CHARGING)
@@ -522,7 +525,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_ONLINE:
-		val->intval = state.online;
+		val->intval = state.online && !state.hiz;
 		break;
 
 	case POWER_SUPPLY_PROP_HEALTH:
@@ -676,7 +679,8 @@ static int bq25890_power_supply_set_property(struct power_supply *psy,
 					     const union power_supply_propval *val)
 {
 	struct bq25890_device *bq = power_supply_get_drvdata(psy);
-	int maxval;
+	struct bq25890_state state;
+	int maxval, ret;
 	u8 lval;
 
 	switch (psp) {
@@ -691,6 +695,12 @@ static int bq25890_power_supply_set_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		lval = bq25890_find_idx(val->intval, TBL_IINLIM);
 		return bq25890_field_write(bq, F_IINLIM, lval);
+	case POWER_SUPPLY_PROP_ONLINE:
+		ret = bq25890_field_write(bq, F_EN_HIZ, !val->intval);
+		if (!ret)
+			bq->force_hiz = !val->intval;
+		bq25890_update_state(bq, psp, &state);
+		return ret;
 	default:
 		return -EINVAL;
 	}
@@ -703,6 +713,7 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+	case POWER_SUPPLY_PROP_ONLINE:
 		return true;
 	default:
 		return false;
@@ -757,6 +768,7 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
 	} state_fields[] = {
 		{F_CHG_STAT,	&state->chrg_status},
 		{F_PG_STAT,	&state->online},
+		{F_EN_HIZ,	&state->hiz},
 		{F_VSYS_STAT,	&state->vsys_status},
 		{F_BOOST_FAULT, &state->boost_fault},
 		{F_BAT_FAULT,	&state->bat_fault},
@@ -772,10 +784,11 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
 		*state_fields[i].data = ret;
 	}
 
-	dev_dbg(bq->dev, "S:CHG/PG/VSYS=%d/%d/%d, F:CHG/BOOST/BAT/NTC=%d/%d/%d/%d\n",
-		state->chrg_status, state->online, state->vsys_status,
-		state->chrg_fault, state->boost_fault, state->bat_fault,
-		state->ntc_fault);
+	dev_dbg(bq->dev, "S:CHG/PG/HIZ/VSYS=%d/%d/%d/%d, F:CHG/BOOST/BAT/NTC=%d/%d/%d/%d\n",
+		state->chrg_status, state->online,
+		state->hiz, state->vsys_status,
+		state->chrg_fault, state->boost_fault,
+		state->bat_fault, state->ntc_fault);
 
 	return 0;
 }
@@ -792,16 +805,33 @@ static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq)
 	if (!memcmp(&bq->state, &new_state, sizeof(new_state)))
 		return IRQ_NONE;
 
-	if (!new_state.online && bq->state.online) {	    /* power removed */
+	/* power removed or HiZ */
+	if ((!new_state.online || new_state.hiz) && bq->state.online) {
 		/* disable ADC */
 		ret = bq25890_field_write(bq, F_CONV_RATE, 0);
 		if (ret < 0)
 			goto error;
-	} else if (new_state.online && !bq->state.online) { /* power inserted */
-		/* enable ADC, to have control of charge current/voltage */
-		ret = bq25890_field_write(bq, F_CONV_RATE, 1);
-		if (ret < 0)
-			goto error;
+	} else if (new_state.online && !bq->state.online) {
+		/*
+		 * Restore HiZ bit in case it was set by user.
+		 * The chip does not retain this bit once the
+		 * cable is re-plugged, hence the bit must be
+		 * reset manually here.
+		 */
+		if (bq->force_hiz) {
+			ret = bq25890_field_write(bq, F_EN_HIZ, bq->force_hiz);
+			if (ret < 0)
+				goto error;
+			new_state.hiz = 1;
+		}
+
+		if (!new_state.hiz) {
+			/* power inserted and not HiZ */
+			/* enable ADC, to have control of charge current/voltage */
+			ret = bq25890_field_write(bq, F_CONV_RATE, 1);
+			if (ret < 0)
+				goto error;
+		}
 	}
 
 	bq->state = new_state;
-- 
2.38.1

[PATCH 06/10] power: supply: bq25890: Fix setting of F_CONV_RATE rate when disabling HiZ mode

[Hans de Goede]

The recent "power: supply: bq25890: Add HiZ mode support" change
leaves F_CONV_RATE rate unset when disabling HiZ mode (setting
POWER_SUPPLY_PROP_ONLINE to 1) while a charger is connected.

Separate the resetting HiZ mode when necessary because of a charger
(re)plug event into its own if which runs first.

And fix the setting of F_CONV_RATE rate by adding helper variables for
the old and new F_CONV_RATE state which check both the online and hiz bits
and then compare the helper variables to see if a F_CONV_RATE update is
necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 41 +++++++++++---------------
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 2dffc5df0969..bd6858231271 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -795,6 +795,7 @@ static int bq25890_get_chip_state(struct bq25890_device *bq,
 
 static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq)
 {
+	bool adc_conv_rate, new_adc_conv_rate;
 	struct bq25890_state new_state;
 	int ret;
 
@@ -805,33 +806,25 @@ static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq)
 	if (!memcmp(&bq->state, &new_state, sizeof(new_state)))
 		return IRQ_NONE;
 
-	/* power removed or HiZ */
-	if ((!new_state.online || new_state.hiz) && bq->state.online) {
-		/* disable ADC */
-		ret = bq25890_field_write(bq, F_CONV_RATE, 0);
+	/*
+	 * Restore HiZ bit in case it was set by user. The chip does not retain
+	 * this bit on cable replug, hence the bit must be reset manually here.
+	 */
+	if (new_state.online && !bq->state.online && bq->force_hiz) {
+		ret = bq25890_field_write(bq, F_EN_HIZ, bq->force_hiz);
 		if (ret < 0)
 			goto error;
-	} else if (new_state.online && !bq->state.online) {
-		/*
-		 * Restore HiZ bit in case it was set by user.
-		 * The chip does not retain this bit once the
-		 * cable is re-plugged, hence the bit must be
-		 * reset manually here.
-		 */
-		if (bq->force_hiz) {
-			ret = bq25890_field_write(bq, F_EN_HIZ, bq->force_hiz);
-			if (ret < 0)
-				goto error;
-			new_state.hiz = 1;
-		}
+		new_state.hiz = 1;
+	}
 
-		if (!new_state.hiz) {
-			/* power inserted and not HiZ */
-			/* enable ADC, to have control of charge current/voltage */
-			ret = bq25890_field_write(bq, F_CONV_RATE, 1);
-			if (ret < 0)
-				goto error;
-		}
+	/* Should period ADC sampling be enabled? */
+	adc_conv_rate = bq->state.online && !bq->state.hiz;
+	new_adc_conv_rate = new_state.online && !new_state.hiz;
+
+	if (new_adc_conv_rate != adc_conv_rate) {
+		ret = bq25890_field_write(bq, F_CONV_RATE, new_adc_conv_rate);
+		if (ret < 0)
+			goto error;
 	}
 
 	bq->state = new_state;
-- 
2.38.1

[PATCH 07/10] power: supply: bq25890: Always take HiZ mode into account for ADC rate

[Hans de Goede]

The code to check if F_CONV_RATE has been set, or if a manual ADC
conversion needs to be triggered, as well as the code to set
the initial F_CONV_RATE value at probe both where not taking
HiZ mode into account. Add checks for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index bd6858231271..03e31c5b0df5 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -467,7 +467,7 @@ static void bq25890_update_state(struct bq25890_device *bq,
 	/* update state in case we lost an interrupt */
 	__bq25890_handle_irq(bq);
 	*state = bq->state;
-	do_adc_conv = !state->online && bq25890_is_adc_property(psp);
+	do_adc_conv = (!state->online || state->hiz) && bq25890_is_adc_property(psp);
 	if (do_adc_conv)
 		bq25890_field_write(bq, F_CONV_START, 1);
 	mutex_unlock(&bq->lock);
@@ -956,7 +956,7 @@ static int bq25890_hw_init(struct bq25890_device *bq)
 	}
 
 	/* Configure ADC for continuous conversions when charging */
-	ret = bq25890_field_write(bq, F_CONV_RATE, !!bq->state.online);
+	ret = bq25890_field_write(bq, F_CONV_RATE, bq->state.online && !bq->state.hiz);
 	if (ret < 0) {
 		dev_dbg(bq->dev, "Config ADC failed %d\n", ret);
 		return ret;
-- 
2.38.1

[PATCH 08/10] power: supply: bq25890: Support boards with more then one charger IC

[Hans de Goede]

Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have
multiple batteries with a separate bq25890 charger for each battery.

This requires the bq25890_charger code to use a unique name per
registered power_supply class device, rather then hardcoding
"bq25890-charger" as power_supply class device name.

Add a "-%d" prefix to the name, allocated through idr in the same way
as several other power_supply drivers are already doing this.

Note this also updates: drivers/platform/x86/x86-android-tablets.c
which refers to the charger by power_supply-class-device-name for
the purpose of setting the "supplied-from" property on the fuel-gauge
to this name.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
As the subsystem maintainer for drivers/platform/x86 I'm fine with
the small x86-android-tablets.c being merged through the
linux-power-supply tree.

Also note I did a full grep for "bq25890-charger" and
x86-android-tablets.c is the only file in the kernel tree referring
to this name.
---
 drivers/platform/x86/x86-android-tablets.c |  2 +-
 drivers/power/supply/bq25890_charger.c     | 29 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/x86-android-tablets.c b/drivers/platform/x86/x86-android-tablets.c
index dd933cf32b38..916e37a4f85e 100644
--- a/drivers/platform/x86/x86-android-tablets.c
+++ b/drivers/platform/x86/x86-android-tablets.c
@@ -187,7 +187,7 @@ struct x86_dev_info {
 /* Generic / shared charger / battery settings */
 static const char * const tusb1211_chg_det_psy[] = { "tusb1211-charger-detect" };
 static const char * const bq24190_psy[] = { "bq24190-charger" };
-static const char * const bq25890_psy[] = { "bq25890-charger" };
+static const char * const bq25890_psy[] = { "bq25890-charger-0" };
 
 static const struct property_entry fg_bq24190_supply_props[] = {
 	PROPERTY_ENTRY_STRING_ARRAY("supplied-from", bq24190_psy),
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 03e31c5b0df5..a0e20cbadeb8 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -108,6 +108,9 @@ struct bq25890_device {
 	struct i2c_client *client;
 	struct device *dev;
 	struct power_supply *charger;
+	struct power_supply_desc desc;
+	char name[28]; /* "bq25890-charger-%d" */
+	int id;
 
 	struct usb_phy *usb_phy;
 	struct notifier_block usb_nb;
@@ -129,6 +132,9 @@ struct bq25890_device {
 	struct mutex lock; /* protect state data */
 };
 
+static DEFINE_IDR(bq25890_id);
+static DEFINE_MUTEX(bq25890_id_mutex);
+
 static const struct regmap_range bq25890_readonly_reg_ranges[] = {
 	regmap_reg_range(0x0b, 0x0c),
 	regmap_reg_range(0x0e, 0x13),
@@ -989,7 +995,6 @@ static char *bq25890_charger_supplied_to[] = {
 };
 
 static const struct power_supply_desc bq25890_power_supply_desc = {
-	.name = "bq25890-charger",
 	.type = POWER_SUPPLY_TYPE_USB,
 	.properties = bq25890_power_supply_props,
 	.num_properties = ARRAY_SIZE(bq25890_power_supply_props),
@@ -1003,12 +1008,21 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
 {
 	struct power_supply_config psy_cfg = { .drv_data = bq, };
 
+	/* Get ID for the device */
+	mutex_lock(&bq25890_id_mutex);
+	bq->id = idr_alloc(&bq25890_id, bq, 0, 0, GFP_KERNEL);
+	mutex_unlock(&bq25890_id_mutex);
+	if (bq->id < 0)
+		return bq->id;
+
+	snprintf(bq->name, sizeof(bq->name), "bq25890-charger-%d", bq->id);
+	bq->desc = bq25890_power_supply_desc;
+	bq->desc.name = bq->name;
+
 	psy_cfg.supplied_to = bq25890_charger_supplied_to;
 	psy_cfg.num_supplicants = ARRAY_SIZE(bq25890_charger_supplied_to);
 
-	bq->charger = devm_power_supply_register(bq->dev,
-						 &bq25890_power_supply_desc,
-						 &psy_cfg);
+	bq->charger = devm_power_supply_register(bq->dev, &bq->desc, &psy_cfg);
 
 	return PTR_ERR_OR_ZERO(bq->charger);
 }
@@ -1354,6 +1368,12 @@ static void bq25890_non_devm_cleanup(void *data)
 	struct bq25890_device *bq = data;
 
 	cancel_delayed_work_sync(&bq->pump_express_work);
+
+	if (bq->id >= 0) {
+		mutex_lock(&bq25890_id_mutex);
+		idr_remove(&bq25890_id, bq->id);
+		mutex_unlock(&bq25890_id_mutex);
+	}
 }
 
 static int bq25890_probe(struct i2c_client *client)
@@ -1368,6 +1388,7 @@ static int bq25890_probe(struct i2c_client *client)
 
 	bq->client = client;
 	bq->dev = dev;
+	bq->id = -1;
 
 	mutex_init(&bq->lock);
 	INIT_DELAYED_WORK(&bq->pump_express_work, bq25890_pump_express_work);
-- 
2.38.1

[PATCH 09/10] power: supply: bq25890: Add support for having a secondary charger IC

[Hans de Goede]

Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have multiple
batteries with a separate bq25890 charger for each battery.

This requires some coordination between the chargers specifically
the main charger needs to put the secondary charger in Hi-Z mode when:

1. Enabling its 5V boost (OTG) output to power an external USB device,
   to avoid the secondary charger IC seeing this as external Vbus and
   then trying to charge the secondary battery from this.

2. Talking the Pump Express protocol to increase the external Vbus voltage.
   Having the secondary charger drawing current when the main charger is
   trying to talk the Pump Express protocol results in the external Vbus
   voltage not being raised.

Add a new "linux,secondary-charger-name" string device-property, which
can be set to the power_supply class device's name of the secondary
charger when there is a secondary charger; and make the Vbus regulator and
Pump Express code put the secondary charger in Hi-Z mode when necessary.

So far this new property is only used on x86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added
to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 45 +++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index a0e20cbadeb8..b0d07ff24ace 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -108,6 +108,7 @@ struct bq25890_device {
 	struct i2c_client *client;
 	struct device *dev;
 	struct power_supply *charger;
+	struct power_supply *secondary_chrg;
 	struct power_supply_desc desc;
 	char name[28]; /* "bq25890-charger-%d" */
 	int id;
@@ -1042,10 +1043,17 @@ static void bq25890_pump_express_work(struct work_struct *data)
 {
 	struct bq25890_device *bq =
 		container_of(data, struct bq25890_device, pump_express_work.work);
+	union power_supply_propval value;
 	int voltage, i, ret;
 
 	dev_dbg(bq->dev, "Start to request input voltage increasing\n");
 
+	/* If there is a second charger put in Hi-Z mode */
+	if (bq->secondary_chrg) {
+		value.intval = 0;
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &value);
+	}
+
 	/* Enable current pulse voltage control protocol */
 	ret = bq25890_field_write(bq, F_PUMPX_EN, 1);
 	if (ret < 0)
@@ -1077,6 +1085,11 @@ static void bq25890_pump_express_work(struct work_struct *data)
 
 	bq25890_field_write(bq, F_PUMPX_EN, 0);
 
+	if (bq->secondary_chrg) {
+		value.intval = 1;
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &value);
+	}
+
 	dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n",
 		 voltage);
 
@@ -1123,6 +1136,17 @@ static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
 static int bq25890_vbus_enable(struct regulator_dev *rdev)
 {
 	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+	union power_supply_propval val = {
+		.intval = 0,
+	};
+
+	/*
+	 * When enabling 5V boost / Vbus output, we need to put the secondary
+	 * charger in Hi-Z mode to avoid it trying to charge the secondary
+	 * battery from the 5V boost output.
+	 */
+	if (bq->secondary_chrg)
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &val);
 
 	return bq25890_set_otg_cfg(bq, 1);
 }
@@ -1130,8 +1154,19 @@ static int bq25890_vbus_enable(struct regulator_dev *rdev)
 static int bq25890_vbus_disable(struct regulator_dev *rdev)
 {
 	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+	union power_supply_propval val = {
+		.intval = 1,
+	};
+	int ret;
+
+	ret = bq25890_set_otg_cfg(bq, 0);
+	if (ret)
+		return ret;
 
-	return bq25890_set_otg_cfg(bq, 0);
+	if (bq->secondary_chrg)
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &val);
+
+	return 0;
 }
 
 static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
@@ -1342,6 +1377,14 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 {
 	int ret;
 	struct bq25890_init_data *init = &bq->init_data;
+	const char *str;
+
+	ret = device_property_read_string(bq->dev, "linux,secondary-charger-name", &str);
+	if (ret == 0) {
+		bq->secondary_chrg = power_supply_get_by_name(str);
+		if (!bq->secondary_chrg)
+			return -EPROBE_DEFER;
+	}
 
 	/* Optional, left at 0 if property is not present */
 	device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
-- 
2.38.1

[PATCH 10/10] power: supply: bq25890: Add new linux,iinlim-percentage property

[Hans de Goede]

Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have
multiple batteries with a separate bq25890 charger for each battery.

This requires the maximum current the external power-supply can deliver
to be divided over the chargers. The Android vendor kernel shipped
on the YT3-X90F divides this current with a 40/60 percent split so that
batteries are done charging at approx. the same time if both were fully
empty at the start.

Add support for a new "linux,iinlim-percentage" percentage property which
can be set to indicate that a bq25890 charger should only use that
percentage of the external power-supply's maximum current.

So far this new property is only used on x86/ACPI (non devicetree) devs,
IOW it is not used in actual devicetree files. The devicetree-bindings
maintainers have requested properties like these to not be added to the
devicetree-bindings, so the new property is deliberately not added
to the existing devicetree-bindings.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index b0d07ff24ace..2bd7721b969f 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -126,6 +126,7 @@ struct bq25890_device {
 	bool read_back_init_data;
 	bool force_hiz;
 	u32 pump_express_vbus_max;
+	u32 iinlim_percentage;
 	enum bq25890_chip_version chip_version;
 	struct bq25890_init_data init_data;
 	struct bq25890_state state;
@@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy,
 	}
 }
 
+/*
+ * If there are multiple chargers the maximum current the external power-supply
+ * can deliver needs to be divided over the chargers. This is done according
+ * to the bq->iinlim_percentage setting.
+ */
+static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq,
+						    int iinlim_ua)
+{
+	iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100;
+	return bq25890_find_idx(iinlim_ua, TBL_IINLIM);
+}
+
 /* On the BQ25892 try to get charger-type info from our supplier */
 static void bq25890_charger_external_power_changed(struct power_supply *psy)
 {
@@ -745,7 +758,7 @@ static void bq25890_charger_external_power_changed(struct power_supply *psy)
 
 	switch (val.intval) {
 	case POWER_SUPPLY_USB_TYPE_DCP:
-		input_current_limit = bq25890_find_idx(2000000, TBL_IINLIM);
+		input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 2000000);
 		if (bq->pump_express_vbus_max) {
 			queue_delayed_work(system_power_efficient_wq,
 					   &bq->pump_express_work,
@@ -754,11 +767,11 @@ static void bq25890_charger_external_power_changed(struct power_supply *psy)
 		break;
 	case POWER_SUPPLY_USB_TYPE_CDP:
 	case POWER_SUPPLY_USB_TYPE_ACA:
-		input_current_limit = bq25890_find_idx(1500000, TBL_IINLIM);
+		input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 1500000);
 		break;
 	case POWER_SUPPLY_USB_TYPE_SDP:
 	default:
-		input_current_limit = bq25890_find_idx(500000, TBL_IINLIM);
+		input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 500000);
 	}
 
 	bq25890_field_write(bq, F_IINLIM, input_current_limit);
@@ -1390,6 +1403,11 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 	device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
 				 &bq->pump_express_vbus_max);
 
+	/* Optional, left at 100% if property is not present */
+	bq->iinlim_percentage = 100;
+	device_property_read_u32(bq->dev, "linux,iinlim-percentage",
+				 &bq->iinlim_percentage);
+
 	bq->skip_reset = device_property_read_bool(bq->dev, "linux,skip-reset");
 	bq->read_back_init_data = device_property_read_bool(bq->dev,
 						"linux,read-back-settings");
-- 
2.38.1

Re: [PATCH 01/10] power: supply: bq25890: Only use pdata->regulator_init_data for vbus

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> bq25890_platform_data.regulator_init_data is intended to only provide
> regulator init_data for the vbus regulator.
> 
> Remove this from the regulator_config before registering the vsys
> regulator. Otherwise the regulator_register() call for vsys will fail
> because it tries to register duplicate consumer_dev_name + supply
> names from init_data->consumer_supplies[], leading to the entire
> probe of the bq25890 driver failing:
> 
> [   32.017501] bq25890-charger i2c-bq25892_main: Failed to set supply vbus
> [   32.017525] bq25890-charger i2c-bq25892_main: error -EBUSY: registering vsys regulator
> [   32.124978] bq25890-charger: probe of i2c-bq25892_main failed with error -16
> 
> Fixes: 14a3d159abf8 ("power: supply: bq25890: Add Vsys regulator")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 02/10] power: supply: bq25890: Ensure pump_express_work is cancelled on remove

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> The pump_express_work which gets queued from an external_power_changed
> callback might be pending / running on remove() (or on probe failure).
> 
> Add a devm action cancelling the work, to ensure that it is cancelled.
> 
> Note the devm action is added before devm_power_supply_register(), making
> it run after devm unregisters the power_supply, so that the work cannot
> be queued anymore (this is also why a devm action is used for this).
> 
> Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

A comment in the code matching the last paragraph of this commit message 
would be helpful I think.

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 03/10] power: supply: bq25890: Fix usb-notifier probe and remove races

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> There are 2 races surrounding the usb-notifier:
> 
> 1. The notifier, and thus usb_work, may run before the bq->charger
>     power_supply class device is registered. But usb_work may call
>     power_supply_changed() which relies on the psy device being registered.
> 
> 2. usb_work may be pending/running at remove() time, so it needs to be
>     cancelled on remove after unregistering the usb-notifier.
> 
> Fix 1. by moving usb-notifier registration to after the power_supply
> registration.
> 
> Fix 2. by adding a cancel_work_sync() call directly after the usb-notifier
> unregistration.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/power/supply/bq25890_charger.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 30d77afab839..032a10a3877b 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1387,16 +1387,10 @@ static int bq25890_probe(struct i2c_client *client)
>   	if (ret)
>   		return ret;
>   
> -	if (!IS_ERR_OR_NULL(bq->usb_phy)) {
> -		INIT_WORK(&bq->usb_work, bq25890_usb_work);
> -		bq->usb_nb.notifier_call = bq25890_usb_notifier;
> -		usb_register_notifier(bq->usb_phy, &bq->usb_nb);
> -	}
> -
>   	ret = bq25890_power_supply_init(bq);
>   	if (ret < 0) {
>   		dev_err(dev, "Failed to register power supply\n");
> -		goto err_unregister_usb_notifier;
> +		return ret;

You can even use 'return dev_err_probe()' above ^ to simplify that piece 
of code now.

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 06/10] power: supply: bq25890: Fix setting of F_CONV_RATE rate when disabling HiZ mode

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> The recent "power: supply: bq25890: Add HiZ mode support" change
> leaves F_CONV_RATE rate unset when disabling HiZ mode (setting
> POWER_SUPPLY_PROP_ONLINE to 1) while a charger is connected.
> 
> Separate the resetting HiZ mode when necessary because of a charger
> (re)plug event into its own if which runs first.

I think this one sentence needs rephrasing ^ .

> And fix the setting of F_CONV_RATE rate by adding helper variables for
> the old and new F_CONV_RATE state which check both the online and hiz bits
> and then compare the helper variables to see if a F_CONV_RATE update is
> necessary.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 07/10] power: supply: bq25890: Always take HiZ mode into account for ADC rate

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> The code to check if F_CONV_RATE has been set, or if a manual ADC
> conversion needs to be triggered, as well as the code to set
> the initial F_CONV_RATE value at probe both where not taking
> HiZ mode into account. Add checks for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 08/10] power: supply: bq25890: Support boards with more then one charger IC

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have
> multiple batteries with a separate bq25890 charger for each battery.
> 
> This requires the bq25890_charger code to use a unique name per
> registered power_supply class device, rather then hardcoding
> "bq25890-charger" as power_supply class device name.
> 
> Add a "-%d" prefix to the name, allocated through idr in the same way
> as several other power_supply drivers are already doing this.
> 
> Note this also updates: drivers/platform/x86/x86-android-tablets.c
> which refers to the charger by power_supply-class-device-name for
> the purpose of setting the "supplied-from" property on the fuel-gauge
> to this name.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 09/10] power: supply: bq25890: Add support for having a secondary charger IC

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have multiple
> batteries with a separate bq25890 charger for each battery.
> 
> This requires some coordination between the chargers specifically
> the main charger needs to put the secondary charger in Hi-Z mode when:
> 
> 1. Enabling its 5V boost (OTG) output to power an external USB device,
>     to avoid the secondary charger IC seeing this as external Vbus and
>     then trying to charge the secondary battery from this.
> 
> 2. Talking the Pump Express protocol to increase the external Vbus voltage.
>     Having the secondary charger drawing current when the main charger is
>     trying to talk the Pump Express protocol results in the external Vbus
>     voltage not being raised.
> 
> Add a new "linux,secondary-charger-name" string device-property, which
> can be set to the power_supply class device's name of the secondary
> charger when there is a secondary charger; and make the Vbus regulator and
> Pump Express code put the secondary charger in Hi-Z mode when necessary.
> 
> So far this new property is only used on x86/ACPI (non devicetree) devs,
> IOW it is not used in actual devicetree files. The devicetree-bindings
> maintainers have requested properties like these to not be added to the
> devicetree-bindings, so the new property is deliberately not added
> to the existing devicetree-bindings.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I kind-of wonder whether there shouldn't be some generic implementation 
of this kind of charger interaction on subsystem level. But maybe that 
is something for a subsequent series.

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH 10/10] power: supply: bq25890: Add new linux,iinlim-percentage property

[Marek Vasut]

On 11/27/22 19:02, Hans de Goede wrote:
> Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have
> multiple batteries with a separate bq25890 charger for each battery.
> 
> This requires the maximum current the external power-supply can deliver
> to be divided over the chargers. The Android vendor kernel shipped
> on the YT3-X90F divides this current with a 40/60 percent split so that
> batteries are done charging at approx. the same time if both were fully
> empty at the start.
> 
> Add support for a new "linux,iinlim-percentage" percentage property which
> can be set to indicate that a bq25890 charger should only use that
> percentage of the external power-supply's maximum current.
> 
> So far this new property is only used on x86/ACPI (non devicetree) devs,
> IOW it is not used in actual devicetree files. The devicetree-bindings
> maintainers have requested properties like these to not be added to the
> devicetree-bindings, so the new property is deliberately not added
> to the existing devicetree-bindings.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index b0d07ff24ace..2bd7721b969f 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -126,6 +126,7 @@ struct bq25890_device {
>   	bool read_back_init_data;
>   	bool force_hiz;
>   	u32 pump_express_vbus_max;
> +	u32 iinlim_percentage;

If this is percentage, u8 should be enough, right ?

>   	enum bq25890_chip_version chip_version;
>   	struct bq25890_init_data init_data;
>   	struct bq25890_state state;
> @@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy,
>   	}
>   }
>   
> +/*
> + * If there are multiple chargers the maximum current the external power-supply
> + * can deliver needs to be divided over the chargers. This is done according
> + * to the bq->iinlim_percentage setting.
> + */
> +static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq,
> +						    int iinlim_ua)
> +{
> +	iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100;

Can this ever add up to value above 100 ?
Should this use some clamp() ?

> +	return bq25890_find_idx(iinlim_ua, TBL_IINLIM);
> +}
> +
>   /* On the BQ25892 try to get charger-type info from our supplier */
>   static void bq25890_charger_external_power_changed(struct power_supply *psy)
>   {

[...]

Re: [PATCH 02/10] power: supply: bq25890: Ensure pump_express_work is cancelled on remove

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

Hi,

On Sun, Nov 27, 2022 at 07:02:25PM +0100, Hans de Goede wrote:
> The pump_express_work which gets queued from an external_power_changed
> callback might be pending / running on remove() (or on probe failure).
> 
> Add a devm action cancelling the work, to ensure that it is cancelled.
> 
> Note the devm action is added before devm_power_supply_register(), making
> it run after devm unregisters the power_supply, so that the work cannot
> be queued anymore (this is also why a devm action is used for this).
> 
> Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq25890_charger.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 512c81662eea..30d77afab839 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
>  	return 0;
>  }
>  
> +static void bq25890_non_devm_cleanup(void *data)
> +{
> +	struct bq25890_device *bq = data;
> +
> +	cancel_delayed_work_sync(&bq->pump_express_work);
> +}
> +
>  static int bq25890_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client)
>  	/* OTG reporting */
>  	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>  
> +	ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq);
> +	if (ret)
> +		return ret;

ret = devm_delayed_work_autocancel(dev, &bq->pump_express_work, bq25890_pump_express_work);
if (ret)
    return ret;

(+ removing the INIT_DELAYED_WORK)

-- Sebastian

> +
>  	ret = bq25890_register_regulator(bq);
>  	if (ret)
>  		return ret;
> -- 
> 2.38.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/10] power: supply: bq25890: Only use pdata->regulator_init_data for vbus

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

Hi,

On Sun, Nov 27, 2022 at 10:16:20PM +0100, Marek Vasut wrote:
> On 11/27/22 19:02, Hans de Goede wrote:
> > bq25890_platform_data.regulator_init_data is intended to only provide
> > regulator init_data for the vbus regulator.
> > 
> > Remove this from the regulator_config before registering the vsys
> > regulator. Otherwise the regulator_register() call for vsys will fail
> > because it tries to register duplicate consumer_dev_name + supply
> > names from init_data->consumer_supplies[], leading to the entire
> > probe of the bq25890 driver failing:
> > 
> > [   32.017501] bq25890-charger i2c-bq25892_main: Failed to set supply vbus
> > [   32.017525] bq25890-charger i2c-bq25892_main: error -EBUSY: registering vsys regulator
> > [   32.124978] bq25890-charger: probe of i2c-bq25892_main failed with error -16
> > 
> > Fixes: 14a3d159abf8 ("power: supply: bq25890: Add Vsys regulator")
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Reviewed-by: Marek Vasut <marex@denx.de>

Thanks, queued.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/10] power: supply: bq25890: Ensure pump_express_work is cancelled on remove

[Hans de Goede]

Hi Sebastian,

On 11/28/22 00:17, Sebastian Reichel wrote:
> Hi,
> 
> On Sun, Nov 27, 2022 at 07:02:25PM +0100, Hans de Goede wrote:
>> The pump_express_work which gets queued from an external_power_changed
>> callback might be pending / running on remove() (or on probe failure).
>>
>> Add a devm action cancelling the work, to ensure that it is cancelled.
>>
>> Note the devm action is added before devm_power_supply_register(), making
>> it run after devm unregisters the power_supply, so that the work cannot
>> be queued anymore (this is also why a devm action is used for this).
>>
>> Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/bq25890_charger.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 512c81662eea..30d77afab839 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
>>  	return 0;
>>  }
>>  
>> +static void bq25890_non_devm_cleanup(void *data)
>> +{
>> +	struct bq25890_device *bq = data;
>> +
>> +	cancel_delayed_work_sync(&bq->pump_express_work);
>> +}
>> +
>>  static int bq25890_probe(struct i2c_client *client)
>>  {
>>  	struct device *dev = &client->dev;
>> @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client)
>>  	/* OTG reporting */
>>  	bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>>  
>> +	ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq);
>> +	if (ret)
>> +		return ret;
> 
> ret = devm_delayed_work_autocancel(dev, &bq->pump_express_work, bq25890_pump_express_work);
> if (ret)
>     return ret;
> 
> (+ removing the INIT_DELAYED_WORK)

That is a good point, but patch 8/10 builds on top of this
and uses bq25890_non_devm_cleanup() to release the idr id
needed when using multiple chargers on one board.

And like cancelling the work this too can only be done
after unregistering the psy device, and since the psy device
is unregistered using devm this means the idr_remove() needs
to use a devm callback too (to get the ordering right).

I do plan to prepare a v2 for patches 2-10 addressing Marek's
remarks this morning but given the need to have a devm action
anyways (devm_delayed_work_autocancel() is just a convenience
wrapper around devm_add_action) I plan to keep this as is.

Otherwise we end up with 2 devm actions without any need for
having 2 of them.

Regards,

Hans







> 
> -- Sebastian
> 
>> +
>>  	ret = bq25890_register_regulator(bq);
>>  	if (ret)
>>  		return ret;
>> -- 
>> 2.38.1
>>

Re: [PATCH 10/10] power: supply: bq25890: Add new linux,iinlim-percentage property

[Hans de Goede]

Hi Marek,

On 11/27/22 22:34, Marek Vasut wrote:
> On 11/27/22 19:02, Hans de Goede wrote:
>> Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have
>> multiple batteries with a separate bq25890 charger for each battery.
>>
>> This requires the maximum current the external power-supply can deliver
>> to be divided over the chargers. The Android vendor kernel shipped
>> on the YT3-X90F divides this current with a 40/60 percent split so that
>> batteries are done charging at approx. the same time if both were fully
>> empty at the start.
>>
>> Add support for a new "linux,iinlim-percentage" percentage property which
>> can be set to indicate that a bq25890 charger should only use that
>> percentage of the external power-supply's maximum current.
>>
>> So far this new property is only used on x86/ACPI (non devicetree) devs,
>> IOW it is not used in actual devicetree files. The devicetree-bindings
>> maintainers have requested properties like these to not be added to the
>> devicetree-bindings, so the new property is deliberately not added
>> to the existing devicetree-bindings.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index b0d07ff24ace..2bd7721b969f 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -126,6 +126,7 @@ struct bq25890_device {
>>       bool read_back_init_data;
>>       bool force_hiz;
>>       u32 pump_express_vbus_max;
>> +    u32 iinlim_percentage;
> 
> If this is percentage, u8 should be enough, right ?

It is not a charger-chip register value and it is used in
calculations so it is best if this is native integer size.

And I was passing its address directly to device_property_read_u32(),
but that has changed in v2.

>>       enum bq25890_chip_version chip_version;
>>       struct bq25890_init_data init_data;
>>       struct bq25890_state state;
>> @@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy,
>>       }
>>   }
>>   +/*
>> + * If there are multiple chargers the maximum current the external power-supply
>> + * can deliver needs to be divided over the chargers. This is done according
>> + * to the bq->iinlim_percentage setting.
>> + */
>> +static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq,
>> +                            int iinlim_ua)
>> +{
>> +    iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100;
> 
> Can this ever add up to value above 100 ?
> Should this use some clamp() ?

bq->iinlim_percentage should never be more then 100. I have added a check
for this when reading the property for version 2 of the series.

Thank you for all the reviews. I've also addressed all your other small
remarks and I will send out a v2 series with these fixed.

Regards,

Hans



> 
>> +    return bq25890_find_idx(iinlim_ua, TBL_IINLIM);
>> +}
>> +
>>   /* On the BQ25892 try to get charger-type info from our supplier */
>>   static void bq25890_charger_external_power_changed(struct power_supply *psy)
>>   {
> 
> [...]
>