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

[Ruan Jinjie <ruanjinjie@xxxxxxxxxx>]

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,
>