diff mbox series

[3/3] power: supply: axp288_charger: Add charge-inhibit support

Message ID 20240104183516.312044-3-hdegoede@redhat.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [1/3] power: supply: axp288_charger: Call power_supply_changed() after set_property() | expand

Commit Message

Hans de Goede Jan. 4, 2024, 6:35 p.m. UTC 
Add charge-inhibit support by adding a rw status attribute and
inhibiting charging when "Not charging" or "Discharging"
is written to it.

The userspace API with status being writable is a bit weird,
but this is already done this way in the following psy drivers:
axp20x_battery.c, bq24735-charger.c, bq25980_charger.c, ip5xxx_power.c,
rt9467-charger.c, rt9471.c.

Note on systems with an AXP288 some (about 400mA depending on load)
current will be drawn from the battery when charging is disabled
even if there is an external power-supply which can provide all
the necessary current. So unfortunately one cannot simply disable
charging to keep the battery in good health when using a tablet
with an AXP288 permanently connected to an external power-supply.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 43 +++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

Comments

Sebastian Reichel Jan. 27, 2024, 12:46 a.m. UTC | #1 
Hello Hans,

On Thu, Jan 04, 2024 at 07:35:16PM +0100, Hans de Goede wrote:
> Add charge-inhibit support by adding a rw status attribute and
> inhibiting charging when "Not charging" or "Discharging"
> is written to it.
> 
> The userspace API with status being writable is a bit weird,
> but this is already done this way in the following psy drivers:
> axp20x_battery.c, bq24735-charger.c, bq25980_charger.c, ip5xxx_power.c,
> rt9467-charger.c, rt9471.c.

We have since then added a new property for this:

POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR

That can have auto (0), inhibit charge (1) or force discharge (2).

Greetings,

-- Sebastian

> Note on systems with an AXP288 some (about 400mA depending on load)
> current will be drawn from the battery when charging is disabled
> even if there is an external power-supply which can provide all
> the necessary current. So unfortunately one cannot simply disable
> charging to keep the battery in good health when using a tablet
> with an AXP288 permanently connected to an external power-supply.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/axp288_charger.c | 43 +++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index a327933cfd6a..351431f3cf0e 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -2,7 +2,7 @@
>  /*
>   * axp288_charger.c - X-power AXP288 PMIC Charger driver
>   *
> - * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
> + * Copyright (C) 2016-2024 Hans de Goede <hdegoede@redhat.com>
>   * Copyright (C) 2014 Intel Corporation
>   * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>   */
> @@ -148,6 +148,8 @@ struct axp288_chrg_info {
>  	unsigned int op_mode;
>  	unsigned int backend_control;
>  	bool valid;
> +	bool charge_enable;
> +	bool charge_inhibit;
>  };
>  
>  static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
> @@ -285,9 +287,9 @@ static int axp288_charger_vbus_path_select(struct axp288_chrg_info *info,
>  	return ret;
>  }
>  
> -static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
> -								bool enable)
> +static int axp288_charger_update_charge_en(struct axp288_chrg_info *info)
>  {
> +	bool enable = info->charge_enable && !info->charge_inhibit;
>  	int ret;
>  
>  	if (enable)
> @@ -302,6 +304,18 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
>  	return ret;
>  }
>  
> +static int axp288_charger_enable_charger(struct axp288_chrg_info *info, bool enable)
> +{
> +	info->charge_enable = enable;
> +	return axp288_charger_update_charge_en(info);
> +}
> +
> +static int axp288_charger_inhibit_charger(struct axp288_chrg_info *info, bool inhibit)
> +{
> +	info->charge_inhibit = inhibit;
> +	return axp288_charger_update_charge_en(info);
> +}
> +
>  static int axp288_get_charger_health(struct axp288_chrg_info *info)
>  {
>  	if (!(info->input_status & PS_STAT_VBUS_PRESENT))
> @@ -327,6 +341,19 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
>  
>  	mutex_lock(&info->lock);
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		switch (val->intval) {
> +		case POWER_SUPPLY_STATUS_CHARGING:
> +			ret = axp288_charger_inhibit_charger(info, false);
> +			break;
> +		case POWER_SUPPLY_STATUS_DISCHARGING:
> +		case POWER_SUPPLY_STATUS_NOT_CHARGING:
> +			ret = axp288_charger_inhibit_charger(info, true);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		break;
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>  		scaled_val = min(val->intval, info->max_cc);
>  		scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
> @@ -423,6 +450,14 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
>  		goto out;
>  
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		if (info->charge_enable && !info->charge_inhibit)
> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> +		else if (info->charge_enable && info->charge_inhibit)
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		else /* !info->charge_enable && xxx */
> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		/* Check for OTG case first */
>  		if (info->otg.id_short) {
> @@ -472,6 +507,7 @@ static int axp288_charger_property_is_writeable(struct power_supply *psy,
>  	int ret;
>  
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> @@ -485,6 +521,7 @@ static int axp288_charger_property_is_writeable(struct power_supply *psy,
>  }
>  
>  static enum power_supply_property axp288_usb_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_PRESENT,
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_TYPE,
> -- 
> 2.43.0
> 
>
Hans de Goede Jan. 27, 2024, 1:06 p.m. UTC | #2 
Hi Sebastian,

On 1/27/24 01:46, Sebastian Reichel wrote:
> Hello Hans,
> 
> On Thu, Jan 04, 2024 at 07:35:16PM +0100, Hans de Goede wrote:
>> Add charge-inhibit support by adding a rw status attribute and
>> inhibiting charging when "Not charging" or "Discharging"
>> is written to it.
>>
>> The userspace API with status being writable is a bit weird,
>> but this is already done this way in the following psy drivers:
>> axp20x_battery.c, bq24735-charger.c, bq25980_charger.c, ip5xxx_power.c,
>> rt9467-charger.c, rt9471.c.
> 
> We have since then added a new property for this:
> 
> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
> 
> That can have auto (0), inhibit charge (1) or force discharge (2).

Right I'm aware of that, but that is a property which so far
has only been used on batteries, not chargers. So putting it
on the charger would make things more complicated when
adding support for this property to say upower (which is
something which people are looking into to allow a fuel-gauge
calibration UI).

I did think about using this and then putting it in
the axp288_fuel_gauge.c driver where this IMHO would belong.

The problem is that the very same register bit is also toggled
on/off when plugging in an external charger, so the control of
the bit needs to stay in axp288_charger.c as is done in this
patch.

So one possible solution which I came up with when originally
preparing this series would be for axp288_charger.c to export a:

int axp288_charger_inhibit_charger(struct power_supply *psy, bool inhibit);

And then have axp288_fuel_gauge.c call
power_supply_get_by_name("axp288_charger");
and then call axp288_charger_inhibit_charger()
with on the retrieved "axp288_charger" psy.

And add a POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR prop
to the fuel-gauge/battery driver this way.

Putting a POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR prop directly on
the charger (type = POWER_SUPPLY_TYPE_USB) would be another option.
This would actually make sense based on the property having charge
in the name, but as mentioned so far we have only been using
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR on type = POWER_SUPPLY_TYPE_BATTERY
supplies.

My own preference would be my first suggestion of exporting
axp288_charger_inhibit_charger() and adding
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR on the battery/fuel-gauge
psy class device, so as to keep things consistent for userspace.

Regards,

Hans














> 
> Greetings,
> 
> -- Sebastian
> 
>> Note on systems with an AXP288 some (about 400mA depending on load)
>> current will be drawn from the battery when charging is disabled
>> even if there is an external power-supply which can provide all
>> the necessary current. So unfortunately one cannot simply disable
>> charging to keep the battery in good health when using a tablet
>> with an AXP288 permanently connected to an external power-supply.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/axp288_charger.c | 43 +++++++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
>> index a327933cfd6a..351431f3cf0e 100644
>> --- a/drivers/power/supply/axp288_charger.c
>> +++ b/drivers/power/supply/axp288_charger.c
>> @@ -2,7 +2,7 @@
>>  /*
>>   * axp288_charger.c - X-power AXP288 PMIC Charger driver
>>   *
>> - * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
>> + * Copyright (C) 2016-2024 Hans de Goede <hdegoede@redhat.com>
>>   * Copyright (C) 2014 Intel Corporation
>>   * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>   */
>> @@ -148,6 +148,8 @@ struct axp288_chrg_info {
>>  	unsigned int op_mode;
>>  	unsigned int backend_control;
>>  	bool valid;
>> +	bool charge_enable;
>> +	bool charge_inhibit;
>>  };
>>  
>>  static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
>> @@ -285,9 +287,9 @@ static int axp288_charger_vbus_path_select(struct axp288_chrg_info *info,
>>  	return ret;
>>  }
>>  
>> -static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
>> -								bool enable)
>> +static int axp288_charger_update_charge_en(struct axp288_chrg_info *info)
>>  {
>> +	bool enable = info->charge_enable && !info->charge_inhibit;
>>  	int ret;
>>  
>>  	if (enable)
>> @@ -302,6 +304,18 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
>>  	return ret;
>>  }
>>  
>> +static int axp288_charger_enable_charger(struct axp288_chrg_info *info, bool enable)
>> +{
>> +	info->charge_enable = enable;
>> +	return axp288_charger_update_charge_en(info);
>> +}
>> +
>> +static int axp288_charger_inhibit_charger(struct axp288_chrg_info *info, bool inhibit)
>> +{
>> +	info->charge_inhibit = inhibit;
>> +	return axp288_charger_update_charge_en(info);
>> +}
>> +
>>  static int axp288_get_charger_health(struct axp288_chrg_info *info)
>>  {
>>  	if (!(info->input_status & PS_STAT_VBUS_PRESENT))
>> @@ -327,6 +341,19 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
>>  
>>  	mutex_lock(&info->lock);
>>  	switch (psp) {
>> +	case POWER_SUPPLY_PROP_STATUS:
>> +		switch (val->intval) {
>> +		case POWER_SUPPLY_STATUS_CHARGING:
>> +			ret = axp288_charger_inhibit_charger(info, false);
>> +			break;
>> +		case POWER_SUPPLY_STATUS_DISCHARGING:
>> +		case POWER_SUPPLY_STATUS_NOT_CHARGING:
>> +			ret = axp288_charger_inhibit_charger(info, true);
>> +			break;
>> +		default:
>> +			ret = -EINVAL;
>> +		}
>> +		break;
>>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>  		scaled_val = min(val->intval, info->max_cc);
>>  		scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
>> @@ -423,6 +450,14 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
>>  		goto out;
>>  
>>  	switch (psp) {
>> +	case POWER_SUPPLY_PROP_STATUS:
>> +		if (info->charge_enable && !info->charge_inhibit)
>> +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>> +		else if (info->charge_enable && info->charge_inhibit)
>> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +		else /* !info->charge_enable && xxx */
>> +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>> +		break;
>>  	case POWER_SUPPLY_PROP_PRESENT:
>>  		/* Check for OTG case first */
>>  		if (info->otg.id_short) {
>> @@ -472,6 +507,7 @@ static int axp288_charger_property_is_writeable(struct power_supply *psy,
>>  	int ret;
>>  
>>  	switch (psp) {
>> +	case POWER_SUPPLY_PROP_STATUS:
>>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>>  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> @@ -485,6 +521,7 @@ static int axp288_charger_property_is_writeable(struct power_supply *psy,
>>  }
>>  
>>  static enum power_supply_property axp288_usb_props[] = {
>> +	POWER_SUPPLY_PROP_STATUS,
>>  	POWER_SUPPLY_PROP_PRESENT,
>>  	POWER_SUPPLY_PROP_ONLINE,
>>  	POWER_SUPPLY_PROP_TYPE,
>> -- 
>> 2.43.0
>>
>>
Sebastian Reichel Feb. 2, 2024, 5:45 p.m. UTC | #3 
Hi Hans,

On Sat, Jan 27, 2024 at 02:06:23PM +0100, Hans de Goede wrote:
> > We have since then added a new property for this:
> > POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
> 
> Right I'm aware of that, but that is a property which so far
> has only been used on batteries, not chargers. [...]

Right. It's a design that leaked from the added ACPI vendor
solutions added to the kernel.

> So one possible solution which I came up with when originally
> preparing this series would be for axp288_charger.c to export a:
> 
> int axp288_charger_inhibit_charger(struct power_supply *psy, bool inhibit);
> 
> And then have axp288_fuel_gauge.c call
> power_supply_get_by_name("axp288_charger");
> and then call axp288_charger_inhibit_charger()
> with on the retrieved "axp288_charger" psy.
> 
> And add a POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR prop
> to the fuel-gauge/battery driver this way.

I think at some point we might have to add the charge inhibit
property to a USB/MAINS typed device, considering not all
fuel-gauges come bundled with the charger counterpart, but
let's go with your suggested route for now.

Greetings,

-- Sebastian
diff mbox series

Patch

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index a327933cfd6a..351431f3cf0e 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -2,7 +2,7 @@ 
 /*
  * axp288_charger.c - X-power AXP288 PMIC Charger driver
  *
- * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
+ * Copyright (C) 2016-2024 Hans de Goede <hdegoede@redhat.com>
  * Copyright (C) 2014 Intel Corporation
  * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
  */
@@ -148,6 +148,8 @@  struct axp288_chrg_info {
 	unsigned int op_mode;
 	unsigned int backend_control;
 	bool valid;
+	bool charge_enable;
+	bool charge_inhibit;
 };
 
 static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
@@ -285,9 +287,9 @@  static int axp288_charger_vbus_path_select(struct axp288_chrg_info *info,
 	return ret;
 }
 
-static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
-								bool enable)
+static int axp288_charger_update_charge_en(struct axp288_chrg_info *info)
 {
+	bool enable = info->charge_enable && !info->charge_inhibit;
 	int ret;
 
 	if (enable)
@@ -302,6 +304,18 @@  static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 	return ret;
 }
 
+static int axp288_charger_enable_charger(struct axp288_chrg_info *info, bool enable)
+{
+	info->charge_enable = enable;
+	return axp288_charger_update_charge_en(info);
+}
+
+static int axp288_charger_inhibit_charger(struct axp288_chrg_info *info, bool inhibit)
+{
+	info->charge_inhibit = inhibit;
+	return axp288_charger_update_charge_en(info);
+}
+
 static int axp288_get_charger_health(struct axp288_chrg_info *info)
 {
 	if (!(info->input_status & PS_STAT_VBUS_PRESENT))
@@ -327,6 +341,19 @@  static int axp288_charger_usb_set_property(struct power_supply *psy,
 
 	mutex_lock(&info->lock);
 	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		switch (val->intval) {
+		case POWER_SUPPLY_STATUS_CHARGING:
+			ret = axp288_charger_inhibit_charger(info, false);
+			break;
+		case POWER_SUPPLY_STATUS_DISCHARGING:
+		case POWER_SUPPLY_STATUS_NOT_CHARGING:
+			ret = axp288_charger_inhibit_charger(info, true);
+			break;
+		default:
+			ret = -EINVAL;
+		}
+		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 		scaled_val = min(val->intval, info->max_cc);
 		scaled_val = DIV_ROUND_CLOSEST(scaled_val, 1000);
@@ -423,6 +450,14 @@  static int axp288_charger_usb_get_property(struct power_supply *psy,
 		goto out;
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (info->charge_enable && !info->charge_inhibit)
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (info->charge_enable && info->charge_inhibit)
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		else /* !info->charge_enable && xxx */
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		/* Check for OTG case first */
 		if (info->otg.id_short) {
@@ -472,6 +507,7 @@  static int axp288_charger_property_is_writeable(struct power_supply *psy,
 	int ret;
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
@@ -485,6 +521,7 @@  static int axp288_charger_property_is_writeable(struct power_supply *psy,
 }
 
 static enum power_supply_property axp288_usb_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_TYPE,