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

On Mon, 2022-07-11 at 15:29 +0200, Andy Shevchenko wrote:
> On Mon, Jul 11, 2022 at 2:38 PM 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 pointers with NULL being the way to
> > "notify" that we should do a "system" lookup for channels. This
> > make
> > it very confusing and prone to errors as commit 9f63cc0921ec
> > ("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.
> > 
> > This change also makes some functional changes on how errors were
> > being
> > handled. In the original behavior, even if we get an error like '-
> > ENOMEM',
> > we still continue with the search. We should only continue to
> > lookup for
> > the channel when it makes sense to do so. Hence, the main error
> > handling
> > in 'of_iio_channel_get_by_name()' is changed to the following
> > logic:
> > 
> >  * If a channel 'name' is provided and we do find it via
> > 'io-channel-names', we should be able to get it. If we get any
> > error,
> > we should not proceed with the lookup. Moreover, we should return
> > an error
> > so that callers won't proceed with a system lookup.
> >  * If a channel 'name' is provided and we cannot find it ('index <
> > 0'),
> > 'of_parse_phandle_with_args()' is expected to fail with '-EINVAL'.
> > Hence,
> > we should only continue if we get that error.
> >  * If a channel 'name' is not provided we should only carry on with
> > the
> > search if 'of_parse_phandle_with_args()' returns '-ENOENT'.
> > 
> > Also note that a system channel lookup is only done if the returned
> > error code (from 'of_iio_channel_get_by_name()' or
> > 'of_iio_channel_get_all()' is -ENODEV.
> 
> LGTM (but I might miss something, it's a bit too many conditionals),
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 

Agreed. It ended up being more complicated than I thought...

- Nuno Sá