Re: [PATCH 22/34] iio: inkern: only return error codes in iio_channel_get_*() APIs

From: "Nuno Sá" <noname.nuno@gmail.com>
To: "Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Nuno Sá" <nuno.sa@analog.com>
Cc: dl-linux-imx <linux-imx@nxp.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	chrome-platform@lists.linux.dev,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	 linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	 OpenBMC Maillist <openbmc@lists.ozlabs.org>,
	Cai Huoqing <cai.huoqing@linux.dev>,
	Benjamin Fair <benjaminfair@google.com>,
	Jishnu Prakash <quic_jprakash@quicinc.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Amit Kucheria <amitk@kernel.org>, Andy Gross <agross@kernel.org>,
	Michael Hennerich <Michael.Hennerich@analog.com>,
	Haibo Chen <haibo.chen@nxp.com>,
	Benson Leung <bleung@chromium.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	 Christophe Branchereau <cbranchereau@gmail.com>,
	Patrick Venture <venture@google.com>,
	Arnd Bergmann <arnd@arndb.de>, Nancy Yuen <yuenn@google.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	 Gwendal Grignou <gwendal@chromium.org>,
	Saravanan Sekar <sravanhome@gmail.com>,
	Tali Perry <tali.perry1@gmail.com>,
	 Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Paul Cercueil <paul@crapouillou.net>,
	Thara Gopinath <thara.gopinath@linaro.org>,
	Avi Fishman <avifishman70@gmail.com>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Claudiu Beznea <claudiu.beznea@microchip.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabrice Gasnier <fabrice.gasnier@foss.st.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Tomer Maimon <tmaimon77@gmail.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	 Jonathan Cameron <jic23@kernel.org>,
	Zhang Rui <rui.zhang@intel.com>, Shawn Guo <shawnguo@kernel.org>,
	Guenter Roeck <groeck@chromium.org>,
	Fabio Estevam <festevam@gmail.com>,
	Olivier Moysan <olivier.moysan@foss.st.com>,
	Eugen Hristev <eugen.hristev@microchip.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH 22/34] iio: inkern: only return error codes in iio_channel_get_*() APIs
Date: Fri, 10 Jun 2022 21:48:22 +0200	[thread overview]
Message-ID: <25e286370d34419c3fc32a75896d5bfebc4128ea.camel@gmail.com> (raw)
In-Reply-To: <CAHp75VcPoZ4FKTwDN_F76EZAcWYzAe1CHmuRuaP0wg3BgmPtfA@mail.gmail.com>

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@analog.com> 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á