Re: [PATCH v5 03/13] iio: core: add hierarchical channel relationships

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 21 May 2026 14:48:30 +0100
Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote:

> On 26/05/18 03:26PM, Rodrigo Alencar wrote:
> > On 26/05/17 06:56PM, sashiko-bot@kernel.org wrote:  
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [Medium] Exposing the hierarchical relationship via channel labels creates an ambiguous UAPI for userspace.
> > > --
> > > 
> > > commit 64c6f961f1d7645c58f732e4bd4c11200b999e93
> > > Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > > 
> > > iio: core: add hierarchical channel relationships
> > > 
> > > This commit introduces a parent-child relationship between IIO channels.
> > > It adds a parent pointer to the iio_chan_spec struct and exposes this
> > > relationship to userspace through a new "parent" sysfs attribute that
> > > returns the parent channel's label.
> > >   
> > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > > index 5c8404efd0a56..348ac7a597385 100644
> > > > --- a/drivers/iio/industrialio-core.c
> > > > +++ b/drivers/iio/industrialio-core.c  
> > > 
> > > [ ... ]
> > >   
> > > > @@ -1263,6 +1271,31 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev,
> > > >  	return 1;
> > > >  }
> > > >  
> > > > +static int iio_device_add_channel_parent(struct iio_dev *indio_dev,
> > > > +					 struct iio_chan_spec const *chan)
> > > > +{
> > > > +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > > +	int ret;
> > > > +
> > > > +	if (!chan->parent || (!indio_dev->info->read_label &&
> > > > +			      !chan->parent->extend_name))
> > > > +		return 0;  
> > > 
> > > Is relying on channel labels robust enough for programmatic parent resolution?
> > > 
> > > IIO channel labels are optional, human-readable descriptors and are not
> > > guaranteed by the core to be unique. If multiple channels within a device
> > > share the same label, how can userspace definitively resolve which specific
> > > channel is the parent?
> > > 
> > > Also, if a driver establishes a hierarchy where chan->parent is set, but the
> > > parent channel lacks a label, this attribute is silently omitted, hiding the
> > > relationship entirely.
> > > 
> > > Would it be more reliable to expose a definitive, unique identifier (such as
> > > the parent channel's sysfs prefix or index) rather than relying on an optional
> > > descriptive string?  
> > 
> > A bit impressive that sashiko can comment on that architecture level.
> > 
> > Those are all good points. I suppose I went for this because otherwise it would
> > require refactoring core code to get the needed parts from __iio_device_attr_init()
> > that gets you the channel name without the attribute part.
> > 
> > Maybe we should have a function that outputs a channel name into a buffer based on
> > __iio_device_attr_init() behavior, and that could be reused.  
> 
> This is the function with behavior extracted from __iio_device_attr_init():
> 
> 	static int __iio_chan_prefix_emit(const struct iio_chan_spec *chan,
> 					  enum iio_shared_by shared_by,
> 					  char *buf, size_t len)
> 	{
> 		const char *dir = iio_direction[chan->output];
> 		const char *type = iio_chan_type_name_spec[chan->type];
> 		int n = 0;
> 
> 		switch (shared_by) {
> 		case IIO_SHARED_BY_ALL:
> 			break;
> 		case IIO_SHARED_BY_DIR:
> 			n = scnprintf(buf, len, "%s", dir);
> 			break;
> 		case IIO_SHARED_BY_TYPE:
> 			n = scnprintf(buf, len, "%s_%s", dir, type);
> 			if (chan->differential)
> 				n += scnprintf(buf + n, len - n, "-%s", type);
> 			break;
> 		case IIO_SEPARATE:
> 			if (chan->indexed) {
> 				n = scnprintf(buf, len, "%s_%s%d", dir, type,
> 					      chan->channel);
> 				if (chan->differential)
> 					n += scnprintf(buf + n, len - n, "-%s%d", type,
> 						       chan->channel2);
> 			} else {
> 				if (chan->differential) {
> 					WARN(1, "Differential channels must be indexed\n");
> 					return -EINVAL;
> 				}
> 				n = scnprintf(buf, len, "%s_%s", dir, type);
> 			}
> 
> 			if (chan->modified) {
> 				if (chan->differential) {
> 					WARN(1, "Differential channels can not have modifier\n");
> 					return -EINVAL;
> 				}
> 				n += scnprintf(buf + n, len - n, "_%s",
> 					       iio_modifier_names[chan->channel2]);
> 			}
> 
> 			if (chan->extend_name)
> 				n += scnprintf(buf + n, len - n, "_%s", chan->extend_name);
> 			break;
> 		}
> 
> 		return n;
> 	}
> 
> I think it is clear and reusable.

You only what the SEPARATE case but given the rest is much simpler anyway probably fine
to just use this function.

Jonathan



> 
> > > > +
> > > > +	ret = __iio_add_chan_devattr("parent",
> > > > +				     chan,
> > > > +				     &iio_read_channel_parent,
> > > > +				     NULL,
> > > > +				     0,
> > > > +				     IIO_SEPARATE,
> > > > +				     &indio_dev->dev,
> > > > +				     NULL,
> > > > +				     &iio_dev_opaque->channel_attr_list);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 1;
> > > > +}  
> > > 
> > > -- 
> > > Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@analog.com?part=3  
>