Re: [PATCH 2/2] hwmon: Add "label" attribute

[Paul Cercueil]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Tested-by: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
> > ---
> >   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;