[1/2] media: ov6650: Move v4l2_clk_get() to ov6650_video_probe() helper
Commit Message
In preparation for adding asynchronous subdevice support to the driver,
don't acquire v4l2_clk from the driver .probe() callback as that may
fail if the clock is provided by a bridge driver which may be not yet
initialized. Move the v4l2_clk_get() to ov6650_video_probe() helper
which is going to be converted to v4l2_subdev_internal_ops.registered()
callback, executed only when the bridge driver is ready.
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
drivers/media/i2c/ov6650.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
Comments
Hi Janus,
Thanks for the patchset.
On Mon, Mar 25, 2019 at 01:35:00AM +0100, Janusz Krzysztofik wrote:
> In preparation for adding asynchronous subdevice support to the driver,
> don't acquire v4l2_clk from the driver .probe() callback as that may
> fail if the clock is provided by a bridge driver which may be not yet
> initialized. Move the v4l2_clk_get() to ov6650_video_probe() helper
> which is going to be converted to v4l2_subdev_internal_ops.registered()
> callback, executed only when the bridge driver is ready.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
> drivers/media/i2c/ov6650.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index c33fd584cb44..f10b8053ed73 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -810,9 +810,16 @@ static int ov6650_video_probe(struct i2c_client *client)
> u8 pidh, pidl, midh, midl;
> int ret;
>
> + priv->clk = v4l2_clk_get(&client->dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + ret = PTR_ERR(priv->clk);
> + dev_err(&client->dev, "v4l2_clk request err: %d\n", ret);
> + return ret;
> + }
> +
> ret = ov6650_s_power(&priv->subdev, 1);
> if (ret < 0)
> - return ret;
> + goto eclkput;
>
> /*
> * check and show product ID and manufacturer ID
> @@ -847,6 +854,11 @@ static int ov6650_video_probe(struct i2c_client *client)
>
> done:
> ov6650_s_power(&priv->subdev, 0);
> + if (ret) {
> +eclkput:
> + v4l2_clk_put(priv->clk);
Could you rework the error handling, please? Labels should be outside
conditionals. See e.g. the smiapp driver
(drivers/media/i2c/smiapp/smiapp-core.c). I've pushed my master branch
here:
<URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
The patch also does not apply on top of the media tree master + your
earlier patch cleanly.
> + }
> +
> return ret;
> }
>
> @@ -989,18 +1001,9 @@ static int ov6650_probe(struct i2c_client *client,
> priv->code = MEDIA_BUS_FMT_YUYV8_2X8;
> priv->colorspace = V4L2_COLORSPACE_JPEG;
>
> - priv->clk = v4l2_clk_get(&client->dev, NULL);
> - if (IS_ERR(priv->clk)) {
> - ret = PTR_ERR(priv->clk);
> - goto eclkget;
> - }
> -
> ret = ov6650_video_probe(client);
> - if (ret) {
> - v4l2_clk_put(priv->clk);
> -eclkget:
> + if (ret)
> v4l2_ctrl_handler_free(&priv->hdl);
> - }
>
> return ret;
> }
Hi Sakari,
On Friday, March 29, 2019 7:39:24 PM CET Sakari Ailus wrote:
> Hi Janus,
>
> Thanks for the patchset.
>
> On Mon, Mar 25, 2019 at 01:35:00AM +0100, Janusz Krzysztofik wrote:
> > ...
> >
> > done:
> > ov6650_s_power(&priv->subdev, 0);
> > + if (ret) {
> > +eclkput:
> > + v4l2_clk_put(priv->clk);
>
> Could you rework the error handling, please? Labels should be outside
> conditionals.
OK.
> See e.g. the smiapp driver
> (drivers/media/i2c/smiapp/smiapp-core.c). I've pushed my master branch
> here:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
I'm currently syncing my linux-media remote tracking branches with
git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git
Should I switch to a different repository, e.g. yours?
Thanks,
Janusz
On Fri, Mar 29, 2019 at 08:48:19PM +0100, Janusz Krzysztofik wrote:
> Hi Sakari,
>
> On Friday, March 29, 2019 7:39:24 PM CET Sakari Ailus wrote:
> > Hi Janus,
> >
> > Thanks for the patchset.
> >
> > On Mon, Mar 25, 2019 at 01:35:00AM +0100, Janusz Krzysztofik wrote:
> > > ...
> > >
> > > done:
> > > ov6650_s_power(&priv->subdev, 0);
> > > + if (ret) {
> > > +eclkput:
> > > + v4l2_clk_put(priv->clk);
> >
> > Could you rework the error handling, please? Labels should be outside
> > conditionals.
>
> OK.
>
> > See e.g. the smiapp driver
> > (drivers/media/i2c/smiapp/smiapp-core.c). I've pushed my master branch
> > here:
> >
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
>
> I'm currently syncing my linux-media remote tracking branches with
> git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media.git
> Should I switch to a different repository, e.g. yours?
Usually Mauro's tree is the best choice. I sometimes forget to update mine.
In this case just make sure these patches apply on top of the one merged
previouly if there's a dependency.
@@ -810,9 +810,16 @@ static int ov6650_video_probe(struct i2c_client *client)
u8 pidh, pidl, midh, midl;
int ret;
+ priv->clk = v4l2_clk_get(&client->dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ dev_err(&client->dev, "v4l2_clk request err: %d\n", ret);
+ return ret;
+ }
+
ret = ov6650_s_power(&priv->subdev, 1);
if (ret < 0)
- return ret;
+ goto eclkput;
/*
* check and show product ID and manufacturer ID
@@ -847,6 +854,11 @@ static int ov6650_video_probe(struct i2c_client *client)
done:
ov6650_s_power(&priv->subdev, 0);
+ if (ret) {
+eclkput:
+ v4l2_clk_put(priv->clk);
+ }
+
return ret;
}
@@ -989,18 +1001,9 @@ static int ov6650_probe(struct i2c_client *client,
priv->code = MEDIA_BUS_FMT_YUYV8_2X8;
priv->colorspace = V4L2_COLORSPACE_JPEG;
- priv->clk = v4l2_clk_get(&client->dev, NULL);
- if (IS_ERR(priv->clk)) {
- ret = PTR_ERR(priv->clk);
- goto eclkget;
- }
-
ret = ov6650_video_probe(client);
- if (ret) {
- v4l2_clk_put(priv->clk);
-eclkget:
+ if (ret)
v4l2_ctrl_handler_free(&priv->hdl);
- }
return ret;
}