Re: [PATCH RFC v3 5/9] power: supply: sysfs: rework uevent property loop

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

Hi,

On 9/4/24 9:25 PM, Thomas Weißschuh wrote:
> 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().
> 
> 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


p.s.

Last review for me in this set. I'm afraid I don't have the bandwidth
atm to also review the actual extension API.





> ---
>  drivers/power/supply/power_supply_sysfs.c | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 4ab08386bcb7..915a4ba62258 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -290,6 +290,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  				dev_dbg_ratelimited(dev,
>  					"driver has no data for `%s' property\n",
>  					attr->attr.name);
> +			else if (ret == -EINVAL) /* property is not supported */
> +				return -ENODATA;
>  			else if (ret != -ENODEV && ret != -EAGAIN)
>  				dev_err_ratelimited(dev,
>  					"driver failed to report `%s' property: %zd\n",
> @@ -451,11 +453,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;
>  
> @@ -483,22 +481,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;
>  	}
>