| Message ID | 20220130204557.15662-13-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power-suppy/i2c/extcon: Fix charger setup on Xiaomi Mi Pad 2 and Lenovo Yogabook | expand |
On Sun, Jan 30, 2022 at 09:45:49PM +0100, Hans de Goede wrote: > From: Yauhen Kharuzhy <jekhor@gmail.com> > > Add a "linux,pump-express-vbus-max" property which indicates if the Pump > Express+ protocol should be used to increase the charging protocol. > > If this new property is set and a DCP charger is detected then request > a higher charging voltage through the Pump Express+ protocol. > > 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. > > Changes by Hans de Goede: > - Port to my bq25890 patch-series + various cleanups > - Make behavior configurable through a new "linux,pump-express-vbus-max" > device-property > - Sleep 1 second before re-checking the Vbus voltage after requesting > it to be raised, to ensure that the ADC has time to sampled the new Vbus > - Add VBUSV bq25890_tables[] entry and use it in bq25890_get_vbus_voltage() > - Tweak commit message ... > +static void bq25890_pump_express_work(struct work_struct *data) > +{ > + struct bq25890_device *bq = > + container_of(data, struct bq25890_device, pump_express_work.work); > + int voltage, i, ret; > + > + dev_dbg(bq->dev, "Start to request input voltage increasing\n"); > + > + /* Enable current pulse voltage control protocol */ > + ret = bq25890_field_write(bq, F_PUMPX_EN, 1); > + if (ret < 0) > + goto error_print; > + > + for (i = 0; i < PUMP_EXPRESS_MAX_TRIES; i++) { > + voltage = bq25890_get_vbus_voltage(bq); > + if (voltage < 0) > + goto error_print; It also can be (at least in align with the rest error paths) ret = bq25890_get_vbus_voltage(bq); if (ret < 0) goto error_print; voltage = ret; followed up (but not necessarily)... > + dev_dbg(bq->dev, "input voltage = %d uV\n", voltage); > + > + if ((voltage + PUMP_EXPRESS_VBUS_MARGIN_uV) > > + bq->pump_express_vbus_max) > + break; > + > + ret = bq25890_field_write(bq, F_PUMPX_UP, 1); > + if (ret < 0) > + goto error_print; > + > + /* Note a single PUMPX up pulse-sequence takes 2.1s */ > + ret = regmap_field_read_poll_timeout(bq->rmap_fields[F_PUMPX_UP], > + ret, !ret, 100000, 3000000); > + if (ret < 0) > + goto error_print; > + > + /* Make sure ADC has sampled Vbus before checking again */ > + msleep(1000); > + } > + > + bq25890_field_write(bq, F_PUMPX_EN, 0); > + > + dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n", > + voltage); > + return; > +error_print: if (ret < 0) But it's up to you. > + dev_err(bq->dev, "Failed to request hi-voltage charging\n"); > +}
Hi, On 1/31/22 14:48, Andy Shevchenko wrote: > On Sun, Jan 30, 2022 at 09:45:49PM +0100, Hans de Goede wrote: >> From: Yauhen Kharuzhy <jekhor@gmail.com> >> >> Add a "linux,pump-express-vbus-max" property which indicates if the Pump >> Express+ protocol should be used to increase the charging protocol. >> >> If this new property is set and a DCP charger is detected then request >> a higher charging voltage through the Pump Express+ protocol. >> >> 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. >> >> Changes by Hans de Goede: >> - Port to my bq25890 patch-series + various cleanups >> - Make behavior configurable through a new "linux,pump-express-vbus-max" >> device-property >> - Sleep 1 second before re-checking the Vbus voltage after requesting >> it to be raised, to ensure that the ADC has time to sampled the new Vbus >> - Add VBUSV bq25890_tables[] entry and use it in bq25890_get_vbus_voltage() >> - Tweak commit message > > ... > >> +static void bq25890_pump_express_work(struct work_struct *data) >> +{ >> + struct bq25890_device *bq = >> + container_of(data, struct bq25890_device, pump_express_work.work); >> + int voltage, i, ret; >> + >> + dev_dbg(bq->dev, "Start to request input voltage increasing\n"); >> + >> + /* Enable current pulse voltage control protocol */ >> + ret = bq25890_field_write(bq, F_PUMPX_EN, 1); >> + if (ret < 0) >> + goto error_print; >> + >> + for (i = 0; i < PUMP_EXPRESS_MAX_TRIES; i++) { > >> + voltage = bq25890_get_vbus_voltage(bq); >> + if (voltage < 0) >> + goto error_print; > > It also can be (at least in align with the rest error paths) > > ret = bq25890_get_vbus_voltage(bq); > if (ret < 0) > goto error_print; > voltage = ret; > > followed up (but not necessarily)... The suggested pattern is useful when ret needs to be set on the error-exit path, but we are not doing that here. So I prefer to just keep this as is. Regards, Hans > >> + dev_dbg(bq->dev, "input voltage = %d uV\n", voltage); >> + >> + if ((voltage + PUMP_EXPRESS_VBUS_MARGIN_uV) > >> + bq->pump_express_vbus_max) >> + break; >> + >> + ret = bq25890_field_write(bq, F_PUMPX_UP, 1); >> + if (ret < 0) >> + goto error_print; >> + >> + /* Note a single PUMPX up pulse-sequence takes 2.1s */ >> + ret = regmap_field_read_poll_timeout(bq->rmap_fields[F_PUMPX_UP], >> + ret, !ret, 100000, 3000000); >> + if (ret < 0) >> + goto error_print; >> + >> + /* Make sure ADC has sampled Vbus before checking again */ >> + msleep(1000); >> + } >> + >> + bq25890_field_write(bq, F_PUMPX_EN, 0); >> + >> + dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n", >> + voltage); > >> + return; >> +error_print: > > if (ret < 0) > > But it's up to you. > >> + dev_err(bq->dev, "Failed to request hi-voltage charging\n"); >> +} >
On Mon, Jan 31, 2022 at 04:18:23PM +0100, Hans de Goede wrote: > On 1/31/22 14:48, Andy Shevchenko wrote: > > On Sun, Jan 30, 2022 at 09:45:49PM +0100, Hans de Goede wrote: ... > >> + for (i = 0; i < PUMP_EXPRESS_MAX_TRIES; i++) { > > > >> + voltage = bq25890_get_vbus_voltage(bq); > >> + if (voltage < 0) > >> + goto error_print; > > > > It also can be (at least in align with the rest error paths) > > > > ret = bq25890_get_vbus_voltage(bq); > > if (ret < 0) > > goto error_print; > > voltage = ret; > > > > followed up (but not necessarily)... > > The suggested pattern is useful when ret needs to be set on the error-exit > path, but we are not doing that here. So I prefer to just keep this as is. Are you talking about above proposal? Still wouldn't be better to use it that if we want, for example, to print an error code, it can be done easily? For the sake of consistency. > >> + } > >> + > >> + bq25890_field_write(bq, F_PUMPX_EN, 0); > >> + > >> + dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n", > >> + voltage); > > > >> + return; > >> +error_print: > > > > if (ret < 0) > > > > But it's up to you. > > > >> + dev_err(bq->dev, "Failed to request hi-voltage charging\n");
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index 3de72f0fbf3e..179abed92f9b 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -27,6 +27,10 @@ #define BQ25895_ID 7 #define BQ25896_ID 0 +#define PUMP_EXPRESS_START_DELAY (5 * HZ) +#define PUMP_EXPRESS_MAX_TRIES 6 +#define PUMP_EXPRESS_VBUS_MARGIN_uV 1000000 + enum bq25890_chip_version { BQ25890, BQ25892, @@ -107,6 +111,7 @@ struct bq25890_device { struct usb_phy *usb_phy; struct notifier_block usb_nb; struct work_struct usb_work; + struct delayed_work pump_express_work; unsigned long usb_event; struct regmap *rmap; @@ -114,6 +119,7 @@ struct bq25890_device { bool skip_reset; bool read_back_init_data; + u32 pump_express_vbus_max; enum bq25890_chip_version chip_version; struct bq25890_init_data init_data; struct bq25890_state state; @@ -265,6 +271,7 @@ enum bq25890_table_ids { TBL_VREG, TBL_BOOSTV, TBL_SYSVMIN, + TBL_VBUSV, TBL_VBATCOMP, TBL_RBATCOMP, @@ -325,14 +332,15 @@ static const union { } bq25890_tables[] = { /* range tables */ /* TODO: BQ25896 has max ICHG 3008 mA */ - [TBL_ICHG] = { .rt = {0, 5056000, 64000} }, /* uA */ - [TBL_ITERM] = { .rt = {64000, 1024000, 64000} }, /* uA */ - [TBL_IINLIM] = { .rt = {100000, 3250000, 50000} }, /* uA */ - [TBL_VREG] = { .rt = {3840000, 4608000, 16000} }, /* uV */ - [TBL_BOOSTV] = { .rt = {4550000, 5510000, 64000} }, /* uV */ - [TBL_SYSVMIN] = { .rt = {3000000, 3700000, 100000} }, /* uV */ - [TBL_VBATCOMP] ={ .rt = {0, 224000, 32000} }, /* uV */ - [TBL_RBATCOMP] ={ .rt = {0, 140000, 20000} }, /* uOhm */ + [TBL_ICHG] = { .rt = {0, 5056000, 64000} }, /* uA */ + [TBL_ITERM] = { .rt = {64000, 1024000, 64000} }, /* uA */ + [TBL_IINLIM] = { .rt = {100000, 3250000, 50000} }, /* uA */ + [TBL_VREG] = { .rt = {3840000, 4608000, 16000} }, /* uV */ + [TBL_BOOSTV] = { .rt = {4550000, 5510000, 64000} }, /* uV */ + [TBL_SYSVMIN] = { .rt = {3000000, 3700000, 100000} }, /* uV */ + [TBL_VBUSV] = { .rt = {2600000, 15300000, 100000} }, /* uV */ + [TBL_VBATCOMP] = { .rt = {0, 224000, 32000} }, /* uV */ + [TBL_RBATCOMP] = { .rt = {0, 140000, 20000} }, /* uOhm */ /* lookup tables */ [TBL_TREG] = { .lt = {bq25890_treg_tbl, BQ25890_TREG_TBL_SIZE} }, @@ -435,6 +443,17 @@ static bool bq25890_is_adc_property(enum power_supply_property psp) static irqreturn_t __bq25890_handle_irq(struct bq25890_device *bq); +static int bq25890_get_vbus_voltage(struct bq25890_device *bq) +{ + int ret; + + ret = bq25890_field_read(bq, F_VBUSV); + if (ret < 0) + return ret; + + 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) @@ -613,6 +632,11 @@ static void bq25890_charger_external_power_changed(struct power_supply *psy) switch (val.intval) { case POWER_SUPPLY_USB_TYPE_DCP: input_current_limit = bq25890_find_idx(2000000, TBL_IINLIM); + if (bq->pump_express_vbus_max) { + queue_delayed_work(system_power_efficient_wq, + &bq->pump_express_work, + PUMP_EXPRESS_START_DELAY); + } break; case POWER_SUPPLY_USB_TYPE_CDP: case POWER_SUPPLY_USB_TYPE_ACA: @@ -878,6 +902,53 @@ static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val) return ret; } +static void bq25890_pump_express_work(struct work_struct *data) +{ + struct bq25890_device *bq = + container_of(data, struct bq25890_device, pump_express_work.work); + int voltage, i, ret; + + dev_dbg(bq->dev, "Start to request input voltage increasing\n"); + + /* Enable current pulse voltage control protocol */ + ret = bq25890_field_write(bq, F_PUMPX_EN, 1); + if (ret < 0) + goto error_print; + + for (i = 0; i < PUMP_EXPRESS_MAX_TRIES; i++) { + voltage = bq25890_get_vbus_voltage(bq); + if (voltage < 0) + goto error_print; + dev_dbg(bq->dev, "input voltage = %d uV\n", voltage); + + if ((voltage + PUMP_EXPRESS_VBUS_MARGIN_uV) > + bq->pump_express_vbus_max) + break; + + ret = bq25890_field_write(bq, F_PUMPX_UP, 1); + if (ret < 0) + goto error_print; + + /* Note a single PUMPX up pulse-sequence takes 2.1s */ + ret = regmap_field_read_poll_timeout(bq->rmap_fields[F_PUMPX_UP], + ret, !ret, 100000, 3000000); + if (ret < 0) + goto error_print; + + /* Make sure ADC has sampled Vbus before checking again */ + msleep(1000); + } + + bq25890_field_write(bq, F_PUMPX_EN, 0); + + dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n", + voltage); + + return; +error_print: + dev_err(bq->dev, "Failed to request hi-voltage charging\n"); +} + static void bq25890_usb_work(struct work_struct *data) { int ret; @@ -1068,6 +1139,10 @@ static int bq25890_fw_probe(struct bq25890_device *bq) int ret; struct bq25890_init_data *init = &bq->init_data; + /* Optional, left at 0 if property is not present */ + device_property_read_u32(bq->dev, "linux,pump-express-vbus-max", + &bq->pump_express_vbus_max); + bq->skip_reset = device_property_read_bool(bq->dev, "linux,skip-reset"); bq->read_back_init_data = device_property_read_bool(bq->dev, "linux,read-back-settings"); @@ -1100,6 +1175,7 @@ static int bq25890_probe(struct i2c_client *client, bq->dev = dev; mutex_init(&bq->lock); + INIT_DELAYED_WORK(&bq->pump_express_work, bq25890_pump_express_work); bq->rmap = devm_regmap_init_i2c(client, &bq25890_regmap_config); if (IS_ERR(bq->rmap))