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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;







[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux