From: "Pop, Cristian" <Cristian.Pop@analog.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel
Date: Fri, 4 Sep 2020 11:14:04 +0000 [thread overview]
Message-ID: <BN6PR03MB2514AE2330F47235028B4455E72D0@BN6PR03MB2514.namprd03.prod.outlook.com> (raw)
In-Reply-To: <20200830122425.3e00332b@archlinux>
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,
next prev parent reply other threads:[~2020-09-04 12:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-24 8:36 [RFC PATCH v2] iio: core: Add optional symbolic label to a device channel Cristian Pop
2020-08-30 11:24 ` Jonathan Cameron
2020-09-04 11:14 ` Pop, Cristian [this message]
2020-09-04 13:35 ` Pop, Cristian
2020-09-06 15:53 ` Jonathan Cameron
2020-09-11 13:33 ` Pop, Cristian
2020-09-13 9:13 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN6PR03MB2514AE2330F47235028B4455E72D0@BN6PR03MB2514.namprd03.prod.outlook.com \
--to=cristian.pop@analog.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).