On Thu, 21 May 2026 14:48:30 +0100 Rodrigo Alencar <[email protected]> wrote:
> On 26/05/18 03:26PM, Rodrigo Alencar wrote: > > On 26/05/17 06:56PM, [email protected] 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 <[email protected]> > > > > > > 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/[email protected]?part=3 > > > >
