On Thu, Jun 18, 2026 at 05:14:19PM +0100, Rodrigo Alencar wrote:
> On 18/06/26 16:06, Nuno Sá wrote:
> > On Thu, Jun 18, 2026 at 02:27:22PM +0100, Rodrigo Alencar via B4 Relay
> > wrote:
> > > From: Rodrigo Alencar <[email protected]>
> > >
> > > Move logic to create a channel prefix for naming attribute files into a
> > > separate __iio_chan_prefix_emit() function for reuse.
>
> ...
>
> > > +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:
> > > + buf[0] = '\0'; /* empty channel prefix */
> > > + 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;
> >
> > WARN() looks too much to me. dev_error() as we're treating it as such. I
> > guess you don't want to pass struct device but not really an issue IMHO.
>
> __iio_device_attr_init() also used WARN(), probably because it didnt have
> access to a dev pointer. It would not be a problem to add an extra param.
Hmm, fair enough. Maybe a chance to change it. Not sure how others feel
about it.
>
> >
> > > + }
> > > + 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;
> > > + }
> > > +
> > > + if (n > 0 && n < len - 1) { /* prefix termination if not empty */
> > > + buf[n++] = '_';
> > > + buf[n] = '\0';
> > > + }
> > > +
> >
> > Can't we handle the above in the caller on kasprintf()? Then we could
> > simplify and return in place.
>
> I felt like doing this here would get a cleaner logic in the caller, which
> would have to add the '_' conditionally.
>
I think it makes things more clear in the caller given we return n
anyways but I don't feel strong about it.
- Nuno Sá
> >
> > > + return n;
> > > +}
> > > +
> > > /**
> > > * iio_device_id() - query the unique ID for the device
> > > * @indio_dev: Device structure whose ID is being queried
> > > @@ -1100,106 +1159,19 @@ int __iio_device_attr_init(struct
> > > device_attribute *dev_attr,
> > > size_t len),
> > > enum iio_shared_by shared_by)
> > > {
> > > - int ret = 0;
> > > - char *name = NULL;
> > > - char *full_postfix;
> > > + char prefix[NAME_MAX + 1];
> > > + int ret;
> > >
> > > sysfs_attr_init(&dev_attr->attr);
> > >
> > > - /* Build up postfix of <extend_name>_<modifier>_postfix */
> > > - if (chan->modified && (shared_by == IIO_SEPARATE)) {
> > > - if (chan->extend_name)
> > > - full_postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
> > > -
> > > iio_modifier_names[chan->channel2],
> > > - chan->extend_name,
> > > - postfix);
> > > - else
> > > - full_postfix = kasprintf(GFP_KERNEL, "%s_%s",
> > > -
> > > iio_modifier_names[chan->channel2],
> > > - postfix);
> > > - } else {
> > > - if (chan->extend_name == NULL || shared_by != IIO_SEPARATE)
> > > - full_postfix = kstrdup(postfix, GFP_KERNEL);
> > > - else
> > > - full_postfix = kasprintf(GFP_KERNEL,
> > > - "%s_%s",
> > > - chan->extend_name,
> > > - postfix);
> > > - }
> > > - if (full_postfix == NULL)
> > > + ret = __iio_chan_prefix_emit(chan, shared_by, prefix, sizeof(prefix));
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + dev_attr->attr.name = kasprintf(GFP_KERNEL, "%s%s", prefix, postfix);
> > > + if (!dev_attr->attr.name)
> > > return -ENOMEM;
> >
> > I don't oppose the change. Looks like a nice cleanup. But bear in mind
> > this very sensible as any subtle mistake means ABI breakage.
>
> Yes! I tried to be careful... this is dangerous stuff!
>
> --
> Kind regards,
>
> Rodrigo Alencar
