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

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

[Ruan Jinjie]

The devm_pinctrl_get() function returns error pointers and never
returns NULL. Update the checks accordingly.

Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
v2:
- Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind.
- Update the commit title and message.
---
 drivers/i2c/busses/i2c-at91-master.c | 2 +-
 drivers/i2c/busses/i2c-imx.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 94cff1cd527e..2bf1df5ef473 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -831,7 +831,7 @@ static int at91_init_twi_recovery_gpio(struct platform_device *pdev,
 	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
 
 	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
+	if (IS_ERR(rinfo->pinctrl)) {
 		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
 		return PTR_ERR(rinfo->pinctrl);
 	}
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 10e89586ca72..05d55893f04e 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
 
 	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);
 	}
-- 
2.34.1

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

[Oleksij Rempel]

On Thu, Aug 17, 2023 at 10:20:17AM +0800, Ruan Jinjie wrote:
> The devm_pinctrl_get() function returns error pointers and never
> returns NULL. Update the checks accordingly.
> 
> Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
> Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
> v2:
> - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind.
> - Update the commit title and message.
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 2 +-
>  drivers/i2c/busses/i2c-imx.c         | 2 +-

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Linus Walleij]

On Thu, Aug 17, 2023 at 4:21 AM Ruan Jinjie <ruanjinjie@huawei.com> wrote:

> The devm_pinctrl_get() function returns error pointers and never
> returns NULL. Update the checks accordingly.
>
> Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
> Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

That's right.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for fixing this Ruan!

Yours,
Linus Walleij

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

[Nicolas Ferre]

On 17/08/2023 at 04:20, Ruan Jinjie wrote:
> The devm_pinctrl_get() function returns error pointers and never
> returns NULL. Update the checks accordingly.
> 
> Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")

Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

> Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
> v2:
> - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving them behind.
> - Update the commit title and message.
> ---
>   drivers/i2c/busses/i2c-at91-master.c | 2 +-
>   drivers/i2c/busses/i2c-imx.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
> index 94cff1cd527e..2bf1df5ef473 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -831,7 +831,7 @@ static int at91_init_twi_recovery_gpio(struct platform_device *pdev,
>          struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> 
>          rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
> -       if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
> +       if (IS_ERR(rinfo->pinctrl)) {
>                  dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
>                  return PTR_ERR(rinfo->pinctrl);
>          }
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 10e89586ca72..05d55893f04e 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>          struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> 
>          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);
>          }
> --
> 2.34.1
> 

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

[Leo Li]

> -----Original Message-----
> From: Ruan Jinjie <ruanjinjie@huawei.com>
> Sent: Wednesday, August 16, 2023 9:20 PM
> To: linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Codrin
> Ciubotariu <codrin.ciubotariu@microchip.com>; Andi Shyti
> <andi.shyti@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; Claudiu Beznea
> <claudiu.beznea@tuxon.dev>; Oleksij Rempel <linux@rempel-privat.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>;
> Wolfram Sang <wsa@kernel.org>; Linus Walleij <linus.walleij@linaro.org>;
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Leo Li
> <leoyang.li@nxp.com>
> Cc: ruanjinjie@huawei.com
> Subject: [PATCH -next v2] I2C: Fix return value check for devm_pinctrl_get()
> 
> The devm_pinctrl_get() function returns error pointers and never returns
> NULL. Update the checks accordingly.

Not exactly.  It can return NULL when CONFIG_PINCTRL is not defined.  We probably should fix that API too.

include/linux/pinctrl/consumer.h:
static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
{
        return NULL;
}

Regards,
Leo
> 
> Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
> Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> ---
> v2:
> - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving
> them behind.
> - Update the commit title and message.
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 2 +-
>  drivers/i2c/busses/i2c-imx.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-
> at91-master.c
> index 94cff1cd527e..2bf1df5ef473 100644
> --- a/drivers/i2c/busses/i2c-at91-master.c
> +++ b/drivers/i2c/busses/i2c-at91-master.c
> @@ -831,7 +831,7 @@ static int at91_init_twi_recovery_gpio(struct
> platform_device *pdev,
>  	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
> 
>  	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
> -	if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
> +	if (IS_ERR(rinfo->pinctrl)) {
>  		dev_info(dev->dev, "can't get pinctrl, bus recovery not
> supported\n");
>  		return PTR_ERR(rinfo->pinctrl);
>  	}
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> 10e89586ca72..05d55893f04e 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct
> imx_i2c_struct *i2c_imx,
>  	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> 
>  	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);
>  	}
> --
> 2.34.1

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

[Yann Sionneau]

Hi,

Le 17/08/2023 à 19:30, Leo Li a écrit :

>> The devm_pinctrl_get() function returns error pointers and never returns
>> NULL. Update the checks accordingly.
> Not exactly.  It can return NULL when CONFIG_PINCTRL is not defined.  We probably should fix that API too.
>
> include/linux/pinctrl/consumer.h:
> static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
> {
>          return NULL;
> }

So, as Leo pointed out it seems devm_pinctrl_get() can in fact return 
NULL, when CONFIG_PINCTRL is not defined.

What do we do about this?

Proposals:

1/ make sure all call sites of devm_pinctrl_get() do check for error 
with IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL

2/ change the fallback implementation in 
include/linux/pinctrl/consumer.h to return ERR_PTR(-Esomething) (which 
errno?)

3/ another solution?

Regards,

-- 

Yann

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

[Ruan Jinjie]

On 2023/8/18 7:07, Yann Sionneau wrote:
> Hi,
> 
> Le 17/08/2023 à 19:30, Leo Li a écrit :
> 
>>> The devm_pinctrl_get() function returns error pointers and never returns
>>> NULL. Update the checks accordingly.
>> Not exactly.  It can return NULL when CONFIG_PINCTRL is not defined. 
>> We probably should fix that API too.
>>
>> include/linux/pinctrl/consumer.h:
>> static inline struct pinctrl * __must_check devm_pinctrl_get(struct
>> device *dev)
>> {
>>          return NULL;
>> }
> 
> So, as Leo pointed out it seems devm_pinctrl_get() can in fact return
> NULL, when CONFIG_PINCTRL is not defined.
> 
> What do we do about this?
> 
> Proposals:
> 
> 1/ make sure all call sites of devm_pinctrl_get() do check for error
> with IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL

I think it's the best.

> 
> 2/ change the fallback implementation in
> include/linux/pinctrl/consumer.h to return ERR_PTR(-Esomething) (which
> errno?)

It seems a convention to return NULL if the related macro is not defined.

> 
> 3/ another solution?

Make I2C_IMX and I2C_AT91 config depends on PINCTRL config is another
option. However it seems that the function call devm_pinctrl_get() has
an optional recovery feature from the following notes and dev_info(). So
this dependency is not necessary.

1378 /*
1379  * We switch SCL and SDA to their GPIO function and do some bitbanging
1380  * for bus recovery. These alternative pinmux settings can be
1381  * described in the device tree by a separate pinctrl state "gpio". If
1382  * this is missing this is not a big problem, the only implication is
1383  * that we can't do bus recovery.
1384  */
1385 static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
1386         struct platform_device *pdev)
1387 {
1388     struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
1389
1390     i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);

828 static int at91_init_twi_recovery_gpio(struct platform_device *pdev,
829                        struct at91_twi_dev *dev)
830 {
831     struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
832
833     rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
834     if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
835         dev_info(dev->dev, "can't get pinctrl, bus recovery not
supported\n");
836         return PTR_ERR(rinfo->pinctrl);
837     }


> 
> Regards,
> 

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

[Sascha Hauer]

On Fri, Aug 18, 2023 at 01:07:27AM +0200, Yann Sionneau wrote:
> Hi,
> 
> Le 17/08/2023 à 19:30, Leo Li a écrit :
> 
> > > The devm_pinctrl_get() function returns error pointers and never returns
> > > NULL. Update the checks accordingly.
> > Not exactly.  It can return NULL when CONFIG_PINCTRL is not defined.  We probably should fix that API too.
> > 
> > include/linux/pinctrl/consumer.h:
> > static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
> > {
> >          return NULL;
> > }
> 
> So, as Leo pointed out it seems devm_pinctrl_get() can in fact return NULL,
> when CONFIG_PINCTRL is not defined.
> 
> What do we do about this?
> 
> Proposals:
> 
> 1/ make sure all call sites of devm_pinctrl_get() do check for error with
> IS_ERR *and* check for NULL => therefore using IS_ERR_OR_NULL
> 
> 2/ change the fallback implementation in include/linux/pinctrl/consumer.h to
> return ERR_PTR(-Esomething) (which errno?)
> 
> 3/ another solution?

NULL is returned on purpose. When PINCTRL is disabled NULL becomes a
valid pinctrl cookie which can be passed to the other stub functions.
With this drivers using pinctrl can get through their probe function
without an error when PINCTRL is disabled.

The same approach is taken by the clk and regulator API.

It is correct to test the return value of devm_pinctrl_get() with
IS_ERR(), only the commit message of these patches is a bit inaccurate.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Linus Walleij]

On Fri, Aug 18, 2023 at 7:33 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:

> NULL is returned on purpose. When PINCTRL is disabled NULL becomes a
> valid pinctrl cookie which can be passed to the other stub functions.
> With this drivers using pinctrl can get through their probe function
> without an error when PINCTRL is disabled.
>
> The same approach is taken by the clk and regulator API.
>
> It is correct to test the return value of devm_pinctrl_get() with
> IS_ERR(), only the commit message of these patches is a bit inaccurate.

Sascha is spot on, maybe copyedit some of the above
into the commit message and resend?

Yours,
Linus Walleij

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

[Ruan Jinjie]

On 2023/8/18 15:18, Linus Walleij wrote:
> On Fri, Aug 18, 2023 at 7:33 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
>> NULL is returned on purpose. When PINCTRL is disabled NULL becomes a
>> valid pinctrl cookie which can be passed to the other stub functions.
>> With this drivers using pinctrl can get through their probe function
>> without an error when PINCTRL is disabled.
>>
>> The same approach is taken by the clk and regulator API.
>>
>> It is correct to test the return value of devm_pinctrl_get() with
>> IS_ERR(), only the commit message of these patches is a bit inaccurate.
> 
> Sascha is spot on, maybe copyedit some of the above
> into the commit message and resend?

OK! I'll resend it.

> 
> Yours,
> Linus Walleij

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

[Ruan Jinjie]

On 2023/8/18 1:30, Leo Li wrote:
> 
> 
>> -----Original Message-----
>> From: Ruan Jinjie <ruanjinjie@huawei.com>
>> Sent: Wednesday, August 16, 2023 9:20 PM
>> To: linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; Codrin
>> Ciubotariu <codrin.ciubotariu@microchip.com>; Andi Shyti
>> <andi.shyti@kernel.org>; Nicolas Ferre <nicolas.ferre@microchip.com>;
>> Alexandre Belloni <alexandre.belloni@bootlin.com>; Claudiu Beznea
>> <claudiu.beznea@tuxon.dev>; Oleksij Rempel <linux@rempel-privat.de>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Shawn Guo
>> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
>> Estevam <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>;
>> Wolfram Sang <wsa@kernel.org>; Linus Walleij <linus.walleij@linaro.org>;
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de>; Leo Li
>> <leoyang.li@nxp.com>
>> Cc: ruanjinjie@huawei.com
>> Subject: [PATCH -next v2] I2C: Fix return value check for devm_pinctrl_get()
>>
>> The devm_pinctrl_get() function returns error pointers and never returns
>> NULL. Update the checks accordingly.
> 
> Not exactly.  It can return NULL when CONFIG_PINCTRL is not defined.  We probably should fix that API too.
> 
> include/linux/pinctrl/consumer.h:
> static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
> {
>         return NULL;
> }

As ARCH_AT91 select PINCTRL and I2C_AT91 depends on ARCH_AT91, it can
not be NULL except for I2C_AT91  is selected by COMPILE_TEST.

And ARCH_MXC select PINCTRL and I2C_IMX depends on ARCH_MXC it can not
be NULL except for I2C_IMX is selected by ARCH_LAYERSCAPE or COLDFIRE or
COMPILE_TEST.

In general, it is possible to be null.

> 
> Regards,
> Leo
>>
>> Fixes: 543aa2c4da8b ("i2c: at91: Move to generic GPIO bus recovery")
>> Fixes: fd8961c5ba9e ("i2c: imx: make bus recovery through pinctrl optional")
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
>> ---
>> v2:
>> - Remove NULL check instead of using IS_ERR_OR_NULL() to avoid leaving
>> them behind.
>> - Update the commit title and message.
>> ---
>>  drivers/i2c/busses/i2c-at91-master.c | 2 +-
>>  drivers/i2c/busses/i2c-imx.c         | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-
>> at91-master.c
>> index 94cff1cd527e..2bf1df5ef473 100644
>> --- a/drivers/i2c/busses/i2c-at91-master.c
>> +++ b/drivers/i2c/busses/i2c-at91-master.c
>> @@ -831,7 +831,7 @@ static int at91_init_twi_recovery_gpio(struct
>> platform_device *pdev,
>>  	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
>>
>>  	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
>> -	if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
>> +	if (IS_ERR(rinfo->pinctrl)) {
>>  		dev_info(dev->dev, "can't get pinctrl, bus recovery not
>> supported\n");
>>  		return PTR_ERR(rinfo->pinctrl);
>>  	}
>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
>> 10e89586ca72..05d55893f04e 100644
>> --- a/drivers/i2c/busses/i2c-imx.c
>> +++ b/drivers/i2c/busses/i2c-imx.c
>> @@ -1388,7 +1388,7 @@ static int i2c_imx_init_recovery_info(struct
>> imx_i2c_struct *i2c_imx,
>>  	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
>>
>>  	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);
>>  	}
>> --
>> 2.34.1
>