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@xxxxxxxxx>
> ---
> 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;
> }
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
