* [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR @ 2018-03-01 4:09 Gustavo A. R. Silva 2018-03-01 8:35 ` Philipp Zabel 2018-03-01 16:02 ` Fabio Estevam 0 siblings, 2 replies; 7+ messages in thread From: Gustavo A. R. Silva @ 2018-03-01 4:09 UTC (permalink / raw) To: Steve Longerbeam, Philipp Zabel, Mauro Carvalho Chehab, Greg Kroah-Hartman Cc: linux-media, devel, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 4:09 [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR Gustavo A. R. Silva @ 2018-03-01 8:35 ` Philipp Zabel 2018-03-01 16:02 ` Fabio Estevam 1 sibling, 0 replies; 7+ messages in thread From: Philipp Zabel @ 2018-03-01 8:35 UTC (permalink / raw) To: Gustavo A. R. Silva, Steve Longerbeam, Mauro Carvalho Chehab, Greg Kroah-Hartman Cc: linux-media, devel, linux-kernel, Gustavo A. R. Silva 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 4:09 [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR Gustavo A. R. Silva 2018-03-01 8:35 ` Philipp Zabel @ 2018-03-01 16:02 ` Fabio Estevam 2018-03-01 16:24 ` Fabio Estevam 2018-03-01 16:27 ` Philipp Zabel 1 sibling, 2 replies; 7+ messages in thread From: Fabio Estevam @ 2018-03-01 16:02 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Steve Longerbeam, Philipp Zabel, Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media, devel, linux-kernel, Gustavo A. R. Silva 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 16:02 ` Fabio Estevam @ 2018-03-01 16:24 ` Fabio Estevam 2018-03-01 16:27 ` Philipp Zabel 1 sibling, 0 replies; 7+ messages in thread From: Fabio Estevam @ 2018-03-01 16:24 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: devel, Greg Kroah-Hartman, linux-kernel, Gustavo A. R. Silva, Philipp Zabel, Steve Longerbeam, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 16:02 ` Fabio Estevam 2018-03-01 16:24 ` Fabio Estevam @ 2018-03-01 16:27 ` Philipp Zabel 2018-03-01 16:43 ` Fabio Estevam 1 sibling, 1 reply; 7+ messages in thread From: Philipp Zabel @ 2018-03-01 16:27 UTC (permalink / raw) To: Fabio Estevam, Gustavo A. R. Silva Cc: devel, Greg Kroah-Hartman, linux-kernel, Gustavo A. R. Silva, Steve Longerbeam, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 16:27 ` Philipp Zabel @ 2018-03-01 16:43 ` Fabio Estevam 2018-03-02 10:54 ` Philipp Zabel 0 siblings, 1 reply; 7+ messages in thread From: Fabio Estevam @ 2018-03-01 16:43 UTC (permalink / raw) To: Philipp Zabel Cc: devel, Gustavo A. R. Silva, Greg Kroah-Hartman, linux-kernel, Gustavo A. R. Silva, Steve Longerbeam, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR 2018-03-01 16:43 ` Fabio Estevam @ 2018-03-02 10:54 ` Philipp Zabel 0 siblings, 0 replies; 7+ messages in thread From: Philipp Zabel @ 2018-03-02 10:54 UTC (permalink / raw) To: Fabio Estevam Cc: devel, Gustavo A. R. Silva, Greg Kroah-Hartman, linux-kernel, Gustavo A. R. Silva, Steve Longerbeam, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-03-02 10:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-01 4:09 [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR Gustavo A. R. Silva 2018-03-01 8:35 ` Philipp Zabel 2018-03-01 16:02 ` Fabio Estevam 2018-03-01 16:24 ` Fabio Estevam 2018-03-01 16:27 ` Philipp Zabel 2018-03-01 16:43 ` Fabio Estevam 2018-03-02 10:54 ` Philipp Zabel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).