Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 20 Feb 2022 13:18:09 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Nandor Han <nandor.han@vaisala.com>
Cc:     lars@metafoo.de, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] iio: core: provide a default value `label` property
Message-ID: <20220220131809.1bc184e0@jic23-huawei>
In-Reply-To: <20220216135604.3435769-1-nandor.han@vaisala.com>
References: <20220216135604.3435769-1-nandor.han@vaisala.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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)