Re: [PATCH 10/21] media: ov5640: Add HBLANK control

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 31, 2022 at 03:32:34PM +0100, Jacopo Mondi wrote:
> Add the HBLANK control as read-only.
> 
> The hblank value is fixed in the mode definition and is updated
> everytime a new format is applied.

s/everytime/every time/

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5640.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 1e2f37c93f0d..b2961e8e07c3 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -266,6 +266,7 @@ struct ov5640_ctrls {
>  	struct v4l2_ctrl_handler handler;
>  	struct v4l2_ctrl *pixel_rate;
>  	struct v4l2_ctrl *link_freq;
> +	struct v4l2_ctrl *hblank;
>  	struct {
>  		struct v4l2_ctrl *auto_exp;
>  		struct v4l2_ctrl *exposure;
> @@ -2530,6 +2531,7 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor)
>  	struct v4l2_mbus_framefmt *fmt = &sensor->fmt;
>  	enum ov5640_pixel_rate_id pixel_rate_id = mode->pixel_rate;
>  	u32 num_lanes = sensor->ep.bus.mipi_csi2.num_data_lanes;
> +	unsigned int hblank;
>  	unsigned int i = 0;
>  	u32 pixel_rate;
>  	s64 link_freq;
> @@ -2580,6 +2582,10 @@ static int ov5640_update_pixel_rate(struct ov5640_dev *sensor)
>  	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate, pixel_rate);
>  	__v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, i);
>  
> +	hblank = mode->ppl - mode->crop.width;
> +	__v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> +				 hblank, hblank, 1, hblank);
> +
>  	return 0;
>  }
>  
> @@ -3044,6 +3050,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  	const struct v4l2_ctrl_ops *ops = &ov5640_ctrl_ops;
>  	struct ov5640_ctrls *ctrls = &sensor->ctrls;
>  	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +	unsigned int hblank;
>  	int ret;
>  
>  	v4l2_ctrl_handler_init(hdl, 32);
> @@ -3062,6 +3069,10 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  					ARRAY_SIZE(ov5640_csi2_link_freqs) - 1,
>  					4, ov5640_csi2_link_freqs);
>  
> +	hblank = mode->ppl - mode->crop.width;
> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, hblank,
> +					  hblank, 1, hblank);

I was going to say that you should increase the number of controls
passed to v4l2_ctrl_handler_init(), but it's already 32, higher than the
actual number of controls. Maybe you could reduce it to the right number
on top of this series ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	/* Auto/manual white balance */
>  	ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
>  					   V4L2_CID_AUTO_WHITE_BALANCE,
> @@ -3111,6 +3122,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
>  
>  	ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	ctrls->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  	ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
>  	ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
>  

-- 
Regards,

Laurent Pinchart