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

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

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)
>