Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get()

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Ruan Jinjie <ruanjinjie@huawei.com>
Cc: "Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	"Andi Shyti" <andi.shyti@kernel.org>,
	"Fabio Estevam" <festevam@gmail.com>,
	linus.walleij@linaro.org,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	leoyang.li@nxp.com, "Wolfram Sang" <wsa@kernel.org>,
	o.rempel@pengutronix.de, linux-i2c@vger.kernel.org,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Claudiu Beznea" <claudiu.beznea@tuxon.dev>,
	"Codrin Ciubotariu" <codrin.ciubotariu@microchip.com>,
	"Oleksij Rempel" <linux@rempel-privat.de>,
	"Shawn Guo" <shawnguo@kernel.org>,
	s.hauer@pengutronix.de, linux-arm-kernel@lists.infradead.org,
	"NXP Linux Team" <linux-imx@nxp.com>
Subject: Re: [PATCH -next v2 RESEND] I2C: Fix return value check for devm_pinctrl_get()
Date: Fri, 18 Aug 2023 09:20:05 +0100	[thread overview]
Message-ID: <ZN8ptWv6Dn2pdWtF@shell.armlinux.org.uk> (raw)
In-Reply-To: <20230818074509.295220-1-ruanjinjie@huawei.com>

On Fri, Aug 18, 2023 at 03:45:08PM +0800, Ruan Jinjie wrote:
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 10e89586ca72..05d55893f04e 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>  	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>  
>  	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> -	if (!i2c_imx->pinctrl || IS_ERR(i2c_imx->pinctrl)) {
> +	if (IS_ERR(i2c_imx->pinctrl)) {
>  		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
>  		return PTR_ERR(i2c_imx->pinctrl);
>  	}

I haven't looked at the AT91 version, but... isn't the original code
entirely correct?

If pinctrl is not available (thus devm_pinctrl_get() returns NULL) then
recovery can't work, because we can't switch the I2C pins between the
I2C controller and GPIO. So, isn't it quite correct to print
"can't get pinctrl, bus recovery not supported" because the I2C bus
can't be recovered without pinctrl?

The PTR_ERR() is also fine - because if pinctrl is not present and
returns NULL, we'll end up returning zero, which is exactly what we
want.

The alternative would be to open code that, maybe with a more accurate
message:

	if (!i2c_imx->pinctrl) {
		dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
		return 0;
	}
	if (IS_ERR(i2c_imx->pinctrl) {
		...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel