| Message ID | 20221126120849.74632-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | d1b25092b3dce96a69fc31b462e61291848fda9f |
| 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 bfdd2213ba69a..374ab66ba8770 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -454,20 +454,18 @@ static int bq25890_get_vbus_voltage(struct bq25890_device *bq) return bq25890_find_val(ret, TBL_VBUSV); } -static int bq25890_power_supply_get_property(struct power_supply *psy, - enum power_supply_property psp, - union power_supply_propval *val) +static void bq25890_update_state(struct bq25890_device *bq, + enum power_supply_property psp, + struct bq25890_state *state) { - struct bq25890_device *bq = power_supply_get_drvdata(psy); - struct bq25890_state state; bool do_adc_conv; int ret; mutex_lock(&bq->lock); /* update state in case we lost an interrupt */ __bq25890_handle_irq(bq); - state = bq->state; - do_adc_conv = !state.online && bq25890_is_adc_property(psp); + *state = bq->state; + do_adc_conv = !state->online && bq25890_is_adc_property(psp); if (do_adc_conv) bq25890_field_write(bq, F_CONV_START, 1); mutex_unlock(&bq->lock); @@ -475,6 +473,17 @@ static int bq25890_power_supply_get_property(struct power_supply *psy, if (do_adc_conv) regmap_field_read_poll_timeout(bq->rmap_fields[F_CONV_START], ret, !ret, 25000, 1000000); +} + +static int bq25890_power_supply_get_property(struct power_supply *psy, + enum power_supply_property psp, + union power_supply_propval *val) +{ + struct bq25890_device *bq = power_supply_get_drvdata(psy); + struct bq25890_state state; + int ret; + + bq25890_update_state(bq, psp, &state); switch (psp) { case POWER_SUPPLY_PROP_STATUS: