| Message ID | 20221109221504.79562-2-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] power: supply: bq25890: Factor out chip state update | expand |
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)
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. :)
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) >
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. >> 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 ... [...]
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. > 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 ? > 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.
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 >
On 11/26/22 16:50, Hans de Goede wrote: > Hi, Hi, [...] >>> 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. Glad I could help. I dropped the RB from v2 2/2 since there are changes which could use review. If you could have a look again at that patch, that would be nice.
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)
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> --- drivers/power/supply/bq25890_charger.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)