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

[Hans de Goede <hdegoede@redhat.com>]

Hi Thomas,

On 3/3/24 4:31 PM, 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 set implement desc->charge_behaviours.
> the new logic will preserve the simple output format in this case.

Since patch 1/3 now drops the charge behaviours from mm8013 I believe
these 2 paragraphs should be replaced with:

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

> Fixes: 1b0b6cc8030d ("power: supply: add charge_behaviour attributes")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++++++
>  include/linux/power_supply.h              |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 10fec411794b..a20aa0156b0a 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,23 @@ 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,
> +						  union power_supply_propval *value,
> +						  char *buf)
> +{
> +	int ret;
> +
> +	ret = power_supply_get_property(psy,
> +					POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +					value);
> +	if (ret < 0)
> +		return ret;

power_supply_show_property() has already called power_supply_get_property()
before calling this, so this call can be dropped. At which point
power_supply_show_charge_behaviour() becomes just a wrapper around
power_supply_charge_behaviour_show() and thus can be dropped itself.

And then ... (continued below).

> +
> +	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> +						  value->intval, buf);
> +}
> +
>  static ssize_t power_supply_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> @@ -303,6 +320,9 @@ 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, &value, buf);

replace this line with:

		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
							 value.intval, buf);

Making this patch a nice simple patch :)

Regards,

Hans




> +		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..a50ee69503bf 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -242,6 +242,7 @@ struct power_supply_config {
>  struct power_supply_desc {
>  	const char *name;
>  	enum power_supply_type type;
> +	u8 charge_behaviours;
>  	const enum power_supply_usb_type *usb_types;
>  	size_t num_usb_types;
>  	const enum power_supply_property *properties;
>