| Message ID | 20230821032914.1976317-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v3] I2C: Make return value check more accurate and explicit for devm_pinctrl_get() | expand |
On Mon, Aug 21, 2023 at 11:29:14AM +0800, Jinjie Ruan wrote: > 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, it is 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. > > However, open code that with a more accurate message will be more explicit > for NULL case when CONFIG_PINCTRL is not defined. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk> > --- > v3: > - Split into two if() statements instead of removing the NULL checks. > - Remove the fix tags. > - Update the commit title and message. > - Update to bring the author's name before. > v2: > - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind. > - Update the commit title and message. > --- > drivers/i2c/busses/i2c-at91-master.c | 6 +++++- > drivers/i2c/busses/i2c-imx.c | 6 +++++- > 2 files changed, 10 insertions(+), 2 deletions(-) Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> for the i2c-imx.c Thank you! Regards, Oleksij
Hi, On Thu, Aug 24, 2023 at 10:34:53AM +0200, Oleksij Rempel wrote: > On Mon, Aug 21, 2023 at 11:29:14AM +0800, Jinjie Ruan wrote: > > 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, it is 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. > > > > However, open code that with a more accurate message will be more explicit > > for NULL case when CONFIG_PINCTRL is not defined. > > > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk> > > --- > > v3: > > - Split into two if() statements instead of removing the NULL checks. > > - Remove the fix tags. > > - Update the commit title and message. > > - Update to bring the author's name before. > > v2: > > - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind. > > - Update the commit title and message. > > --- > > drivers/i2c/busses/i2c-at91-master.c | 6 +++++- > > drivers/i2c/busses/i2c-imx.c | 6 +++++- > > 2 files changed, 10 insertions(+), 2 deletions(-) > > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> > > for the i2c-imx.c For some reason lore+lei failed to deliver this message to my inbox :/ Anyway, looks good! Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thank you, Jinjie for following up. Andi
On Mon, Aug 21, 2023 at 11:29:14AM +0800, Jinjie Ruan wrote: > 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, it is 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. > > However, open code that with a more accurate message will be more explicit > for NULL case when CONFIG_PINCTRL is not defined. > > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> > Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index 94cff1cd527e..d311981d3e60 100644 --- a/drivers/i2c/busses/i2c-at91-master.c +++ b/drivers/i2c/busses/i2c-at91-master.c @@ -831,7 +831,11 @@ static int at91_init_twi_recovery_gpio(struct platform_device *pdev, struct i2c_bus_recovery_info *rinfo = &dev->rinfo; rinfo->pinctrl = devm_pinctrl_get(&pdev->dev); - if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) { + if (!rinfo->pinctrl) { + dev_info(dev->dev, "pinctrl unavailable, bus recovery not supported\n"); + return 0; + } + if (IS_ERR(rinfo->pinctrl)) { dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n"); return PTR_ERR(rinfo->pinctrl); } diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 10e89586ca72..1775a79aeba2 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -1388,7 +1388,11 @@ 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 (!i2c_imx->pinctrl) { + dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n"); + return 0; + } + 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); }