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

[Hans de Goede <hdegoede@redhat.com>]

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)