| Message ID | 20230125105850.17935-3-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 6adaa9a4ece44e22e0c4d2e9dbce175679383cc5 |
| Headers | show |
| Series | power: supply: bq25890: Remaining dual-charger support patches | expand |
Hi Marek, On 1/25/23 14:02, Marek Vasut wrote: > On 1/25/23 11:58, Hans de Goede wrote: > > [...] > >> @@ -1390,6 +1404,17 @@ 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); >> + ret = device_property_read_u32(bq->dev, "linux,iinlim-percentage", &val); >> + if (ret == 0) { >> + if (val > 100) { >> + dev_err(bq->dev, "Error linux,iinlim-percentage %u > 100\n", val); >> + return -EINVAL; >> + } >> + bq->iinlim_percentage = val; >> + } else { >> + bq->iinlim_percentage = 100; >> + } > > Should we really return -EINVAL if > 100 % or shall we clamp the value instead ? Either one will work, returning -EINVAL frpm probe() for invalid property values is something other drivers do to AFAIK. I don't really have a preference either way. Regards, Hans
On 1/25/23 14:12, Hans de Goede wrote: > Hi Marek, Hi, > On 1/25/23 14:02, Marek Vasut wrote: >> On 1/25/23 11:58, Hans de Goede wrote: >> >> [...] >> >>> @@ -1390,6 +1404,17 @@ 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); >>> + ret = device_property_read_u32(bq->dev, "linux,iinlim-percentage", &val); >>> + if (ret == 0) { >>> + if (val > 100) { >>> + dev_err(bq->dev, "Error linux,iinlim-percentage %u > 100\n", val); >>> + return -EINVAL; >>> + } >>> + bq->iinlim_percentage = val; >>> + } else { >>> + bq->iinlim_percentage = 100; >>> + } >> >> Should we really return -EINVAL if > 100 % or shall we clamp the value instead ? > > Either one will work, returning -EINVAL frpm probe() for invalid property > values is something other drivers do to AFAIK. > > I don't really have a preference either way. I think my argument is this -- assume the device comes with a DT baked in ROM, and the DT cannot be changed, and the DT is defective. Should we really fail or gracefully handle the condition, print a warning, fix it up and still probe ? This is very much a hypothetical case however, I think in most cases, the DT based systems with this chip will have exchangeable DT and the system integrator would be able to correct the defective DT. So maybe it is indeed better to fail and make sure the system integrator does notice the DT defect. In either case: Reviewed-by: Marek Vasut <marex@denx.de>
diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c index aff55bf3ecc3..bfe08d7bfaf3 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); @@ -1378,6 +1391,7 @@ static int bq25890_fw_probe(struct bq25890_device *bq) int ret; struct bq25890_init_data *init = &bq->init_data; const char *str; + u32 val; ret = device_property_read_string(bq->dev, "linux,secondary-charger-name", &str); if (ret == 0) { @@ -1390,6 +1404,17 @@ 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); + ret = device_property_read_u32(bq->dev, "linux,iinlim-percentage", &val); + if (ret == 0) { + if (val > 100) { + dev_err(bq->dev, "Error linux,iinlim-percentage %u > 100\n", val); + return -EINVAL; + } + bq->iinlim_percentage = val; + } else { + bq->iinlim_percentage = 100; + } + 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> --- Changes in v2: - Check that "linux,iinlim-percentage" is not > 100 when reading it --- drivers/power/supply/bq25890_charger.c | 31 +++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)