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... ... > + if (nummaps == 0) /* return -ENODEV to search map table */ Comment is superfluous, the next line is self-explaining. > + 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. -- With Best Regards, Andy Shevchenko