Message-ID: <BN6PR03MB25142BDF4D295086CE214F84E7240@BN6PR03MB2514.namprd03.prod.outlook.com>
Date:   Fri, 11 Sep 2020 13:33:53 +0000
From:   "Pop, Cristian" <Cristian.Pop@...log.com>
To:     Jonathan Cameron <jic23@...nel.org>
CC:     "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device
 channel



> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Sunday, September 6, 2020 6:53 PM
> To: Pop, Cristian <Cristian.Pop@...log.com>
> Cc: linux-iio@...r.kernel.org; linux-kernel@...r.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@...log.com> wrote:
> 
> > > -----Original Message-----
> > > From: Pop, Cristian
> > > Sent: Friday, September 4, 2020 2:14 PM
> > > To: Jonathan Cameron <jic23@...nel.org>
> > > Cc: linux-iio@...r.kernel.org; linux-kernel@...r.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@...nel.org>
> > > > Sent: Sunday, August 30, 2020 2:24 PM
> > > > To: Pop, Cristian <Cristian.Pop@...log.com>
> > > > Cc: linux-iio@...r.kernel.org; linux-kernel@...r.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@...log.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@...log.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,
> >

Powered by blists - more mailing lists