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.
Yours,
Linus Walleij
