| Message ID | 20221126120849.74632-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v2,1/2] power: supply: bq25890: Factor out chip state update | expand |
Hi Marek, On 11/26/22 13:08, 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. > > The IRQ handler reinstates the HiZ mode in case a cable is replugged by > the user, the chip itself clears the HiZ mode bit when cable is plugged > in by the user and the chip detects PG bad-to-good transition. > > 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> > --- > V2: - Mix HiZ mode check into POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_CHARGE_TYPE, POWER_SUPPLY_PROP_ONLINE > read back, so those behave as if the system was offline > in case HiZ mode is enabled and cable is plugged in > - Cache user HiZ configuration in bq->force_hiz, reinstate > the user HiZ configuration in IRQ handler on offline to > online transition as the chip clears the HiZ bit on that > transition when the cable is replugged. > --- > drivers/power/supply/bq25890_charger.c | 58 +++++++++++++++++++------- > 1 file changed, 44 insertions(+), 14 deletions(-) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 374ab66ba8770..e40c8a94cf2e1 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; > @@ -119,6 +120,7 @@ struct bq25890_device { > > bool skip_reset; > bool read_back_init_data; > + bool force_hiz; > u32 pump_express_vbus_max; > enum bq25890_chip_version chip_version; > struct bq25890_init_data init_data; > @@ -487,7 +489,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, > > switch (psp) { > case POWER_SUPPLY_PROP_STATUS: > - if (!state.online) > + if (!state.online || state.hiz) > val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > else if (state.chrg_status == STATUS_NOT_CHARGING) > val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > @@ -502,7 +504,8 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, > break; > > case POWER_SUPPLY_PROP_CHARGE_TYPE: > - if (!state.online || state.chrg_status == STATUS_NOT_CHARGING || > + if (!state.online || state.hiz || > + state.chrg_status == STATUS_NOT_CHARGING || > state.chrg_status == STATUS_TERMINATION_DONE) > val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; > else if (state.chrg_status == STATUS_PRE_CHARGING) > @@ -522,7 +525,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, > break; > > case POWER_SUPPLY_PROP_ONLINE: > - val->intval = state.online; > + val->intval = state.online & !state.hiz; Please use "&&" instead of "&" here, since these are both 1 bit values the "&" will also work but "&&" better expresses that this is a boolean compare and you use "||" in the negated cases above, so using "&&" is consistent with that. I have fixed this up in my local copy of the patch. I have also noticed some other issues, which are best addressed with a follow-up patch. Once I have run a few final tests I plan to submit a bigger bq25890_charger patch series, which includes a bugfix to your previous series, as well as a few follow up patches to this series. To make things easier for Sebastian, I'm going to include your patches in my bigger series, making that series look like this: 1. A couple of bug fixes for the current bq25890 code 2. Your patches (this series) with the mentioned small "&" -> "&&" squashed in + my Reviewed-by 3. Some follow up patches from me to this series 4. My recent patches building on top of this series. This way Sebastian can apply all the patches without conflict, which I hope makes things easier for him. Marek, I will Cc you on the entire series. Regards, Hans > break; > > case POWER_SUPPLY_PROP_HEALTH: > @@ -676,7 +679,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 +695,12 @@ 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); > + if (!ret) > + bq->force_hiz = !val->intval; > + bq25890_update_state(bq, psp, &state); > + return ret; > default: > return -EINVAL; > } > @@ -703,6 +713,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 +768,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 +784,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,16 +805,33 @@ 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 */ > - /* enable ADC, to have control of charge current/voltage */ > - ret = bq25890_field_write(bq, F_CONV_RATE, 1); > - if (ret < 0) > - goto error; > + } else if (new_state.online && !bq->state.online) { > + /* > + * Restore HiZ bit in case it was set by user. > + * The chip does not retain this bit once the > + * cable is re-plugged, hence the bit must be > + * reset manually here. > + */ > + if (bq->force_hiz) { > + ret = bq25890_field_write(bq, F_EN_HIZ, bq->force_hiz); > + if (ret < 0) > + goto error; > + new_state.hiz = 1; > + } > + > + if (!new_state.hiz) { > + /* 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) > + goto error; > + } > } > > bq->state = new_state;
On 11/27/22 17:43, Hans de Goede wrote: Hi, [...] >> @@ -522,7 +525,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, >> break; >> >> case POWER_SUPPLY_PROP_ONLINE: >> - val->intval = state.online; >> + val->intval = state.online & !state.hiz; > > Please use "&&" instead of "&" here, since these are both 1 bit values the "&" > will also work but "&&" better expresses that this is a boolean compare and you > use "||" in the negated cases above, so using "&&" is consistent with that. > > I have fixed this up in my local copy of the patch. > > I have also noticed some other issues, which are best addressed with a follow-up > patch. > > Once I have run a few final tests I plan to submit a bigger bq25890_charger > patch series, which includes a bugfix to your previous series, as well as > a few follow up patches to this series. > > To make things easier for Sebastian, I'm going to include your patches > in my bigger series, making that series look like this: > > 1. A couple of bug fixes for the current bq25890 code > 2. Your patches (this series) with the mentioned small "&" -> "&&" squashed in + my Reviewed-by > 3. Some follow up patches from me to this series > 4. My recent patches building on top of this series. > > This way Sebastian can apply all the patches without conflict, > which I hope makes things easier for him. > > Marek, I will Cc you on the entire series. Sounds good, thanks !
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 374ab66ba8770..e40c8a94cf2e1 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; @@ -119,6 +120,7 @@ struct bq25890_device { bool skip_reset; bool read_back_init_data; + bool force_hiz; u32 pump_express_vbus_max; enum bq25890_chip_version chip_version; struct bq25890_init_data init_data; @@ -487,7 +489,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, switch (psp) { case POWER_SUPPLY_PROP_STATUS: - if (!state.online) + if (!state.online || state.hiz) val->intval = POWER_SUPPLY_STATUS_DISCHARGING; else if (state.chrg_status == STATUS_NOT_CHARGING) val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; @@ -502,7 +504,8 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, break; case POWER_SUPPLY_PROP_CHARGE_TYPE: - if (!state.online || state.chrg_status == STATUS_NOT_CHARGING || + if (!state.online || state.hiz || + state.chrg_status == STATUS_NOT_CHARGING || state.chrg_status == STATUS_TERMINATION_DONE) val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE; else if (state.chrg_status == STATUS_PRE_CHARGING) @@ -522,7 +525,7 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, break; case POWER_SUPPLY_PROP_ONLINE: - val->intval = state.online; + val->intval = state.online & !state.hiz; break; case POWER_SUPPLY_PROP_HEALTH: @@ -676,7 +679,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 +695,12 @@ 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); + if (!ret) + bq->force_hiz = !val->intval; + bq25890_update_state(bq, psp, &state); + return ret; default: return -EINVAL; } @@ -703,6 +713,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 +768,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 +784,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,16 +805,33 @@ 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 */ - /* enable ADC, to have control of charge current/voltage */ - ret = bq25890_field_write(bq, F_CONV_RATE, 1); - if (ret < 0) - goto error; + } else if (new_state.online && !bq->state.online) { + /* + * Restore HiZ bit in case it was set by user. + * The chip does not retain this bit once the + * cable is re-plugged, hence the bit must be + * reset manually here. + */ + if (bq->force_hiz) { + ret = bq25890_field_write(bq, F_EN_HIZ, bq->force_hiz); + if (ret < 0) + goto error; + new_state.hiz = 1; + } + + if (!new_state.hiz) { + /* 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) + goto error; + } } bq->state = new_state;
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. The IRQ handler reinstates the HiZ mode in case a cable is replugged by the user, the chip itself clears the HiZ mode bit when cable is plugged in by the user and the chip detects PG bad-to-good transition. 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> --- V2: - Mix HiZ mode check into POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_CHARGE_TYPE, POWER_SUPPLY_PROP_ONLINE read back, so those behave as if the system was offline in case HiZ mode is enabled and cable is plugged in - Cache user HiZ configuration in bq->force_hiz, reinstate the user HiZ configuration in IRQ handler on offline to online transition as the chip clears the HiZ bit on that transition when the cable is replugged. --- drivers/power/supply/bq25890_charger.c | 58 +++++++++++++++++++------- 1 file changed, 44 insertions(+), 14 deletions(-)