[RFC PATCH] iio: core: provide a default value `label` property

[RFC PATCH] iio: core: provide a default value `label` property

[Nandor Han]

The label property is used to correctly identify the same IIO device
over reboots. The implementation requires that a value will be provided
through device-tree. This sometime could requires many changes to
device-trees when multiple devices want to use the label property.
In order to prevent this, we could use the device-tree node
name as default value. The device-tree node name is unique and
also reflects the device which makes it a good choice as default value.
This change is backward compatible since doesn't affect the users that
do configure a label using the device-tree or the ones that are not
using the labels at all.

Use the device-tree node name as a default value for `label` property,
in case there isn't one configured through device-tree.

Signed-off-by: Nandor Han <nandor.han@vaisala.com>
---

Notes:
    Testing
    -------
    Using mx6sxsabresd dev board with device-tree:
    ```
    &adc1 {
        vref-supply = <&reg_vref_3v3>;
        label = "adc1";
        status = "okay";
    };
    
    &adc2 {
        vref-supply = <&reg_vref_3v3>;
        status = "okay";
    };
    ```
    1. Verify that label property is visible and readable:PASS
    > iio:device0 # ls -la label
    -r--r--r--    1 root     root          4096 Jan  1 00:00 label
    > iio:device0 # cat label
    adc1
    ```
    2. Verify that default label property is used for adc2 device: PASS
    ```
    > iio:device1 # cat label
    adc@2284000
    ```

 drivers/iio/industrialio-core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e1ed44dec2ab..bd26df90ce41 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1895,6 +1895,7 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	const char *label;
+	const char *node_name;
 	int ret;
 
 	if (!indio_dev->info)
@@ -1906,8 +1907,13 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 		indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
 
 	label = of_get_property(indio_dev->dev.of_node, "label", NULL);
-	if (label)
+	if (label) {
 		indio_dev->label = label;
+	} else {
+		node_name = of_node_full_name(indio_dev->dev.of_node);
+		if (node_name)
+			indio_dev->label = node_name;
+	}
 
 	ret = iio_check_unique_scan_index(indio_dev);
 	if (ret < 0)
-- 
2.34.1

Re: [RFC PATCH] iio: core: provide a default value `label` property

[Jonathan Cameron]

On Wed, 16 Feb 2022 15:56:04 +0200
Nandor Han <nandor.han@vaisala.com> wrote:

> The label property is used to correctly identify the same IIO device
> over reboots. The implementation requires that a value will be provided
> through device-tree. This sometime could requires many changes to
> device-trees when multiple devices want to use the label property.
> In order to prevent this, we could use the device-tree node
> name as default value. The device-tree node name is unique and
> also reflects the device which makes it a good choice as default value.
> This change is backward compatible since doesn't affect the users that
> do configure a label using the device-tree or the ones that are not
> using the labels at all.
> 
> Use the device-tree node name as a default value for `label` property,
> in case there isn't one configured through device-tree.

Interesting idea.  However a few concerns come to mind.
1) If we start having a default for this, then it will get used as ABI
   and if a label is applied later to the DT then we will end up breaking
   userspace scripts.
2) If we do this it should be firmware agnostics (we need to fix
   the existing code to be such as well).
3) Is the node name always unique (think multiple accelerometers on
   different i2c masters)?
3) I'm fairly sure this information is readily available anyway.
   either via the of_node link for the iio\:deviceX 
   So why not have your usespace use that instead of label?
   I'm not a fan of duplicating information that is readily available
   anyway - be it as name and reg in the of_node directory.

Thanks,

Jonathan

> 
> Signed-off-by: Nandor Han <nandor.han@vaisala.com>
> ---
> 
> Notes:
>     Testing
>     -------
>     Using mx6sxsabresd dev board with device-tree:
>     ```
>     &adc1 {
>         vref-supply = <&reg_vref_3v3>;
>         label = "adc1";
>         status = "okay";
>     };
>     
>     &adc2 {
>         vref-supply = <&reg_vref_3v3>;
>         status = "okay";
>     };
>     ```
>     1. Verify that label property is visible and readable:PASS
>     > iio:device0 # ls -la label  
>     -r--r--r--    1 root     root          4096 Jan  1 00:00 label
>     > iio:device0 # cat label  
>     adc1
>     ```
>     2. Verify that default label property is used for adc2 device: PASS
>     ```
>     > iio:device1 # cat label  
>     adc@2284000
>     ```
> 
>  drivers/iio/industrialio-core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index e1ed44dec2ab..bd26df90ce41 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1895,6 +1895,7 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	const char *label;
> +	const char *node_name;
>  	int ret;
>  
>  	if (!indio_dev->info)
> @@ -1906,8 +1907,13 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  		indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
>  
>  	label = of_get_property(indio_dev->dev.of_node, "label", NULL);
> -	if (label)
> +	if (label) {
>  		indio_dev->label = label;
> +	} else {
> +		node_name = of_node_full_name(indio_dev->dev.of_node);
> +		if (node_name)
> +			indio_dev->label = node_name;
> +	}
>  
>  	ret = iio_check_unique_scan_index(indio_dev);
>  	if (ret < 0)

Re: [RFC PATCH] iio: core: provide a default value `label` property

[Lars-Peter Clausen]

On 2/20/22 14:18, Jonathan Cameron wrote:
> On Wed, 16 Feb 2022 15:56:04 +0200
> Nandor Han <nandor.han@vaisala.com> wrote:
>
>> The label property is used to correctly identify the same IIO device
>> over reboots. The implementation requires that a value will be provided
>> through device-tree. This sometime could requires many changes to
>> device-trees when multiple devices want to use the label property.
>> In order to prevent this, we could use the device-tree node
>> name as default value. The device-tree node name is unique and
>> also reflects the device which makes it a good choice as default value.
>> This change is backward compatible since doesn't affect the users that
>> do configure a label using the device-tree or the ones that are not
>> using the labels at all.
>>
>> Use the device-tree node name as a default value for `label` property,
>> in case there isn't one configured through device-tree.
> Interesting idea.  However a few concerns come to mind.
> 1) If we start having a default for this, then it will get used as ABI
>     and if a label is applied later to the DT then we will end up breaking
>     userspace scripts.
> 2) If we do this it should be firmware agnostics (we need to fix
>     the existing code to be such as well).
> 3) Is the node name always unique (think multiple accelerometers on
>     different i2c masters)?
> 3) I'm fairly sure this information is readily available anyway.
>     either via the of_node link for the iio\:deviceX
>     So why not have your usespace use that instead of label?
>     I'm not a fan of duplicating information that is readily available
>     anyway - be it as name and reg in the of_node directory.

I'm not a big fan of this either for the above reasons.

Re: [RFC PATCH] iio: core: provide a default value `label` property

[Nandor Han]

On 2/20/22 15:18, Jonathan Cameron wrote:
> On Wed, 16 Feb 2022 15:56:04 +0200
> Nandor Han <nandor.han@vaisala.com> wrote:
> 

Thanks for reviewing the patch and provide feed back.

>> The label property is used to correctly identify the same IIO device
>> over reboots. The implementation requires that a value will be provided
>> through device-tree. This sometime could requires many changes to
>> device-trees when multiple devices want to use the label property.
>> In order to prevent this, we could use the device-tree node
>> name as default value. The device-tree node name is unique and
>> also reflects the device which makes it a good choice as default value.
>> This change is backward compatible since doesn't affect the users that
>> do configure a label using the device-tree or the ones that are not
>> using the labels at all.
>>
>> Use the device-tree node name as a default value for `label` property,
>> in case there isn't one configured through device-tree.
> 
> Interesting idea.  However a few concerns come to mind.
> 1) If we start having a default for this, then it will get used as ABI
>     and if a label is applied later to the DT then we will end up breaking
>     userspace scripts.

When a label is explicitly configured means that the userspace expects 
to have that value available. Therefore, I don't see this as ABI change, 
given that this affects the property label content and not for example 
the property name.

> 2) If we do this it should be firmware agnostics (we need to fix
>     the existing code to be such as well).

Not sure I understand this. If you could explain a bit more I would 
really appriciate.

> 3) Is the node name always unique (think multiple accelerometers on
>     different i2c masters)?
> 3) I'm fairly sure this information is readily available anyway.
>     either via the of_node link for the iio\:deviceX
>     So why not have your usespace use that instead of label?
>     I'm not a fan of duplicating information that is readily available
>     anyway - be it as name and reg in the of_node directory.
> 

The node name supposed to be unique AFAIK and you're right it is 
available already in the userspace.
My point with this patch is to provide a default value for the label 
content and I'm open to suggestions related to content. The of_node name 
was something that I thought that is unique and easy to use, but if 
somebody has better suggestions I'm really open to these.

> Thanks,
> 
> Jonathan
> 

<snip>


Thanks again and regards,
    Nandor

Re: [RFC PATCH] iio: core: provide a default value `label` property

[Jonathan Cameron]

On Tue, 22 Feb 2022 09:42:12 +0200
Nandor Han <nandor.han@vaisala.com> wrote:

> On 2/20/22 15:18, Jonathan Cameron wrote:
> > On Wed, 16 Feb 2022 15:56:04 +0200
> > Nandor Han <nandor.han@vaisala.com> wrote:
> >   
> 
> Thanks for reviewing the patch and provide feed back.
> 
> >> The label property is used to correctly identify the same IIO device
> >> over reboots. The implementation requires that a value will be provided
> >> through device-tree. This sometime could requires many changes to
> >> device-trees when multiple devices want to use the label property.
> >> In order to prevent this, we could use the device-tree node
> >> name as default value. The device-tree node name is unique and
> >> also reflects the device which makes it a good choice as default value.
> >> This change is backward compatible since doesn't affect the users that
> >> do configure a label using the device-tree or the ones that are not
> >> using the labels at all.
> >>
> >> Use the device-tree node name as a default value for `label` property,
> >> in case there isn't one configured through device-tree.  
> > 
> > Interesting idea.  However a few concerns come to mind.
> > 1) If we start having a default for this, then it will get used as ABI
> >     and if a label is applied later to the DT then we will end up breaking
> >     userspace scripts.  
> 
> When a label is explicitly configured means that the userspace expects 
> to have that value available. Therefore, I don't see this as ABI change, 
> given that this affects the property label content and not for example 
> the property name.

The potential issue is that with this userspace code may rely on the common
option (matches device tree node name) and then get confused on it changing.

If it wasn't there previously and appears (which is what happens when
a label is added currently) userspace is unlikely to have in some fashion
depended on it not being there...

If someone modifies an existing label they can reasonably expect to break
compatibility because they made something 'go away'.

> 
> > 2) If we do this it should be firmware agnostics (we need to fix
> >     the existing code to be such as well).  
> 
> Not sure I understand this. If you could explain a bit more I would 
> really appriciate.

Typo in that didn't help. (agnostic).  Anyhow, basically it has to work
for ACPI as well.

> 
> > 3) Is the node name always unique (think multiple accelerometers on
> >     different i2c masters)?
> > 3) I'm fairly sure this information is readily available anyway.
> >     either via the of_node link for the iio\:deviceX
> >     So why not have your usespace use that instead of label?
> >     I'm not a fan of duplicating information that is readily available
> >     anyway - be it as name and reg in the of_node directory.
> >   
> 
> The node name supposed to be unique AFAIK and you're right it is 
> available already in the userspace.

It's not unique.  As per https://elinux.org/Device_Tree_Usage,
"sibling nodes are expected to be unique".  If you have multiple
i2c masters and the same device under each of them with the
same i2c address they are not siblings (different parents) and
will have the same node name. 


> My point with this patch is to provide a default value for the label 
> content and I'm open to suggestions related to content. The of_node name 
> was something that I thought that is unique and easy to use, but if 
> somebody has better suggestions I'm really open to these.

I don't see why we want a default label. If it's not provided it's
not there (no file) and userspace can go use something else for
it's unique naming. Note that for older kernels they need to do
that anyway because label never existed. So userspace will need
to work with possibility of it being absent. As userspace is
going to do that today, why add another option so we have:

1) No label attribute.
2) Label attribute == node name
3) Label attribute something else

vs having to handle 2 cases.

1) No label attribute
2) Label attribute present.

So adding a default makes userspace code more complex, not less.

Jonathan
> 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> <snip>
> 
> 
> Thanks again and regards,
>     Nandor

Re: [RFC PATCH] iio: core: provide a default value `label` property

[Nandor Han]

On 2/22/22 18:36, Jonathan Cameron wrote:
> On Tue, 22 Feb 2022 09:42:12 +0200
> Nandor Han <nandor.han@vaisala.com> wrote:
> 
>> On 2/20/22 15:18, Jonathan Cameron wrote:
>>> On Wed, 16 Feb 2022 15:56:04 +0200
>>> Nandor Han <nandor.han@vaisala.com> wrote:
>>>    
>>
>> Thanks for reviewing the patch and provide feed back.
>>
>>>> The label property is used to correctly identify the same IIO device
>>>> over reboots. The implementation requires that a value will be provided
>>>> through device-tree. This sometime could requires many changes to
>>>> device-trees when multiple devices want to use the label property.
>>>> In order to prevent this, we could use the device-tree node
>>>> name as default value. The device-tree node name is unique and
>>>> also reflects the device which makes it a good choice as default value.
>>>> This change is backward compatible since doesn't affect the users that
>>>> do configure a label using the device-tree or the ones that are not
>>>> using the labels at all.
>>>>
>>>> Use the device-tree node name as a default value for `label` property,
>>>> in case there isn't one configured through device-tree.
>>>
>>> Interesting idea.  However a few concerns come to mind.
>>> 1) If we start having a default for this, then it will get used as ABI
>>>      and if a label is applied later to the DT then we will end up breaking
>>>      userspace scripts.
>>
>> When a label is explicitly configured means that the userspace expects
>> to have that value available. Therefore, I don't see this as ABI change,
>> given that this affects the property label content and not for example
>> the property name.
> 
> The potential issue is that with this userspace code may rely on the common
> option (matches device tree node name) and then get confused on it changing.
> 
> If it wasn't there previously and appears (which is what happens when
> a label is added currently) userspace is unlikely to have in some fashion
> depended on it not being there...
> 

If they rely on missing the label, it's true this will break that.

> If someone modifies an existing label they can reasonably expect to break
> compatibility because they made something 'go away'.
> 
>>
>>> 2) If we do this it should be firmware agnostics (we need to fix
>>>      the existing code to be such as well).
>>
>> Not sure I understand this. If you could explain a bit more I would
>> really appriciate.
> 
> Typo in that didn't help. (agnostic).  Anyhow, basically it has to work
> for ACPI as well.
> 
>>
>>> 3) Is the node name always unique (think multiple accelerometers on
>>>      different i2c masters)?
>>> 3) I'm fairly sure this information is readily available anyway.
>>>      either via the of_node link for the iio\:deviceX
>>>      So why not have your usespace use that instead of label?
>>>      I'm not a fan of duplicating information that is readily available
>>>      anyway - be it as name and reg in the of_node directory.
>>>    
>>
>> The node name supposed to be unique AFAIK and you're right it is
>> available already in the userspace.
> 
> It's not unique.  As per https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Felinux.org%2FDevice_Tree_Usage&amp;data=04%7C01%7Cnandor.han%40vaisala.com%7Cdbfd5d178b484f5258fe08d9f6216e65%7C6d7393e041f54c2e9b124c2be5da5c57%7C0%7C0%7C637811445712047047%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=0y2Vx5Cqso5iYAvA58tXnfzI%2BJ9oUbhvDIIkC8XP6Ik%3D&amp;reserved=0,
> "sibling nodes are expected to be unique".  If you have multiple
> i2c masters and the same device under each of them with the
> same i2c address they are not siblings (different parents) and
> will have the same node name.
> 

Thanks for the info. Keep that in mind

> 
>> My point with this patch is to provide a default value for the label
>> content and I'm open to suggestions related to content. The of_node name
>> was something that I thought that is unique and easy to use, but if
>> somebody has better suggestions I'm really open to these.
> 
> I don't see why we want a default label. If it's not provided it's
> not there (no file) and userspace can go use something else for
> it's unique naming. Note that for older kernels they need to do
> that anyway because label never existed. So userspace will need
> to work with possibility of it being absent. As userspace is
> going to do that today, why add another option so we have:
> 
> 1) No label attribute.
> 2) Label attribute == node name
> 3) Label attribute something else
> 
> vs having to handle 2 cases.
> 
> 1) No label attribute
> 2) Label attribute present.
> 
> So adding a default makes userspace code more complex, not less.
> 

Thanks for the explanation and review.

Given the above comments I guess we will drop this idea.



Thanks and Regards,
    Nandor