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

From: Pop, Cristian
Date: Fri Sep 11 2020 - 12:23:02 EST

Next message: Tushar Sugandhi: "Re: [PATCH v3 2/6] IMA: change process_buffer_measurement return type from void to int"
Previous message: Jann Horn: "Re: [PATCH] sched.h: drop in_ubsan field when UBSAN is in trap mode"
Next in thread: Jonathan Cameron: "Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Tushar Sugandhi: "Re: [PATCH v3 2/6] IMA: change process_buffer_measurement return type from void to int"
Previous message: Jann Horn: "Re: [PATCH] sched.h: drop in_ubsan field when UBSAN is in trap mode"
Next in thread: Jonathan Cameron: "Re: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]