[PATCH 0/4] power: supply: core: align charge_behaviour format with docs

[PATCH 0/4] power: supply: core: align charge_behaviour format with docs

[Thomas Weißschuh]

The original submission of the charge_behaviour property did not
implement proper formatting in the default formatting handler in
power_supply_sysfs.c.

At that time it was not a problem because the only provider of the UAPI,
thinkpad_acpi, did its own formatting.

Now there is an in-tree driver, mm8013, and out-of-tree driver which use
the normal power-supply properties and are affected by the wrong
formatting.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (4):
      power: supply: core: fix charge_behaviour formatting
      power: supply: test-power: implement charge_behaviour property
      power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
      power: supply: core: drop workaround for missing POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

 drivers/power/supply/mm8013.c             |  5 +++++
 drivers/power/supply/power_supply_sysfs.c | 28 ++++++++++++++++++++++++++++
 drivers/power/supply/test_power.c         | 10 ++++++++++
 include/linux/power_supply.h              |  1 +
 4 files changed, 44 insertions(+)
---
base-commit: 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478
change-id: 20230929-power_supply-charge_behaviour_prop-10ccfd96a666

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/4] power: supply: core: fix charge_behaviour formatting

[Thomas Weißschuh]

This property is documented to have a special format which exposes all
available behaviours and the currently active one at the same time.
For this special format some helpers are provided.

However the default property logic in power_supply_sysfs.c is not using
the helper and the default logic only prints the currently active
behaviour.

Adjust power_supply_sysfs.c to follow the documented format.

There are currently two in-tree drivers exposing charge behaviours:
thinkpad_acpi and mm8013.
thinkpad_acpi is not affected by the change, as it directly uses the
helpers and does not use the power_supply_sysfs.c logic.

As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
the new logic will preserve the simple output format in this case.

Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
 include/linux/power_supply.h              |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 977611e16373..3680cfc2e908 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
 	return count;
 }
 
+static ssize_t power_supply_show_charge_behaviour(struct device *dev,
+						  struct power_supply *psy,
+						  struct power_supply_attr *ps_attr,
+						  union power_supply_propval *value,
+						  char *buf)
+{
+	union power_supply_propval available;
+	int ret;
+
+	ret = power_supply_get_property(psy,
+					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+					value);
+	if (ret < 0)
+		return ret;
+
+	ret = power_supply_get_property(psy,
+					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
+					&available);
+	if (ret == -EINVAL)
+		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
+	else if (ret < 0)
+		return ret;
+
+	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
+}
+
 static ssize_t power_supply_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
@@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 
 	if (psp == POWER_SUPPLY_PROP_TYPE) {
 		value.intval = psy->desc->type;
+	} else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
+		value.intval = -1;
 	} else {
 		ret = power_supply_get_property(psy, psp, &value);
 
@@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
 		ret = power_supply_show_usb_type(dev, psy->desc,
 						&value, buf);
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
+							 &value, buf);
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sysfs_emit(buf, "%s\n", value.strval);
 		break;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index c0992a77feea..9a6e6b488164 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -135,6 +135,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
 	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
 	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
 	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,

-- 
2.43.0

[PATCH 2/4] power: supply: test-power: implement charge_behaviour property

[Thomas Weißschuh]

To validate the special formatting of the "charge_behaviour" sysfs
property add it to the example driver.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/test_power.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 0d0a77584c5d..4da0420996c9 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -123,6 +123,14 @@ static int test_power_get_battery_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		val->intval = battery_current;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE:
+		val->intval = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
+			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
+			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
+		break;
 	default:
 		pr_info("%s: some properties deliberately report errors.\n",
 			__func__);
@@ -156,6 +164,8 @@ static enum power_supply_property test_power_battery_props[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
 };
 
 static char *test_power_ac_supplied_to[] = {

-- 
2.43.0

[PATCH 3/4] power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Thomas Weißschuh]

The sysfs is documented to report both the current and all available
behaviours. For this POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE needs
to be implemented.

Note that this changes the format of the sysfs file
(to the documented format):

Before: "auto"
After:  "[auto] inhibit-charge"

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/mm8013.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/power/supply/mm8013.c b/drivers/power/supply/mm8013.c
index caa272b03564..695df8bd6cb0 100644
--- a/drivers/power/supply/mm8013.c
+++ b/drivers/power/supply/mm8013.c
@@ -72,6 +72,7 @@ static int mm8013_checkdevice(struct mm8013_chip *chip)
 static enum power_supply_property mm8013_battery_props[] = {
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
@@ -113,6 +114,10 @@ static int mm8013_get_property(struct power_supply *psy,
 		else
 			val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
 		break;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE:
+		val->intval = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
+			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE);
+		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		ret = regmap_read(chip->regmap, REG_FULL_CHARGE_CAPACITY, &regval);
 		if (ret < 0)

-- 
2.43.0

[PATCH 4/4] power: supply: core: drop workaround for missing POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Thomas Weißschuh]

As the mm8013 driver was extended to also report that property the
workaround is not needed anymore.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3680cfc2e908..3804a3bbed24 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -273,7 +273,6 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
 
 static ssize_t power_supply_show_charge_behaviour(struct device *dev,
 						  struct power_supply *psy,
-						  struct power_supply_attr *ps_attr,
 						  union power_supply_propval *value,
 						  char *buf)
 {
@@ -289,9 +288,7 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev,
 	ret = power_supply_get_property(psy,
 					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
 					&available);
-	if (ret == -EINVAL)
-		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
-	else if (ret < 0)
+	if (ret < 0)
 		return ret;
 
 	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
@@ -337,8 +334,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 						&value, buf);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
-		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
-							 &value, buf);
+		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sysfs_emit(buf, "%s\n", value.strval);

-- 
2.43.0

Re: [PATCH 1/4] power: supply: core: fix charge_behaviour formatting

[Hans de Goede]

Hi Thomas,

Thank you for your patches for this.

On 2/4/24 18:26, Thomas Weißschuh wrote:
> This property is documented to have a special format which exposes all
> available behaviours and the currently active one at the same time.
> For this special format some helpers are provided.
> 
> However the default property logic in power_supply_sysfs.c is not using
> the helper and the default logic only prints the currently active
> behaviour.
> 
> Adjust power_supply_sysfs.c to follow the documented format.
> 
> There are currently two in-tree drivers exposing charge behaviours:
> thinkpad_acpi and mm8013.
> thinkpad_acpi is not affected by the change, as it directly uses the
> helpers and does not use the power_supply_sysfs.c logic.
> 
> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
> the new logic will preserve the simple output format in this case.
> 
> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
>  include/linux/power_supply.h              |  1 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 977611e16373..3680cfc2e908 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> +						  struct power_supply *psy,
> +						  struct power_supply_attr *ps_attr,
> +						  union power_supply_propval *value,
> +						  char *buf)
> +{
> +	union power_supply_propval available;
> +	int ret;
> +
> +	ret = power_supply_get_property(psy,
> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +					value);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = power_supply_get_property(psy,
> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> +					&available);
> +	if (ret == -EINVAL)
> +		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
> +	else if (ret < 0)

No need for "else if" here since the if above does a return. So you can just do:

	if (ret < 0)
		return ret;


> +		return ret;
> +
> +	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  
>  	if (psp == POWER_SUPPLY_PROP_TYPE) {
>  		value.intval = psy->desc->type;
> +	} else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
> +		value.intval = -1;
>  	} else {
>  		ret = power_supply_get_property(psy, psp, &value);
>  

I'm not a fan of this, I guess that you are doing this because you do not
want to enter this if:

        if (ps_attr->text_values_len > 0 &&
            value.intval < ps_attr->text_values_len && value.intval >= 0) {
                return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
        }

But by doing this you add both the special case of setting value.intval = -1
here and you now need to do the power_supply_get_property() which is in the else
manually in power_supply_show_charge_behaviour() and the error handling / logging
of power_supply_get_property() in power_supply_show_charge_behaviour() is different.

What I think you can (and should) do here instead is move:

        if (ps_attr->text_values_len > 0 &&
            value.intval < ps_attr->text_values_len && value.intval >= 0) {
                return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
        }

into the default case of the switch (psp) {} below it in a preparation patch.

This if is never entered for the 2 non default cases in that switch, so it
is safe to move it into the default case, e.g. something like this:

	default:
	        if (ps_attr->text_values_len > 0 &&
        	    value.intval < ps_attr->text_values_len && value.intval >= 0)
                	ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
		else
			ret = sysfs_emit(buf, "%d\n", value.intval);

I think that also actually makes things a bit cleaner then the current
early-exit for printing enum values.

And then in the next patch you can just add:

	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
							 &value, buf);
		break;

Without needing to set intval = -1 and without needing to get the value
inside power_supply_show_charge_behaviour() since that will already
be done for you then.

Regards,

Hans







> @@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		ret = power_supply_show_usb_type(dev, psy->desc,
>  						&value, buf);
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> +		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> +							 &value, buf);
> +		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>  		ret = sysfs_emit(buf, "%s\n", value.strval);
>  		break;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index c0992a77feea..9a6e6b488164 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -135,6 +135,7 @@ enum power_supply_property {
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
>  	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>  	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>  	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
> 

Re: [PATCH 4/4] power: supply: core: drop workaround for missing POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Hans de Goede]

Hi Thomas,

On 2/4/24 18:26, Thomas Weißschuh wrote:
> As the mm8013 driver was extended to also report that property the
> workaround is not needed anymore.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 3680cfc2e908..3804a3bbed24 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -273,7 +273,6 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>  
>  static ssize_t power_supply_show_charge_behaviour(struct device *dev,
>  						  struct power_supply *psy,
> -						  struct power_supply_attr *ps_attr,
>  						  union power_supply_propval *value,
>  						  char *buf)
>  {
> @@ -289,9 +288,7 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev,
>  	ret = power_supply_get_property(psy,
>  					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>  					&available);
> -	if (ret == -EINVAL)
> -		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);

More of a remark on patch 1 really, note that if you use
POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[] here in patch 1, e.g. :

		return sysfs_emit(buf, "%s\n",
				  POWER_SUPPLY_CHARGE_BEHAVIOUR_TEXT[value->intval]);

then you can omit the ps_attr argument in patch 1 already, which in turn
will cause this patch to be simpler / smaller.

Regards,

Hans





> -	else if (ret < 0)
> +	if (ret < 0)
>  		return ret;
>  
>  	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> @@ -337,8 +334,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>  						&value, buf);
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> -		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> -							 &value, buf);
> +		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>  		ret = sysfs_emit(buf, "%s\n", value.strval);
> 

Re: [PATCH 2/4] power: supply: test-power: implement charge_behaviour property

[Hans de Goede]

Hi,

On 2/4/24 18:26, Thomas Weißschuh wrote:
> To validate the special formatting of the "charge_behaviour" sysfs
> property add it to the example driver.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/power/supply/test_power.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 0d0a77584c5d..4da0420996c9 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -123,6 +123,14 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  		val->intval = battery_current;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> +		val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> +		break;
> +	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE:
> +		val->intval = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
> +			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
> +			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE);
> +		break;
>  	default:
>  		pr_info("%s: some properties deliberately report errors.\n",
>  			__func__);
> @@ -156,6 +164,8 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_CURRENT_AVG,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>  };
>  
>  static char *test_power_ac_supplied_to[] = {
> 

Re: [PATCH 3/4] power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Hans de Goede]

Hi,

On 2/4/24 18:26, Thomas Weißschuh wrote:
> The sysfs is documented to report both the current and all available
> behaviours. For this POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE needs
> to be implemented.
> 
> Note that this changes the format of the sysfs file
> (to the documented format):
> 
> Before: "auto"
> After:  "[auto] inhibit-charge"
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Changing userspace API like this is never ideal, but given how
new the mm8013 driver is and that this brings things inline
with the docs I think that this should be fine:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  drivers/power/supply/mm8013.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/power/supply/mm8013.c b/drivers/power/supply/mm8013.c
> index caa272b03564..695df8bd6cb0 100644
> --- a/drivers/power/supply/mm8013.c
> +++ b/drivers/power/supply/mm8013.c
> @@ -72,6 +72,7 @@ static int mm8013_checkdevice(struct mm8013_chip *chip)
>  static enum power_supply_property mm8013_battery_props[] = {
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>  	POWER_SUPPLY_PROP_CHARGE_FULL,
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_NOW,
> @@ -113,6 +114,10 @@ static int mm8013_get_property(struct power_supply *psy,
>  		else
>  			val->intval = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE:
> +		val->intval = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
> +			    | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE);
> +		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL:
>  		ret = regmap_read(chip->regmap, REG_FULL_CHARGE_CAPACITY, &regval);
>  		if (ret < 0)
> 

Re: [PATCH 3/4] power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Thomas Weißschuh]

On 2024-02-05 11:00:01+0100, Hans de Goede wrote:
> Hi,
> 
> On 2/4/24 18:26, Thomas Weißschuh wrote:
> > The sysfs is documented to report both the current and all available
> > behaviours. For this POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE needs
> > to be implemented.
> > 
> > Note that this changes the format of the sysfs file
> > (to the documented format):
> > 
> > Before: "auto"
> > After:  "[auto] inhibit-charge"
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> Changing userspace API like this is never ideal, but given how
> new the mm8013 driver is and that this brings things inline
> with the docs I think that this should be fine:

I agree that it's unfortunate.

However looking at the datasheet [0] it seems to me the driver is
not correctly using the API.

Page 23 documents the flag CHG_INH as follows:

  CHG_INH : Charge Inhibit      When the current is more than or equal to charge
                                threshold current,
                                charge inhibit temperature (upper/lower limit) ：1
                                charge permission temperature or the current is
                                less than charge threshold current ：0

This is only diagnostic information and not a control-knob, which the API
was meant for.
So POWER_SUPPLY_STATUS_NOT_CHARGING seems like the better match.

> [..]

Thomas


[0] https://product.minebeamitsumi.com/en/product/category/ics/battery/fuel_gauge/parts/download/__icsFiles/afieldfile/2023/07/12/1_download_01_12.pdf

Re: [PATCH 3/4] power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Konrad Dybcio]

On 4.02.2024 18:26, Thomas Weißschuh wrote:
> The sysfs is documented to report both the current and all available
> behaviours. For this POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE needs
> to be implemented.
> 
> Note that this changes the format of the sysfs file
> (to the documented format):
> 
> Before: "auto"
> After:  "[auto] inhibit-charge"
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

LGTM, thanks

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH 1/4] power: supply: core: fix charge_behaviour formatting

[Hans de Goede]

Hi,

On 2/5/24 10:52, Hans de Goede wrote:
> Hi Thomas,
> 
> Thank you for your patches for this.
> 
> On 2/4/24 18:26, Thomas Weißschuh wrote:
>> This property is documented to have a special format which exposes all
>> available behaviours and the currently active one at the same time.
>> For this special format some helpers are provided.
>>
>> However the default property logic in power_supply_sysfs.c is not using
>> the helper and the default logic only prints the currently active
>> behaviour.
>>
>> Adjust power_supply_sysfs.c to follow the documented format.
>>
>> There are currently two in-tree drivers exposing charge behaviours:
>> thinkpad_acpi and mm8013.
>> thinkpad_acpi is not affected by the change, as it directly uses the
>> helpers and does not use the power_supply_sysfs.c logic.
>>
>> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
>> the new logic will preserve the simple output format in this case.
>>
>> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> ---
>>  drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
>>  include/linux/power_supply.h              |  1 +
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index 977611e16373..3680cfc2e908 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>>  	return count;
>>  }
>>  
>> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
>> +						  struct power_supply *psy,
>> +						  struct power_supply_attr *ps_attr,
>> +						  union power_supply_propval *value,
>> +						  char *buf)
>> +{
>> +	union power_supply_propval available;
>> +	int ret;
>> +
>> +	ret = power_supply_get_property(psy,
>> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
>> +					value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = power_supply_get_property(psy,
>> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>> +					&available);
>> +	if (ret == -EINVAL)
>> +		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
>> +	else if (ret < 0)
> 
> No need for "else if" here since the if above does a return. So you can just do:
> 
> 	if (ret < 0)
> 		return ret;
> 
> 
>> +		return ret;
>> +
>> +	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
>> +}
>> +
>>  static ssize_t power_supply_show_property(struct device *dev,
>>  					  struct device_attribute *attr,
>>  					  char *buf) {
>> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  
>>  	if (psp == POWER_SUPPLY_PROP_TYPE) {
>>  		value.intval = psy->desc->type;
>> +	} else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
>> +		value.intval = -1;
>>  	} else {
>>  		ret = power_supply_get_property(psy, psp, &value);
>>  
> 
> I'm not a fan of this, I guess that you are doing this because you do not
> want to enter this if:
> 
>         if (ps_attr->text_values_len > 0 &&
>             value.intval < ps_attr->text_values_len && value.intval >= 0) {
>                 return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>         }
> 
> But by doing this you add both the special case of setting value.intval = -1
> here and you now need to do the power_supply_get_property() which is in the else
> manually in power_supply_show_charge_behaviour() and the error handling / logging
> of power_supply_get_property() in power_supply_show_charge_behaviour() is different.
> 
> What I think you can (and should) do here instead is move:
> 
>         if (ps_attr->text_values_len > 0 &&
>             value.intval < ps_attr->text_values_len && value.intval >= 0) {
>                 return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>         }
> 
> into the default case of the switch (psp) {} below it in a preparation patch.
> 
> This if is never entered for the 2 non default cases in that switch, so it
> is safe to move it into the default case, e.g. something like this:
> 
> 	default:
> 	        if (ps_attr->text_values_len > 0 &&
>         	    value.intval < ps_attr->text_values_len && value.intval >= 0)
>                 	ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> 		else
> 			ret = sysfs_emit(buf, "%d\n", value.intval);
> 
> I think that also actually makes things a bit cleaner then the current
> early-exit for printing enum values.
> 
> And then in the next patch you can just add:
> 
> 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> 		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> 							 &value, buf);
> 		break;
> 
> Without needing to set intval = -1 and without needing to get the value
> inside power_supply_show_charge_behaviour() since that will already
> be done for you then.

One more note on this. With the suggested preparation patch to move
the if checking for ps_attr->text_values into the default case,
you can then also properly turn POWER_SUPPLY_PROP_USB_TYPE into an
enum too:

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 977611e16373..768df64330f4 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -209,7 +209,7 @@ static struct power_supply_attr power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
 	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
 	POWER_SUPPLY_ENUM_ATTR(TYPE),
-	POWER_SUPPLY_ATTR(USB_TYPE),
+	POWER_SUPPLY_ENUM_ATTR(USB_TYPE),
 	POWER_SUPPLY_ENUM_ATTR(SCOPE),
 	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
 	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),

IOW you can now set ps_attr->text_values* for POWER_SUPPLY_PROP_USB_TYPE
too, without needing to worry this causing power_supply_show_usb_type()
no longer to get called.

The reason I'm mentioning this is because power_supply_show_usb_type()
and power_supply_charge_behaviour_show() show a lot of code-duplication.

And I think that we can have one generic function replacing both
by using ps_attr->text_values* instead of hardcoding POWER_SUPPLY_USB_TYPE_TEXT
resp. power_supply_charge_behaviour_show (and yes this contradicts
my earlier comment on patch 4/4).

Although at a closer look I see now that the usb-types code uses
a list of supported type enum values in the power_supply_desc,
where as the charge_behavior code uses a bitmask which you retrieve
through a new property.

Thinking more about this, I think that adding a fake
POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE property as you do here
might actually not be the best idea. All the other properties
are visible in sysfs, so this one is a bit weird. I think it might
be better to add this info to power_supply_desc like how this is
done for the usb-types.

And:
        const enum power_supply_usb_type *usb_types;
        size_t num_usb_types;

Should probably be converted into a bitmask too. I checked
and there are not that many users of this.

Once we have that as a bitmask too, we can refactor
power_supply_show_usb_type() and power_supply_charge_behaviour_show()
into a single new helper.

But I believe that refactoring:

        const enum power_supply_usb_type *usb_types;
        size_t num_usb_types;

into a bitmask is out of scope for this series. So I guess
that for now just add the bitmask of available charge behaviors
to power_supply_desc rather then making it a fake property
and then in the future we can do the refactor of usb-type
to a bitmask and remove the code duplication between
power_supply_show_usb_type() and power_supply_charge_behaviour_show()

Sebastian, any comments ?

Regards,

Hans









>> @@ -308,6 +336,10 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  		ret = power_supply_show_usb_type(dev, psy->desc,
>>  						&value, buf);
>>  		break;
>> +	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>> +		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
>> +							 &value, buf);
>> +		break;
>>  	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>>  		ret = sysfs_emit(buf, "%s\n", value.strval);
>>  		break;
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index c0992a77feea..9a6e6b488164 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -135,6 +135,7 @@ enum power_supply_property {
>>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
>>  	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
>>  	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
>> +	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
>>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>>  	POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
>>  	POWER_SUPPLY_PROP_INPUT_POWER_LIMIT,
>>
> 

Re: [PATCH 3/4] power: supply: mm8013: implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE

[Konrad Dybcio]

On 5.02.2024 12:21, Thomas Weißschuh wrote:
> On 2024-02-05 11:00:01+0100, Hans de Goede wrote:
>> Hi,
>>
>> On 2/4/24 18:26, Thomas Weißschuh wrote:
>>> The sysfs is documented to report both the current and all available
>>> behaviours. For this POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE needs
>>> to be implemented.
>>>
>>> Note that this changes the format of the sysfs file
>>> (to the documented format):
>>>
>>> Before: "auto"
>>> After:  "[auto] inhibit-charge"
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>
>> Changing userspace API like this is never ideal, but given how
>> new the mm8013 driver is and that this brings things inline
>> with the docs I think that this should be fine:
> 
> I agree that it's unfortunate.
> 
> However looking at the datasheet [0] it seems to me the driver is
> not correctly using the API.
> 
> Page 23 documents the flag CHG_INH as follows:
> 
>   CHG_INH : Charge Inhibit      When the current is more than or equal to charge
>                                 threshold current,
>                                 charge inhibit temperature (upper/lower limit) ：1
>                                 charge permission temperature or the current is
>                                 less than charge threshold current ：0
> 
> This is only diagnostic information and not a control-knob, which the API
> was meant for.
> So POWER_SUPPLY_STATUS_NOT_CHARGING seems like the better match.

Oh, that's definitely something I glossed over, thanks for taking a look!

I'll send a patch untangling this shortly.

Konrad

> 
>> [..]
> 
> Thomas
> 
> 
> [0] https://product.minebeamitsumi.com/en/product/category/ics/battery/fuel_gauge/parts/download/__icsFiles/afieldfile/2023/07/12/1_download_01_12.pdf

Re: [PATCH 1/4] power: supply: core: fix charge_behaviour formatting

[Sebastian Reichel]

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

Hi,

On Mon, Feb 05, 2024 at 12:43:54PM +0100, Hans de Goede wrote:
> On 2/5/24 10:52, Hans de Goede wrote:
> > Thank you for your patches for this.

Indeed, thanks for cleaning up this mess.

> > On 2/4/24 18:26, Thomas Weißschuh wrote:
> >> This property is documented to have a special format which exposes all
> >> available behaviours and the currently active one at the same time.
> >> For this special format some helpers are provided.
> >>
> >> However the default property logic in power_supply_sysfs.c is not using
> >> the helper and the default logic only prints the currently active
> >> behaviour.
> >>
> >> Adjust power_supply_sysfs.c to follow the documented format.
> >>
> >> There are currently two in-tree drivers exposing charge behaviours:
> >> thinkpad_acpi and mm8013.
> >> thinkpad_acpi is not affected by the change, as it directly uses the
> >> helpers and does not use the power_supply_sysfs.c logic.
> >>
> >> As mm8013 does not implement POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE
> >> the new logic will preserve the simple output format in this case.
> >>
> >> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> >> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> >> ---
> >>  drivers/power/supply/power_supply_sysfs.c | 32 +++++++++++++++++++++++++++++++
> >>  include/linux/power_supply.h              |  1 +
> >>  2 files changed, 33 insertions(+)
> >>
> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> >> index 977611e16373..3680cfc2e908 100644
> >> --- a/drivers/power/supply/power_supply_sysfs.c
> >> +++ b/drivers/power/supply/power_supply_sysfs.c
> >> @@ -271,6 +271,32 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
> >>  	return count;
> >>  }
> >>  
> >> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> >> +						  struct power_supply *psy,
> >> +						  struct power_supply_attr *ps_attr,
> >> +						  union power_supply_propval *value,
> >> +						  char *buf)
> >> +{
> >> +	union power_supply_propval available;
> >> +	int ret;
> >> +
> >> +	ret = power_supply_get_property(psy,
> >> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> >> +					value);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	ret = power_supply_get_property(psy,
> >> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE,
> >> +					&available);
> >> +	if (ret == -EINVAL)
> >> +		return sysfs_emit(buf, "%s\n", ps_attr->text_values[value->intval]);
> >> +	else if (ret < 0)
> > 
> > No need for "else if" here since the if above does a return. So you can just do:
> > 
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 
> >> +		return ret;
> >> +
> >> +	return power_supply_charge_behaviour_show(dev, available.intval, value->intval, buf);
> >> +}
> >> +
> >>  static ssize_t power_supply_show_property(struct device *dev,
> >>  					  struct device_attribute *attr,
> >>  					  char *buf) {
> >> @@ -282,6 +308,8 @@ static ssize_t power_supply_show_property(struct device *dev,
> >>  
> >>  	if (psp == POWER_SUPPLY_PROP_TYPE) {
> >>  		value.intval = psy->desc->type;
> >> +	} else if (psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR) {
> >> +		value.intval = -1;
> >>  	} else {
> >>  		ret = power_supply_get_property(psy, psp, &value);
> >>  
> > 
> > I'm not a fan of this, I guess that you are doing this because you do not
> > want to enter this if:
> > 
> >         if (ps_attr->text_values_len > 0 &&
> >             value.intval < ps_attr->text_values_len && value.intval >= 0) {
> >                 return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> >         }
> > 
> > But by doing this you add both the special case of setting value.intval = -1
> > here and you now need to do the power_supply_get_property() which is in the else
> > manually in power_supply_show_charge_behaviour() and the error handling / logging
> > of power_supply_get_property() in power_supply_show_charge_behaviour() is different.
> > 
> > What I think you can (and should) do here instead is move:
> > 
> >         if (ps_attr->text_values_len > 0 &&
> >             value.intval < ps_attr->text_values_len && value.intval >= 0) {
> >                 return sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> >         }
> > 
> > into the default case of the switch (psp) {} below it in a preparation patch.
> > 
> > This if is never entered for the 2 non default cases in that switch, so it
> > is safe to move it into the default case, e.g. something like this:
> > 
> > 	default:
> > 	        if (ps_attr->text_values_len > 0 &&
> >         	    value.intval < ps_attr->text_values_len && value.intval >= 0)
> >                 	ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> > 		else
> > 			ret = sysfs_emit(buf, "%d\n", value.intval);
> > 
> > I think that also actually makes things a bit cleaner then the current
> > early-exit for printing enum values.
> > 
> > And then in the next patch you can just add:
> > 
> > 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> > 		ret = power_supply_show_charge_behaviour(dev, psy, ps_attr,
> > 							 &value, buf);
> > 		break;
> > 
> > Without needing to set intval = -1 and without needing to get the value
> > inside power_supply_show_charge_behaviour() since that will already
> > be done for you then.
> 
> One more note on this. With the suggested preparation patch to move
> the if checking for ps_attr->text_values into the default case,
> you can then also properly turn POWER_SUPPLY_PROP_USB_TYPE into an
> enum too:
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 977611e16373..768df64330f4 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -209,7 +209,7 @@ static struct power_supply_attr power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
>  	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
>  	POWER_SUPPLY_ENUM_ATTR(TYPE),
> -	POWER_SUPPLY_ATTR(USB_TYPE),
> +	POWER_SUPPLY_ENUM_ATTR(USB_TYPE),
>  	POWER_SUPPLY_ENUM_ATTR(SCOPE),
>  	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
>  	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
> 
> IOW you can now set ps_attr->text_values* for POWER_SUPPLY_PROP_USB_TYPE
> too, without needing to worry this causing power_supply_show_usb_type()
> no longer to get called.
> 
> The reason I'm mentioning this is because power_supply_show_usb_type()
> and power_supply_charge_behaviour_show() show a lot of code-duplication.
> 
> And I think that we can have one generic function replacing both
> by using ps_attr->text_values* instead of hardcoding POWER_SUPPLY_USB_TYPE_TEXT
> resp. power_supply_charge_behaviour_show (and yes this contradicts
> my earlier comment on patch 4/4).
> 
> Although at a closer look I see now that the usb-types code uses
> a list of supported type enum values in the power_supply_desc,
> where as the charge_behavior code uses a bitmask which you retrieve
> through a new property.
> 
> Thinking more about this, I think that adding a fake
> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR_AVAILABLE property as you do here
> might actually not be the best idea. All the other properties
> are visible in sysfs, so this one is a bit weird. I think it might
> be better to add this info to power_supply_desc like how this is
> done for the usb-types.
> 
> And:
>         const enum power_supply_usb_type *usb_types;
>         size_t num_usb_types;
> 
> Should probably be converted into a bitmask too. I checked
> and there are not that many users of this.
> 
> Once we have that as a bitmask too, we can refactor
> power_supply_show_usb_type() and power_supply_charge_behaviour_show()
> into a single new helper.
> 
> But I believe that refactoring:
> 
>         const enum power_supply_usb_type *usb_types;
>         size_t num_usb_types;
> 
> into a bitmask is out of scope for this series. So I guess
> that for now just add the bitmask of available charge behaviors
> to power_supply_desc rather then making it a fake property
> and then in the future we can do the refactor of usb-type
> to a bitmask and remove the code duplication between
> power_supply_show_usb_type() and power_supply_charge_behaviour_show()
> 
> Sebastian, any comments ?

Sorry for the delay coming to this series; I fully agree with this.
Thanks for the thorough review.

-- Sebastian

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