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@xxxxxxxxxxxxxxx> 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
