On Mon, 24 Aug 2020 11:36:46 +0300
Cristian Pop <cristian....@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....@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.
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,
