Re: [PATCH 1/1] power: supply: support charge_types in extensions

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

Hi Jelle,

Thank you for your patch.

On 7-Apr-25 10:18 PM, Jelle van der Waa wrote:
> From: Jelle van der Waa <jvanderwaa@redhat.com>
> 
> Similar to charge_behaviour, charge_types is an enum option with
> multiple possible values. To be able to use it with a power_supply
> extension its known values have to be exposed.

"multiple possible values" sounds like the actual setting
is a bitmask where multiple options can be enabled at once.

And "its known values" sounds like this is about the currently
active value(s).

Instead I suggest a text like this:

"""
Similar to charge_behaviour, charge_types is an enum option where reading
the property shows the supported values, with the active value surrounded
by brackets. To be able to use it with a power_supply extension a bitmask
with the supported charge_Types values has to be added to power_supply_ext.
"""

Feel free to re-use this verbatim, or just the bits you like.

> Signed-off-by: Jelle van der Waa <jvanderwaa@redhat.com>
> ---
>  drivers/power/supply/power_supply_sysfs.c | 23 ++++++++++++++++++++++-
>  drivers/power/supply/test_power.c         | 20 ++++++++++++++++++--
>  include/linux/power_supply.h              |  1 +
>  3 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index edb058c19c9c..6d80640511b5 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -321,6 +321,27 @@ static ssize_t power_supply_show_charge_behaviour(struct device *dev,
>  						  value->intval, buf);
>  }
>  
> +static ssize_t power_supply_show_charge_types(struct device *dev,
> +					      struct power_supply *psy,
> +					      enum power_supply_charge_type current_type,
> +					      char *buf)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		power_supply_for_each_extension(reg, psy) {
> +			if (power_supply_ext_has_property(reg->ext,
> +							  POWER_SUPPLY_PROP_CHARGE_TYPES))
> +				return power_supply_charge_types_show(dev,
> +						reg->ext->charge_types,
> +						current_type, buf);
> +		}
> +	}
> +
> +	return power_supply_charge_types_show(dev, psy->desc->charge_types,
> +						  current_type, buf);
> +}
> +

Once the last external user of power_supply_charge_types_show() (dell-laptop)
is ported to become a power-supply extension, we should stop exporting
power_supply_charge_types_show(). The question then becomes do we just stop
exporting it; or do we remove it entirely ? My vote would go to remove it
entirely. This could be done if the above function were to directly call
power_supply_show_enum_with_available() instead of using the helper, e.g.:

static ssize_t power_supply_show_charge_types(struct device *dev,
					      struct power_supply *psy,
					      enum power_supply_charge_type current_type,
					      char *buf)
{
	struct power_supply_ext_registration *reg;

	scoped_guard(rwsem_read, &psy->extensions_sem) {
		power_supply_for_each_extension(reg, psy) {
			if (power_supply_ext_has_property(reg->ext,
							  POWER_SUPPLY_PROP_CHARGE_TYPES))
				return power_supply_show_enum_with_available(dev,
						POWER_SUPPLY_CHARGE_TYPE_TEXT,
						ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT),
						reg->ext->charge_types,
						current_type, buf);
		}
	}

	return power_supply_show_enum_with_available(dev, POWER_SUPPLY_CHARGE_TYPE_TEXT,
						     ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT),
						     psy->desc->charge_types, current_type, buf);
}

Since we already know we want to eventually remove power_supply_charge_types_show()
I think it would be better to write this as above right away.

Sebastian, Thomas any opinions on this ?

>  static ssize_t power_supply_format_property(struct device *dev,
>  					    bool uevent,
>  					    struct device_attribute *attr,
> @@ -365,7 +386,7 @@ static ssize_t power_supply_format_property(struct device *dev,
>  	case POWER_SUPPLY_PROP_CHARGE_TYPES:
>  		if (uevent) /* no possible values in uevents */
>  			goto default_format;
> -		ret = power_supply_charge_types_show(dev, psy->desc->charge_types,
> +		ret = power_supply_show_charge_types(dev, psy,
>  						     value.intval, buf);
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 2a975a110f48..0d9cc0c5613e 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -37,6 +37,8 @@ static int battery_charge_counter	= -1000;
>  static int battery_current		= -1600;
>  static enum power_supply_charge_behaviour battery_charge_behaviour =
>  	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +static enum power_supply_charge_type battery_charge_types =
> +	POWER_SUPPLY_CHARGE_TYPE_NONE;
>  static bool battery_extension;
>  
>  static bool module_initialized;
> @@ -87,7 +89,7 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  		val->intval = battery_status;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> -		val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
> +		val->intval = battery_charge_types;
>  		break;
>  	case POWER_SUPPLY_PROP_HEALTH:
>  		val->intval = battery_health;
> @@ -129,6 +131,9 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>  		val->intval = battery_charge_behaviour;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TYPES:
> +		val->intval = battery_charge_types;
> +		break;
>  	default:
>  		pr_info("%s: some properties deliberately report errors.\n",
>  			__func__);
> @@ -140,7 +145,7 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  static int test_power_battery_property_is_writeable(struct power_supply *psy,
>  						    enum power_supply_property psp)
>  {
> -	return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR;
> +	return psp == POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR | POWER_SUPPLY_PROP_CHARGE_TYPES;
>  }
>  
>  static int test_power_set_battery_property(struct power_supply *psy,
> @@ -156,6 +161,14 @@ static int test_power_set_battery_property(struct power_supply *psy,
>  		}
>  		battery_charge_behaviour = val->intval;
>  		break;
> +	case POWER_SUPPLY_PROP_CHARGE_TYPES:
> +		if (val->intval < 0 ||
> +		    val->intval >= BITS_PER_TYPE(typeof(psy->desc->charge_types)) ||
> +		    !(BIT(val->intval) & psy->desc->charge_types)) {
> +			return -EINVAL;
> +		}
> +		battery_charge_types = val->intval;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -188,6 +201,7 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_CURRENT_AVG,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
>  	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> +	POWER_SUPPLY_PROP_CHARGE_TYPES,
>  };
>  
>  static char *test_power_ac_supplied_to[] = {
> @@ -215,6 +229,8 @@ static const struct power_supply_desc test_power_desc[] = {
>  		.charge_behaviours = BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO)
>  				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)
>  				   | BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE),
> +		.charge_types = BIT(POWER_SUPPLY_CHARGE_TYPE_NONE)
> +				   | BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)
>  	},
>  	[TEST_USB] = {
>  		.name = "test_usb",
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 888824592953..c4cb854971f5 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -288,6 +288,7 @@ struct power_supply_desc {
>  struct power_supply_ext {
>  	const char *const name;
>  	u8 charge_behaviours;
> +	u32 charge_types;
>  	const enum power_supply_property *properties;
>  	size_t num_properties;
>  

Otherwise this patch looks good to me:

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

Regards,

Hans