| Message ID | 20230816094618.2854084-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next] I2C: Use helper function IS_ERR_OR_NULL() | expand |
On 2023/8/16 17:46, Ruan Jinjie wrote: > Use IS_ERR_OR_NULL() instead of open-coding it > to simplify the code. > > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > --- > 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..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { Sorry, there is a problem, please ignore it. > dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); > return PTR_ERR(i2c_imx->pinctrl); > }
Le 16/08/2023 à 11:46, Ruan Jinjie a écrit : > Use IS_ERR_OR_NULL() instead of open-coding it > to simplify the code. > > rinfo->pinctrl = devm_pinctrl_get(&pdev->dev); > - if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) { > + if (IS_ERR_OR_NULL(rinfo->pinctrl)) { Can the return of devm_pinctrl_get really be NULL? Regards,
On Wed, Aug 16, 2023 at 05:48:34PM +0800, Ruan Jinjie wrote: > > > On 2023/8/16 17:46, Ruan Jinjie wrote: > > Use IS_ERR_OR_NULL() instead of open-coding it > > to simplify the code. > > > > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > > --- > > 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..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { > > Sorry, there is a problem, please ignore it. I don't think these conversions are appropriate for devm_pinctrl_get(). If one reads the code for devm_pinctrl_get() and pinctrl_get(), then one finds that it will never return NULL. So the tests for NULL do not serve any practical purpose than giving coders a warm fuzzy feeling that they've tested for a NULL pointer. Practically, they are of no use, so should be removed... thus making the conversion to IS_ERR_OR_NULL() irrelevant.
On Wed, Aug 16, 2023 at 05:46:18PM +0800, Ruan Jinjie wrote: > Use IS_ERR_OR_NULL() instead of open-coding it > to simplify the code. > > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > --- > 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..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { > dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); > return PTR_ERR(i2c_imx->pinctrl); > } As stated in my previous reply to a similar patch, devm_pinctrl_get() can not return NULL, so it makes more sense to remove the test for NULL rather than "cleaning up" the buggy code, but leaving the bugs behind.
On Wed, Aug 16, 2023 at 11:51:54AM +0200, Yann Sionneau wrote: > Le 16/08/2023 à 11:46, Ruan Jinjie a écrit : > > > Use IS_ERR_OR_NULL() instead of open-coding it > > to simplify the code. > > rinfo->pinctrl = devm_pinctrl_get(&pdev->dev); > > - if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) { > > + if (IS_ERR_OR_NULL(rinfo->pinctrl)) { > > Can the return of devm_pinctrl_get really be NULL? No.
On 2023/8/16 18:08, Russell King (Oracle) wrote: > On Wed, Aug 16, 2023 at 05:48:34PM +0800, Ruan Jinjie wrote: >> >> >> On 2023/8/16 17:46, Ruan Jinjie wrote: >>> Use IS_ERR_OR_NULL() instead of open-coding it >>> to simplify the code. >>> >>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> >>> --- >>> 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..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { >> >> Sorry, there is a problem, please ignore it. > > I don't think these conversions are appropriate for devm_pinctrl_get(). > If one reads the code for devm_pinctrl_get() and pinctrl_get(), then > one finds that it will never return NULL. Right! devm_pinctrl_get() never return NULL. > > So the tests for NULL do not serve any practical purpose than giving > coders a warm fuzzy feeling that they've tested for a NULL pointer. > Practically, they are of no use, so should be removed... thus making > the conversion to IS_ERR_OR_NULL() irrelevant. >
On 2023/8/16 18:22, Russell King (Oracle) wrote: > On Wed, Aug 16, 2023 at 05:46:18PM +0800, Ruan Jinjie wrote: >> Use IS_ERR_OR_NULL() instead of open-coding it >> to simplify the code. >> >> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> >> --- >> 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..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { >> dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); >> return PTR_ERR(i2c_imx->pinctrl); >> } > > As stated in my previous reply to a similar patch, devm_pinctrl_get() > can not return NULL, so it makes more sense to remove the test for NULL > rather than "cleaning up" the buggy code, but leaving the bugs behind. Right! I'll fix it sooner. Thank you very much. >
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index 94cff1cd527e..0e454c04a145 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_OR_NULL(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..8807c90df749 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_OR_NULL(i2c_imx->pinctrl)) { dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n"); return PTR_ERR(i2c_imx->pinctrl); }
Use IS_ERR_OR_NULL() instead of open-coding it to simplify the code. Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> --- drivers/i2c/busses/i2c-at91-master.c | 2 +- drivers/i2c/busses/i2c-imx.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)