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

[Jonathan Cameron <jic23@kernel.org>]

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