| Message ID | 20221127180233.103678-11-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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: > 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 the maximum current the external power-supply can deliver > to be divided over the chargers. The Android vendor kernel shipped > on the YT3-X90F divides this current with a 40/60 percent split so that > batteries are done charging at approx. the same time if both were fully > empty at the start. > > Add support for a new "linux,iinlim-percentage" percentage property which > can be set to indicate that a bq25890 charger should only use that > percentage of the external power-supply's maximum current. > > 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> > --- > drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c > index b0d07ff24ace..2bd7721b969f 100644 > --- a/drivers/power/supply/bq25890_charger.c > +++ b/drivers/power/supply/bq25890_charger.c > @@ -126,6 +126,7 @@ struct bq25890_device { > bool read_back_init_data; > bool force_hiz; > u32 pump_express_vbus_max; > + u32 iinlim_percentage; If this is percentage, u8 should be enough, right ? > enum bq25890_chip_version chip_version; > struct bq25890_init_data init_data; > struct bq25890_state state; > @@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy, > } > } > > +/* > + * If there are multiple chargers the maximum current the external power-supply > + * can deliver needs to be divided over the chargers. This is done according > + * to the bq->iinlim_percentage setting. > + */ > +static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq, > + int iinlim_ua) > +{ > + iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100; Can this ever add up to value above 100 ? Should this use some clamp() ? > + return bq25890_find_idx(iinlim_ua, TBL_IINLIM); > +} > + > /* On the BQ25892 try to get charger-type info from our supplier */ > static void bq25890_charger_external_power_changed(struct power_supply *psy) > { [...]
Hi Marek, On 11/27/22 22:34, Marek Vasut wrote: > 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 the maximum current the external power-supply can deliver >> to be divided over the chargers. The Android vendor kernel shipped >> on the YT3-X90F divides this current with a 40/60 percent split so that >> batteries are done charging at approx. the same time if both were fully >> empty at the start. >> >> Add support for a new "linux,iinlim-percentage" percentage property which >> can be set to indicate that a bq25890 charger should only use that >> percentage of the external power-supply's maximum current. >> >> 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> >> --- >> drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++--- >> 1 file changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c >> index b0d07ff24ace..2bd7721b969f 100644 >> --- a/drivers/power/supply/bq25890_charger.c >> +++ b/drivers/power/supply/bq25890_charger.c >> @@ -126,6 +126,7 @@ struct bq25890_device { >> bool read_back_init_data; >> bool force_hiz; >> u32 pump_express_vbus_max; >> + u32 iinlim_percentage; > > If this is percentage, u8 should be enough, right ? It is not a charger-chip register value and it is used in calculations so it is best if this is native integer size. And I was passing its address directly to device_property_read_u32(), but that has changed in v2. >> enum bq25890_chip_version chip_version; >> struct bq25890_init_data init_data; >> struct bq25890_state state; >> @@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy, >> } >> } >> +/* >> + * If there are multiple chargers the maximum current the external power-supply >> + * can deliver needs to be divided over the chargers. This is done according >> + * to the bq->iinlim_percentage setting. >> + */ >> +static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq, >> + int iinlim_ua) >> +{ >> + iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100; > > Can this ever add up to value above 100 ? > Should this use some clamp() ? bq->iinlim_percentage should never be more then 100. I have added a check for this when reading the property for version 2 of the series. Thank you for all the reviews. I've also addressed all your other small remarks and I will send out a v2 series with these fixed. Regards, Hans > >> + return bq25890_find_idx(iinlim_ua, TBL_IINLIM); >> +} >> + >> /* On the BQ25892 try to get charger-type info from our supplier */ >> static void bq25890_charger_external_power_changed(struct power_supply *psy) >> { > > [...] >
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index b0d07ff24ace..2bd7721b969f 100644 --- a/drivers/power/supply/bq25890_charger.c +++ b/drivers/power/supply/bq25890_charger.c @@ -126,6 +126,7 @@ struct bq25890_device { bool read_back_init_data; bool force_hiz; u32 pump_express_vbus_max; + u32 iinlim_percentage; enum bq25890_chip_version chip_version; struct bq25890_init_data init_data; struct bq25890_state state; @@ -727,6 +728,18 @@ static int bq25890_power_supply_property_is_writeable(struct power_supply *psy, } } +/* + * If there are multiple chargers the maximum current the external power-supply + * can deliver needs to be divided over the chargers. This is done according + * to the bq->iinlim_percentage setting. + */ +static int bq25890_charger_get_scaled_iinlim_regval(struct bq25890_device *bq, + int iinlim_ua) +{ + iinlim_ua = iinlim_ua * bq->iinlim_percentage / 100; + return bq25890_find_idx(iinlim_ua, TBL_IINLIM); +} + /* On the BQ25892 try to get charger-type info from our supplier */ static void bq25890_charger_external_power_changed(struct power_supply *psy) { @@ -745,7 +758,7 @@ 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); + input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 2000000); if (bq->pump_express_vbus_max) { queue_delayed_work(system_power_efficient_wq, &bq->pump_express_work, @@ -754,11 +767,11 @@ static void bq25890_charger_external_power_changed(struct power_supply *psy) break; case POWER_SUPPLY_USB_TYPE_CDP: case POWER_SUPPLY_USB_TYPE_ACA: - input_current_limit = bq25890_find_idx(1500000, TBL_IINLIM); + input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 1500000); break; case POWER_SUPPLY_USB_TYPE_SDP: default: - input_current_limit = bq25890_find_idx(500000, TBL_IINLIM); + input_current_limit = bq25890_charger_get_scaled_iinlim_regval(bq, 500000); } bq25890_field_write(bq, F_IINLIM, input_current_limit); @@ -1390,6 +1403,11 @@ static int bq25890_fw_probe(struct bq25890_device *bq) device_property_read_u32(bq->dev, "linux,pump-express-vbus-max", &bq->pump_express_vbus_max); + /* Optional, left at 100% if property is not present */ + bq->iinlim_percentage = 100; + device_property_read_u32(bq->dev, "linux,iinlim-percentage", + &bq->iinlim_percentage); + 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");
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 the maximum current the external power-supply can deliver to be divided over the chargers. The Android vendor kernel shipped on the YT3-X90F divides this current with a 40/60 percent split so that batteries are done charging at approx. the same time if both were fully empty at the start. Add support for a new "linux,iinlim-percentage" percentage property which can be set to indicate that a bq25890 charger should only use that percentage of the external power-supply's maximum current. 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> --- drivers/power/supply/bq25890_charger.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)