Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 30 Aug 2020 12:24:25 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Cristian Pop <cristian.pop@analog.com>
Cc:     <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a
 device channel
Message-ID: <20200830122425.3e00332b@archlinux>
In-Reply-To: <20200824083646.84886-1-cristian.pop@analog.com>
References: <20200824083646.84886-1-cristian.pop@analog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 24 Aug 2020 11:36:46 +0300
Cristian Pop <cristian.pop@analog.com> wrote:

> If a label is defined in the device tree for this channel add that
> to the channel specific attributes. This is useful for userspace to
> be able to identify an individual channel.
> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> ---
>  Changes in v2:
> 	- Move label check before "read_raw" callback.
> 	- Move the responsability to of parsing channel labels, to the
> 	  driver.
> 
>  drivers/iio/industrialio-core.c | 10 ++++++++--
>  include/linux/iio/iio.h         |  2 ++
>  include/linux/iio/types.h       |  1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 1527f01a44f1..32277e94f02d 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -135,6 +135,7 @@ static const char * const iio_modifier_names[] = {
>  /* relies on pairs of these shared then separate */
>  static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_RAW] = "raw",
> +	[IIO_CHAN_INFO_LABEL] = "label",
>  	[IIO_CHAN_INFO_PROCESSED] = "input",
>  	[IIO_CHAN_INFO_SCALE] = "scale",
>  	[IIO_CHAN_INFO_OFFSET] = "offset",
> @@ -653,14 +654,18 @@ static ssize_t iio_read_channel_info(struct device *dev,
>  	int ret;
>  	int val_len = 2;
>  
> -	if (indio_dev->info->read_raw_multi)
> +	if (indio_dev->info->read_raw_multi) {
>  		ret = indio_dev->info->read_raw_multi(indio_dev, this_attr->c,
>  							INDIO_MAX_RAW_ELEMENTS,
>  							vals, &val_len,
>  							this_attr->address);
> -	else
> +	} else {
>  		ret = indio_dev->info->read_raw(indio_dev, this_attr->c,
>  				    &vals[0], &vals[1], this_attr->address);
> +		if (ret < 0 && this_attr->address == IIO_CHAN_INFO_LABEL &&
> +			this_attr->c->label_name)

I'm not keen on this. We shouldn't be calling read_raw at all in this path.
There is no way it can return a valid label.

> +			return sprintf(buf, "%s\n", this_attr->c->label_name);
> +	}
>  
>  	if (ret < 0)
>  		return ret;
> @@ -1399,6 +1404,7 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  			attrcount_orig++;
>  	}
>  	attrcount = attrcount_orig;
> +

Please avoid unrelated white space changes.

>  	/*
>  	 * New channel registration method - relies on the fact a group does
>  	 * not need to be initialized if its name is NULL.
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index a1be82e74c93..39209f3b62be 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -223,6 +223,7 @@ struct iio_event_spec {
>   *			correspond to the first name that the channel is referred
>   *			to by in the datasheet (e.g. IND), or the nearest
>   *			possible compound name (e.g. IND-INC).
> + * @label_name:		Unique name to identify which channel this is.
>   * @modified:		Does a modifier apply to this channel. What these are
>   *			depends on the channel type.  Modifier is set in
>   *			channel2. Examples are IIO_MOD_X for axial sensors about
> @@ -260,6 +261,7 @@ struct iio_chan_spec {
>  	const struct iio_chan_spec_ext_info *ext_info;
>  	const char		*extend_name;
>  	const char		*datasheet_name;
> +	const char		*label_name;

This can't be part of chan_spec as that is constant in many drivers.
We need a separate way of doing this.  Don't use info_mask,
but register it separately for each channel in a similar way
to we do the name and label attributes for the whole device.

I would add a new callback to iio_info that is passed the
iio_chan_spec and returns a const char * for the label name.

The driver would be responsible for doing a lookup based
on what it has cached from the dt parse, probably indexed
off address or scan_index (that can be driver specific)

To create the attribute you need to add this to
iio_device_register_sysfs and use the various core
functions to build the attribute name in a similar fashion to
that done for info mask elements.

It will be more complex than your approach, but make it more
'separable' as a feature in drivers.

Jonathan


>  	unsigned		modified:1;
>  	unsigned		indexed:1;
>  	unsigned		output:1;
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index e6fd3645963c..c8f65f476eb2 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -34,6 +34,7 @@ enum iio_available_type {
>  
>  enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_RAW = 0,
> +	IIO_CHAN_INFO_LABEL,
>  	IIO_CHAN_INFO_PROCESSED,
>  	IIO_CHAN_INFO_SCALE,
>  	IIO_CHAN_INFO_OFFSET,