Messages in this thread | | | Date | Wed, 22 Dec 2021 14:18:46 +0000 | From | Paul Cercueil <> | Subject | Re: [PATCH 2/2] hwmon: Add "label" attribute |
| |
Hi Guenter,
Comments noted, thanks. I'll send a V2 later today.
Cheers, -Paul
Le mar., déc. 21 2021 at 10:17:03 -0800, Guenter Roeck <linux@roeck-us.net> a écrit : > 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; >> >
| |