On Fri, 2022-06-10 at 17:05 +0200, Andy Shevchenko wrote:
> On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> >
> > APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all()
> > were
> > returning a mix of NULL and error pointers being NULL the way to
> > "notify" that we should do a "system" lookup for channels. This
> > make
> > it very confusing and prone to errors as commit dbbccf7c20bf
> > ("iio: inkern: fix return value in
> > devm_of_iio_channel_get_by_name()")
> > proves. On top of this, patterns like 'if (channel != NULL) return
> > channel'
> > were being used where channel could actually be an error code which
> > makes the code hard to read.
>
> ...
>
> > np = np->parent;
> > if (np && !of_get_property(np, "io-channel-ranges",
> > NULL))
> > - return NULL;
> > + return chan;
>
> Shouldn't it return a dedicated error code and not some arbitrary
> one?
> It may be I missed something and chan has a correct error code in
> this
> case...
>
Since in this case we won't look for channels in the parent device, I'm
just honoring the code returned by 'of_iio_channel_get()'.
> ...
>
> > + if (nummaps == 0) /* return -ENODEV to search map
> > table */
>
> Comment is superfluous, the next line is self-explaining.
>
Well, I agree. I just kept as it was on the original code. Can hapilly
remove it if no one objects against it.
> > + return ERR_PTR(-ENODEV);
>
> ...
>
> > - if (channel != NULL)
> > + if (!IS_ERR(channel) || PTR_ERR(channel) == -
> > EPROBE_DEFER)
>
> Btw, in the GPIO library we have a macro or inliner (don't remember)
> that represents such a conditional.
> Perhaps make it (if it's a macro) global, or introduce an inline in
> IIO?
>
> Okay, it's here:
> https://elixir.bootlin.com/linux/v5.19-rc1/source/drivers/gpio/gpiolib.h#L179
>
> It's similar, but not the same, so just play with an idea to
> introduce
> something in this file, maybe it's worth doing this, maybe not.
>
I would also argue that could be something done after this series gets
applied...
- Nuno Sá
