| Message ID | 20211221175029.144906-3-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: Add "label" attribute | expand |
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; >
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; >> >
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); + 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, { 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) + return 0; + + if (attr == &dev_attr_label.attr && + !device_property_present(dev, "label")) return 0; return attr->mode;