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

[Paul Cercueil <paul@crapouillou.net>]

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);
>>   \x7f+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
>>   };
>>   \x7f@@ -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);
>>   \x7f-	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;
>>   \x7f  	return attr->mode;
>> 
>