Re: [PATCH V3] media: i2c: imx334: Add support for 1280x720 & 640x480 resolutions

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Shravan,

Apologies for the late review.

On Mon, Dec 16, 2024 at 10:09:26AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Add support for 1280x720@30 and 640x480@30 resolutions
> optimized the resolution arrays by added common register
> array
> 
> Updated 1920x1080@30 resolution register array with
> common register array
> 
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
> 
> changelog:
> 
> v2 -> v3
> Removied blank lines reported by media CI robot 
> 
> v1 -> v2
> Optimized the resolution arrays by added common register array
> 
> ---
>  drivers/media/i2c/imx334.c | 119 ++++++++++++++++++++++++++++++-------
>  1 file changed, 99 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index a544fc3df39c..58ad67dbb331 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -167,8 +167,8 @@ static const s64 link_freq[] = {
>  	IMX334_LINK_FREQ_445M,
>  };
>  
> -/* Sensor mode registers for 1920x1080@30fps */
> -static const struct imx334_reg mode_1920x1080_regs[] = {
> +/* Sensor common mode registers for 445M link frequency */
> +static const struct imx334_reg common_mode_regs_445m[] = {
>  	{0x3000, 0x01},
>  	{0x3018, 0x04},
>  	{0x3030, 0xca},
> @@ -176,26 +176,10 @@ static const struct imx334_reg mode_1920x1080_regs[] = {
>  	{0x3032, 0x00},
>  	{0x3034, 0x4c},
>  	{0x3035, 0x04},
> -	{0x302c, 0xf0},
> -	{0x302d, 0x03},
> -	{0x302e, 0x80},
> -	{0x302f, 0x07},
> -	{0x3074, 0xcc},
> -	{0x3075, 0x02},
> -	{0x308e, 0xcd},
> -	{0x308f, 0x02},
> -	{0x3076, 0x38},
> -	{0x3077, 0x04},
> -	{0x3090, 0x38},
> -	{0x3091, 0x04},
> -	{0x3308, 0x38},
> -	{0x3309, 0x04},
> -	{0x30C6, 0x00},
> +	{0x30c6, 0x00},
>  	{0x30c7, 0x00},
>  	{0x30ce, 0x00},
>  	{0x30cf, 0x00},
> -	{0x30d8, 0x18},
> -	{0x30d9, 0x0a},
>  	{0x304c, 0x00},
>  	{0x304e, 0x00},
>  	{0x304f, 0x00},
> @@ -210,7 +194,7 @@ static const struct imx334_reg mode_1920x1080_regs[] = {
>  	{0x300d, 0x29},
>  	{0x314c, 0x29},
>  	{0x314d, 0x01},
> -	{0x315a, 0x06},
> +	{0x315a, 0x0a},
>  	{0x3168, 0xa0},
>  	{0x316a, 0x7e},
>  	{0x319e, 0x02},
> @@ -330,6 +314,66 @@ static const struct imx334_reg mode_1920x1080_regs[] = {
>  	{0x3002, 0x00},
>  };
>  
> +/* Sensor mode registers for 640x480@30fps */
> +static const struct imx334_reg mode_640x480_regs[] = {
> +	{0x302c, 0x70},
> +	{0x302d, 0x06},
> +	{0x302e, 0x80},
> +	{0x302f, 0x02},
> +	{0x3074, 0x48},
> +	{0x3075, 0x07},
> +	{0x308e, 0x49},
> +	{0x308f, 0x07},
> +	{0x3076, 0xe0},
> +	{0x3077, 0x01},
> +	{0x3090, 0xe0},
> +	{0x3091, 0x01},
> +	{0x3308, 0xe0},
> +	{0x3309, 0x01},
> +	{0x30d8, 0x30},
> +	{0x30d9, 0x0b},
> +};
> +
> +/* Sensor mode registers for 1280x720@30fps */
> +static const struct imx334_reg mode_1280x720_regs[] = {
> +	{0x302c, 0x30},
> +	{0x302d, 0x05},
> +	{0x302e, 0x00},
> +	{0x302f, 0x05},
> +	{0x3074, 0x84},
> +	{0x3075, 0x03},
> +	{0x308e, 0x85},
> +	{0x308f, 0x03},
> +	{0x3076, 0xd0},
> +	{0x3077, 0x02},
> +	{0x3090, 0xd0},
> +	{0x3091, 0x02},
> +	{0x3308, 0xd0},
> +	{0x3309, 0x02},
> +	{0x30d8, 0x30},
> +	{0x30d9, 0x0b},
> +};
> +
> +/* Sensor mode registers for 1920x1080@30fps */
> +static const struct imx334_reg mode_1920x1080_regs[] = {
> +	{0x302c, 0xf0},
> +	{0x302d, 0x03},
> +	{0x302e, 0x80},
> +	{0x302f, 0x07},
> +	{0x3074, 0xcc},
> +	{0x3075, 0x02},
> +	{0x308e, 0xcd},
> +	{0x308f, 0x02},
> +	{0x3076, 0x38},
> +	{0x3077, 0x04},
> +	{0x3090, 0x38},
> +	{0x3091, 0x04},
> +	{0x3308, 0x38},
> +	{0x3309, 0x04},
> +	{0x30d8, 0x18},
> +	{0x30d9, 0x0a},
> +};
> +
>  /* Sensor mode registers for 3840x2160@30fps */
>  static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3000, 0x01},
> @@ -505,6 +549,32 @@ static const struct imx334_mode supported_modes[] = {
>  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>  			.regs = mode_1920x1080_regs,
>  		},
> +	}, {
> +		.width = 1280,
> +		.height = 720,
> +		.hblank = 2480,
> +		.vblank = 1170,
> +		.vblank_min = 45,
> +		.vblank_max = 132840,
> +		.pclk = 297000000,
> +		.link_freq_idx = 1,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +			.regs = mode_1280x720_regs,
> +		},
> +	}, {
> +		.width = 640,
> +		.height = 480,
> +		.hblank = 2480,
> +		.vblank = 1170,
> +		.vblank_min = 45,
> +		.vblank_max = 132840,
> +		.pclk = 297000000,
> +		.link_freq_idx = 1,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_640x480_regs),
> +			.regs = mode_640x480_regs,
> +		},
>  	},
>  };
>  
> @@ -989,6 +1059,15 @@ static int imx334_start_streaming(struct imx334 *imx334)
>  	const struct imx334_reg_list *reg_list;
>  	int ret;
>  
> +	if (link_freq[imx334->cur_mode->link_freq_idx] == IMX334_LINK_FREQ_445M) {

Could you add a struct field for the common registers and the length of the
list, instead of referring to the link frequency index?

It'd be nice to have the same split done to the other frequency as well.

> +		ret = imx334_write_regs(imx334, common_mode_regs_445m,
> +					ARRAY_SIZE(common_mode_regs_445m));
> +		if (ret) {
> +			dev_err(imx334->dev, "fail to write common registers");
> +			return ret;
> +		}
> +	}
> +
>  	/* Write sensor mode registers */
>  	reg_list = &imx334->cur_mode->reg_list;
>  	ret = imx334_write_regs(imx334, reg_list->regs,

-- 
Kind regards,

Sakari Ailus