[0/2] hwmon: Add "label" attribute

Hi Jean, Guenter, Rob,

This patchset adds support for specifying a hwmon device's label from
Device Tree. When the "label" device property is present, its value is
exported to the userspace via the "label" sysfs attribute.

This is useful for userspace to be able to identify an individual device
when multiple individual chips are present in the system.

Note that this mechanism already exists in IIO.

Patch [1/2] adds the "label" property to a new
dt-bindings/hwmon/common.yaml file.

Patch [2/2] adds the change to the core drivers/hwmon/hwmon.c file.

Cheers,
-Paul

Paul Cercueil (2):
  dt-bindings: hwmon: Introduce common properties
  hwmon: Add "label" attribute

 .../devicetree/bindings/hwmon/common.yaml     | 31 +++++++++++++++++++
 drivers/hwmon/hwmon.c                         | 22 ++++++++++++-
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/common.yaml

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