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

[Linus Walleij]

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.

Yours,
Linus Walleij