Re: [PATCH v4 6/9] power: supply: sysfs: rework uevent property loop

[Armin Wolf <W_Armin@gmx.de>]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> Instead of looping through all properties known to be supported by the
> psy, loop over all known properties and decide based on the return value
> of power_supply_get_property() whether the property existed.
>
> This makes the code shorter now and even more so when power supply
> extensions are added.
> It also simplifies the locking, as it can all happen inside
> power_supply_get_property().

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/power/supply/power_supply_sysfs.c | 28 +++++-----------------------
>   1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167..99bfe1f03eb8326d38c4e2831c9670313b42e425 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -289,7 +289,7 @@ static ssize_t power_supply_format_property(struct device *dev,
>   				dev_dbg_ratelimited(dev,
>   					"driver has no data for `%s' property\n",
>   					attr->attr.name);
> -			else if (ret != -ENODEV && ret != -EAGAIN)
> +			else if (ret != -ENODEV && ret != -EAGAIN && ret != -EINVAL)
>   				dev_err_ratelimited(dev,
>   					"driver failed to report `%s' property: %zd\n",
>   					attr->attr.name, ret);
> @@ -441,7 +441,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>   	dev_attr = &pwr_attr->dev_attr;
>
>   	ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
> -	if (ret == -ENODEV || ret == -ENODATA) {
> +	if (ret == -ENODEV || ret == -ENODATA || ret == -EINVAL) {
>   		/*
>   		 * When a battery is absent, we expect -ENODEV. Don't abort;
>   		 * send the uevent with at least the PRESENT=0 property
> @@ -462,11 +462,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>
>   int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   {
> -	const struct power_supply *psy = dev_get_drvdata(dev);
> -	const enum power_supply_property *battery_props =
> -		power_supply_battery_info_properties;
> -	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
> -					 sizeof(unsigned long) + 1] = {0};
> +	struct power_supply *psy = dev_get_drvdata(dev);
>   	int ret = 0, j;
>   	char *prop_buf;
>
> @@ -494,22 +490,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   	if (ret)
>   		goto out;
>
> -	for (j = 0; j < psy->desc->num_properties; j++) {
> -		set_bit(psy->desc->properties[j], psy_drv_properties);
> -		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> -				      prop_buf);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> -		if (test_bit(battery_props[j], psy_drv_properties))
> -			continue;
> -		if (!power_supply_battery_info_has_prop(psy->battery_info,
> -				battery_props[j]))
> -			continue;
> -		ret = add_prop_uevent(dev, env, battery_props[j],
> -			      prop_buf);
> +	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
> +		ret = add_prop_uevent(dev, env, j, prop_buf);
>   		if (ret)
>   			goto out;
>   	}
>