Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] hwmon: Add "label" attribute | Date | Tue, 21 Dec 2021 10:17:03 -0800 |
| |
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; >
| |