staging/imx: Fix inconsistent IS_ERR and PTR_ERR
Subsystems (5)
PATCH 1/1 staging/imx: Fix inconsistent IS_ERR and PTR_ERR
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.
--- drivers/staging/media/imx/imx-media-csi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/media/imx/imx-media-csi.c +1 -1
@@ -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; }
Signature · 1 line
2.7.4
Quoted 17 lines · Gustavo A. R. Silva
Fix inconsistent
IS_ERRandPTR_ERRinimx_csi_probe. The proper pointer to be passed as argument is pinctrl instead ofpriv->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 <[email protected]>--- drivers/staging/media/imx/imx-media-csi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/media/imx/imx-media-csi.c +0
-0
drivers/staging/media/imx/imx-media-csi.c static int imx_csi_probe(struct platform_device *pdev) · line 1803
Quoted 7 lines
drivers/staging/media/imx/imx-media-csi.c +1
-1@@ -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,
regards
Philipp
Fabio Estevam
reviewerQuoted 17 lines · Gustavo A. R. Silva
Fix inconsistent
IS_ERRandPTR_ERRinimx_csi_probe. The proper pointer to be passed as argument is pinctrl instead ofpriv->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 <[email protected]>--- drivers/staging/media/imx/imx-media-csi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/media/imx/imx-media-csi.c +0
-0
drivers/staging/media/imx/imx-media-csi.c static int imx_csi_probe(struct platform_device *pdev) · line 1802
Quoted 6 lines
drivers/staging/media/imx/imx-media-csi.c +1
-1@@ -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 thatdevm_pinctrl_get_select_default() always fails.
I added this debug line:
drivers/staging/media/imx/imx-media-csi.c +1 -0
@@ -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 sincedevm_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?
drivers/staging/media/imx/imx-media-csi.c +0 -1
@@ -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;Fabio Estevam
reviewerSteve, Phiipp,
Quoted 7 lines · Fabio Estevam
So
imx_csi_probe()does not succeed anymore sincedevm_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:
(quoted diff) +0 -0
b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..c40f786 100644
drivers/staging/media/imx/imx-media-csi.c +2 -5
@@ -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
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Philipp Zabel
maintainerQuoted 62 lines · Fabio Estevam
Quoted 17 lines · Gustavo A. R. Silva
Fix inconsistent
IS_ERRandPTR_ERRinimx_csi_probe. The proper pointer to be passed as argument is pinctrl instead ofpriv->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 <[email protected]>--- drivers/staging/media/imx/imx-media-csi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/media/imx/imx-media-csi.c +0
-0drivers/staging/media/imx/imx-media-csi.c static int imx_csi_probe(struct platform_device *pdev) · line 1802
Quoted 6 lines
drivers/staging/media/imx/imx-media-csi.c +1
-1@@ -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:
drivers/staging/media/imx/imx-media-csi.c +1
-0@@ -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
Quoted 5 lines
So
imx_csi_probe()does not succeed anymore sincedevm_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.
Quoted 1 line
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
Quoted 1 line
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Fabio Estevam
reviewerQuoted 7 lines · Philipp Zabel
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
Quoted 3 lines
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:
arch/arm/boot/dts/imx6qdl-sabresd.dtsi +1 -4
@@ -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 thatdevm_pinctrl_get_select_default() succeeds.
If you agree I can send the following change:
(quoted diff) +0 -0
b/drivers/staging/media/imx/imx-media-csi.c
index 5a195f8..c40f786 100644
drivers/staging/media/imx/imx-media-csi.c +2 -5
@@ -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
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Philipp Zabel
maintainerHi Fabio,
Quoted 37 lines · Fabio Estevam
Quoted 7 lines · Philipp Zabel
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.dtsiand even in this casedevm_pinctrl_get_select_default()failsQuoted 3 lines
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:
arch/arm/boot/dts/imx6qdl-sabresd.dtsi +1
-4@@ -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 {
Quoted 1 line
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.
Quoted 11 lines
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:
drivers/staging/media/imx/imx-media-csi.c +0
-0
drivers/staging/media/imx/imx-media-csi.c static int imx_csi_probe(struct platform_device *pdev) · line 1801
Quoted 10 lines
drivers/staging/media/imx/imx-media-csi.c +2
-5@@ -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.
Quoted 7 lines
drivers/staging/media/imx/imx-media-csi.c +0
-0ret =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
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel