[PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Gustavo A. R. Silva]

Fix inconsistent IS_ERR and PTR_ERR in imx_csi_probe.
The proper pointer to be passed as argument is pinctrl
instead of priv->vdev.

This issue was detected with the help of Coccinelle.

Fixes: 52e17089d185 ("media: imx: Don't initialize vars that won't be used")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..4f290a0 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1798,7 +1798,7 @@ static int imx_csi_probe(struct platform_device *pdev)
 	priv->dev->of_node = pdata->of_node;
 	pinctrl = devm_pinctrl_get_select_default(priv->dev);
 	if (IS_ERR(pinctrl)) {
-		ret = PTR_ERR(priv->vdev);
+		ret = PTR_ERR(pinctrl);
 		goto free;
 	}
 
-- 
2.7.4

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Philipp Zabel]

On Wed, 2018-02-28 at 22:09 -0600, Gustavo A. R. Silva wrote:
> Fix inconsistent IS_ERR and PTR_ERR in imx_csi_probe.
> The proper pointer to be passed as argument is pinctrl
> instead of priv->vdev.
> 
> This issue was detected with the help of Coccinelle.
> 
> Fixes: 52e17089d185 ("media: imx: Don't initialize vars that won't be used")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 5a195f8..4f290a0 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1798,7 +1798,7 @@ static int imx_csi_probe(struct platform_device *pdev)
>  	priv->dev->of_node = pdata->of_node;
>  	pinctrl = devm_pinctrl_get_select_default(priv->dev);
>  	if (IS_ERR(pinctrl)) {
> -		ret = PTR_ERR(priv->vdev);
> +		ret = PTR_ERR(pinctrl);
>  		goto free;
>  	}

Thanks,
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Fabio Estevam]

On Thu, Mar 1, 2018 at 1:09 AM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> Fix inconsistent IS_ERR and PTR_ERR in imx_csi_probe.
> The proper pointer to be passed as argument is pinctrl
> instead of priv->vdev.
>
> This issue was detected with the help of Coccinelle.
>
> Fixes: 52e17089d185 ("media: imx: Don't initialize vars that won't be used")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 5a195f8..4f290a0 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1798,7 +1798,7 @@ static int imx_csi_probe(struct platform_device *pdev)
>         priv->dev->of_node = pdata->of_node;
>         pinctrl = devm_pinctrl_get_select_default(priv->dev);
>         if (IS_ERR(pinctrl)) {
> -               ret = PTR_ERR(priv->vdev);
> +               ret = PTR_ERR(pinctrl);
>                 goto free;

This patch is correct, but now I am seeing that
devm_pinctrl_get_select_default() always fails.

I added this debug line:

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1799,6 +1799,7 @@ static int imx_csi_probe(struct platform_device *pdev)
        pinctrl = devm_pinctrl_get_select_default(priv->dev);
        if (IS_ERR(pinctrl)) {
                ret = PTR_ERR(pinctrl);
+               pr_err("************ pinctrl failed\n");
                goto free;
        }

and this is what I get in imx6q-sabresd:

[    3.453905] imx-media: subdev ipu1_vdic bound
[    3.458601] imx-media: subdev ipu2_vdic bound
[    3.463341] imx-media: subdev ipu1_ic_prp bound
[    3.468924] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video0
[    3.476237] imx-media: subdev ipu1_ic_prpenc bound
[    3.481621] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video1
[    3.488805] imx-media: subdev ipu1_ic_prpvf bound
[    3.493659] imx-media: subdev ipu2_ic_prp bound
[    3.498839] ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video2
[    3.505958] imx-media: subdev ipu2_ic_prpenc bound
[    3.511318] ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video3
[    3.518335] imx-media: subdev ipu2_ic_prpvf bound
[    3.524622] ipu1_csi0: Registered ipu1_csi0 capture as /dev/video4
[    3.530902] imx-media: subdev ipu1_csi0 bound
[    3.535453] ************ pinctrl failed
[    3.539684] ************ pinctrl failed
[    3.543677] ************ pinctrl failed
[    3.548278] imx-media: subdev imx6-mipi-csi2 bound

So imx_csi_probe() does not succeed anymore since
devm_pinctrl_get_select_default() always fails.

Not sure I understand the comments that explain the need for pinctrl
handling inside the driver.

Can't we just get rid of it like this?

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -14,7 +14,6 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
-#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -1739,7 +1738,6 @@ static const struct v4l2_subdev_internal_ops
csi_internal_ops = {
 static int imx_csi_probe(struct platform_device *pdev)
 {
        struct ipu_client_platformdata *pdata;
-       struct pinctrl *pinctrl;
        struct csi_priv *priv;
        int ret;

@@ -1789,19 +1787,7 @@ static int imx_csi_probe(struct platform_device *pdev)
        v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0);
        priv->sd.ctrl_handler = &priv->ctrl_hdlr;

-       /*
-        * The IPUv3 driver did not assign an of_node to this
-        * device. As a result, pinctrl does not automatically
-        * configure our pin groups, so we need to do that manually
-        * here, after setting this device's of_node.
-        */
        priv->dev->of_node = pdata->of_node;
-       pinctrl = devm_pinctrl_get_select_default(priv->dev);
-       if (IS_ERR(pinctrl)) {
-               ret = PTR_ERR(pinctrl);
-               goto free;
-       }
-
        ret = v4l2_async_register_subdev(&priv->sd);
        if (ret)
                goto free;

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Fabio Estevam]

Steve, Phiipp,

On Thu, Mar 1, 2018 at 1:02 PM, Fabio Estevam <festevam@gmail.com> wrote:

> So imx_csi_probe() does not succeed anymore since
> devm_pinctrl_get_select_default() always fails.
>
> Not sure I understand the comments that explain the need for pinctrl
> handling inside the driver.
>
> Can't we just get rid of it like this?

Just tested and if devm_pinctrl_get_select_default() is removed, I am
not able to change the ipu csi pinctrl settings anymore.

I had to ignore devm_pinctrl_get_select_default() error value so that
the driver can probe again:

diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..c40f786 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1797,11 +1797,8 @@ static int imx_csi_probe(struct platform_device *pdev)
         */
        priv->dev->of_node = pdata->of_node;
        pinctrl = devm_pinctrl_get_select_default(priv->dev);
-       if (IS_ERR(pinctrl)) {
-               ret = PTR_ERR(priv->vdev);
-               goto free;
-       }
-
+       if (IS_ERR(pinctrl))
+               dev_dbg(priv->dev, "pintrl_get_select_default() failed\n");
        ret = v4l2_async_register_subdev(&priv->sd);
        if (ret)
                goto free;

Is there a better solution for this issue?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Philipp Zabel]

On Thu, 2018-03-01 at 13:02 -0300, Fabio Estevam wrote:
> On Thu, Mar 1, 2018 at 1:09 AM, Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
> > Fix inconsistent IS_ERR and PTR_ERR in imx_csi_probe.
> > The proper pointer to be passed as argument is pinctrl
> > instead of priv->vdev.
> > 
> > This issue was detected with the help of Coccinelle.
> > 
> > Fixes: 52e17089d185 ("media: imx: Don't initialize vars that won't be used")
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >  drivers/staging/media/imx/imx-media-csi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > index 5a195f8..4f290a0 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -1798,7 +1798,7 @@ static int imx_csi_probe(struct platform_device *pdev)
> >         priv->dev->of_node = pdata->of_node;
> >         pinctrl = devm_pinctrl_get_select_default(priv->dev);
> >         if (IS_ERR(pinctrl)) {
> > -               ret = PTR_ERR(priv->vdev);
> > +               ret = PTR_ERR(pinctrl);
> >                 goto free;
> 
> This patch is correct, but now I am seeing that
> devm_pinctrl_get_select_default() always fails.
> 
> I added this debug line:
> 
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1799,6 +1799,7 @@ static int imx_csi_probe(struct platform_device *pdev)
>         pinctrl = devm_pinctrl_get_select_default(priv->dev);
>         if (IS_ERR(pinctrl)) {
>                 ret = PTR_ERR(pinctrl);
> +               pr_err("************ pinctrl failed\n");
>                 goto free;
>         }
> 
> and this is what I get in imx6q-sabresd:
> 
> [    3.453905] imx-media: subdev ipu1_vdic bound
> [    3.458601] imx-media: subdev ipu2_vdic bound
> [    3.463341] imx-media: subdev ipu1_ic_prp bound
> [    3.468924] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video0
> [    3.476237] imx-media: subdev ipu1_ic_prpenc bound
> [    3.481621] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video1
> [    3.488805] imx-media: subdev ipu1_ic_prpvf bound
> [    3.493659] imx-media: subdev ipu2_ic_prp bound
> [    3.498839] ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video2
> [    3.505958] imx-media: subdev ipu2_ic_prpenc bound
> [    3.511318] ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video3
> [    3.518335] imx-media: subdev ipu2_ic_prpvf bound
> [    3.524622] ipu1_csi0: Registered ipu1_csi0 capture as /dev/video4
> [    3.530902] imx-media: subdev ipu1_csi0 bound
> [    3.535453] ************ pinctrl failed
> [    3.539684] ************ pinctrl failed
> [    3.543677] ************ pinctrl failed
> [    3.548278] imx-media: subdev imx6-mipi-csi2 bound
> 
> So imx_csi_probe() does not succeed anymore since
> devm_pinctrl_get_select_default() always fails.
> 
> Not sure I understand the comments that explain the need for pinctrl
> handling inside the driver.

Oh, this only works for csi ports that have pinctrl in their csi port
node, like:

&ipu1_csi0 {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_ipu1_csi0>;
};

For all other ports it fails. So we indeed have to silently continue if
there is no pinctrl in the port node.

> Can't we just get rid of it like this?

pinctrl would have to be moved out of the csi port nodes, for example
into their parent ipu nodes, or maybe more correctly, into the video mux
nodes in each device tree.

regards
Philipp
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Fabio Estevam]

On Thu, Mar 1, 2018 at 1:27 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Oh, this only works for csi ports that have pinctrl in their csi port
> node, like:
>
> &ipu1_csi0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_ipu1_csi0>;
> };

This is the case for imx6qdl-sabresd.dtsi and even in this case
devm_pinctrl_get_select_default() fails

> pinctrl would have to be moved out of the csi port nodes, for example
> into their parent ipu nodes, or maybe more correctly, into the video mux
> nodes in each device tree.

Tried it like this:

--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -154,12 +154,9 @@
 };

 &ipu1_csi0_mux_from_parallel_sensor {
-       remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
-};
-
-&ipu1_csi0 {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_ipu1_csi0>;
+       remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
 };

 &mipi_csi {


but still get the devm_pinctrl_get_select_default() failure.

I was not able to change the dts so that
devm_pinctrl_get_select_default() succeeds.

If you agree I can send the following change:

diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..c40f786 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1797,11 +1797,8 @@ static int imx_csi_probe(struct platform_device *pdev)
         */
        priv->dev->of_node = pdata->of_node;
        pinctrl = devm_pinctrl_get_select_default(priv->dev);
-       if (IS_ERR(pinctrl)) {
-               ret = PTR_ERR(priv->vdev);
-               goto free;
-       }
-
+       if (IS_ERR(pinctrl))
+               dev_dbg(priv->dev, "pintrl_get_select_default() failed\n");
        ret = v4l2_async_register_subdev(&priv->sd);
        if (ret)
                goto free;

So that the error is ignored and we still can change the pinctrl values via dts.

What do you think?
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR

[Philipp Zabel]

Hi Fabio,

On Thu, 2018-03-01 at 13:43 -0300, Fabio Estevam wrote:
> On Thu, Mar 1, 2018 at 1:27 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > Oh, this only works for csi ports that have pinctrl in their csi port
> > node, like:
> > 
> > &ipu1_csi0 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_ipu1_csi0>;
> > };
> 
> This is the case for imx6qdl-sabresd.dtsi and even in this case
> devm_pinctrl_get_select_default() fails
> 
> > pinctrl would have to be moved out of the csi port nodes, for example
> > into their parent ipu nodes, or maybe more correctly, into the video mux
> > nodes in each device tree.
> 
> Tried it like this:
> 
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -154,12 +154,9 @@
>  };
> 
>  &ipu1_csi0_mux_from_parallel_sensor {
> -       remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
> -};
> -
> -&ipu1_csi0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_ipu1_csi0>;
> +       remote-endpoint = <&ov5642_to_ipu1_csi0_mux>;
>  };
> 
>  &mipi_csi {
> 
> 
> but still get the devm_pinctrl_get_select_default() failure.

Yes, this would still require to ignore the pinctrl error in the CSI
driver.

I just think that this is might be more correct, since the external pins
are directly connected to the mux input port.

> I was not able to change the dts so that
> devm_pinctrl_get_select_default() succeeds.
> 
> If you agree I can send the following change:
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 5a195f8..c40f786 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1797,11 +1797,8 @@ static int imx_csi_probe(struct platform_device *pdev)
>          */
>         priv->dev->of_node = pdata->of_node;
>         pinctrl = devm_pinctrl_get_select_default(priv->dev);
> -       if (IS_ERR(pinctrl)) {
> -               ret = PTR_ERR(priv->vdev);
> -               goto free;
> -       }
> -
> +       if (IS_ERR(pinctrl))
> +               dev_dbg(priv->dev, "pintrl_get_select_default() failed\n");

I would add the error code to the debug print.

>         ret = v4l2_async_register_subdev(&priv->sd);
>         if (ret)
>                 goto free;
> 
> So that the error is ignored and we still can change the pinctrl values via dts.
> 
> What do you think?

Maybe we should still throw the error if it is anything other than
-ENODEV (which we expect in case there is no pinctrl property in the csi
port node):

	if (IS_ERR(pinctrl)) {
		ret = PTR_ERR(pinctrl);
		dev_dbg(priv->dev, "pinctrl_get_select_default() failed: %d\n",
			ret);
		if (ret != -ENODEV)
			goto free;
	}

regards
Philipp
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel