On Thu, 18 Jun 2026 14:27:23 +0100 Rodrigo Alencar via B4 Relay <[email protected]> wrote:
> From: Rodrigo Alencar <[email protected]> > > Add parent-child relationship between iio channels by creating a parent > pointer field in iio_chan_spec struct and exposing a sysfs attribute that > returns the parent channel label. > > Signed-off-by: Rodrigo Alencar <[email protected]> Generally I like this. A couple of trivial suggestions inline. Jonathan > --- > drivers/iio/industrialio-core.c | 44 > +++++++++++++++++++++++++++++++++++++++++ > include/linux/iio/iio.h | 5 +++++ > 2 files changed, 49 insertions(+) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 9373006235c8..3d12269f26f4 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -854,6 +854,21 @@ static ssize_t iio_read_channel_label(struct device *dev, > to_iio_dev_attr(attr)->c, buf); > } > > +static ssize_t iio_read_channel_parent(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + const struct iio_chan_spec *parent = to_iio_dev_attr(attr)->c->parent; > + int len; > + > + len = __iio_chan_prefix_emit(parent, IIO_SEPARATE, buf, PAGE_SIZE - 1); > + if (len < 0) > + return len; > + > + buf[len - 1] = '\n'; /* replace underscore termination with newline */ That's irritating. Can we instead make __iio_chan_prefix_emit() not include the _ ? That might mean a wrapper to just do the last bit of logic in there. > + return len; > +} > + > static ssize_t iio_read_channel_info(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -1263,6 +1278,30 @@ 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) > + return 0; > + > + 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); Probably younger me style, but can we group a few of those into logical sets to reduce the number of lines? > + if (ret < 0) > + return ret; > + > + return 1; Hmm. Can we make it explicit this is about an attr in the function name. I think that would make the positive return more something we might expect than something that sounds like it is adding something non countable. > +} > + > static int iio_device_add_info_mask_type(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > enum iio_shared_by shared_by, > @@ -1401,6 +1440,11 @@ static int iio_device_add_channel_sysfs(struct iio_dev > *indio_dev, > return ret; > attrcount += ret; > > + ret = iio_device_add_channel_parent(indio_dev, chan); > + if (ret < 0) > + return ret; > + attrcount += ret; > + > if (chan->ext_info) { > unsigned int i = 0; > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index 1c7d12af22da..9470ab8eb726 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -264,6 +264,10 @@ struct iio_scan_type { > * @ext_info: Array of extended info attributes for this > channel. > * The array is NULL terminated, the last element should > * have its name field set to NULL. > + * @parent: Optional pointer to the parent channel spec for > + * hierarchical channel relationships. When set, a > read-only > + * "parent" sysfs attribute is created containing the > + * parent channel's sysfs name prefix (e.g. "in_voltage0"). > * @extend_name: Allows labeling of channel attributes with an > * informative name. Note this has no effect codes etc, > * unlike modifiers. > @@ -309,6 +313,7 @@ struct iio_chan_spec { > const struct iio_event_spec *event_spec; > unsigned int num_event_specs; > const struct iio_chan_spec_ext_info *ext_info; > + const struct iio_chan_spec *parent; > const char *extend_name; > const char *datasheet_name; > unsigned int modified:1; >
