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

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

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