Re: [PATCH 5/6] media: ov6650: Use the generic clock framework

[Janusz Krzysztofik <jmkrzyszt@gmail.com>]

On Monday, January 4, 2021 5:57:38 P.M. CET Ezequiel Garcia wrote:
> Commit ce548396a433 ("media: mach-omap1: board-ams-delta.c: remove soc_camera dependencies")
> removed the last in-tree user of this sensor. New users
> will be required to use the generic clock framework,
> so it's possible to convert the driver to use it.
> 
> Convert the driver to use the CCF, and drop the legacy
> v4l2-clk API.
> 
> Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/i2c/ov6650.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index d73f9f540932..0f8242054603 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -22,13 +22,13 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/module.h>
>  
> -#include <media/v4l2-clk.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  
> @@ -194,7 +194,7 @@ struct ov6650 {
>  		struct v4l2_ctrl *blue;
>  		struct v4l2_ctrl *red;
>  	};
> -	struct v4l2_clk		*clk;
> +	struct clk		*clk;
>  	bool			half_scale;	/* scale down output by 2 */
>  	struct v4l2_rect	rect;		/* sensor cropping window */
>  	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
> @@ -459,9 +459,9 @@ static int ov6650_s_power(struct v4l2_subdev *sd, int on)
>  	int ret = 0;
>  
>  	if (on)
> -		ret = v4l2_clk_enable(priv->clk);
> +		ret = clk_prepare_enable(priv->clk);
>  	else
> -		v4l2_clk_disable(priv->clk);
> +		clk_disable_unprepare(priv->clk);
>  
>  	return ret;
>  }
> @@ -821,14 +821,14 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  	u8 pidh, pidl, midh, midl;
>  	int i, ret = 0;
>  
> -	priv->clk = v4l2_clk_get(&client->dev, NULL);
> +	priv->clk = devm_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);
> +		dev_err(&client->dev, "clk request err: %d\n", ret);
>  		return ret;
>  	}
>  
> -	rate = v4l2_clk_get_rate(priv->clk);
> +	rate = clk_get_rate(priv->clk);
>  	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
>  		if (rate != ov6650_xclk[i].rate)
>  			continue;
> @@ -839,8 +839,8 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  		break;
>  	}
>  	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
> -		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> -		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
> +		ret = clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> +		if (ret || clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
>  			continue;
>  
>  		xclk = &ov6650_xclk[i];
> @@ -852,12 +852,12 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  		dev_err(&client->dev, "unable to get supported clock rate\n");
>  		if (!ret)
>  			ret = -EINVAL;
> -		goto eclkput;
> +		return ret;
>  	}
>  
>  	ret = ov6650_s_power(sd, 1);
>  	if (ret < 0)
> -		goto eclkput;
> +		return ret;
>  
>  	msleep(20);
>  
> @@ -901,9 +901,6 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  	ov6650_s_power(sd, 0);
>  	if (!ret)
>  		return 0;

I think the above two lines are no longer needed and should be removed.  
Anyway,

Reviewed-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Thanks,
Janusz

> -eclkput:
> -	v4l2_clk_put(priv->clk);
> -
>  	return ret;
>  }
>  
> @@ -1089,7 +1086,6 @@ static int ov6650_remove(struct i2c_client *client)
>  {
>  	struct ov6650 *priv = to_ov6650(client);
>  
> -	v4l2_clk_put(priv->clk);
>  	v4l2_async_unregister_subdev(&priv->subdev);
>  	v4l2_ctrl_handler_free(&priv->hdl);
>  	return 0;
>