diff mbox series

[10/10] power: supply: bq25890: Add new linux,iinlim-percentage property

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

Commit Message

Hans de Goede Nov. 27, 2022, 6:02 p.m. UTC
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(-)

Comments

Marek Vasut Nov. 27, 2022, 9:34 p.m. UTC | #1
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)
>   {

[...]
Hans de Goede Nov. 28, 2022, 9:16 a.m. UTC | #2
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 mbox series

Patch

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