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

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

[Paul Cercueil]

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

-- 
2.34.1

[PATCH 1/2] dt-bindings: hwmon: Introduce common properties

[Paul Cercueil]

Introduce a file for common properties of hwmon sensors.

As of now it contains only the "label" property, which can contain a
descriptive label that allows to uniquely identify a device within the
system.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../devicetree/bindings/hwmon/common.yaml     | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/common.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/common.yaml b/Documentation/devicetree/bindings/hwmon/common.yaml
new file mode 100644
index 000000000000..997f74127d8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/common.yaml
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Common properties for hwmon sensors
+
+maintainers:
+  - Jean Delvare <jdelvare@suse.com>
+  - Guenter Roeck <linux@roeck-us.net>
+
+description: |
+  This document defines device tree properties common to several hwmon
+  sensors. It doesn't constitue a device tree binding specification by itself but
+  is meant to be referenced by device tree bindings.
+
+  When referenced from sensor tree bindings the properties defined in this
+  document are defined as follows. The sensor tree bindings are responsible for
+  defining whether each property is required or optional.
+
+properties:
+  label:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: >
+      Descriptive label that allows to uniquely identify a device within
+      the system.
+
+additionalProperties: true
+
+...
-- 
2.34.1

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

[Paul Cercueil]

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.

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);
+	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,
 {
 	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)
+		return 0;
+
+	if (attr == &dev_attr_label.attr &&
+	    !device_property_present(dev, "label"))
 		return 0;
 
 	return attr->mode;
-- 
2.34.1

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

[Guenter Roeck]

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

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

[Paul Cercueil]

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

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce common properties

[Rob Herring]

On Tue, Dec 21, 2021 at 05:50:28PM +0000, Paul Cercueil wrote:
> Introduce a file for common properties of hwmon sensors.
> 
> As of now it contains only the "label" property, which can contain a
> descriptive label that allows to uniquely identify a device within the
> system.

I don't think we need this. What we need is a global (in dtschema) 
type definition and then any users just add 'label: true'.

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/hwmon/common.yaml     | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/common.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/common.yaml b/Documentation/devicetree/bindings/hwmon/common.yaml
> new file mode 100644
> index 000000000000..997f74127d8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/common.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common properties for hwmon sensors
> +
> +maintainers:
> +  - Jean Delvare <jdelvare@suse.com>
> +  - Guenter Roeck <linux@roeck-us.net>
> +
> +description: |
> +  This document defines device tree properties common to several hwmon
> +  sensors. It doesn't constitue a device tree binding specification by itself but
> +  is meant to be referenced by device tree bindings.
> +
> +  When referenced from sensor tree bindings the properties defined in this
> +  document are defined as follows. The sensor tree bindings are responsible for
> +  defining whether each property is required or optional.
> +
> +properties:
> +  label:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: >
> +      Descriptive label that allows to uniquely identify a device within
> +      the system.
> +
> +additionalProperties: true
> +
> +...
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce common properties

[Paul Cercueil]

Hi Rob,

Le mer., déc. 22 2021 at 15:36:23 -0400, Rob Herring <robh@kernel.org> 
a écrit :
> On Tue, Dec 21, 2021 at 05:50:28PM +0000, Paul Cercueil wrote:
>>  Introduce a file for common properties of hwmon sensors.
>> 
>>  As of now it contains only the "label" property, which can contain a
>>  descriptive label that allows to uniquely identify a device within 
>> the
>>  system.
> 
> I don't think we need this. What we need is a global (in dtschema)
> type definition and then any users just add 'label: true'.

Well, users would also need to set an actual label, otherwise this 
defeats the point :)

Cheers,
-Paul

> 
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../devicetree/bindings/hwmon/common.yaml     | 31 
>> +++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/hwmon/common.yaml
>> 
>>  diff --git a/Documentation/devicetree/bindings/hwmon/common.yaml 
>> b/Documentation/devicetree/bindings/hwmon/common.yaml
>>  new file mode 100644
>>  index 000000000000..997f74127d8c
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/hwmon/common.yaml
>>  @@ -0,0 +1,31 @@
>>  +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>  +%YAML 1.2
>>  +---
>>  +$id: http://devicetree.org/schemas/hwmon/common.yaml#
>>  +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>  +
>>  +title: Common properties for hwmon sensors
>>  +
>>  +maintainers:
>>  +  - Jean Delvare <jdelvare@suse.com>
>>  +  - Guenter Roeck <linux@roeck-us.net>
>>  +
>>  +description: |
>>  +  This document defines device tree properties common to several 
>> hwmon
>>  +  sensors. It doesn't constitue a device tree binding 
>> specification by itself but
>>  +  is meant to be referenced by device tree bindings.
>>  +
>>  +  When referenced from sensor tree bindings the properties defined 
>> in this
>>  +  document are defined as follows. The sensor tree bindings are 
>> responsible for
>>  +  defining whether each property is required or optional.
>>  +
>>  +properties:
>>  +  label:
>>  +    $ref: /schemas/types.yaml#/definitions/string
>>  +    description: >
>>  +      Descriptive label that allows to uniquely identify a device 
>> within
>>  +      the system.
>>  +
>>  +additionalProperties: true
>>  +
>>  +...
>>  --
>>  2.34.1
>> 
>> 

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce common properties

[Paul Cercueil]

Le jeu., déc. 23 2021 at 09:19:43 +0000, Paul Cercueil 
<paul@crapouillou.net> a écrit :
> Hi Rob,
> 
> Le mer., déc. 22 2021 at 15:36:23 -0400, Rob Herring 
> <robh@kernel.org> a écrit :
>> On Tue, Dec 21, 2021 at 05:50:28PM +0000, Paul Cercueil wrote:
>>>  Introduce a file for common properties of hwmon sensors.
>>> 
>>>  As of now it contains only the "label" property, which can contain 
>>> a
>>>  descriptive label that allows to uniquely identify a device within 
>>> \x7f\x7fthe
>>>  system.
>> 
>> I don't think we need this. What we need is a global (in dtschema)
>> type definition and then any users just add 'label: true'.
> 
> Well, users would also need to set an actual label, otherwise this 
> defeats the point :)

Disregard that, I'm stupid. 'label: true' is for the schema and not the 
DTS files.

-Paul

Re: [PATCH 1/2] dt-bindings: hwmon: Introduce common properties

[Paul Cercueil]

Hi Rob,

Le mer., déc. 22 2021 at 15:36:23 -0400, Rob Herring <robh@kernel.org> 
a écrit :
> On Tue, Dec 21, 2021 at 05:50:28PM +0000, Paul Cercueil wrote:
>>  Introduce a file for common properties of hwmon sensors.
>> 
>>  As of now it contains only the "label" property, which can contain a
>>  descriptive label that allows to uniquely identify a device within 
>> the
>>  system.
> 
> I don't think we need this. What we need is a global (in dtschema)
> type definition and then any users just add 'label: true'.

I created a PR there: 
https://github.com/devicetree-org/dt-schema/pull/65

Cheers,
-Paul

> 
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../devicetree/bindings/hwmon/common.yaml     | 31 
>> +++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/hwmon/common.yaml
>> 
>>  diff --git a/Documentation/devicetree/bindings/hwmon/common.yaml 
>> b/Documentation/devicetree/bindings/hwmon/common.yaml
>>  new file mode 100644
>>  index 000000000000..997f74127d8c
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/hwmon/common.yaml
>>  @@ -0,0 +1,31 @@
>>  +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>  +%YAML 1.2
>>  +---
>>  +$id: http://devicetree.org/schemas/hwmon/common.yaml#
>>  +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>  +
>>  +title: Common properties for hwmon sensors
>>  +
>>  +maintainers:
>>  +  - Jean Delvare <jdelvare@suse.com>
>>  +  - Guenter Roeck <linux@roeck-us.net>
>>  +
>>  +description: |
>>  +  This document defines device tree properties common to several 
>> hwmon
>>  +  sensors. It doesn't constitue a device tree binding 
>> specification by itself but
>>  +  is meant to be referenced by device tree bindings.
>>  +
>>  +  When referenced from sensor tree bindings the properties defined 
>> in this
>>  +  document are defined as follows. The sensor tree bindings are 
>> responsible for
>>  +  defining whether each property is required or optional.
>>  +
>>  +properties:
>>  +  label:
>>  +    $ref: /schemas/types.yaml#/definitions/string
>>  +    description: >
>>  +      Descriptive label that allows to uniquely identify a device 
>> within
>>  +      the system.
>>  +
>>  +additionalProperties: true
>>  +
>>  +...
>>  --
>>  2.34.1
>> 
>>