lkml.org 
[lkml]   [2021]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/2] hwmon: Add "label" attribute
Date
On 12/21/21 9:50 AM, Paul Cercueil wrote:
> If a label is defined in the device tree for this device add that
> to the device specific attributes. This is useful for userspace to
> be able to identify an individual device when multiple identical
> chips are present in the system.
>

This is an ABI change which needs to be documented in
Documentation/hwmon/sysfs-interface.rst.

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Cosmin Tanislav <cosmin.tanislav@analog.com>
> ---
> drivers/hwmon/hwmon.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 3501a3ead4ba..15826260a463 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -71,8 +71,23 @@ name_show(struct device *dev, struct device_attribute *attr, char *buf)
> }
> static DEVICE_ATTR_RO(name);
>
> +static ssize_t
> +label_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + const char *label;
> + int ret;
> +
> + ret = device_property_read_string(dev, "label", &label);

Requires "#include <linux/property.h>". Also, reading and verifying the label
each time it is read is excessive. More on that see below.

> + if (ret < 0)
> + return ret;
> +
> + return sysfs_emit(buf, "%s\n", label);
> +}
> +static DEVICE_ATTR_RO(label);
> +
> static struct attribute *hwmon_dev_attrs[] = {
> &dev_attr_name.attr,
> + &dev_attr_label.attr,
> NULL
> };
>
> @@ -81,7 +96,12 @@ static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,

That function name is no longer appropriate and should be changed to
something like hwmon_dev_attr_is_visible.

> {
> struct device *dev = kobj_to_dev(kobj);
>
> - if (to_hwmon_device(dev)->name == NULL)
> + if (attr == &dev_attr_name.attr &&
> + to_hwmon_device(dev)->name == NULL)

Unnecessary continuation line.

> + return 0;
> +
> + if (attr == &dev_attr_label.attr &&
> + !device_property_present(dev, "label"))

If the property is present but not a string, each read of "label" would
return a runtime error. I don't like that. I would suggest to store
a pointer to the label in struct hwmon_device, set it during registration
(eg by using devm_strdup() if it is defined), and use

if (attr == &dev_attr_label.attr && to_hwmon_device(dev)->label == NULL)
return 0;

to check if it is present.

> return 0;
>
> return attr->mode;
>

\
 
 \ /
  Last update: 2021-12-21 19:17    [W:0.071 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site