[PATCH -next] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[PATCH -next] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1@huawei.com>

In case of error, the function pinctrl_register() returns
NULL not ERR_PTR(). The IS_ERR() test in the return value
check should be replaced with NULL test.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/pinctrl/pinctrl-artpec6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-artpec6.c b/drivers/pinctrl/pinctrl-artpec6.c
index b5a2eff..6a16019 100644
--- a/drivers/pinctrl/pinctrl-artpec6.c
+++ b/drivers/pinctrl/pinctrl-artpec6.c
@@ -938,9 +938,9 @@ static int artpec6_pmx_probe(struct platform_device *pdev)
 	pmx->num_pin_groups = ARRAY_SIZE(artpec6_pin_groups);
 	pmx->pctl	    = pinctrl_register(&artpec6_desc, &pdev->dev, pmx);
 
-	if (!pmx->pctl) {
+	if (IS_ERR(pmx->pctl)) {
 		dev_err(&pdev->dev, "could not register pinctrl driver\n");
-		return -EINVAL;
+		return PTR_ERR(pmx->pctl);
 	}
 
 	platform_set_drvdata(pdev, pmx);

Re: [PATCH -next] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Jesper Nilsson]

On Tue, Apr 25, 2017 at 03:29:45AM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> In case of error, the function pinctrl_register() returns
> NULL not ERR_PTR(). The IS_ERR() test in the return value
> check should be replaced with NULL test.

Uh, the comment above reads as the exact opposite
of what the patch does. Your point is valid though,
but the comment needs rephrasing.

> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

/Jesper

> ---
>  drivers/pinctrl/pinctrl-artpec6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-artpec6.c b/drivers/pinctrl/pinctrl-artpec6.c
> index b5a2eff..6a16019 100644
> --- a/drivers/pinctrl/pinctrl-artpec6.c
> +++ b/drivers/pinctrl/pinctrl-artpec6.c
> @@ -938,9 +938,9 @@ static int artpec6_pmx_probe(struct platform_device *pdev)
>  	pmx->num_pin_groups = ARRAY_SIZE(artpec6_pin_groups);
>  	pmx->pctl	    = pinctrl_register(&artpec6_desc, &pdev->dev, pmx);
>  
> -	if (!pmx->pctl) {
> +	if (IS_ERR(pmx->pctl)) {
>  		dev_err(&pdev->dev, "could not register pinctrl driver\n");
> -		return -EINVAL;
> +		return PTR_ERR(pmx->pctl);
>  	}
>  
>  	platform_set_drvdata(pdev, pmx);

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

RE: [PATCH -next] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[weiyongjun (A)]

> -----Original Message-----
> From: Jesper Nilsson [mailto:jesper.nilsson@axis.com]
> Sent: Tuesday, April 25, 2017 4:31 PM
> To: Wei Yongjun <weiyj.lk@gmail.com>
> Cc: Jesper Nilsson <jespern@axis.com>; Lars Persson <larper@axis.com>;
> Niklas Cassel <niklass@axis.com>; Linus Walleij <linus.walleij@linaro.org>;
> weiyongjun (A) <weiyongjun1@huawei.com>; linux-arm-kernel@axis.com;
> linux-gpio@vger.kernel.org
> Subject: Re: [PATCH -next] pinctrl: artpec6: Fix return value check in
> artpec6_pmx_probe()
> 
> On Tue, Apr 25, 2017 at 03:29:45AM +0000, Wei Yongjun wrote:
> > From: Wei Yongjun <weiyongjun1@huawei.com>
> >
> > In case of error, the function pinctrl_register() returns
> > NULL not ERR_PTR(). The IS_ERR() test in the return value
> > check should be replaced with NULL test.
> 
> Uh, the comment above reads as the exact opposite
> of what the patch does. Your point is valid though,
> but the comment needs rephrasing.
> 

Oh, sorry, I will send the v2 patch with comment fixed soon.
Thanks.

Wei Yongjun

[PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1@huawei.com>

In case of error, the function pinctrl_register() returns
ERR_PTR() not NULL. The NULL test in the return value check
should be replaced with IS_ERR().

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
v1 -> v2: fix comment
---
 drivers/pinctrl/pinctrl-artpec6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-artpec6.c b/drivers/pinctrl/pinctrl-artpec6.c
index b5a2eff..6a16019 100644
--- a/drivers/pinctrl/pinctrl-artpec6.c
+++ b/drivers/pinctrl/pinctrl-artpec6.c
@@ -938,9 +938,9 @@ static int artpec6_pmx_probe(struct platform_device *pdev)
 	pmx->num_pin_groups = ARRAY_SIZE(artpec6_pin_groups);
 	pmx->pctl	    = pinctrl_register(&artpec6_desc, &pdev->dev, pmx);
 
-	if (!pmx->pctl) {
+	if (IS_ERR(pmx->pctl)) {
 		dev_err(&pdev->dev, "could not register pinctrl driver\n");
-		return -EINVAL;
+		return PTR_ERR(pmx->pctl);
 	}
 
 	platform_set_drvdata(pdev, pmx);

Re: [PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Jesper Nilsson]

On Tue, Apr 25, 2017 at 11:29:30AM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> In case of error, the function pinctrl_register() returns
> ERR_PTR() not NULL. The NULL test in the return value check
> should be replaced with IS_ERR().
> 
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>

Although I'm wondering if moving to pinctrl_register_and_init()
would sense here. Linus?

/Jesper

> ---
> v1 -> v2: fix comment
> ---
>  drivers/pinctrl/pinctrl-artpec6.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-artpec6.c b/drivers/pinctrl/pinctrl-artpec6.c
> index b5a2eff..6a16019 100644
> --- a/drivers/pinctrl/pinctrl-artpec6.c
> +++ b/drivers/pinctrl/pinctrl-artpec6.c
> @@ -938,9 +938,9 @@ static int artpec6_pmx_probe(struct platform_device *pdev)
>  	pmx->num_pin_groups = ARRAY_SIZE(artpec6_pin_groups);
>  	pmx->pctl	    = pinctrl_register(&artpec6_desc, &pdev->dev, pmx);
>  
> -	if (!pmx->pctl) {
> +	if (IS_ERR(pmx->pctl)) {
>  		dev_err(&pdev->dev, "could not register pinctrl driver\n");
> -		return -EINVAL;
> +		return PTR_ERR(pmx->pctl);
>  	}
>  
>  	platform_set_drvdata(pdev, pmx);

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Linus Walleij]

On Tue, Apr 25, 2017 at 3:40 PM, Jesper Nilsson <jesper.nilsson@axis.com> wrote:
> On Tue, Apr 25, 2017 at 11:29:30AM +0000, Wei Yongjun wrote:
>> From: Wei Yongjun <weiyongjun1@huawei.com>
>>
>> In case of error, the function pinctrl_register() returns
>> ERR_PTR() not NULL. The NULL test in the return value check
>> should be replaced with IS_ERR().
>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>
> Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>
>
> Although I'm wondering if moving to pinctrl_register_and_init()
> would sense here. Linus?

That depends, check on the commits introducing it for the
usecase.

OTOMH it was introduced for when you have hogs in drivers
doing all set-up of groups and functions from the device tree.

Tony?

Yours,
Linus Walleij

Re: [PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Jesper Nilsson]

On Fri, Apr 28, 2017 at 09:43:07AM +0200, Linus Walleij wrote:
> On Tue, Apr 25, 2017 at 3:40 PM, Jesper Nilsson <jesper.nilsson@axis.com> wrote:
> > On Tue, Apr 25, 2017 at 11:29:30AM +0000, Wei Yongjun wrote:
> >> From: Wei Yongjun <weiyongjun1@huawei.com>
> >>
> >> In case of error, the function pinctrl_register() returns
> >> ERR_PTR() not NULL. The NULL test in the return value check
> >> should be replaced with IS_ERR().
> >>
> >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> >
> > Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>
> >
> > Although I'm wondering if moving to pinctrl_register_and_init()
> > would sense here. Linus?
> 
> That depends, check on the commits introducing it for the
> usecase.
> 
> OTOMH it was introduced for when you have hogs in drivers
> doing all set-up of groups and functions from the device tree.

Yeah, so in this case it is not necessary, but
the comment above pinctrl_register() explicitly says:
"To avoid issues later on, please use the
new pinctrl_register_and_init() below instead."
Which to me indicates that it is recommended
to use the pinctrl_register_and_init() in future code...

> Tony?
> 
> Yours,
> Linus Walleij

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Tony Lindgren]

* Jesper Nilsson <jesper.nilsson@axis.com> [170428 01:01]:
> On Fri, Apr 28, 2017 at 09:43:07AM +0200, Linus Walleij wrote:
> > On Tue, Apr 25, 2017 at 3:40 PM, Jesper Nilsson <jesper.nilsson@axis.com> wrote:
> > > On Tue, Apr 25, 2017 at 11:29:30AM +0000, Wei Yongjun wrote:
> > >> From: Wei Yongjun <weiyongjun1@huawei.com>
> > >>
> > >> In case of error, the function pinctrl_register() returns
> > >> ERR_PTR() not NULL. The NULL test in the return value check
> > >> should be replaced with IS_ERR().
> > >>
> > >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > >
> > > Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>
> > >
> > > Although I'm wondering if moving to pinctrl_register_and_init()
> > > would sense here. Linus?
> > 
> > That depends, check on the commits introducing it for the
> > usecase.
> > 
> > OTOMH it was introduced for when you have hogs in drivers
> > doing all set-up of groups and functions from the device tree.
> 
> Yeah, so in this case it is not necessary, but
> the comment above pinctrl_register() explicitly says:
> "To avoid issues later on, please use the
> new pinctrl_register_and_init() below instead."
> Which to me indicates that it is recommended
> to use the pinctrl_register_and_init() in future code...
> 
> > Tony?

If you want to use GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS
want or allocate pins dynamically in the driver, then init needs to be
updated. So yeah I'd use that style init for any new code.

Regards,

Tony

Re: [PATCH -next v2] pinctrl: artpec6: Fix return value check in artpec6_pmx_probe()

[Linus Walleij]

On Tue, Apr 25, 2017 at 1:29 PM, Wei Yongjun <weiyj.lk@gmail.com> wrote:

> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> In case of error, the function pinctrl_register() returns
> ERR_PTR() not NULL. The NULL test in the return value check
> should be replaced with IS_ERR().
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
> v1 -> v2: fix comment

Patch applied with Jesper's ACK.

Yours,
Linus Walleij