| Message ID | 20240904-power-supply-extensions-v3-5-62efeb93f8ec@weissschuh.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: extension API | expand |
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; > } >
Hi, On Wed, Sep 04, 2024 at 09:25:38PM GMT, 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> > --- > 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; I think it's better to update the check in add_prop_uevent, so that it also skips -EINVAL. That way sysfs still exposes the correct error code. Otherwise LGTM, even though I wonder about the performance impact of this change. I suppose this is not called often enough to really matter, though. Greetings, -- Sebastian > 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; > } > > -- > 2.46.0 > >
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; }
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> --- drivers/power/supply/power_supply_sysfs.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)