Hi Sakari, > -----Original Message----- > From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > Sent: Wednesday, February 12, 2025 6:15 PM > To: shravan Chippa - I35088 <Shravan.Chippa@xxxxxxxxxxxxx> > Cc: mchehab@xxxxxxxxxx; kieran.bingham@xxxxxxxxxxxxxxxx; linux- > media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Conor Dooley - > M52691 <Conor.Dooley@xxxxxxxxxxxxx>; Valentina Fernandez Alanis - > M63239 <Valentina.FernandezAlanis@xxxxxxxxxxxxx>; Praveen Kumar - > I30718 <Praveen.Kumar@xxxxxxxxxxxxx> > Subject: Re: [PATCH V3] media: i2c: imx334: Add support for 1280x720 & > 640x480 resolutions > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > 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@xxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxx> > > --- > > > > 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. All value validated with respect to one link frequency (LINK_FREQ_445M) In this case i will try to get common register values for both link frequencies (819M and 445M). Thanks, Shravan > > > + 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