Hi Russel,
> > > > > 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.
>
> IS_ERR_OR_NULL() - is a macro I personally hate, it causes a lot of
> trouble. I have mutt setup to mark IS_ERR_OR_NULL with a red background
> so it stands out in patches. It is utterly evil, and I really wish we
> could get rid of that damn macro.
>
> It also looks wrong.
>
> if (IS_ERR_OR_NULL(x))
> return PTR_ERR(x);
>
> rings alarm bells for some people, because if x is NULL, then
> PTR_ERR(x) is zero.
>
> While this may be what is intended in this case, for a great many
> places in the kernel, this is a bug. So I can guarantee that
> _someone_ will come along and want to "fix" that to make the NULL
> case return an error code, and in doing so end up breaking the
> driver.
>
> So... no, just don't.
>
> This is why having two if() statements are a good idea, and is
> what Linus means by "making the implicit explicit" - because it
> then becomes absolutely obvious what we want to do in the NULL
> case, and what we want to do in the error case.
>
> There is none of this ambiguity that I point out above.
Yes, I fully agree, IS_ERR_OR_NULL() shoud be almost never be
used in an exit path (unless you are in a void function and few
other cases, like (borderline) this one).
I'm OK also if Ruan goes with what you suggested.
Andi
