| Message ID | 20221127180233.103678-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | power: supply: bq25890: Fixes for 6.2 + further work for 6.3 | expand |
On 11/27/22 19:02, Hans de Goede wrote: > The pump_express_work which gets queued from an external_power_changed > callback might be pending / running on remove() (or on probe failure). > > Add a devm action cancelling the work, to ensure that it is cancelled. > > Note the devm action is added before devm_power_supply_register(), making > it run after devm unregisters the power_supply, so that the work cannot > be queued anymore (this is also why a devm action is used for this). > > Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> A comment in the code matching the last paragraph of this commit message would be helpful I think. Reviewed-by: Marek Vasut <marex@denx.de>
On 11/27/22 19:02, Hans de Goede wrote: > The recent "power: supply: bq25890: Add HiZ mode support" change > leaves F_CONV_RATE rate unset when disabling HiZ mode (setting > POWER_SUPPLY_PROP_ONLINE to 1) while a charger is connected. > > Separate the resetting HiZ mode when necessary because of a charger > (re)plug event into its own if which runs first. I think this one sentence needs rephrasing ^ . > And fix the setting of F_CONV_RATE rate by adding helper variables for > the old and new F_CONV_RATE state which check both the online and hiz bits > and then compare the helper variables to see if a F_CONV_RATE update is > necessary. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Marek Vasut <marex@denx.de>
On 11/27/22 19:02, Hans de Goede wrote: > The code to check if F_CONV_RATE has been set, or if a manual ADC > conversion needs to be triggered, as well as the code to set > the initial F_CONV_RATE value at probe both where not taking > HiZ mode into account. Add checks for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Reviewed-by: Marek Vasut <marex@denx.de>
On 11/27/22 19:02, Hans de Goede wrote: > Some devices, such as the Lenovo Yoga Tab 3 Pro (YT3-X90F) have multiple > batteries with a separate bq25890 charger for each battery. > > This requires some coordination between the chargers specifically > the main charger needs to put the secondary charger in Hi-Z mode when: > > 1. Enabling its 5V boost (OTG) output to power an external USB device, > to avoid the secondary charger IC seeing this as external Vbus and > then trying to charge the secondary battery from this. > > 2. Talking the Pump Express protocol to increase the external Vbus voltage. > Having the secondary charger drawing current when the main charger is > trying to talk the Pump Express protocol results in the external Vbus > voltage not being raised. > > Add a new "linux,secondary-charger-name" string device-property, which > can be set to the power_supply class device's name of the secondary > charger when there is a secondary charger; and make the Vbus regulator and > Pump Express code put the secondary charger in Hi-Z mode when necessary. > > So far this new property is only used on x86/ACPI (non devicetree) devs, > IOW it is not used in actual devicetree files. The devicetree-bindings > maintainers have requested properties like these to not be added to the > devicetree-bindings, so the new property is deliberately not added > to the existing devicetree-bindings. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I kind-of wonder whether there shouldn't be some generic implementation of this kind of charger interaction on subsystem level. But maybe that is something for a subsequent series. Reviewed-by: Marek Vasut <marex@denx.de>
Hi, On Sun, Nov 27, 2022 at 07:02:25PM +0100, Hans de Goede wrote: > The pump_express_work which gets queued from an external_power_changed > callback might be pending / running on remove() (or on probe failure). > > Add a devm action cancelling the work, to ensure that it is cancelled. > > Note the devm action is added before devm_power_supply_register(), making > it run after devm unregisters the power_supply, so that the work cannot > be queued anymore (this is also why a devm action is used for this). > > Fixes: 48f45b094dbb ("power: supply: bq25890: Support higher charging voltages through Pump Express+ protocol") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/power/supply/bq25890_charger.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index 512c81662eea..30d77afab839 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -1317,6 +1317,13 @@ static int bq25890_fw_probe(struct bq25890_device *bq) > return 0; > } > > +static void bq25890_non_devm_cleanup(void *data) > +{ > + struct bq25890_device *bq = data; > + > + cancel_delayed_work_sync(&bq->pump_express_work); > +} > + > static int bq25890_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > @@ -1372,6 +1379,10 @@ static int bq25890_probe(struct i2c_client *client) > /* OTG reporting */ > bq->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > > + ret = devm_add_action_or_reset(dev, bq25890_non_devm_cleanup, bq); > + if (ret) > + return ret; ret = devm_delayed_work_autocancel(dev, &bq->pump_express_work, bq25890_pump_express_work); if (ret) return ret; (+ removing the INIT_DELAYED_WORK) -- Sebastian > + > ret = bq25890_register_regulator(bq); > if (ret) > return ret; > -- > 2.38.1 >
Hi Sebastian, Marek, I have been working on getting a Lenovo Yoga Tab 3 Pro (YT3-X90F) to work with the mainline kernel. This tablet has 2 batteries with 2 bq25892 chargers both connected to a single Micro-USB connector. Supporting the 2 charger board also requires the recent HiZ mode patches from Marek, to avoid merging order problems / conflicts I have included a copy of Marek's series here so this series obsoletes the: [PATCH 1/2] power: supply: bq25890: Factor out chip state update [PATCH 2/2] power: supply: bq25890: Add HiZ mode support [PATCH v2 1/2] power: supply: bq25890: Factor out chip state update [PATCH v2 2/2] power: supply: bq25890: Add HiZ mode support patches from Marek. While working on adding support for this I also noticed some generic issues with the bq25890 driver currently in linux-power-supply/for-next and I also have some fixes to the HiZ support on top of Marek's support. So this entire series consist of 4 parts: 1. Patches 1-3: Generic bugfixes for the bq25890 charger in its current state in linux-power-supply/for-next. Patch 1/10 fixes an actual regression on some boards with for-next so that one definitely needs to go to 6.2. The other 2 fixes can go to either 6.2 or 6.3 2. Patches 4-5: Marek's HiZ support work, thank you Marek. 3. Patches 6-7: Some fixes / improvements from me to Marek's HiZ support. 4. Patch 8-10: The actual support for boards with 2 chargers. Stating the obvious here: given where we are in the cycle I expect parts 2-4 / patches 4-10 to all be 6.3 material. Regards, Hans Hans de Goede (8): power: supply: bq25890: Only use pdata->regulator_init_data for vbus power: supply: bq25890: Ensure pump_express_work is cancelled on remove power: supply: bq25890: Fix usb-notifier probe and remove races power: supply: bq25890: Fix setting of F_CONV_RATE rate when disabling HiZ mode power: supply: bq25890: Always take HiZ mode into account for ADC rate power: supply: bq25890: Support boards with more then one charger IC power: supply: bq25890: Add support for having a secondary charger IC power: supply: bq25890: Add new linux,iinlim-percentage property Marek Vasut (2): power: supply: bq25890: Factor out chip state update power: supply: bq25890: Add HiZ mode support drivers/platform/x86/x86-android-tablets.c | 2 +- drivers/power/supply/bq25890_charger.c | 213 ++++++++++++++++----- 2 files changed, 169 insertions(+), 46 deletions(-)