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

[Guenter Roeck <linux@roeck-us.net>]

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;
>