Re: [PATCH v2 12/23] media: ov5640: Remove ov5640_mode_init_data

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

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 10, 2022 at 12:04:47PM +0100, Jacopo Mondi wrote:
> The ov5640_mode_init_data is a fictional sensor more which is used to
> program the initial sensor settings.
> 
> It is only used to initialize the sensor and can be replaced
> it with a throw-away mode which just wraps the register table.
> 
> Also rename the register table to drop the format from the name to make
> it clear an actual sensor mode has to be applied after the initial
> programming.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  drivers/media/i2c/ov5640.c | 47 ++++++++------------------------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 17835e71665a..2a922224ca9d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -382,7 +382,7 @@ static inline bool ov5640_is_csi2(struct ov5640_dev *sensor)
>   * over i2c.
>   */
>  /* YUV422 UYVY VGA@30fps */
> -static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> +static const struct reg_value ov5640_init_setting[] = {
>  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
>  	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
>  	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> @@ -620,30 +620,6 @@ static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
>  	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 70},
>  };
>  
> -/* power-on sensor init reg table */
> -static const struct ov5640_mode_info ov5640_mode_init_data = {
> -		.id		= 0,
> -		.dn_mode	= SUBSAMPLING,
> -		.pixel_rate	= OV5640_PIXEL_RATE_96M,
> -		.analog_crop = {
> -			.left	= OV5640_PIXEL_ARRAY_LEFT,
> -			.top	= OV5640_PIXEL_ARRAY_TOP,
> -			.width	= OV5640_PIXEL_ARRAY_WIDTH,
> -			.height	= OV5640_PIXEL_ARRAY_HEIGHT,
> -		},
> -		.crop = {
> -			.left	= 2,
> -			.top	= 4,
> -			.width	= 640,
> -			.height	= 480,
> -		},
> -		.htot		= 1896,
> -		.vblank_def	= 504,
> -		.reg_data	= ov5640_init_setting_30fps_VGA,
> -		.reg_data_size	= ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
> -		.max_fps	= OV5640_30_FPS
> -};
> -
>  static const struct ov5640_mode_info ov5640_mode_data[OV5640_NUM_MODES] = {
>  	{
>  		/* 160x120 */
> @@ -1440,17 +1416,16 @@ static int ov5640_set_timings(struct ov5640_dev *sensor,
>  	return 0;
>  }
>  
> -static int ov5640_load_regs(struct ov5640_dev *sensor,
> -			    const struct ov5640_mode_info *mode)
> +static void ov5640_load_regs(struct ov5640_dev *sensor,
> +			     const struct reg_value *regs, unsigned int regnum)
>  {
> -	const struct reg_value *regs = mode->reg_data;
>  	unsigned int i;
>  	u32 delay_ms;
>  	u16 reg_addr;
>  	u8 mask, val;
>  	int ret = 0;
>  
> -	for (i = 0; i < mode->reg_data_size; ++i, ++regs) {
> +	for (i = 0; i < regnum; ++i, ++regs) {
>  		delay_ms = regs->delay_ms;
>  		reg_addr = regs->reg_addr;
>  		val = regs->val;
> @@ -1472,8 +1447,6 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>  		if (delay_ms)
>  			usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
>  	}
> -
> -	return ov5640_set_timings(sensor, mode);
>  }
>  
>  static int ov5640_set_autoexposure(struct ov5640_dev *sensor, bool on)
> @@ -1926,7 +1899,8 @@ static int ov5640_set_mode_exposure_calc(struct ov5640_dev *sensor,
>  		return ret;
>  
>  	/* Write capture setting */
> -	ret = ov5640_load_regs(sensor, mode);
> +	ov5640_load_regs(sensor, mode->reg_data, mode->reg_data_size);
> +	ret = ov5640_set_timings(sensor, mode);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -2050,7 +2024,8 @@ static int ov5640_set_mode_direct(struct ov5640_dev *sensor,
>  		return -EINVAL;
>  
>  	/* Write capture setting */
> -	return ov5640_load_regs(sensor, mode);
> +	ov5640_load_regs(sensor, mode->reg_data, mode->reg_data_size);
> +	return ov5640_set_timings(sensor, mode);
>  }
>  
>  static int ov5640_set_mode(struct ov5640_dev *sensor)
> @@ -2148,10 +2123,8 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
>  	int ret;
>  
>  	/* first load the initial register values */
> -	ret = ov5640_load_regs(sensor, &ov5640_mode_init_data);
> -	if (ret < 0)
> -		return ret;
> -	sensor->last_mode = &ov5640_mode_init_data;
> +	ov5640_load_regs(sensor, ov5640_init_setting,
> +			 ARRAY_SIZE(ov5640_init_setting));
>  
>  	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
>  			     (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |

-- 
Regards,

Laurent Pinchart