Re: [PATCH 09/13] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

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

Hi,

On 11/7/21 19:49, Yauhen Kharuzhy wrote:
> On Sat, Oct 30, 2021 at 08:28:09PM +0200, Hans de Goede wrote:
>> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>> patch for adding regulator support.
> 
> The same notice here: if we enabling the boost mode to supply USB OTG
> device, we should disable charging and vice versa. I don't remember if
> enabling of OTG with CHG enabled really caused issues but I avoided this
> in my Yoga Book work.
> 
> I made quick check — seems that charging can be enabled during of boost
> operation, there are no visible side effects in registers and no
> limitation in the datasheet, so your approach may be good.

Right, the charging bit can simply be always on, if the OTG boost
converter is enabled then the charger it will automatically stop
charging (AFAIK it is partly re-using the same hw even).

I've no idea why the charging bit is being cleared by the BIOS, there
really is no need for this, with the exception of either:

1. Using an external Vbus boost converter, because that can e.g.
deliver more current, then charging does need to be disabled.

2. Disabling charging for battery health reasons

1. is not the case here; and 2. is something which we could export
as something userspace can request but that is optional.

So for normal use there really is no reason to set the charging
bit to 0 ever (and it is 1 on reset), so just setting it to 1
makes the behavior the same as on many ARM devices where this
charger is also used and the same as how similar chargers
like the bq2419x and bq2429x are used on other X86 devices.

Regards,

Hans




> 
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 163ca5d761aa..7504e30f1e4d 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -773,6 +773,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>>  	return PTR_ERR_OR_ZERO(bq->charger);
>>  }
>>  
>> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = bq25890_field_write(bq, F_OTG_CFG, val);
>> +	if (ret < 0)
>> +		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>>  static void bq25890_usb_work(struct work_struct *data)
>>  {
>>  	int ret;
>> @@ -782,25 +793,16 @@ static void bq25890_usb_work(struct work_struct *data)
>>  	switch (bq->usb_event) {
>>  	case USB_EVENT_ID:
>>  		/* Enable boost mode */
>> -		ret = bq25890_field_write(bq, F_OTG_CFG, 1);
>> -		if (ret < 0)
>> -			goto error;
>> +		bq25890_set_otg_cfg(bq, 1);
>>  		break;
>>  
>>  	case USB_EVENT_NONE:
>>  		/* Disable boost mode */
>> -		ret = bq25890_field_write(bq, F_OTG_CFG, 0);
>> -		if (ret < 0)
>> -			goto error;
>> -
>> -		power_supply_changed(bq->charger);
>> +		ret = bq25890_set_otg_cfg(bq, 0);
>> +		if (ret == 0)
>> +			power_supply_changed(bq->charger);
>>  		break;
>>  	}
>> -
>> -	return;
>> -
>> -error:
>> -	dev_err(bq->dev, "Error switching to boost/charger mode.\n");
>>  }
>>  
>>  static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
>> -- 
>> 2.31.1
>>
>