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

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

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


>