lkml.org 
[lkml]   [2020]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel
    Date


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

    \
     
     \ /
      Last update: 2020-09-11 18:23    [W:2.267 / U:0.896 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site