| Message ID | 1394791952-12941-1-git-send-email-josh.wu@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Josh, On 03/14/2014 11:12 AM, Josh Wu wrote: > + clk = v4l2_clk_get(&client->dev, "mclk"); > + if (IS_ERR(clk)) > + return -EPROBE_DEFER; You should instead make it: return PTR_ERR(clk); But you will need this patch for that to work: http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/clk/clk.c?id=a34cd4666f3da84228a82f70c94b8d9b692034ea With this patch there is no need to overwrite any returned error value with EPROBE_DEFER. > @@ -1097,23 +1106,26 @@ static int ov2640_probe(struct i2c_client *client, > v4l2_ctrl_new_std(&priv->hdl,&ov2640_ctrl_ops, > V4L2_CID_HFLIP, 0, 1, 1, 0); > priv->subdev.ctrl_handler =&priv->hdl; > - if (priv->hdl.error) > - return priv->hdl.error; > - > - priv->clk = v4l2_clk_get(&client->dev, "mclk"); > - if (IS_ERR(priv->clk)) { > - ret = PTR_ERR(priv->clk); > - goto eclkget; -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Sylwester Thanks for your review. On 3/15/2014 5:17 AM, Sylwester Nawrocki wrote: > Hi Josh, > > On 03/14/2014 11:12 AM, Josh Wu wrote: >> + clk = v4l2_clk_get(&client->dev, "mclk"); >> + if (IS_ERR(clk)) >> + return -EPROBE_DEFER; > > You should instead make it: > > return PTR_ERR(clk); > > But you will need this patch for that to work: > http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/clk/clk.c?id=a34cd4666f3da84228a82f70c94b8d9b692034ea > > > With this patch there is no need to overwrite any returned error > value with EPROBE_DEFER. Thanks for the information. I will use this in v2 version. Best Regards, Josh Wu > >> @@ -1097,23 +1106,26 @@ static int ov2640_probe(struct i2c_client >> *client, >> v4l2_ctrl_new_std(&priv->hdl,&ov2640_ctrl_ops, >> V4L2_CID_HFLIP, 0, 1, 1, 0); >> priv->subdev.ctrl_handler =&priv->hdl; >> - if (priv->hdl.error) >> - return priv->hdl.error; >> - >> - priv->clk = v4l2_clk_get(&client->dev, "mclk"); >> - if (IS_ERR(priv->clk)) { >> - ret = PTR_ERR(priv->clk); >> - goto eclkget; > > -- > Regards, > Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Josh, On 19/03/14 10:17, Josh Wu wrote: > On 3/15/2014 5:17 AM, Sylwester Nawrocki wrote: >> > On 03/14/2014 11:12 AM, Josh Wu wrote: >>> >> + clk = v4l2_clk_get(&client->dev, "mclk"); >>> >> + if (IS_ERR(clk)) >>> >> + return -EPROBE_DEFER; >> > >> > You should instead make it: >> > >> > return PTR_ERR(clk); >> > >> > But you will need this patch for that to work: >> > http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/clk/clk.c?id=a34cd4666f3da84228a82f70c94b8d9b692034ea >> > >> > >> > With this patch there is no need to overwrite any returned error >> > value with EPROBE_DEFER. > > Thanks for the information. I will use this in v2 version. Oops, I missed somehow that it's v4l2_clk_get(), rather than clk_get(). So it seems it will not work when you return PTR_ERR(clk), since v4l2_clk_get() returns -ENODEV when clock is not found. I think we should modify v4l2_clk_get() so it returns EPROBE_DEFER rather than ENODEV on error. I anticipate v4l2_clk_get() might be using clk_get() internally in future, and the v4l2 clk look up will be used as a fallback only. So sensor drivers should just do something like: clk = v4l2_clk_get(...); if (IS_ERR(clk)) return PTR_ERR(clk); -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Sylwester On 3/20/2014 10:44 PM, Sylwester Nawrocki wrote: > Hi Josh, > > On 19/03/14 10:17, Josh Wu wrote: >> On 3/15/2014 5:17 AM, Sylwester Nawrocki wrote: >>>> On 03/14/2014 11:12 AM, Josh Wu wrote: >>>>>> + clk = v4l2_clk_get(&client->dev, "mclk"); >>>>>> + if (IS_ERR(clk)) >>>>>> + return -EPROBE_DEFER; >>>> You should instead make it: >>>> >>>> return PTR_ERR(clk); >>>> >>>> But you will need this patch for that to work: >>>> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/clk/clk.c?id=a34cd4666f3da84228a82f70c94b8d9b692034ea >>>> >>>> >>>> With this patch there is no need to overwrite any returned error >>>> value with EPROBE_DEFER. >> Thanks for the information. I will use this in v2 version. > Oops, I missed somehow that it's v4l2_clk_get(), rather than > clk_get(). So it seems it will not work when you return PTR_ERR(clk), > since v4l2_clk_get() returns -ENODEV when clock is not found. right, I missed that. So this version is still valid one. The v2 that I already sent should be dropped. > I think we should modify v4l2_clk_get() so it returns EPROBE_DEFER > rather than ENODEV on error. I anticipate v4l2_clk_get() might be > using clk_get() internally in future, and the v4l2 clk look up > will be used as a fallback only. So sensor drivers should just > do something like: > > clk = v4l2_clk_get(...); > if (IS_ERR(clk)) > return PTR_ERR(clk); I noticed that there are some driver like ov772x, ov9640 and etc, are not supported the async probe yet. If we return the EPROBE_DEFER for all v4l2_clk_get(), that will cause the no-async probe function work incorrectly. So IMO we can do your above suggestion after all sensors support async probe. Thanks. > > -- > Regards, > Sylwester Best Regards, Josh Wu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/i2c/soc_camera/ov2640.c b/drivers/media/i2c/soc_camera/ov2640.c index 6c6b1c3..fb9b6e9 100644 --- a/drivers/media/i2c/soc_camera/ov2640.c +++ b/drivers/media/i2c/soc_camera/ov2640.c @@ -22,6 +22,7 @@ #include <linux/videodev2.h> #include <media/soc_camera.h> +#include <media/v4l2-async.h> #include <media/v4l2-clk.h> #include <media/v4l2-subdev.h> #include <media/v4l2-ctrls.h> @@ -1069,6 +1070,7 @@ static int ov2640_probe(struct i2c_client *client, struct ov2640_priv *priv; struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client); struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); + struct v4l2_clk *clk; int ret; if (!ssdd) { @@ -1083,13 +1085,20 @@ static int ov2640_probe(struct i2c_client *client, return -EIO; } + clk = v4l2_clk_get(&client->dev, "mclk"); + if (IS_ERR(clk)) + return -EPROBE_DEFER; + priv = devm_kzalloc(&client->dev, sizeof(struct ov2640_priv), GFP_KERNEL); if (!priv) { dev_err(&adapter->dev, "Failed to allocate memory for private data!\n"); - return -ENOMEM; + ret = -ENOMEM; + goto err_kzalloc; } + priv->clk = clk; + v4l2_i2c_subdev_init(&priv->subdev, client, &ov2640_subdev_ops); v4l2_ctrl_handler_init(&priv->hdl, 2); v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops, @@ -1097,23 +1106,26 @@ static int ov2640_probe(struct i2c_client *client, v4l2_ctrl_new_std(&priv->hdl, &ov2640_ctrl_ops, V4L2_CID_HFLIP, 0, 1, 1, 0); priv->subdev.ctrl_handler = &priv->hdl; - if (priv->hdl.error) - return priv->hdl.error; - - priv->clk = v4l2_clk_get(&client->dev, "mclk"); - if (IS_ERR(priv->clk)) { - ret = PTR_ERR(priv->clk); - goto eclkget; + if (priv->hdl.error) { + ret = priv->hdl.error; + goto err_kzalloc; } ret = ov2640_video_probe(client); - if (ret) { - v4l2_clk_put(priv->clk); -eclkget: - v4l2_ctrl_handler_free(&priv->hdl); - } else { - dev_info(&adapter->dev, "OV2640 Probed\n"); - } + if (ret) + goto err_probe; + + ret = v4l2_async_register_subdev(&priv->subdev); + if (ret) + goto err_probe; + + dev_info(&adapter->dev, "OV2640 Probed\n"); + return 0; + +err_probe: + v4l2_ctrl_handler_free(&priv->hdl); +err_kzalloc: + v4l2_clk_put(clk); return ret; } @@ -1122,6 +1134,7 @@ static int ov2640_remove(struct i2c_client *client) { struct ov2640_priv *priv = to_ov2640(client); + v4l2_async_unregister_subdev(&priv->subdev); v4l2_clk_put(priv->clk); v4l2_device_unregister_subdev(&priv->subdev); v4l2_ctrl_handler_free(&priv->hdl);
Move the clock detection code to the beginning of the probe(). If we meet any error in the clock detecting, then defer the probe. Signed-off-by: Josh Wu <josh.wu@atmel.com> --- drivers/media/i2c/soc_camera/ov2640.c | 43 +++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 15 deletions(-)