[RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Cristian Pop]

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)
+			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;
+
 	/*
 	 * 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;
 	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,
-- 
2.17.1

Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Jonathan Cameron]

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,

RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Pop, Cristian]

Best ragards,
Cristian Pop

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, August 30, 2020 2:24 PM
> To: Pop, Cristian <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
> 
> [External]
> 
> 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.
> 
If "label_name" can't be part of the "struct iio_chan_spec " why would I pass
it to the callback function? Just identify the channel?
I would implement a callback :
int (*read_string)(struct iio_dev *indio_dev,
	const struct iio_chan_spec *chan, const char *buf, long info);
and use this callback for other purposes as well, not only for "label" attribute.

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

RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Pop, Cristian]

> -----Original Message-----
> From: Pop, Cristian
> Sent: Friday, September 4, 2020 2:14 PM
> To: Jonathan Cameron <jic23@kernel.org>
> 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
> 
> Best ragards,
> Cristian Pop
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, August 30, 2020 2:24 PM
> > To: Pop, Cristian <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
> >
> > [External]
> >
> > 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.  
What do you mean by "chan_spec is constant in many drivers"? Instances of the "struct iio_chan_spec" are created in the driver. Also it makes sense for me to add "const char              *label_name;" in this structure since it is an attribute of the channel, and it doesn't change at runtime, only when parsing the device tree and assigning the value to it, when an instance of "iio_chan_spec" is created.
>> 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.
Don't understand this part. "name" and "label" of the device are elements of "struct iio_dev", as "const char  *label_name;" is part of "struct iio_chan_spec", the equivalent structure for holding channel attributes.
Who will hold the label values otherwise, if not the " iio_chan_spec " structure?
> > 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.
I do agree with the callback, it can be a more generic callback, to return strings, for other purposes also.
Something like this:
    int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec const *chan,  char *string,  long mask);
or
    int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec const *chan, const char **string, long mask);
The callback will be called in "iio_read_channel_info", so I think that a system file will be created for it.
> >
> > 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,

Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Jonathan Cameron]

On Fri, 4 Sep 2020 13:35:12 +0000
"Pop, Cristian" <Cristian.Pop@analog.com> wrote:

> > -----Original Message-----
> > From: Pop, Cristian
> > Sent: Friday, September 4, 2020 2:14 PM
> > To: Jonathan Cameron <jic23@kernel.org>
> > 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

Hi Cristian,

If possible fix your word wrapping for future replies.
I've tried to unwind it below but it makes it very hard
to read and reply to.

> > 
> > Best ragards,
> > Cristian Pop
> >   
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Sunday, August 30, 2020 2:24 PM
> > > To: Pop, Cristian <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
> > >
> > > [External]
> > >
> > > 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.    
> What do you mean by "chan_spec is constant in many drivers"?
> Instances of the "struct iio_chan_spec" are created in the driver. 

No they aren't.  Grep for struct iio_chan_spec 
Something like... 
git grep iio_chan_spec -- drivers/iio/ | grep const | grep channels

A very very large number of drivers keep this data constant.
If the driver doesn't provide flexibility on enabled channels
(for example such flexibility makes no sense on an accelerometer)
then the instances of this structure are constant.

There are lots of good reasons to do this if at all possible and
I'm not happy changing it just to put in an optional string.

> Also it makes sense for me to add "const char              *label_name;"
> in this structure since it is an attribute of the channel, and it doesn't
> change at runtime, only when parsing the device tree and assigning the value to it

That is at runtime.  The structure is constant at build time in many/most drivers.
There are 459 instances using the above grep for starters that would all
need to change.

>, when an instance of "iio_chan_spec" is created.
> >> 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.  
> Don't understand this part. "name" and "label" of the device are elements of
> "struct iio_dev", as "const char  *label_name;" is part of "struct iio_chan_spec", the equivalent structure for holding channel attributes.
It is not the equivalent structure.  There is no equivalent per channel structure.
iio_chan_spec is a specification for a channel, not a dynamic structure holding
information about it, whereas iio_dev is such a dynamic structure holding
dynamic information about the device (plus a few small bits of constant info).

If we need a dynamic structure per channel then we need to create a new one,
not take a massive amount of constant data and make it dynamic in order
to add a single dynamic field.

> Who will hold the label values otherwise, if not the " iio_chan_spec " structure?

A new element.  For now we should keep this in the drivers because
it is a driver decision to provide this information.  For a lot of
drivers it makes no sense to have a label so we don't want to put the
burden on the IIO core.

For now, a driver should just copy the labels into driver local storage
and return them from the new callback.

Later, we can look at adding utility functions etc if it turns out to make
sense.  However, I'm not interested in doing that until we have a
reasonable number of drivers using the facility and hence
a good understanding of the common elements of such functions.

> > > 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.  
> I do agree with the callback, it can be a more generic callback,
> to return strings, for other purposes also.
> Something like this:
>     int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec const *chan,  char *string,  long mask);
> or
>     int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec const *chan, const char **string, long mask);
> The callback will be called in "iio_read_channel_info", so I think that a system file will be created for it.

What else is it used for?  If you are refactoring some other code
to use this new callback then it is worth considering.  Until then
it's an unused generalization so a bad idea.

Thanks,

Jonathan

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

RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Pop, Cristian]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, September 6, 2020 6:53 PM
> To: Pop, Cristian <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
> 
> [External]
> 
> On Fri, 4 Sep 2020 13:35:12 +0000
> "Pop, Cristian" <Cristian.Pop@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Pop, Cristian
> > > Sent: Friday, September 4, 2020 2:14 PM
> > > To: Jonathan Cameron <jic23@kernel.org>
> > > 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
> 
> Hi Cristian,
> 
> If possible fix your word wrapping for future replies.
> I've tried to unwind it below but it makes it very hard to read and reply to.
> 
> > >
> > > Best ragards,
> > > Cristian Pop
> > >
> > > > -----Original Message-----
> > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > Sent: Sunday, August 30, 2020 2:24 PM
> > > > To: Pop, Cristian <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
> > > >
> > > > [External]
> > > >
> > > > 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.
> > What do you mean by "chan_spec is constant in many drivers"?
> > Instances of the "struct iio_chan_spec" are created in the driver.
> 
> No they aren't.  Grep for struct iio_chan_spec Something like...
> git grep iio_chan_spec -- drivers/iio/ | grep const | grep channels
> 
> A very very large number of drivers keep this data constant.
> If the driver doesn't provide flexibility on enabled channels (for example such
> flexibility makes no sense on an accelerometer) then the instances of this
> structure are constant.
> 
> There are lots of good reasons to do this if at all possible and I'm not happy
> changing it just to put in an optional string.
> 
> > Also it makes sense for me to add "const char              *label_name;"
> > in this structure since it is an attribute of the channel, and it
> > doesn't change at runtime, only when parsing the device tree and
> > assigning the value to it
> 
> That is at runtime.  The structure is constant at build time in many/most
> drivers.
> There are 459 instances using the above grep for starters that would all need
> to change.
> 
> >, when an instance of "iio_chan_spec" is created.
> > >> Don't use info_mask, but
Hi Jonathan!

How would you inform iio -core that label is present for a specific channel and
a file system for it should be created?

Best regards,
Cristian

> > > > register it separately for each channel in a similar way to we do
> > > > the name and label attributes for the whole device.
> > Don't understand this part. "name" and "label" of the device are
> > elements of "struct iio_dev", as "const char  *label_name;" is part of "struct
> iio_chan_spec", the equivalent structure for holding channel attributes.
> It is not the equivalent structure.  There is no equivalent per channel structure.
> iio_chan_spec is a specification for a channel, not a dynamic structure holding
> information about it, whereas iio_dev is such a dynamic structure holding
> dynamic information about the device (plus a few small bits of constant info).
> 
> If we need a dynamic structure per channel then we need to create a new one,
> not take a massive amount of constant data and make it dynamic in order to
> add a single dynamic field.
> 
> > Who will hold the label values otherwise, if not the " iio_chan_spec "
> structure?
> 
> A new element.  For now we should keep this in the drivers because it is a
> driver decision to provide this information.  For a lot of drivers it makes no
> sense to have a label so we don't want to put the burden on the IIO core.
> 
> For now, a driver should just copy the labels into driver local storage and
> return them from the new callback.
> 
> Later, we can look at adding utility functions etc if it turns out to make sense.
> However, I'm not interested in doing that until we have a reasonable number
> of drivers using the facility and hence a good understanding of the common
> elements of such functions.
> 
> > > > 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.
> > I do agree with the callback, it can be a more generic callback, to
> > return strings, for other purposes also.
> > Something like this:
> >     int (*read_string)(struct iio_dev *indio_dev,  struct
> > iio_chan_spec const *chan,  char *string,  long mask); or
> >     int (*read_string)(struct iio_dev *indio_dev,  struct
> > iio_chan_spec const *chan, const char **string, long mask); The callback will
> be called in "iio_read_channel_info", so I think that a system file will be
> created for it.
> 
> What else is it used for?  If you are refactoring some other code to use this
> new callback then it is worth considering.  Until then it's an unused
> generalization so a bad idea.
> 
> Thanks,
> 
> Jonathan
> 
> > > >
> > > > 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,
> >

Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel

[Jonathan Cameron]

On Fri, 11 Sep 2020 13:33:53 +0000
"Pop, Cristian" <Cristian.Pop@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, September 6, 2020 6:53 PM
> > To: Pop, Cristian <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
> > 
> > [External]
> > 
> > On Fri, 4 Sep 2020 13:35:12 +0000
> > "Pop, Cristian" <Cristian.Pop@analog.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Pop, Cristian
> > > > Sent: Friday, September 4, 2020 2:14 PM
> > > > To: Jonathan Cameron <jic23@kernel.org>
> > > > 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  
> > 
> > Hi Cristian,
> > 
> > If possible fix your word wrapping for future replies.
> > I've tried to unwind it below but it makes it very hard to read and reply to.
> >   
> > > >
> > > > Best ragards,
> > > > Cristian Pop
> > > >  
> > > > > -----Original Message-----
> > > > > From: Jonathan Cameron <jic23@kernel.org>
> > > > > Sent: Sunday, August 30, 2020 2:24 PM
> > > > > To: Pop, Cristian <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
> > > > >
> > > > > [External]
> > > > >
> > > > > 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.  
> > > What do you mean by "chan_spec is constant in many drivers"?
> > > Instances of the "struct iio_chan_spec" are created in the driver.  
> > 
> > No they aren't.  Grep for struct iio_chan_spec Something like...
> > git grep iio_chan_spec -- drivers/iio/ | grep const | grep channels
> > 
> > A very very large number of drivers keep this data constant.
> > If the driver doesn't provide flexibility on enabled channels (for example such
> > flexibility makes no sense on an accelerometer) then the instances of this
> > structure are constant.
> > 
> > There are lots of good reasons to do this if at all possible and I'm not happy
> > changing it just to put in an optional string.
> >   
> > > Also it makes sense for me to add "const char              *label_name;"
> > > in this structure since it is an attribute of the channel, and it
> > > doesn't change at runtime, only when parsing the device tree and
> > > assigning the value to it  
> > 
> > That is at runtime.  The structure is constant at build time in many/most
> > drivers.
> > There are 459 instances using the above grep for starters that would all need
> > to change.
> >   
> > >, when an instance of "iio_chan_spec" is created.  
> > > >> Don't use info_mask, but  
> Hi Jonathan!
> 
> How would you inform iio -core that label is present for a specific channel and
> a file system for it should be created?
It think we probably only need to control this at whole device level. It would be
odd for someone to bother labelling 'some' channels on a given device.
If we need to, an additional callback could be connected up to the is_visible
for the sysfs attributes.
If not, simple presence of the callback function probably does the job, or
possibly a device wide flag.

Jonathan


> 
> Best regards,
> Cristian
> 
> > > > > register it separately for each channel in a similar way to we do
> > > > > the name and label attributes for the whole device.  
> > > Don't understand this part. "name" and "label" of the device are
> > > elements of "struct iio_dev", as "const char  *label_name;" is part of "struct  
> > iio_chan_spec", the equivalent structure for holding channel attributes.
> > It is not the equivalent structure.  There is no equivalent per channel structure.
> > iio_chan_spec is a specification for a channel, not a dynamic structure holding
> > information about it, whereas iio_dev is such a dynamic structure holding
> > dynamic information about the device (plus a few small bits of constant info).
> > 
> > If we need a dynamic structure per channel then we need to create a new one,
> > not take a massive amount of constant data and make it dynamic in order to
> > add a single dynamic field.
> >   
> > > Who will hold the label values otherwise, if not the " iio_chan_spec "  
> > structure?
> > 
> > A new element.  For now we should keep this in the drivers because it is a
> > driver decision to provide this information.  For a lot of drivers it makes no
> > sense to have a label so we don't want to put the burden on the IIO core.
> > 
> > For now, a driver should just copy the labels into driver local storage and
> > return them from the new callback.
> > 
> > Later, we can look at adding utility functions etc if it turns out to make sense.
> > However, I'm not interested in doing that until we have a reasonable number
> > of drivers using the facility and hence a good understanding of the common
> > elements of such functions.
> >   
> > > > > 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.  
> > > I do agree with the callback, it can be a more generic callback, to
> > > return strings, for other purposes also.
> > > Something like this:
> > >     int (*read_string)(struct iio_dev *indio_dev,  struct
> > > iio_chan_spec const *chan,  char *string,  long mask); or
> > >     int (*read_string)(struct iio_dev *indio_dev,  struct
> > > iio_chan_spec const *chan, const char **string, long mask); The callback will  
> > be called in "iio_read_channel_info", so I think that a system file will be
> > created for it.
> > 
> > What else is it used for?  If you are refactoring some other code to use this
> > new callback then it is worth considering.  Until then it's an unused
> > generalization so a bad idea.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > > >
> > > > > 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,  
> > >  
>