[PATCH 1/2] power: supply: bq25890: Factor out chip state update

[PATCH 1/2] power: supply: bq25890: Factor out chip state update

[Marek Vasut]

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.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: Sebastian Reichel <sre@kernel.org>
---
 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 f0362dcb935e9..676eb66374e01 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.35.1

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

[Marek Vasut]

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.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/supply/bq25890_charger.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 676eb66374e01..70b5783999345 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;
@@ -676,7 +677,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 +693,10 @@ 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);
+		bq25890_update_state(bq, psp, &state);
+		return ret;
 	default:
 		return -EINVAL;
 	}
@@ -703,6 +709,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 +764,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 +780,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,12 +801,14 @@ 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 */
+	} else if (new_state.online && !new_state.hiz && !bq->state.online) {
+		/* 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)
-- 
2.35.1

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

Hi,

On 11/9/22 23:15, Marek Vasut wrote:
> 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.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> Cc: Sebastian Reichel <sre@kernel.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Also your timing is excellent :)  As a hobby project I'm working
on a x86 Lenovo Android tablet which has 2 separate batteries and
each battery has its own bq25892 chip.

This requires putting the secondary bq25892 in Hi-Z mode when
e.g. enabling the 5V USB/OTG boost regulator on the primary
bq25892 to make the micro-usb output 5V.

Which is functionality which I can nicely build on top of this
series.

Regards,

Hans

> ---
>  drivers/power/supply/bq25890_charger.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 676eb66374e01..70b5783999345 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;
> @@ -676,7 +677,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 +693,10 @@ 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);
> +		bq25890_update_state(bq, psp, &state);
> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -703,6 +709,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 +764,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 +780,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,12 +801,14 @@ 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 */
> +	} else if (new_state.online && !new_state.hiz && !bq->state.online) {
> +		/* 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)

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Marek Vasut]

On 11/19/22 14:52, Hans de Goede wrote:
> Hi,
> 
> On 11/9/22 23:15, Marek Vasut wrote:
>> 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.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>> Cc: Sebastian Reichel <sre@kernel.org>
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Also your timing is excellent :)  As a hobby project I'm working
> on a x86 Lenovo Android tablet which has 2 separate batteries and
> each battery has its own bq25892 chip.
> 
> This requires putting the secondary bq25892 in Hi-Z mode when
> e.g. enabling the 5V USB/OTG boost regulator on the primary
> bq25892 to make the micro-usb output 5V.
> 
> Which is functionality which I can nicely build on top of this
> series.

:)

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

On 11/19/22 14:52, Hans de Goede wrote:
> Hi,
> 
> On 11/9/22 23:15, Marek Vasut wrote:
>> 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.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>> Cc: Sebastian Reichel <sre@kernel.org>
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Also your timing is excellent :)  As a hobby project I'm working
> on a x86 Lenovo Android tablet which has 2 separate batteries and
> each battery has its own bq25892 chip.
> 
> This requires putting the secondary bq25892 in Hi-Z mode when
> e.g. enabling the 5V USB/OTG boost regulator on the primary
> bq25892 to make the micro-usb output 5V.
> 
> Which is functionality which I can nicely build on top of this
> series.

So one thing which I noticed while working on my own stuff
on top of this, is that the charger IC resets (disables) Hi-Z
mode when its internal PG (power-good) signal goes from false
to true.

The Android kernel fork for the tablet I'm working on detects
the PG false -> true transition in its IRQ handler and then
re-enabled Hi-Z mode if it was requested.

I wonder if we should do something similar: remember the last
value written to /sys/class/power_supply/bq2589o-charger/online
and then in the IRQ handler if Hi-Z mode was requested re-enable
Hi-Z mode ?

Regards,

Hans



>> ---
>>  drivers/power/supply/bq25890_charger.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 676eb66374e01..70b5783999345 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;
>> @@ -676,7 +677,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 +693,10 @@ 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);
>> +		bq25890_update_state(bq, psp, &state);
>> +		return ret;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -703,6 +709,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 +764,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 +780,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,12 +801,14 @@ 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 */
>> +	} else if (new_state.online && !new_state.hiz && !bq->state.online) {
>> +		/* 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)
> 

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Marek Vasut]

On 11/20/22 22:29, Hans de Goede wrote:
> 
> 
> On 11/19/22 14:52, Hans de Goede wrote:
>> Hi,
>>
>> On 11/9/22 23:15, Marek Vasut wrote:
>>> 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.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>> Cc: Sebastian Reichel <sre@kernel.org>
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Also your timing is excellent :)  As a hobby project I'm working
>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>> each battery has its own bq25892 chip.
>>
>> This requires putting the secondary bq25892 in Hi-Z mode when
>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>> bq25892 to make the micro-usb output 5V.
>>
>> Which is functionality which I can nicely build on top of this
>> series.
> 
> So one thing which I noticed while working on my own stuff
> on top of this, is that the charger IC resets (disables) Hi-Z
> mode when its internal PG (power-good) signal goes from false
> to true.
> 
> The Android kernel fork for the tablet I'm working on detects
> the PG false -> true transition in its IRQ handler and then
> re-enabled Hi-Z mode if it was requested.
> 
> I wonder if we should do something similar: remember the last
> value written to /sys/class/power_supply/bq2589o-charger/online
> and then in the IRQ handler if Hi-Z mode was requested re-enable
> Hi-Z mode ?

Uhhhhh, that sounds like the HiZ mode is unreliable .

Of course, there seems to be no way to completely inhibit the PG 
detection, is there ?

I wonder whether we should support this kind of workaround at all ?

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

Hi,

On 11/21/22 01:50, Marek Vasut wrote:
> On 11/20/22 22:29, Hans de Goede wrote:
>>
>>
>> On 11/19/22 14:52, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11/9/22 23:15, Marek Vasut wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>
>>> Thanks, patch looks good to me:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Also your timing is excellent :)  As a hobby project I'm working
>>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>>> each battery has its own bq25892 chip.
>>>
>>> This requires putting the secondary bq25892 in Hi-Z mode when
>>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>>> bq25892 to make the micro-usb output 5V.
>>>
>>> Which is functionality which I can nicely build on top of this
>>> series.
>>
>> So one thing which I noticed while working on my own stuff
>> on top of this, is that the charger IC resets (disables) Hi-Z
>> mode when its internal PG (power-good) signal goes from false
>> to true.
>>
>> The Android kernel fork for the tablet I'm working on detects
>> the PG false -> true transition in its IRQ handler and then
>> re-enabled Hi-Z mode if it was requested.
>>
>> I wonder if we should do something similar: remember the last
>> value written to /sys/class/power_supply/bq2589o-charger/online
>> and then in the IRQ handler if Hi-Z mode was requested re-enable
>> Hi-Z mode ?
> 
> Uhhhhh, that sounds like the HiZ mode is unreliable .

I think it is working as designed (although this is not documented)
the charger is designed to be able to operate autonomously, so it
makes sense to clear Hi-Z mode when a charger is unplugged + re-plugged.

> Of course, there seems to be no way to completely inhibit the PG detection, is there ?

not that I have been able to find.

> I wonder whether we should support this kind of workaround at all ?

You mean Hi-z mode itself? That is definitely useful to have, necessary
actually for the tablet I'm working on in my spare time now.

Or do you mean the Android workaround to re-enable Hi-Z mode in the IRQ
handler ?

Regards,

Hans

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Marek Vasut]

On 11/21/22 09:04, Hans de Goede wrote:
> Hi,

Hi,

> On 11/21/22 01:50, Marek Vasut wrote:
>> On 11/20/22 22:29, Hans de Goede wrote:
>>>
>>>
>>> On 11/19/22 14:52, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 11/9/22 23:15, Marek Vasut wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>>
>>>> Thanks, patch looks good to me:
>>>>
>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> Also your timing is excellent :)  As a hobby project I'm working
>>>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>>>> each battery has its own bq25892 chip.
>>>>
>>>> This requires putting the secondary bq25892 in Hi-Z mode when
>>>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>>>> bq25892 to make the micro-usb output 5V.
>>>>
>>>> Which is functionality which I can nicely build on top of this
>>>> series.
>>>
>>> So one thing which I noticed while working on my own stuff
>>> on top of this, is that the charger IC resets (disables) Hi-Z
>>> mode when its internal PG (power-good) signal goes from false
>>> to true.
>>>
>>> The Android kernel fork for the tablet I'm working on detects
>>> the PG false -> true transition in its IRQ handler and then
>>> re-enabled Hi-Z mode if it was requested.
>>>
>>> I wonder if we should do something similar: remember the last
>>> value written to /sys/class/power_supply/bq2589o-charger/online
>>> and then in the IRQ handler if Hi-Z mode was requested re-enable
>>> Hi-Z mode ?
>>
>> Uhhhhh, that sounds like the HiZ mode is unreliable .
> 
> I think it is working as designed (although this is not documented)
> the charger is designed to be able to operate autonomously, so it
> makes sense to clear Hi-Z mode when a charger is unplugged + re-plugged.

That does make sense, although I would've liked a bit which would set 
HiZ mode and ignore the charger replug, so the chip would guarantee no 
power draw from the charger until such bit is cleared.

>> Of course, there seems to be no way to completely inhibit the PG detection, is there ?
> 
> not that I have been able to find.
> 
>> I wonder whether we should support this kind of workaround at all ?
> 
> You mean Hi-z mode itself? That is definitely useful to have, necessary
> actually for the tablet I'm working on in my spare time now.

Yes, the HiZ mode itself, since it cannot be made persistent and resets 
itself on replug.

> Or do you mean the Android workaround to re-enable Hi-Z mode in the IRQ
> handler ?

I mean, either
- we drop the HiZ mode support completely
or
- we add HiZ mode + persistency workaround

I don't like the second option, but if you need it and we have no better 
options ... hum ...

[...]

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

Hi Marek,

On 11/26/22 12:06, Marek Vasut wrote:
> On 11/21/22 09:04, Hans de Goede wrote:
>> Hi,
> 
> Hi,
> 
>> On 11/21/22 01:50, Marek Vasut wrote:
>>> On 11/20/22 22:29, Hans de Goede wrote:
>>>>
>>>>
>>>> On 11/19/22 14:52, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/9/22 23:15, Marek Vasut wrote:
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>>>
>>>>> Thanks, patch looks good to me:
>>>>>
>>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>>>
>>>>> Also your timing is excellent :)  As a hobby project I'm working
>>>>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>>>>> each battery has its own bq25892 chip.
>>>>>
>>>>> This requires putting the secondary bq25892 in Hi-Z mode when
>>>>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>>>>> bq25892 to make the micro-usb output 5V.
>>>>>
>>>>> Which is functionality which I can nicely build on top of this
>>>>> series.
>>>>
>>>> So one thing which I noticed while working on my own stuff
>>>> on top of this, is that the charger IC resets (disables) Hi-Z
>>>> mode when its internal PG (power-good) signal goes from false
>>>> to true.
>>>>
>>>> The Android kernel fork for the tablet I'm working on detects
>>>> the PG false -> true transition in its IRQ handler and then
>>>> re-enabled Hi-Z mode if it was requested.
>>>>
>>>> I wonder if we should do something similar: remember the last
>>>> value written to /sys/class/power_supply/bq2589o-charger/online
>>>> and then in the IRQ handler if Hi-Z mode was requested re-enable
>>>> Hi-Z mode ?
>>>
>>> Uhhhhh, that sounds like the HiZ mode is unreliable .
>>
>> I think it is working as designed (although this is not documented)
>> the charger is designed to be able to operate autonomously, so it
>> makes sense to clear Hi-Z mode when a charger is unplugged + re-plugged.
> 
> That does make sense, although I would've liked a bit which would set HiZ mode and ignore the charger replug, so the chip would guarantee no power draw from the charger until such bit is cleared.

Well the re-enable Hi-Z mode on IRQ workaround you implemented gives
this behavior. While also ensuring that if the device dies because
of an empty battery it can still be recharged (since then the IRQ
handler won't run).

>>> Of course, there seems to be no way to completely inhibit the PG detection, is there ?
>>
>> not that I have been able to find.
>>
>>> I wonder whether we should support this kind of workaround at all ?
>>
>> You mean Hi-z mode itself? That is definitely useful to have, necessary
>> actually for the tablet I'm working on in my spare time now.
> 
> Yes, the HiZ mode itself, since it cannot be made persistent and resets itself on replug.
> 
>> Or do you mean the Android workaround to re-enable Hi-Z mode in the IRQ
>> handler ?
> 
> I mean, either
> - we drop the HiZ mode support completely
> or
> - we add HiZ mode + persistency workaround
> 
> I don't like the second option, but if you need it and we have no better options ... hum ...

I see that you have posted a v2 adding the workaround, thank you.

Note that in the mean time I did find a way to make things work
without the workaround:

1. Set current limit of Vboost output of main charger high enough
that both the external usb device can use 500mA + the secondary
charger can charge the secondary battery (from the main battery)
with 500 mA

2. Sleep 300 ms for the situation to stabilize

3. Set Hi-Z mode on secondary charger so that it stops charging
from the main battery.

I just finished running a whole bunch of tests with this setup and
it works well.

I do like the v2 of your patches better because that really guarantees
the second charger is "offline" when we want it to be offline and allows
me to put it in Hi-Z mode before enabling the 5v boost output on the
main charger instead of letting the secondary battery briefly charge
from the main battery. It also allows me to remove a struct delayed_Work
which I added for the 300ms delay ...

Can you please let me know if you want to move forward with your v2,
or since that version is not strictly necessary if you would prefer
to rollback to v1 ?

Then I can adjust my patches accordingly before posting them

I was pretty much about to post them just now :)

Regards,

Hans

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Marek Vasut]

On 11/26/22 16:03, Hans de Goede wrote:
> Hi Marek,

Hi,

> On 11/26/22 12:06, Marek Vasut wrote:
>> On 11/21/22 09:04, Hans de Goede wrote:
>>> Hi,
>>
>> Hi,
>>
>>> On 11/21/22 01:50, Marek Vasut wrote:
>>>> On 11/20/22 22:29, Hans de Goede wrote:
>>>>>
>>>>>
>>>>> On 11/19/22 14:52, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 11/9/22 23:15, Marek Vasut wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>> ---
>>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>>>>
>>>>>> Thanks, patch looks good to me:
>>>>>>
>>>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>
>>>>>> Also your timing is excellent :)  As a hobby project I'm working
>>>>>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>>>>>> each battery has its own bq25892 chip.
>>>>>>
>>>>>> This requires putting the secondary bq25892 in Hi-Z mode when
>>>>>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>>>>>> bq25892 to make the micro-usb output 5V.
>>>>>>
>>>>>> Which is functionality which I can nicely build on top of this
>>>>>> series.
>>>>>
>>>>> So one thing which I noticed while working on my own stuff
>>>>> on top of this, is that the charger IC resets (disables) Hi-Z
>>>>> mode when its internal PG (power-good) signal goes from false
>>>>> to true.
>>>>>
>>>>> The Android kernel fork for the tablet I'm working on detects
>>>>> the PG false -> true transition in its IRQ handler and then
>>>>> re-enabled Hi-Z mode if it was requested.
>>>>>
>>>>> I wonder if we should do something similar: remember the last
>>>>> value written to /sys/class/power_supply/bq2589o-charger/online
>>>>> and then in the IRQ handler if Hi-Z mode was requested re-enable
>>>>> Hi-Z mode ?
>>>>
>>>> Uhhhhh, that sounds like the HiZ mode is unreliable .
>>>
>>> I think it is working as designed (although this is not documented)
>>> the charger is designed to be able to operate autonomously, so it
>>> makes sense to clear Hi-Z mode when a charger is unplugged + re-plugged.
>>
>> That does make sense, although I would've liked a bit which would set HiZ mode and ignore the charger replug, so the chip would guarantee no power draw from the charger until such bit is cleared.
> 
> Well the re-enable Hi-Z mode on IRQ workaround you implemented gives
> this behavior. While also ensuring that if the device dies because
> of an empty battery it can still be recharged (since then the IRQ
> handler won't run).
> 
>>>> Of course, there seems to be no way to completely inhibit the PG detection, is there ?
>>>
>>> not that I have been able to find.
>>>
>>>> I wonder whether we should support this kind of workaround at all ?
>>>
>>> You mean Hi-z mode itself? That is definitely useful to have, necessary
>>> actually for the tablet I'm working on in my spare time now.
>>
>> Yes, the HiZ mode itself, since it cannot be made persistent and resets itself on replug.
>>
>>> Or do you mean the Android workaround to re-enable Hi-Z mode in the IRQ
>>> handler ?
>>
>> I mean, either
>> - we drop the HiZ mode support completely
>> or
>> - we add HiZ mode + persistency workaround
>>
>> I don't like the second option, but if you need it and we have no better options ... hum ...
> 
> I see that you have posted a v2 adding the workaround, thank you.
> 
> Note that in the mean time I did find a way to make things work
> without the workaround:
> 
> 1. Set current limit of Vboost output of main charger high enough
> that both the external usb device can use 500mA + the secondary
> charger can charge the secondary battery (from the main battery)
> with 500 mA
> 
> 2. Sleep 300 ms for the situation to stabilize
> 
> 3. Set Hi-Z mode on secondary charger so that it stops charging
> from the main battery.
> 
> I just finished running a whole bunch of tests with this setup and
> it works well.
> 
> I do like the v2 of your patches better because that really guarantees
> the second charger is "offline" when we want it to be offline and allows
> me to put it in Hi-Z mode before enabling the 5v boost output on the
> main charger instead of letting the secondary battery briefly charge
> from the main battery. It also allows me to remove a struct delayed_Work
> which I added for the 300ms delay ...

Pardon my ignorance here, but doesn't that implementation above work 
only in case you have two chargers ? Note that in my case, I have one 
charger and one battery.

> Can you please let me know if you want to move forward with your v2,
> or since that version is not strictly necessary if you would prefer
> to rollback to v1 ?

If we want to have HiZ mode support upstream, we might as well keep the 
workaround to retain the HiZ mode across replugs. So let's move forward 
with v2 ?

> Then I can adjust my patches accordingly before posting them
> 
> I was pretty much about to post them just now :)

Sorry about the delay this week.

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Hans de Goede]

Hi,

On 11/26/22 16:15, Marek Vasut wrote:
> On 11/26/22 16:03, Hans de Goede wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 11/26/22 12:06, Marek Vasut wrote:
>>> On 11/21/22 09:04, Hans de Goede wrote:
>>>> Hi,
>>>
>>> Hi,
>>>
>>>> On 11/21/22 01:50, Marek Vasut wrote:
>>>>> On 11/20/22 22:29, Hans de Goede wrote:
>>>>>>
>>>>>>
>>>>>> On 11/19/22 14:52, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 11/9/22 23:15, Marek Vasut wrote:
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> ---
>>>>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
>>>>>>>> Cc: Sebastian Reichel <sre@kernel.org>
>>>>>>>
>>>>>>> Thanks, patch looks good to me:
>>>>>>>
>>>>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>
>>>>>>> Also your timing is excellent :)  As a hobby project I'm working
>>>>>>> on a x86 Lenovo Android tablet which has 2 separate batteries and
>>>>>>> each battery has its own bq25892 chip.
>>>>>>>
>>>>>>> This requires putting the secondary bq25892 in Hi-Z mode when
>>>>>>> e.g. enabling the 5V USB/OTG boost regulator on the primary
>>>>>>> bq25892 to make the micro-usb output 5V.
>>>>>>>
>>>>>>> Which is functionality which I can nicely build on top of this
>>>>>>> series.
>>>>>>
>>>>>> So one thing which I noticed while working on my own stuff
>>>>>> on top of this, is that the charger IC resets (disables) Hi-Z
>>>>>> mode when its internal PG (power-good) signal goes from false
>>>>>> to true.
>>>>>>
>>>>>> The Android kernel fork for the tablet I'm working on detects
>>>>>> the PG false -> true transition in its IRQ handler and then
>>>>>> re-enabled Hi-Z mode if it was requested.
>>>>>>
>>>>>> I wonder if we should do something similar: remember the last
>>>>>> value written to /sys/class/power_supply/bq2589o-charger/online
>>>>>> and then in the IRQ handler if Hi-Z mode was requested re-enable
>>>>>> Hi-Z mode ?
>>>>>
>>>>> Uhhhhh, that sounds like the HiZ mode is unreliable .
>>>>
>>>> I think it is working as designed (although this is not documented)
>>>> the charger is designed to be able to operate autonomously, so it
>>>> makes sense to clear Hi-Z mode when a charger is unplugged + re-plugged.
>>>
>>> That does make sense, although I would've liked a bit which would set HiZ mode and ignore the charger replug, so the chip would guarantee no power draw from the charger until such bit is cleared.
>>
>> Well the re-enable Hi-Z mode on IRQ workaround you implemented gives
>> this behavior. While also ensuring that if the device dies because
>> of an empty battery it can still be recharged (since then the IRQ
>> handler won't run).
>>
>>>>> Of course, there seems to be no way to completely inhibit the PG detection, is there ?
>>>>
>>>> not that I have been able to find.
>>>>
>>>>> I wonder whether we should support this kind of workaround at all ?
>>>>
>>>> You mean Hi-z mode itself? That is definitely useful to have, necessary
>>>> actually for the tablet I'm working on in my spare time now.
>>>
>>> Yes, the HiZ mode itself, since it cannot be made persistent and resets itself on replug.
>>>
>>>> Or do you mean the Android workaround to re-enable Hi-Z mode in the IRQ
>>>> handler ?
>>>
>>> I mean, either
>>> - we drop the HiZ mode support completely
>>> or
>>> - we add HiZ mode + persistency workaround
>>>
>>> I don't like the second option, but if you need it and we have no better options ... hum ...
>>
>> I see that you have posted a v2 adding the workaround, thank you.
>>
>> Note that in the mean time I did find a way to make things work
>> without the workaround:
>>
>> 1. Set current limit of Vboost output of main charger high enough
>> that both the external usb device can use 500mA + the secondary
>> charger can charge the secondary battery (from the main battery)
>> with 500 mA
>>
>> 2. Sleep 300 ms for the situation to stabilize
>>
>> 3. Set Hi-Z mode on secondary charger so that it stops charging
>> from the main battery.
>>
>> I just finished running a whole bunch of tests with this setup and
>> it works well.
>>
>> I do like the v2 of your patches better because that really guarantees
>> the second charger is "offline" when we want it to be offline and allows
>> me to put it in Hi-Z mode before enabling the 5v boost output on the
>> main charger instead of letting the secondary battery briefly charge
>> from the main battery. It also allows me to remove a struct delayed_Work
>> which I added for the 300ms delay ...
> 
> Pardon my ignorance here, but doesn't that implementation above work only in case you have two chargers ? Note that in my case, I have one charger and one battery.

Right the workaround above is specifically for the tablet with
2 chargers which I'm working on. Iy is not a generic fix/WA for
the auto-reset of Hi-Z mode issue.

>> Can you please let me know if you want to move forward with your v2,
>> or since that version is not strictly necessary if you would prefer
>> to rollback to v1 ?
> 
> If we want to have HiZ mode support upstream, we might as well keep the workaround to retain the HiZ mode across replugs. So let's move forward with v2 ?

Ack, sounds good to me, thanks.

>> Then I can adjust my patches accordingly before posting them
>>
>> I was pretty much about to post them just now :)
> 
> Sorry about the delay this week.

No problem and thank you for your work on this.

Regards,

Hans


> 

Re: [PATCH 2/2] power: supply: bq25890: Add HiZ mode support

[Marek Vasut]

On 11/26/22 16:50, Hans de Goede wrote:
> Hi,

Hi,

[...]

>>> I do like the v2 of your patches better because that really guarantees
>>> the second charger is "offline" when we want it to be offline and allows
>>> me to put it in Hi-Z mode before enabling the 5v boost output on the
>>> main charger instead of letting the secondary battery briefly charge
>>> from the main battery. It also allows me to remove a struct delayed_Work
>>> which I added for the 300ms delay ...
>>
>> Pardon my ignorance here, but doesn't that implementation above work only in case you have two chargers ? Note that in my case, I have one charger and one battery.
> 
> Right the workaround above is specifically for the tablet with
> 2 chargers which I'm working on. Iy is not a generic fix/WA for
> the auto-reset of Hi-Z mode issue.
> 
>>> Can you please let me know if you want to move forward with your v2,
>>> or since that version is not strictly necessary if you would prefer
>>> to rollback to v1 ?
>>
>> If we want to have HiZ mode support upstream, we might as well keep the workaround to retain the HiZ mode across replugs. So let's move forward with v2 ?
> 
> Ack, sounds good to me, thanks.
> 
>>> Then I can adjust my patches accordingly before posting them
>>>
>>> I was pretty much about to post them just now :)
>>
>> Sorry about the delay this week.
> 
> No problem and thank you for your work on this.

Glad I could help.

I dropped the RB from v2 2/2 since there are changes which could use 
review. If you could have a look again at that patch, that would be nice.

Re: [PATCH 1/2] power: supply: bq25890: Factor out chip state update

[Hans de Goede]

Hi,

On 11/9/22 23:15, Marek Vasut wrote:
> 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.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> Cc: Sebastian Reichel <sre@kernel.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  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 f0362dcb935e9..676eb66374e01 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: