lkml.org 
[lkml]   [2021]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] hwmon: Add "label" attribute
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;
>>
>


\
 
 \ /
  Last update: 2021-12-22 15:19    [W:0.044 / U:1.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site