[PATCH v2 0/2] power: supply: bq25890: Remaining dual-charger support patches

[PATCH v2 0/2] power: supply: bq25890: Remaining dual-charger support patches

[Hans de Goede]

Hi Sebastian, Marek,

Most of my recent bq25890 series has been merged into:
linux-power-supply.git/for-next except for these 2.

AFAIK there are no outstanding review remarks wrt this 2 patches,
so I'm resending them to avoid them falling through the cracks
(or to restart / renew the discussion surrounding them if
necessary).

Regards,

Hans


Hans de Goede (2):
  power: supply: bq25890: Add support for having a secondary charger IC
  power: supply: bq25890: Add new linux,iinlim-percentage property

 drivers/power/supply/bq25890_charger.c | 76 ++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

-- 
2.39.0

[PATCH v2 1/2] power: supply: bq25890: Add support for having a secondary charger IC

[Hans de Goede]

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.

Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 45 +++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 30854d08bba9..aff55bf3ecc3 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -108,6 +108,7 @@ struct bq25890_device {
 	struct i2c_client *client;
 	struct device *dev;
 	struct power_supply *charger;
+	struct power_supply *secondary_chrg;
 	struct power_supply_desc desc;
 	char name[28]; /* "bq25890-charger-%d" */
 	int id;
@@ -1042,10 +1043,17 @@ static void bq25890_pump_express_work(struct work_struct *data)
 {
 	struct bq25890_device *bq =
 		container_of(data, struct bq25890_device, pump_express_work.work);
+	union power_supply_propval value;
 	int voltage, i, ret;
 
 	dev_dbg(bq->dev, "Start to request input voltage increasing\n");
 
+	/* If there is a second charger put in Hi-Z mode */
+	if (bq->secondary_chrg) {
+		value.intval = 0;
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &value);
+	}
+
 	/* Enable current pulse voltage control protocol */
 	ret = bq25890_field_write(bq, F_PUMPX_EN, 1);
 	if (ret < 0)
@@ -1077,6 +1085,11 @@ static void bq25890_pump_express_work(struct work_struct *data)
 
 	bq25890_field_write(bq, F_PUMPX_EN, 0);
 
+	if (bq->secondary_chrg) {
+		value.intval = 1;
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &value);
+	}
+
 	dev_info(bq->dev, "Hi-voltage charging requested, input voltage is %d mV\n",
 		 voltage);
 
@@ -1123,6 +1136,17 @@ static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,
 static int bq25890_vbus_enable(struct regulator_dev *rdev)
 {
 	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+	union power_supply_propval val = {
+		.intval = 0,
+	};
+
+	/*
+	 * When enabling 5V boost / Vbus output, we need to put the secondary
+	 * charger in Hi-Z mode to avoid it trying to charge the secondary
+	 * battery from the 5V boost output.
+	 */
+	if (bq->secondary_chrg)
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &val);
 
 	return bq25890_set_otg_cfg(bq, 1);
 }
@@ -1130,8 +1154,19 @@ static int bq25890_vbus_enable(struct regulator_dev *rdev)
 static int bq25890_vbus_disable(struct regulator_dev *rdev)
 {
 	struct bq25890_device *bq = rdev_get_drvdata(rdev);
+	union power_supply_propval val = {
+		.intval = 1,
+	};
+	int ret;
+
+	ret = bq25890_set_otg_cfg(bq, 0);
+	if (ret)
+		return ret;
 
-	return bq25890_set_otg_cfg(bq, 0);
+	if (bq->secondary_chrg)
+		power_supply_set_property(bq->secondary_chrg, POWER_SUPPLY_PROP_ONLINE, &val);
+
+	return 0;
 }
 
 static int bq25890_vbus_is_enabled(struct regulator_dev *rdev)
@@ -1342,6 +1377,14 @@ static int bq25890_fw_probe(struct bq25890_device *bq)
 {
 	int ret;
 	struct bq25890_init_data *init = &bq->init_data;
+	const char *str;
+
+	ret = device_property_read_string(bq->dev, "linux,secondary-charger-name", &str);
+	if (ret == 0) {
+		bq->secondary_chrg = power_supply_get_by_name(str);
+		if (!bq->secondary_chrg)
+			return -EPROBE_DEFER;
+	}
 
 	/* Optional, left at 0 if property is not present */
 	device_property_read_u32(bq->dev, "linux,pump-express-vbus-max",
-- 
2.39.0

[PATCH v2 2/2] power: supply: bq25890: Add new linux,iinlim-percentage property

[Hans de Goede]

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(-)

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");
-- 
2.39.0

Re: [PATCH v2 2/2] power: supply: bq25890: Add new linux,iinlim-percentage property

[Marek Vasut]

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 ?

Re: [PATCH v2 2/2] power: supply: bq25890: Add new linux,iinlim-percentage property

[Hans de Goede]

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

Re: [PATCH v2 2/2] power: supply: bq25890: Add new linux,iinlim-percentage property

[Marek Vasut]

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>

Re: [PATCH v2 0/2] power: supply: bq25890: Remaining dual-charger support patches

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

Hi,

On Wed, Jan 25, 2023 at 11:58:48AM +0100, Hans de Goede wrote:
> Hi Sebastian, Marek,
> 
> Most of my recent bq25890 series has been merged into:
> linux-power-supply.git/for-next except for these 2.
> 
> AFAIK there are no outstanding review remarks wrt this 2 patches,
> so I'm resending them to avoid them falling through the cracks
> (or to restart / renew the discussion surrounding them if
> necessary).

Yes, I just lacked more review time. Both are merged now.

Thanks,

-- Sebastian

> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (2):
>   power: supply: bq25890: Add support for having a secondary charger IC
>   power: supply: bq25890: Add new linux,iinlim-percentage property
> 
>  drivers/power/supply/bq25890_charger.c | 76 ++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 4 deletions(-)
> 
> -- 
> 2.39.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]