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

[Andi Shyti]

Hi,

On Fri, Aug 18, 2023 at 06:42:11PM +0200, Linus Walleij wrote:
> On Fri, Aug 18, 2023 at 10:20 AM Russell King (Oracle)
> <linux at armlinux.org.uk> wrote:
> > On Fri, Aug 18, 2023 at 03:45:08PM +0800, Ruan Jinjie wrote:
> 
> > >       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.
> 
> Oh, you're probably absolutely right about that.
> 
> > 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) {
> >                 ...
> 
> This is a way better patch. It makes the implicit explicit.

we could also use

	if (IS_ERR_OR_NULL(i2c_imx->pinctrl))
		...

without changing any logic in the driver.

Andi