| Message ID | 20211030182813.116672-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power-suppy/i2c/extcon: Add support for cht-wc PMIC without USB-PD support | expand |
On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Add a bq25890_set_otg_cfg() helper function, this is a preparation > patch for adding regulator support. ... > switch (bq->usb_event) { > case USB_EVENT_ID: > /* Enable boost mode */ > - ret = bq25890_field_write(bq, F_OTG_CFG, 1); > - if (ret < 0) > - goto error; > + bq25890_set_otg_cfg(bq, 1); > break; > > case USB_EVENT_NONE: > /* Disable boost mode */ > - ret = bq25890_field_write(bq, F_OTG_CFG, 0); > - if (ret < 0) > - goto error; > - > - power_supply_changed(bq->charger); > + ret = bq25890_set_otg_cfg(bq, 0); > + if (ret == 0) > + power_supply_changed(bq->charger); > break; > } While at it, default: break; ?
On Sat, Oct 30, 2021 at 08:28:09PM +0200, Hans de Goede wrote: > Add a bq25890_set_otg_cfg() helper function, this is a preparation > patch for adding regulator support. The same notice here: if we enabling the boost mode to supply USB OTG device, we should disable charging and vice versa. I don't remember if enabling of OTG with CHG enabled really caused issues but I avoided this in my Yoga Book work. I made quick check — seems that charging can be enabled during of boost operation, there are no visible side effects in registers and no limitation in the datasheet, so your approach may be good. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------ > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 163ca5d761aa..7504e30f1e4d 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -773,6 +773,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq) > return PTR_ERR_OR_ZERO(bq->charger); > } > > +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val) > +{ > + int ret; > + > + ret = bq25890_field_write(bq, F_OTG_CFG, val); > + if (ret < 0) > + dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret); > + > + return ret; > +} > + > static void bq25890_usb_work(struct work_struct *data) > { > int ret; > @@ -782,25 +793,16 @@ static void bq25890_usb_work(struct work_struct *data) > switch (bq->usb_event) { > case USB_EVENT_ID: > /* Enable boost mode */ > - ret = bq25890_field_write(bq, F_OTG_CFG, 1); > - if (ret < 0) > - goto error; > + bq25890_set_otg_cfg(bq, 1); > break; > > case USB_EVENT_NONE: > /* Disable boost mode */ > - ret = bq25890_field_write(bq, F_OTG_CFG, 0); > - if (ret < 0) > - goto error; > - > - power_supply_changed(bq->charger); > + ret = bq25890_set_otg_cfg(bq, 0); > + if (ret == 0) > + power_supply_changed(bq->charger); > break; > } > - > - return; > - > -error: > - dev_err(bq->dev, "Error switching to boost/charger mode.\n"); > } > > static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val, > -- > 2.31.1 >
Hi, On 11/7/21 19:49, Yauhen Kharuzhy wrote: > On Sat, Oct 30, 2021 at 08:28:09PM +0200, Hans de Goede wrote: >> Add a bq25890_set_otg_cfg() helper function, this is a preparation >> patch for adding regulator support. > > The same notice here: if we enabling the boost mode to supply USB OTG > device, we should disable charging and vice versa. I don't remember if > enabling of OTG with CHG enabled really caused issues but I avoided this > in my Yoga Book work. > > I made quick check — seems that charging can be enabled during of boost > operation, there are no visible side effects in registers and no > limitation in the datasheet, so your approach may be good. Right, the charging bit can simply be always on, if the OTG boost converter is enabled then the charger it will automatically stop charging (AFAIK it is partly re-using the same hw even). I've no idea why the charging bit is being cleared by the BIOS, there really is no need for this, with the exception of either: 1. Using an external Vbus boost converter, because that can e.g. deliver more current, then charging does need to be disabled. 2. Disabling charging for battery health reasons 1. is not the case here; and 2. is something which we could export as something userspace can request but that is optional. So for normal use there really is no reason to set the charging bit to 0 ever (and it is 1 on reset), so just setting it to 1 makes the behavior the same as on many ARM devices where this charger is also used and the same as how similar chargers like the bq2419x and bq2429x are used on other X86 devices. Regards, Hans > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------ >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c >> index 163ca5d761aa..7504e30f1e4d 100644 >> --- a/drivers/power/supply/bq25890_charger.c >> +++ b/drivers/power/supply/bq25890_charger.c >> @@ -773,6 +773,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq) >> return PTR_ERR_OR_ZERO(bq->charger); >> } >> >> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val) >> +{ >> + int ret; >> + >> + ret = bq25890_field_write(bq, F_OTG_CFG, val); >> + if (ret < 0) >> + dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret); >> + >> + return ret; >> +} >> + >> static void bq25890_usb_work(struct work_struct *data) >> { >> int ret; >> @@ -782,25 +793,16 @@ static void bq25890_usb_work(struct work_struct *data) >> switch (bq->usb_event) { >> case USB_EVENT_ID: >> /* Enable boost mode */ >> - ret = bq25890_field_write(bq, F_OTG_CFG, 1); >> - if (ret < 0) >> - goto error; >> + bq25890_set_otg_cfg(bq, 1); >> break; >> >> case USB_EVENT_NONE: >> /* Disable boost mode */ >> - ret = bq25890_field_write(bq, F_OTG_CFG, 0); >> - if (ret < 0) >> - goto error; >> - >> - power_supply_changed(bq->charger); >> + ret = bq25890_set_otg_cfg(bq, 0); >> + if (ret == 0) >> + power_supply_changed(bq->charger); >> break; >> } >> - >> - return; >> - >> -error: >> - dev_err(bq->dev, "Error switching to boost/charger mode.\n"); >> } >> >> static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val, >> -- >> 2.31.1 >> >
Hi, On 10/31/21 00:10, Andy Shevchenko wrote: > On Sat, Oct 30, 2021 at 9:28 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Add a bq25890_set_otg_cfg() helper function, this is a preparation >> patch for adding regulator support. > > ... > >> switch (bq->usb_event) { >> case USB_EVENT_ID: >> /* Enable boost mode */ >> - ret = bq25890_field_write(bq, F_OTG_CFG, 1); >> - if (ret < 0) >> - goto error; >> + bq25890_set_otg_cfg(bq, 1); >> break; >> >> case USB_EVENT_NONE: >> /* Disable boost mode */ >> - ret = bq25890_field_write(bq, F_OTG_CFG, 0); >> - if (ret < 0) >> - goto error; >> - >> - power_supply_changed(bq->charger); >> + ret = bq25890_set_otg_cfg(bq, 0); >> + if (ret == 0) >> + power_supply_changed(bq->charger); >> break; >> } > > While at it, > > default: > break; bq->usb_event is not an enum, so there is no need for this. Regards, Hans
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 163ca5d761aa..7504e30f1e4d 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -773,6 +773,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq) return PTR_ERR_OR_ZERO(bq->charger); } +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val) +{ + int ret; + + ret = bq25890_field_write(bq, F_OTG_CFG, val); + if (ret < 0) + dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret); + + return ret; +} + static void bq25890_usb_work(struct work_struct *data) { int ret; @@ -782,25 +793,16 @@ static void bq25890_usb_work(struct work_struct *data) switch (bq->usb_event) { case USB_EVENT_ID: /* Enable boost mode */ - ret = bq25890_field_write(bq, F_OTG_CFG, 1); - if (ret < 0) - goto error; + bq25890_set_otg_cfg(bq, 1); break; case USB_EVENT_NONE: /* Disable boost mode */ - ret = bq25890_field_write(bq, F_OTG_CFG, 0); - if (ret < 0) - goto error; - - power_supply_changed(bq->charger); + ret = bq25890_set_otg_cfg(bq, 0); + if (ret == 0) + power_supply_changed(bq->charger); break; } - - return; - -error: - dev_err(bq->dev, "Error switching to boost/charger mode.\n"); } static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
Add a bq25890_set_otg_cfg() helper function, this is a preparation patch for adding regulator support. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------ 1 file changed, 15 insertions(+), 13 deletions(-)