[RFC PATCH] axp209 PMIC: setting constant_charge_current to 0 disables battery charging

[RFC PATCH] axp209 PMIC: setting constant_charge_current to 0 disables battery charging

[Hermann.Lauer]

Dear Maintainers,

this proposed patch allows setting constant_charge_current to 0 on axp209
PMIC to disable charging. constant_charge_current_max with a value of 0 makes
no sense and should still report the maximum allowed value, so the getter code
is unrolled for the POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT case.

This works on the axp209 of Banana{Pi M1,Pi M1+,Pro}. I have no access to
the other chips the driver deals with.

Thanks for comments and greetings
  Hermann

Signed-off-by: Hermann.Lauer@uni-heidelberg.de
---
 patches/axp209-charge-current0.patchl | 18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -40,6 +40,7 @@
 #define AXP209_FG_PERCENT		GENMASK(6, 0)
 #define AXP22X_FG_VALID			BIT(7)
 
+#define AXP20X_CHRG_CTRL1_ENABLE	BIT(7)
 #define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
@@ -249,11 +250,18 @@
 		break;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
-		ret = axp20x_get_constant_charge_current(axp20x_batt,
-							 &val->intval);
+		ret = regmap_read(axp20x_batt->regmap, AXP20X_CHRG_CTRL1, &val->intval);
 		if (ret)
 			return ret;
-		break;
+
+		if (val->intval & AXP20X_CHRG_CTRL1_ENABLE) {
+			val->intval &= AXP20X_CHRG_CTRL1_TGT_CURR;
+			val->intval = val->intval * axp20x_batt->data->ccc_scale +
+					axp20x_batt->data->ccc_offset;
+		} else
+			val->intval = 0;
+
+		return 0;
 
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		val->intval = axp20x_batt->max_ccc;
@@ -392,6 +400,10 @@
 static int axp20x_set_constant_charge_current(struct axp20x_batt_ps *axp_batt,
 					      int charge_current)
 {
+	if (charge_current == 0)
+		return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
+				  AXP20X_CHRG_CTRL1_ENABLE, 0);
+
 	if (charge_current > axp_batt->max_ccc)
 		return -EINVAL;
 
@@ -402,7 +414,8 @@
 		return -EINVAL;
 
 	return regmap_update_bits(axp_batt->regmap, AXP20X_CHRG_CTRL1,
-				  AXP20X_CHRG_CTRL1_TGT_CURR, charge_current);
+				  AXP20X_CHRG_CTRL1_TGT_CURR | AXP20X_CHRG_CTRL1_ENABLE,
+				  charge_current | AXP20X_CHRG_CTRL1_ENABLE);
 }
 
 static int axp20x_set_max_constant_charge_current(struct axp20x_batt_ps *axp,

Re: [RFC PATCH] axp209 PMIC: setting constant_charge_current to 0 disables battery charging

[Chen-Yu Tsai]

Hi,

On Wed, Apr 21, 2021 at 5:05 PM <Hermann.Lauer@uni-heidelberg.de> wrote:
>
> Dear Maintainers,

First of all, please format the subject line like other commits to the
same file. So that would be:

    power: supply: axp20x_battery: <change>

> this proposed patch allows setting constant_charge_current to 0 on axp209
> PMIC to disable charging. constant_charge_current_max with a value of 0 makes
> no sense and should still report the maximum allowed value, so the getter code
> is unrolled for the POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT case.

This is probably not the right way to do it.

The sysfs ABI docs say that some chargers implement a writable "status" sysfs
property to allow disabling the charger [1]. IMO this is the proper place to
enable/disable the charger. POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT only
refers to the "constant current" portion of the charge cycle, and should not
be used to implement full control of the charger.


Regards
ChenYu

[1] https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-class-power#L444

[PATCH] power: supply: axp20x_battery: implement writeable status to enable/disable battery charging

[Hermann.Lauer]

Dear Maintainers,

this patch allows enabling/disabling charging for the axp209 PMIC through a
writeable status property.

This works on the axp209 of Banana {Pi M1+,Pro}. I have no access to
the other chips the driver deals with.

Thanks to ChenYu for the idea and greetings
  Hermann

Signed-off-by: Hermann.Lauer@uni-heidelberg.de
---
 drivers/power/supply/axp20x_battery.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -40,6 +40,7 @@
 #define AXP209_FG_PERCENT		GENMASK(6, 0)
 #define AXP22X_FG_VALID			BIT(7)
 
+#define AXP20X_CHRG_CTRL1_ENABLE	BIT(7)
 #define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
@@ -468,7 +469,17 @@
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		return axp20x_set_max_constant_charge_current(axp20x_batt,
 							      val->intval);
+	case POWER_SUPPLY_PROP_STATUS:
+		switch (val->intval) {
+		case POWER_SUPPLY_STATUS_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
 
+		case POWER_SUPPLY_STATUS_DISCHARGING:
+		case POWER_SUPPLY_STATUS_NOT_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, 0);
+	}
 	default:
 		return -EINVAL;
 	}
@@ -491,7 +502,8 @@
 static int axp20x_battery_prop_writeable(struct power_supply *psy,
 					 enum power_supply_property psp)
 {
-	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
+	return psp == POWER_SUPPLY_PROP_STATUS ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;

Re: [PATCH] power: supply: axp20x_battery: implement writeable status to enable/disable battery charging

[Chen-Yu Tsai]

Hi,

On Wed, May 5, 2021 at 7:29 PM <Hermann.Lauer@uni-heidelberg.de> wrote:
>
> Dear Maintainers,
>
> this patch allows enabling/disabling charging for the axp209 PMIC through a
> writeable status property.

  ^ writable

>
> This works on the axp209 of Banana {Pi M1+,Pro}. I have no access to
> the other chips the driver deals with.

This should work on all the AXP chips, as it's the same bit that
controls the charger on all of them.

> Thanks to ChenYu for the idea and greetings
>   Hermann
>
> Signed-off-by: Hermann.Lauer@uni-heidelberg.de

The patch itself looks good, but your commit message needs a bit of
cleanup. Your commit message should only include details about the
patch, especially why the patch was done, which is kind of missing.
"What" was done is already obvious when looking at the body of the
patch.

Any pleasantries and other comments addressed to maintainers should
be included after the "---" so as not to be included in git history.


Thanks
ChenYu


> ---
>  drivers/power/supply/axp20x_battery.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -40,6 +40,7 @@
>  #define AXP209_FG_PERCENT              GENMASK(6, 0)
>  #define AXP22X_FG_VALID                        BIT(7)
>
> +#define AXP20X_CHRG_CTRL1_ENABLE       BIT(7)
>  #define AXP20X_CHRG_CTRL1_TGT_VOLT     GENMASK(6, 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_1V     (0 << 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_15V    (1 << 5)
> @@ -468,7 +469,17 @@
>         case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>                 return axp20x_set_max_constant_charge_current(axp20x_batt,
>                                                               val->intval);
> +       case POWER_SUPPLY_PROP_STATUS:
> +               switch (val->intval) {
> +               case POWER_SUPPLY_STATUS_CHARGING:
> +                       return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +                               AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
>
> +               case POWER_SUPPLY_STATUS_DISCHARGING:
> +               case POWER_SUPPLY_STATUS_NOT_CHARGING:
> +                       return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +                               AXP20X_CHRG_CTRL1_ENABLE, 0);
> +       }
>         default:
>                 return -EINVAL;
>         }
> @@ -491,7 +502,8 @@
>  static int axp20x_battery_prop_writeable(struct power_supply *psy,
>                                          enum power_supply_property psp)
>  {
> -       return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
> +       return psp == POWER_SUPPLY_PROP_STATUS ||
> +              psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
>                psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
>                psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
>                psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;

[PATCH v2] power: supply: axp20x_battery: implement writeable status to enable/disable battery charging

[Hermann Lauer]

Allow disabling and reenabling battery charging of an axp209 PMIC through a
writable status property. With the current driver code charging is always on.

This works on the axp209 of Banana {Pi M1+,Pro} and should work on all AXP chips.

Signed-off-by: Hermann.Lauer@uni-heidelberg.de
---
v2: add fallthrough and improve commit message (thanks to Maxime and ChenYu)

Thanks to ChenYu for the idea and greetings
  Hermann

 drivers/power/supply/axp20x_battery.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -40,6 +40,7 @@
 #define AXP209_FG_PERCENT		GENMASK(6, 0)
 #define AXP22X_FG_VALID			BIT(7)
 
+#define AXP20X_CHRG_CTRL1_ENABLE	BIT(7)
 #define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
@@ -468,7 +469,18 @@
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		return axp20x_set_max_constant_charge_current(axp20x_batt,
 							      val->intval);
+	case POWER_SUPPLY_PROP_STATUS:
+		switch (val->intval) {
+		case POWER_SUPPLY_STATUS_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
 
+		case POWER_SUPPLY_STATUS_DISCHARGING:
+		case POWER_SUPPLY_STATUS_NOT_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, 0);
+		fallthrough;
+	}
 	default:
 		return -EINVAL;
 	}
@@ -491,7 +503,8 @@
 static int axp20x_battery_prop_writeable(struct power_supply *psy,
 					 enum power_supply_property psp)
 {
-	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
+	return psp == POWER_SUPPLY_PROP_STATUS ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;

[PATCH v3] power: supply: axp20x_battery: implement writeable status to enable/disable battery charging

[Hermann Lauer]

Allow disabling and reenabling battery charging of an axp209 PMIC through a
writable status property. With the current driver code charging is always on.

This works on the axp209 of Banana {Pi M1+,Pro} and should work on all AXP chips.

Signed-off-by: Hermann.Lauer@uni-heidelberg.de
---
v2: add fallthrough and improve commit message (thanks to Maxime and ChenYu)
v3: fix fallthrough usage

Thanks to ChenYu for the idea and greetings
  Hermann

 drivers/power/supply/axp20x_battery.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
--- a/drivers/power/supply/axp20x_battery.c
+++ b/drivers/power/supply/axp20x_battery.c
@@ -40,6 +40,7 @@
 #define AXP209_FG_PERCENT		GENMASK(6, 0)
 #define AXP22X_FG_VALID			BIT(7)
 
+#define AXP20X_CHRG_CTRL1_ENABLE	BIT(7)
 #define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
 #define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
@@ -468,7 +469,18 @@
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
 		return axp20x_set_max_constant_charge_current(axp20x_batt,
 							      val->intval);
+	case POWER_SUPPLY_PROP_STATUS:
+		switch (val->intval) {
+		case POWER_SUPPLY_STATUS_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
 
+		case POWER_SUPPLY_STATUS_DISCHARGING:
+		case POWER_SUPPLY_STATUS_NOT_CHARGING:
+			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
+				AXP20X_CHRG_CTRL1_ENABLE, 0);
+		}
+		fallthrough;
 	default:
 		return -EINVAL;
 	}
@@ -491,7 +503,8 @@
 static int axp20x_battery_prop_writeable(struct power_supply *psy,
 					 enum power_supply_property psp)
 {
-	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
+	return psp == POWER_SUPPLY_PROP_STATUS ||
+	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
 	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;

Re: [PATCH v3] power: supply: axp20x_battery: implement writeable status to enable/disable battery charging

[Sebastian Reichel]

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

Hi,

On Wed, May 12, 2021 at 12:58:56PM +0200, Hermann Lauer wrote:
> Allow disabling and reenabling battery charging of an axp209 PMIC through a
> writable status property. With the current driver code charging is always on.
> 
> This works on the axp209 of Banana {Pi M1+,Pro} and should work on all AXP chips.
> 
> Signed-off-by: Hermann.Lauer@uni-heidelberg.de
> ---

Thanks, queued.

-- Sebastian

> v2: add fallthrough and improve commit message (thanks to Maxime and ChenYu)
> v3: fix fallthrough usage
> 
> Thanks to ChenYu for the idea and greetings
>   Hermann
> 
>  drivers/power/supply/axp20x_battery.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/axp20x_battery.c b/drivers/power/supply/axp20x_battery.c
> --- a/drivers/power/supply/axp20x_battery.c
> +++ b/drivers/power/supply/axp20x_battery.c
> @@ -40,6 +40,7 @@
>  #define AXP209_FG_PERCENT		GENMASK(6, 0)
>  #define AXP22X_FG_VALID			BIT(7)
>  
> +#define AXP20X_CHRG_CTRL1_ENABLE	BIT(7)
>  #define AXP20X_CHRG_CTRL1_TGT_VOLT	GENMASK(6, 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_1V	(0 << 5)
>  #define AXP20X_CHRG_CTRL1_TGT_4_15V	(1 << 5)
> @@ -468,7 +469,18 @@
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>  		return axp20x_set_max_constant_charge_current(axp20x_batt,
>  							      val->intval);
> +	case POWER_SUPPLY_PROP_STATUS:
> +		switch (val->intval) {
> +		case POWER_SUPPLY_STATUS_CHARGING:
> +			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +				AXP20X_CHRG_CTRL1_ENABLE, AXP20X_CHRG_CTRL1_ENABLE);
>  
> +		case POWER_SUPPLY_STATUS_DISCHARGING:
> +		case POWER_SUPPLY_STATUS_NOT_CHARGING:
> +			return regmap_update_bits(axp20x_batt->regmap, AXP20X_CHRG_CTRL1,
> +				AXP20X_CHRG_CTRL1_ENABLE, 0);
> +		}
> +		fallthrough;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -491,7 +503,8 @@
>  static int axp20x_battery_prop_writeable(struct power_supply *psy,
>  					 enum power_supply_property psp)
>  {
> -	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
> +	return psp == POWER_SUPPLY_PROP_STATUS ||
> +	       psp == POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN ||
>  	       psp == POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN ||
>  	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT ||
>  	       psp == POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX;

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