[PATCH V7 0/4] media: i2c: imx334: Add support for 1280x720 & 640x480 resolutions

[PATCH V7 0/4] media: i2c: imx334: Add support for 1280x720 & 640x480 resolutions

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Changelog:
v6 -> v7:
Add new patch: common reg value correction

v5 -> v6:
split the patches from two to three
1) Optimized the resolution arrays by integrating a common register array
2) 3840x2160 mode restored with reset values of imx334
3) 1280x720@30 and 640x480@30 resolutions 

v4 -> v5:
Divided the patch into two separate patches:
- One for common register array values with existing resolutions
- Another for new modes supporting 640x480 and 1280x720 resolutions

v3 -> v4:
In response to the review request, the dependency of the common register
value array on the 445M link frequency has been eliminated.
Although I do not have a test setup for 4k resolution at an 819M
link frequency, I have made adjustments based on the IMX334 data sheet,
the latest 4k resolution, and the common register value array. 
Additionally, the 4k resolution has been switched to master mode to
ensure consistency with other resolutions that also use master mode.

v2 -> v3:
removied blank lines reported by media CI robot 

v1 -> v2:
Optimized the resolution arrays by added common register
array

---

Shravan Chippa (4):
  media: i2c: imx334: Optimized 4k and 2k mode register arrays
  media: i2c: imx334: common reg value correction
  media: i2c: imx334: update mode_3840x2160_regs array
  media: i2c: imx334: add modes for 720p and 480p resolutions

 drivers/media/i2c/imx334.c | 205 +++++++++++++++++++------------------
 1 file changed, 103 insertions(+), 102 deletions(-)

-- 
2.34.1

[PATCH V7 1/4] media: i2c: imx334: Optimized 4k and 2k mode register arrays

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Optimized the resolution arrays by integrating a common register array.

Adjusted the register array values for 1920x1080@30 and 3840x2160@30
resolutions to align with the common register array values.

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 131 +++++++++----------------------------
 1 file changed, 30 insertions(+), 101 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index a544fc3df39c..db61b298ceb3 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 values */
+static const struct imx334_reg common_mode_regs[] = {
 	{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},
@@ -330,24 +314,31 @@ static const struct imx334_reg mode_1920x1080_regs[] = {
 	{0x3002, 0x00},
 };
 
+/* 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},
-	{0x3002, 0x00},
-	{0x3018, 0x04},
-	{0x37b0, 0x36},
-	{0x304c, 0x00},
-	{0x300c, 0x3b},
-	{0x300d, 0x2a},
 	{0x3034, 0x26},
 	{0x3035, 0x02},
-	{0x314c, 0x29},
-	{0x314d, 0x01},
 	{0x315a, 0x02},
-	{0x3168, 0xa0},
-	{0x316a, 0x7e},
-	{0x3288, 0x21},
-	{0x328a, 0x02},
 	{0x302c, 0x3c},
 	{0x302d, 0x00},
 	{0x302e, 0x00},
@@ -356,82 +347,13 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3077, 0x08},
 	{0x3090, 0x70},
 	{0x3091, 0x08},
-	{0x30d8, 0x20},
-	{0x30d9, 0x12},
 	{0x3308, 0x70},
 	{0x3309, 0x08},
-	{0x3414, 0x05},
-	{0x3416, 0x18},
-	{0x35ac, 0x0e},
-	{0x3648, 0x01},
-	{0x364a, 0x04},
-	{0x364c, 0x04},
-	{0x3678, 0x01},
-	{0x367c, 0x31},
-	{0x367e, 0x31},
-	{0x3708, 0x02},
-	{0x3714, 0x01},
-	{0x3715, 0x02},
-	{0x3716, 0x02},
-	{0x3717, 0x02},
-	{0x371c, 0x3d},
-	{0x371d, 0x3f},
-	{0x372c, 0x00},
-	{0x372d, 0x00},
-	{0x372e, 0x46},
-	{0x372f, 0x00},
-	{0x3730, 0x89},
-	{0x3731, 0x00},
-	{0x3732, 0x08},
-	{0x3733, 0x01},
-	{0x3734, 0xfe},
-	{0x3735, 0x05},
-	{0x375d, 0x00},
-	{0x375e, 0x00},
-	{0x375f, 0x61},
-	{0x3760, 0x06},
-	{0x3768, 0x1b},
-	{0x3769, 0x1b},
-	{0x376a, 0x1a},
-	{0x376b, 0x19},
-	{0x376c, 0x18},
-	{0x376d, 0x14},
-	{0x376e, 0x0f},
-	{0x3776, 0x00},
-	{0x3777, 0x00},
-	{0x3778, 0x46},
-	{0x3779, 0x00},
-	{0x377a, 0x08},
-	{0x377b, 0x01},
-	{0x377c, 0x45},
-	{0x377d, 0x01},
-	{0x377e, 0x23},
-	{0x377f, 0x02},
-	{0x3780, 0xd9},
-	{0x3781, 0x03},
-	{0x3782, 0xf5},
-	{0x3783, 0x06},
-	{0x3784, 0xa5},
-	{0x3788, 0x0f},
-	{0x378a, 0xd9},
-	{0x378b, 0x03},
-	{0x378c, 0xeb},
-	{0x378d, 0x05},
-	{0x378e, 0x87},
-	{0x378f, 0x06},
-	{0x3790, 0xf5},
-	{0x3792, 0x43},
-	{0x3794, 0x7a},
-	{0x3796, 0xa1},
-	{0x3e04, 0x0e},
 	{0x319e, 0x00},
 	{0x3a00, 0x01},
 	{0x3a18, 0xbf},
-	{0x3a19, 0x00},
 	{0x3a1a, 0x67},
-	{0x3a1b, 0x00},
 	{0x3a1c, 0x6f},
-	{0x3a1d, 0x00},
 	{0x3a1e, 0xd7},
 	{0x3a1f, 0x01},
 	{0x3a20, 0x6f},
@@ -989,6 +911,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
 	const struct imx334_reg_list *reg_list;
 	int ret;
 
+	ret = imx334_write_regs(imx334, common_mode_regs,
+				ARRAY_SIZE(common_mode_regs));
+	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,
-- 
2.34.1

[PATCH V7 2/4] media: i2c: imx334: common reg value correction

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

correcting the CPWAIT_TIME value as per the data sheet
for the link frequency and input clock

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index db61b298ceb3..0b42bc0470a1 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -191,7 +191,7 @@ static const struct imx334_reg common_mode_regs[] = {
 	{0x31a0, 0x20},
 	{0x31a1, 0x0f},
 	{0x300c, 0x3b},
-	{0x300d, 0x29},
+	{0x300d, 0x2a},
 	{0x314c, 0x29},
 	{0x314d, 0x01},
 	{0x315a, 0x06},
-- 
2.34.1

[PATCH V7 3/4] media: i2c: imx334: update mode_3840x2160_regs array

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

The 3840x2160 mode operates with the imx334 reset values.
If we switch to other modes and then return to the 3840x2160 mode,
it should function correctly. so updated the mode_3840x2160_regs
array with the imx334 reset values.

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 0b42bc0470a1..a7c0bd38c9b8 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -343,6 +343,12 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x302d, 0x00},
 	{0x302e, 0x00},
 	{0x302f, 0x0f},
+	{0x3074, 0xb0},
+	{0x3075, 0x00},
+	{0x308e, 0xb1},
+	{0x308f, 0x00},
+	{0x30d8, 0x20},
+	{0x30d9, 0x12},
 	{0x3076, 0x70},
 	{0x3077, 0x08},
 	{0x3090, 0x70},
-- 
2.34.1

[PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Added support for 1280x720@30 and 640x480@30 resolutions

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 66 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index a7c0bd38c9b8..8cd1eecd0143 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -314,6 +314,46 @@ static const struct imx334_reg common_mode_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},
@@ -433,6 +473,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,
+		},
 	},
 };
 
-- 
2.34.1

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Kieran Bingham]

Quoting shravan kumar (2025-03-05 05:14:42)
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> Added support for 1280x720@30 and 640x480@30 resolutions
> 
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 66 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index a7c0bd38c9b8..8cd1eecd0143 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -314,6 +314,46 @@ static const struct imx334_reg common_mode_regs[] = {
>         {0x3002, 0x00},
>  };
>  
> +/* Sensor mode registers for 640x480@30fps */
> +static const struct imx334_reg mode_640x480_regs[] = {
> +       {0x302c, 0x70},
> +       {0x302d, 0x06},

These two are a single 16 bit register HTRIMMING_START = 1648

> +       {0x302e, 0x80},
> +       {0x302f, 0x02},

These two are a single 16 bit register HNUM = 640

> +       {0x3074, 0x48},
> +       {0x3075, 0x07},

These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864

> +       {0x308e, 0x49},
> +       {0x308f, 0x07},

These two are a single 16 bit register AREA3_ST_ADR_2 = 1865

> +       {0x3076, 0xe0},
> +       {0x3077, 0x01},

These two are a single 16 bit register AREA3_WIDTH_1 = 480

> +       {0x3090, 0xe0},
> +       {0x3091, 0x01},

These two are a single 16 bit register AREA3_WIDTH_2 = 480

> +       {0x3308, 0xe0},
> +       {0x3309, 0x01},

These two are a single 16 bit register Y_OUT_SIZE

Don't you think 
	{ Y_OUT_SIZE, 480 },

Is so much more readable and easier to comprehend and maintain?


> +       {0x30d8, 0x30},
> +       {0x30d9, 0x0b},

These two are a single 16 bit register UNREAD_ED_ADR = 2864

> +};

I'm still sad that we can all know the names of all these registers and
yet this is writing new tables of hex values.


--
Kieran


> +
> +/* 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},
> @@ -433,6 +473,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,
> +               },
>         },
>  };
>  
> -- 
> 2.34.1
>

RE: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Shravan.Chippa]

Hi Kieran

> -----Original Message-----
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Sent: Wednesday, March 5, 2025 3:05 PM
> To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> I35088 <Shravan.Chippa@microchip.com>
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> <Shravan.Chippa@microchip.com>
> Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> resolutions
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Quoting shravan kumar (2025-03-05 05:14:42)
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Added support for 1280x720@30 and 640x480@30 resolutions
> >
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 66
> > ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index a7c0bd38c9b8..8cd1eecd0143 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -314,6 +314,46 @@ static const struct imx334_reg
> common_mode_regs[] = {
> >         {0x3002, 0x00},
> >  };
> >
> > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > +imx334_reg mode_640x480_regs[] = {
> > +       {0x302c, 0x70},
> > +       {0x302d, 0x06},
> 
> These two are a single 16 bit register HTRIMMING_START = 1648
> 
> > +       {0x302e, 0x80},
> > +       {0x302f, 0x02},
> 
> These two are a single 16 bit register HNUM = 640
> 
> > +       {0x3074, 0x48},
> > +       {0x3075, 0x07},
> 
> These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> 
> > +       {0x308e, 0x49},
> > +       {0x308f, 0x07},
> 
> These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> 
> > +       {0x3076, 0xe0},
> > +       {0x3077, 0x01},
> 
> These two are a single 16 bit register AREA3_WIDTH_1 = 480
> 
> > +       {0x3090, 0xe0},
> > +       {0x3091, 0x01},
> 
> These two are a single 16 bit register AREA3_WIDTH_2 = 480
> 
> > +       {0x3308, 0xe0},
> > +       {0x3309, 0x01},
> 
> These two are a single 16 bit register Y_OUT_SIZE
> 
> Don't you think
>         { Y_OUT_SIZE, 480 },
> 
> Is so much more readable and easier to comprehend and maintain?
> 
> 
> > +       {0x30d8, 0x30},
> > +       {0x30d9, 0x0b},
> 
> These two are a single 16 bit register UNREAD_ED_ADR = 2864
> 
> > +};
> 
> I'm still sad that we can all know the names of all these registers and yet this
> is writing new tables of hex values.

Do you want me use call like bellow API with register names:
CCI_REG16_LE(0x30d8);
cci_write();
cci_multi_reg_write();
devm_cci_regmap_init_i2c();


Thanks,
Shravan
> 
> 
> --
> Kieran
> 
> 
> > +
> > +/* 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},
> > @@ -433,6 +473,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,
> > +               },
> >         },
> >  };
> >
> > --
> > 2.34.1
> >

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Tarang Raval]

Hi Kieran, Shravan

> > -----Original Message-----
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Sent: Wednesday, March 5, 2025 3:05 PM
> > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > I35088 <Shravan.Chippa@microchip.com>
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > <Shravan.Chippa@microchip.com>
> > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > resolutions
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Quoting shravan kumar (2025-03-05 05:14:42)
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > Added support for 1280x720@30 and 640x480@30 resolutions
> > >
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > >  drivers/media/i2c/imx334.c | 66
> > > ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > common_mode_regs[] = {
> > >         {0x3002, 0x00},
> > >  };
> > >
> > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > +imx334_reg mode_640x480_regs[] = {
> > > +       {0x302c, 0x70},
> > > +       {0x302d, 0x06},
> >
> > These two are a single 16 bit register HTRIMMING_START = 1648
> >
> > > +       {0x302e, 0x80},
> > > +       {0x302f, 0x02},
> >
> > These two are a single 16 bit register HNUM = 640
> >
> > > +       {0x3074, 0x48},
> > > +       {0x3075, 0x07},
> >
> > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> >
> > > +       {0x308e, 0x49},
> > > +       {0x308f, 0x07},
> >
> > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> >
> > > +       {0x3076, 0xe0},
> > > +       {0x3077, 0x01},
> >
> > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> >
> > > +       {0x3090, 0xe0},
> > > +       {0x3091, 0x01},
> >
> > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> >
> > > +       {0x3308, 0xe0},
> > > +       {0x3309, 0x01},
> >
> > These two are a single 16 bit register Y_OUT_SIZE
> >
> > Don't you think
> >         { Y_OUT_SIZE, 480 },
> >
> > Is so much more readable and easier to comprehend and maintain?
> >
> >
> > > +       {0x30d8, 0x30},
> > > +       {0x30d9, 0x0b},
> >
> > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> >
> > > +};
> >
> > I'm still sad that we can all know the names of all these registers and yet this
> > is writing new tables of hex values.
>
> Do you want me use call like bellow API with register names:
> CCI_REG16_LE(0x30d8);
> cci_write();
> cci_multi_reg_write();
> devm_cci_regmap_init_i2c();

I have made a patch series for this sensor driver's improvement, in which 
I added the V4L2 CCI helper.

I am just waiting for this patch series to be applied so I can send my 
changes on top of it.

However, if Shravan wants to make these changes on his own, I don’t mind.

Best Regards,
Tarang

RE: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Kieran Bingham]

Quoting Shravan.Chippa@microchip.com (2025-03-05 10:22:12)
> Hi Kieran
> 
> > -----Original Message-----
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Sent: Wednesday, March 5, 2025 3:05 PM
> > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > I35088 <Shravan.Chippa@microchip.com>
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > <Shravan.Chippa@microchip.com>
> > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > resolutions
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> > 
> > Quoting shravan kumar (2025-03-05 05:14:42)
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > Added support for 1280x720@30 and 640x480@30 resolutions
> > >
> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > ---
> > >  drivers/media/i2c/imx334.c | 66
> > > ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > common_mode_regs[] = {
> > >         {0x3002, 0x00},
> > >  };
> > >
> > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > +imx334_reg mode_640x480_regs[] = {
> > > +       {0x302c, 0x70},
> > > +       {0x302d, 0x06},
> > 
> > These two are a single 16 bit register HTRIMMING_START = 1648
> > 
> > > +       {0x302e, 0x80},
> > > +       {0x302f, 0x02},
> > 
> > These two are a single 16 bit register HNUM = 640
> > 
> > > +       {0x3074, 0x48},
> > > +       {0x3075, 0x07},
> > 
> > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > 
> > > +       {0x308e, 0x49},
> > > +       {0x308f, 0x07},
> > 
> > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > 
> > > +       {0x3076, 0xe0},
> > > +       {0x3077, 0x01},
> > 
> > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > 
> > > +       {0x3090, 0xe0},
> > > +       {0x3091, 0x01},
> > 
> > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > 
> > > +       {0x3308, 0xe0},
> > > +       {0x3309, 0x01},
> > 
> > These two are a single 16 bit register Y_OUT_SIZE
> > 
> > Don't you think
> >         { Y_OUT_SIZE, 480 },
> > 
> > Is so much more readable and easier to comprehend and maintain?
> > 
> > 
> > > +       {0x30d8, 0x30},
> > > +       {0x30d9, 0x0b},
> > 
> > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > 
> > > +};
> > 
> > I'm still sad that we can all know the names of all these registers and yet this
> > is writing new tables of hex values.
> 
> Do you want me use call like bellow API with register names:
> CCI_REG16_LE(0x30d8);
> cci_write();
> cci_multi_reg_write();
> devm_cci_regmap_init_i2c();
 

Yes please, I would want that very much. I'm not very good at reading
and storing hex values in my head! That's why I broke down the above
registers to strings and decimal values.


The discussions at
https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12@PH0PR11MB5611.namprd11.prod.outlook.com/
reports the full datasheet, and I stated:


> > > This is an enormous amount of duplicated data that could be factored
> > > out.
> > >
> > > These are also /very/ common against the existing mode register
> > > tables too.
> > >
> > > I think several things need to happen in this driver:
> > >
> > >  1. It should be converted to use the CCI helpers.
> > >  2. Whereever identifiable, the register names should be used
> > > instead of
> > >     just the addresses.
> > >  3. The common factors of these tables should be de-duplicated.
> > >
> > > In your additions you only have differences in the following
> > > registers from those entire tables:

You replied with

> >
> > I will try to optimize camera resolution array register value usage by
> > writing common register array values.

which you have done (point 3), and is great progress in the series.

Further down in the thread I stated:


> > >  4. And ideally - the differences which determine the modes should
> > > be
> > >     factored out to calculations so that we are not writing out
> > > large
> > >     tables just to write a parameterised frame size.
> > >
> > >
> > > I would beleive that at least steps 1 and 3 would be achievable, 2
> > > and 4 would depend upon access to the datasheet.
> > >


I still believe steps 1 is important to this development. You have done
step 3 I think. And now both step 2 and (later) step 4 are possible as we
have the datasheet.

--
Kieran


> 
> Thanks,
> Shravan
> > 
> > 
> > --
> > Kieran
> > 
> > 
> > > +
> > > +/* 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},
> > > @@ -433,6 +473,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,
> > > +               },
> > >         },
> > >  };
> > >
> > > --
> > > 2.34.1
> > >

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Kieran Bingham]

Quoting Tarang Raval (2025-03-05 10:40:18)
> Hi Kieran, Shravan
> 
> > > -----Original Message-----
> > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > I35088 <Shravan.Chippa@microchip.com>
> > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > <Shravan.Chippa@microchip.com>
> > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > resolutions
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > content is safe
> > >
> > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > Added support for 1280x720@30 and 640x480@30 resolutions
> > > >
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > >� drivers/media/i2c/imx334.c | 66
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >� 1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > common_mode_regs[] = {
> > > >�������� {0x3002, 0x00},
> > > >� };
> > > >
> > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > +imx334_reg mode_640x480_regs[] = {
> > > > +������ {0x302c, 0x70},
> > > > +������ {0x302d, 0x06},
> > >
> > > These two are a single 16 bit register HTRIMMING_START = 1648
> > >
> > > > +������ {0x302e, 0x80},
> > > > +������ {0x302f, 0x02},
> > >
> > > These two are a single 16 bit register HNUM = 640
> > >
> > > > +������ {0x3074, 0x48},
> > > > +������ {0x3075, 0x07},
> > >
> > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > >
> > > > +������ {0x308e, 0x49},
> > > > +������ {0x308f, 0x07},
> > >
> > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > >
> > > > +������ {0x3076, 0xe0},
> > > > +������ {0x3077, 0x01},
> > >
> > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > >
> > > > +������ {0x3090, 0xe0},
> > > > +������ {0x3091, 0x01},
> > >
> > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > >
> > > > +������ {0x3308, 0xe0},
> > > > +������ {0x3309, 0x01},
> > >
> > > These two are a single 16 bit register Y_OUT_SIZE
> > >
> > > Don't you think
> > >�������� { Y_OUT_SIZE, 480 },
> > >
> > > Is so much more readable and easier to comprehend and maintain?
> > >
> > >
> > > > +������ {0x30d8, 0x30},
> > > > +������ {0x30d9, 0x0b},
> > >
> > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > >
> > > > +};
> > >
> > > I'm still sad that we can all know the names of all these registers and yet this
> > > is writing new tables of hex values.
> >
> > Do you want me use call like bellow API with register names:
> > CCI_REG16_LE(0x30d8);
> > cci_write();
> > cci_multi_reg_write();
> > devm_cci_regmap_init_i2c();
> 
> I have made a patch series for this sensor driver's improvement, in which 
> I added the V4L2 CCI helper.
> 

Has it been posted yet ?

> I am just waiting for this patch series to be applied so I can send my 
> changes on top of it.

If the conversion will happen after I'm fine with that - but it will
need to be tested by someone with the hardware. Changing to CCI regs can
have impacts that are hard to catch unless you really check and re-test
the driver - so Shravan is best placed if he is actively working on the
sensor.

Personally - I would like to see the CCI conversion happen *first* and
then Shravan's changes go on top of that ! It would be /far/ more
reviewable and readable that way.

(And we'd have more confidence that Shravan will have tested the full
CCI implementation on that version).

--
Kieran


> 
> However, if Shravan wants to make these changes on his own, I don�t mind.
> 
> Best Regards,
> Tarang

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Tarang Raval]

> Quoting Tarang Raval (2025-03-05 10:40:18)
> > Hi Kieran, Shravan
> >
> > > > -----Original Message-----
> > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > > I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > > <Shravan.Chippa@microchip.com>
> > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > > resolutions
> > > >
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > > content is safe
> > > >
> > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > Added support for 1280x720@30 and 640x480@30 resolutions
> > > > >
> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > ---
> > > > >� drivers/media/i2c/imx334.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >� 1 file changed, 66 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > --- a/drivers/media/i2c/imx334.c
> > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > > common_mode_regs[] = {
> > > > >�������� {0x3002, 0x00},
> > > > >� };
> > > > >
> > > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > > +imx334_reg mode_640x480_regs[] = {
> > > > > +������ {0x302c, 0x70},
> > > > > +������ {0x302d, 0x06},
> > > >
> > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > >
> > > > > +������ {0x302e, 0x80},
> > > > > +������ {0x302f, 0x02},
> > > >
> > > > These two are a single 16 bit register HNUM = 640
> > > >
> > > > > +������ {0x3074, 0x48},
> > > > > +������ {0x3075, 0x07},
> > > >
> > > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > >
> > > > > +������ {0x308e, 0x49},
> > > > > +������ {0x308f, 0x07},
> > > >
> > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > >
> > > > > +������ {0x3076, 0xe0},
> > > > > +������ {0x3077, 0x01},
> > > >
> > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > >
> > > > > +������ {0x3090, 0xe0},
> > > > > +������ {0x3091, 0x01},
> > > >
> > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > >
> > > > > +������ {0x3308, 0xe0},
> > > > > +������ {0x3309, 0x01},
> > > >
> > > > These two are a single 16 bit register Y_OUT_SIZE
> > > >
> > > > Don't you think
> > > >�������� { Y_OUT_SIZE, 480 },
> > > >
> > > > Is so much more readable and easier to comprehend and maintain?
> > > >
> > > >
> > > > > +������ {0x30d8, 0x30},
> > > > > +������ {0x30d9, 0x0b},
> > > >
> > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > >
> > > > > +};
> > > >
> > > > I'm still sad that we can all know the names of all these registers and yet this
> > > > is writing new tables of hex values.
> > >
> > > Do you want me use call like bellow API with register names:
> > > CCI_REG16_LE(0x30d8);
> > > cci_write();
> > > cci_multi_reg_write();
> > > devm_cci_regmap_init_i2c();
> >
> > I have made a patch series for this sensor driver's improvement, in which
> > I added the V4L2 CCI helper.
> >
> 
> Has it been posted yet ?

Not yet.

As mentioned below, I am waiting for this patch series to be applied.

Best Regards,
Tarang

> > I am just waiting for this patch series to be applied so I can send my
> > changes on top of it.
> 
> If the conversion will happen after I'm fine with that - but it will
> need to be tested by someone with the hardware. Changing to CCI regs can
> have impacts that are hard to catch unless you really check and re-test
> the driver - so Shravan is best placed if he is actively working on the
> sensor.
>
> Personally - I would like to see the CCI conversion happen *first* and
> then Shravan's changes go on top of that ! It would be /far/ more
> reviewable and readable that way.
> 
> (And we'd have more confidence that Shravan will have tested the full
> CCI implementation on that version).
>
> --
> Kieran

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Sakari Ailus]

Hi Kieran, Shravan,

On Wed, Mar 05, 2025 at 10:45:42AM +0000, Kieran Bingham wrote:
> Quoting Shravan.Chippa@microchip.com (2025-03-05 10:22:12)
> > Hi Kieran
> > 
> > > -----Original Message-----
> > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > I35088 <Shravan.Chippa@microchip.com>
> > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > <Shravan.Chippa@microchip.com>
> > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > resolutions
> > > 
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > content is safe
> > > 
> > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > Added support for 1280x720@30 and 640x480@30 resolutions
> > > >
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > >  drivers/media/i2c/imx334.c | 66
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > common_mode_regs[] = {
> > > >         {0x3002, 0x00},
> > > >  };
> > > >
> > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > +imx334_reg mode_640x480_regs[] = {
> > > > +       {0x302c, 0x70},
> > > > +       {0x302d, 0x06},
> > > 
> > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > 
> > > > +       {0x302e, 0x80},
> > > > +       {0x302f, 0x02},
> > > 
> > > These two are a single 16 bit register HNUM = 640
> > > 
> > > > +       {0x3074, 0x48},
> > > > +       {0x3075, 0x07},
> > > 
> > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > 
> > > > +       {0x308e, 0x49},
> > > > +       {0x308f, 0x07},
> > > 
> > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > 
> > > > +       {0x3076, 0xe0},
> > > > +       {0x3077, 0x01},
> > > 
> > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > 
> > > > +       {0x3090, 0xe0},
> > > > +       {0x3091, 0x01},
> > > 
> > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > 
> > > > +       {0x3308, 0xe0},
> > > > +       {0x3309, 0x01},
> > > 
> > > These two are a single 16 bit register Y_OUT_SIZE
> > > 
> > > Don't you think
> > >         { Y_OUT_SIZE, 480 },
> > > 
> > > Is so much more readable and easier to comprehend and maintain?
> > > 
> > > 
> > > > +       {0x30d8, 0x30},
> > > > +       {0x30d9, 0x0b},
> > > 
> > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > 
> > > > +};
> > > 
> > > I'm still sad that we can all know the names of all these registers and yet this
> > > is writing new tables of hex values.
> > 
> > Do you want me use call like bellow API with register names:
> > CCI_REG16_LE(0x30d8);
> > cci_write();
> > cci_multi_reg_write();
> > devm_cci_regmap_init_i2c();
>  
> 
> Yes please, I would want that very much. I'm not very good at reading
> and storing hex values in my head! That's why I broke down the above
> registers to strings and decimal values.

I agree, it'd be good to do these while changing the driver now. I think it
could be done after adding the new modes, now that the patches already
exist.

> 
> 
> The discussions at
> https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12@PH0PR11MB5611.namprd11.prod.outlook.com/
> reports the full datasheet, and I stated:
> 
> 
> > > > This is an enormous amount of duplicated data that could be factored
> > > > out.
> > > >
> > > > These are also /very/ common against the existing mode register
> > > > tables too.
> > > >
> > > > I think several things need to happen in this driver:
> > > >
> > > >  1. It should be converted to use the CCI helpers.
> > > >  2. Whereever identifiable, the register names should be used
> > > > instead of
> > > >     just the addresses.
> > > >  3. The common factors of these tables should be de-duplicated.
> > > >
> > > > In your additions you only have differences in the following
> > > > registers from those entire tables:
> 
> You replied with
> 
> > >
> > > I will try to optimize camera resolution array register value usage by
> > > writing common register array values.
> 
> which you have done (point 3), and is great progress in the series.
> 
> Further down in the thread I stated:
> 
> 
> > > >  4. And ideally - the differences which determine the modes should
> > > > be
> > > >     factored out to calculations so that we are not writing out
> > > > large
> > > >     tables just to write a parameterised frame size.
> > > >
> > > >
> > > > I would beleive that at least steps 1 and 3 would be achievable, 2
> > > > and 4 would depend upon access to the datasheet.
> > > >
> 
> 
> I still believe steps 1 is important to this development. You have done
> step 3 I think. And now both step 2 and (later) step 4 are possible as we
> have the datasheet.

In this case the datasheet doesn't appear to be documenting the PLL.

-- 
Regards,

Sakari Ailus

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Kieran Bingham]

Quoting Tarang Raval (2025-03-05 11:30:57)
> > Quoting Tarang Raval (2025-03-05 10:40:18)
> > > Hi Kieran, Shravan
> > >
> > > > > -----Original Message-----
> > > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > > > I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > > > <Shravan.Chippa@microchip.com>
> > > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > > > resolutions
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > > > content is safe
> > > > >
> > > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > Added support for 1280x720@30 and 640x480@30 resolutions
> > > > > >
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > ---
> > > > > >� drivers/media/i2c/imx334.c | 66
> > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > >� 1 file changed, 66 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > > > common_mode_regs[] = {
> > > > > >�������� {0x3002, 0x00},
> > > > > >� };
> > > > > >
> > > > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > > > +imx334_reg mode_640x480_regs[] = {
> > > > > > +������ {0x302c, 0x70},
> > > > > > +������ {0x302d, 0x06},
> > > > >
> > > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > > >
> > > > > > +������ {0x302e, 0x80},
> > > > > > +������ {0x302f, 0x02},
> > > > >
> > > > > These two are a single 16 bit register HNUM = 640
> > > > >
> > > > > > +������ {0x3074, 0x48},
> > > > > > +������ {0x3075, 0x07},
> > > > >
> > > > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > > >
> > > > > > +������ {0x308e, 0x49},
> > > > > > +������ {0x308f, 0x07},
> > > > >
> > > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > > >
> > > > > > +������ {0x3076, 0xe0},
> > > > > > +������ {0x3077, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > > >
> > > > > > +������ {0x3090, 0xe0},
> > > > > > +������ {0x3091, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > > >
> > > > > > +������ {0x3308, 0xe0},
> > > > > > +������ {0x3309, 0x01},
> > > > >
> > > > > These two are a single 16 bit register Y_OUT_SIZE
> > > > >
> > > > > Don't you think
> > > > >�������� { Y_OUT_SIZE, 480 },
> > > > >
> > > > > Is so much more readable and easier to comprehend and maintain?
> > > > >
> > > > >
> > > > > > +������ {0x30d8, 0x30},
> > > > > > +������ {0x30d9, 0x0b},
> > > > >
> > > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > > >
> > > > > > +};
> > > > >
> > > > > I'm still sad that we can all know the names of all these registers and yet this
> > > > > is writing new tables of hex values.
> > > >
> > > > Do you want me use call like bellow API with register names:
> > > > CCI_REG16_LE(0x30d8);
> > > > cci_write();
> > > > cci_multi_reg_write();
> > > > devm_cci_regmap_init_i2c();
> > >
> > > I have made a patch series for this sensor driver's improvement, in which
> > > I added the V4L2 CCI helper.
> > >
> > 
> > Has it been posted yet ?
> 
> Not yet.
> 
> As mentioned below, I am waiting for this patch series to be applied.
> 
> Best Regards,
> Tarang
> 
> > > I am just waiting for this patch series to be applied so I can send my
> > > changes on top of it.
> > 
> > If the conversion will happen after I'm fine with that - but it will
> > need to be tested by someone with the hardware. Changing to CCI regs can
> > have impacts that are hard to catch unless you really check and re-test
> > the driver - so Shravan is best placed if he is actively working on the
> > sensor.
> >
> > Personally - I would like to see the CCI conversion happen *first* and
> > then Shravan's changes go on top of that ! It would be /far/ more
> > reviewable and readable that way.
> > 
> > (And we'd have more confidence that Shravan will have tested the full
> > CCI implementation on that version).

Well, knowing that there is already work in place to improve the
readability of the driver - I'm fine seeing these patches merged first -
Shravan - will you be able to test Tarang's work on top ?

--
Kieran


> >
> > --
> > Kieran

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Kieran Bingham]

Quoting Sakari Ailus (2025-03-05 11:50:53)
> Hi Kieran, Shravan,
> 
> On Wed, Mar 05, 2025 at 10:45:42AM +0000, Kieran Bingham wrote:
> > Quoting Shravan.Chippa@microchip.com (2025-03-05 10:22:12)
> > > Hi Kieran
> > > 
> > > > -----Original Message-----
> > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > > I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > > <Shravan.Chippa@microchip.com>
> > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > > resolutions
> > > > 
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > > content is safe
> > > > 
> > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > Added support for 1280x720@30 and 640x480@30 resolutions
> > > > >
> > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > ---
> > > > >  drivers/media/i2c/imx334.c | 66
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 66 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > --- a/drivers/media/i2c/imx334.c
> > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > common_mode_regs[] = {
> > > > >         {0x3002, 0x00},
> > > > >  };
> > > > >
> > > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > > +imx334_reg mode_640x480_regs[] = {
> > > > > +       {0x302c, 0x70},
> > > > > +       {0x302d, 0x06},
> > > > 
> > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > > 
> > > > > +       {0x302e, 0x80},
> > > > > +       {0x302f, 0x02},
> > > > 
> > > > These two are a single 16 bit register HNUM = 640
> > > > 
> > > > > +       {0x3074, 0x48},
> > > > > +       {0x3075, 0x07},
> > > > 
> > > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > > 
> > > > > +       {0x308e, 0x49},
> > > > > +       {0x308f, 0x07},
> > > > 
> > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > > 
> > > > > +       {0x3076, 0xe0},
> > > > > +       {0x3077, 0x01},
> > > > 
> > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > > 
> > > > > +       {0x3090, 0xe0},
> > > > > +       {0x3091, 0x01},
> > > > 
> > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > > 
> > > > > +       {0x3308, 0xe0},
> > > > > +       {0x3309, 0x01},
> > > > 
> > > > These two are a single 16 bit register Y_OUT_SIZE
> > > > 
> > > > Don't you think
> > > >         { Y_OUT_SIZE, 480 },
> > > > 
> > > > Is so much more readable and easier to comprehend and maintain?
> > > > 
> > > > 
> > > > > +       {0x30d8, 0x30},
> > > > > +       {0x30d9, 0x0b},
> > > > 
> > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > > 
> > > > > +};
> > > > 
> > > > I'm still sad that we can all know the names of all these registers and yet this
> > > > is writing new tables of hex values.
> > > 
> > > Do you want me use call like bellow API with register names:
> > > CCI_REG16_LE(0x30d8);
> > > cci_write();
> > > cci_multi_reg_write();
> > > devm_cci_regmap_init_i2c();
> >  
> > 
> > Yes please, I would want that very much. I'm not very good at reading
> > and storing hex values in my head! That's why I broke down the above
> > registers to strings and decimal values.
> 
> I agree, it'd be good to do these while changing the driver now. I think it
> could be done after adding the new modes, now that the patches already
> exist.

That's ok with me! Either way I'm happy to see the drivers are getting
cleaned up!

> 
> > 
> > 
> > The discussions at
> > https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12@PH0PR11MB5611.namprd11.prod.outlook.com/
> > reports the full datasheet, and I stated:
> > 
> > 
> > > > > This is an enormous amount of duplicated data that could be factored
> > > > > out.
> > > > >
> > > > > These are also /very/ common against the existing mode register
> > > > > tables too.
> > > > >
> > > > > I think several things need to happen in this driver:
> > > > >
> > > > >  1. It should be converted to use the CCI helpers.
> > > > >  2. Whereever identifiable, the register names should be used
> > > > > instead of
> > > > >     just the addresses.
> > > > >  3. The common factors of these tables should be de-duplicated.
> > > > >
> > > > > In your additions you only have differences in the following
> > > > > registers from those entire tables:
> > 
> > You replied with
> > 
> > > >
> > > > I will try to optimize camera resolution array register value usage by
> > > > writing common register array values.
> > 
> > which you have done (point 3), and is great progress in the series.
> > 
> > Further down in the thread I stated:
> > 
> > 
> > > > >  4. And ideally - the differences which determine the modes should
> > > > > be
> > > > >     factored out to calculations so that we are not writing out
> > > > > large
> > > > >     tables just to write a parameterised frame size.
> > > > >
> > > > >
> > > > > I would beleive that at least steps 1 and 3 would be achievable, 2
> > > > > and 4 would depend upon access to the datasheet.
> > > > >
> > 
> > 
> > I still believe steps 1 is important to this development. You have done
> > step 3 I think. And now both step 2 and (later) step 4 are possible as we
> > have the datasheet.
> 
> In this case the datasheet doesn't appear to be documenting the PLL.

:-(

But at least we will have enough to handle all the vmax/and cropping
more directly.

--
Kieran


> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Dave Stevenson]

Hi Kieran, Sakari, Shravan, and Tarang

On Wed, 5 Mar 2025 at 12:17, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Sakari Ailus (2025-03-05 11:50:53)
> > Hi Kieran, Shravan,
> >
> > On Wed, Mar 05, 2025 at 10:45:42AM +0000, Kieran Bingham wrote:
> > > Quoting Shravan.Chippa@microchip.com (2025-03-05 10:22:12)
> > > > Hi Kieran
> > > >
> > > > > -----Original Message-----
> > > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan Chippa -
> > > > > I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley
> > > > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > > > <Shravan.Chippa@microchip.com>
> > > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> > > > > resolutions
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > > > > content is safe
> > > > >
> > > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > Added support for 1280x720@30 and 640x480@30 resolutions

Silly question - why is the frame rate specified here? Is that the
minimum, maximum, or default framerate, as we have V4L2_CID_VBLANK for
varying the frame rate.

> > > > > >
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx334.c | 66
> > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 66 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > > > index a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > > common_mode_regs[] = {
> > > > > >         {0x3002, 0x00},
> > > > > >  };
> > > > > >
> > > > > > +/* Sensor mode registers for 640x480@30fps */ static const struct
> > > > > > +imx334_reg mode_640x480_regs[] = {
> > > > > > +       {0x302c, 0x70},
> > > > > > +       {0x302d, 0x06},
> > > > >
> > > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > > >
> > > > > > +       {0x302e, 0x80},
> > > > > > +       {0x302f, 0x02},
> > > > >
> > > > > These two are a single 16 bit register HNUM = 640
> > > > >
> > > > > > +       {0x3074, 0x48},
> > > > > > +       {0x3075, 0x07},
> > > > >
> > > > > These two are a single 16 bit (well, 12 bit value) AREA3_ST_ADR_1 = 1864
> > > > >
> > > > > > +       {0x308e, 0x49},
> > > > > > +       {0x308f, 0x07},
> > > > >
> > > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > > >
> > > > > > +       {0x3076, 0xe0},
> > > > > > +       {0x3077, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > > >
> > > > > > +       {0x3090, 0xe0},
> > > > > > +       {0x3091, 0x01},
> > > > >
> > > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > > >
> > > > > > +       {0x3308, 0xe0},
> > > > > > +       {0x3309, 0x01},
> > > > >
> > > > > These two are a single 16 bit register Y_OUT_SIZE
> > > > >
> > > > > Don't you think
> > > > >         { Y_OUT_SIZE, 480 },
> > > > >
> > > > > Is so much more readable and easier to comprehend and maintain?
> > > > >
> > > > >
> > > > > > +       {0x30d8, 0x30},
> > > > > > +       {0x30d9, 0x0b},
> > > > >
> > > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > > >
> > > > > > +};
> > > > >
> > > > > I'm still sad that we can all know the names of all these registers and yet this
> > > > > is writing new tables of hex values.
> > > >
> > > > Do you want me use call like bellow API with register names:
> > > > CCI_REG16_LE(0x30d8);
> > > > cci_write();
> > > > cci_multi_reg_write();
> > > > devm_cci_regmap_init_i2c();
> > >
> > >
> > > Yes please, I would want that very much. I'm not very good at reading
> > > and storing hex values in my head! That's why I broke down the above
> > > registers to strings and decimal values.
> >
> > I agree, it'd be good to do these while changing the driver now. I think it
> > could be done after adding the new modes, now that the patches already
> > exist.
>
> That's ok with me! Either way I'm happy to see the drivers are getting
> cleaned up!
>
> >
> > >
> > >
> > > The discussions at
> > > https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12@PH0PR11MB5611.namprd11.prod.outlook.com/
> > > reports the full datasheet, and I stated:
> > >
> > >
> > > > > > This is an enormous amount of duplicated data that could be factored
> > > > > > out.
> > > > > >
> > > > > > These are also /very/ common against the existing mode register
> > > > > > tables too.
> > > > > >
> > > > > > I think several things need to happen in this driver:
> > > > > >
> > > > > >  1. It should be converted to use the CCI helpers.
> > > > > >  2. Whereever identifiable, the register names should be used
> > > > > > instead of
> > > > > >     just the addresses.
> > > > > >  3. The common factors of these tables should be de-duplicated.
> > > > > >
> > > > > > In your additions you only have differences in the following
> > > > > > registers from those entire tables:
> > >
> > > You replied with
> > >
> > > > >
> > > > > I will try to optimize camera resolution array register value usage by
> > > > > writing common register array values.
> > >
> > > which you have done (point 3), and is great progress in the series.
> > >
> > > Further down in the thread I stated:
> > >
> > >
> > > > > >  4. And ideally - the differences which determine the modes should
> > > > > > be
> > > > > >     factored out to calculations so that we are not writing out
> > > > > > large
> > > > > >     tables just to write a parameterised frame size.
> > > > > >
> > > > > >
> > > > > > I would beleive that at least steps 1 and 3 would be achievable, 2
> > > > > > and 4 would depend upon access to the datasheet.
> > > > > >
> > >
> > >
> > > I still believe steps 1 is important to this development. You have done
> > > step 3 I think. And now both step 2 and (later) step 4 are possible as we
> > > have the datasheet.
> >
> > In this case the datasheet doesn't appear to be documenting the PLL.
>
> :-(
>
> But at least we will have enough to handle all the vmax/and cropping
> more directly.

I haven't fully analysed the datasheet, but isn't this just another
Sony Starvis sensor, working in the same way as imx415 and imx290?

I don't think you can reasonably treat it as generic PLLs without
effectively reverse engineering the hardware.
INCK gets handled by the registers as specified in "INCK Setting" to
get the source clocks consistent regardless of INCK.
Source clock vs link frequency registers need to be teased out of the
"INCK Setting Register" table. Quick analysis:
- BCWAIT_TIME, CP_WAIT, INCKSEL2, INCKSEL3, and INCKSEL4 only change with INCK.
- PLL_IF_GC and SYS_MODE only change with link frequency.
- INCKSEL1 is the awkward one if there is a need to support the
1188Mbit/s data rate, otherwise it only changes with INCK.

The pixel rate stays the same for all modes (binning may be the exception here)
HMAX (V4L2_CID_HBLANK) is changed to vary the line time.
Whilst VMAX is listed as a constant for each mode, it can be changed
to alter frame rate.

Link frequency is independent of resolution, but HMAX has to be set to
ensure that we don't overflow the FIFO between array and CSI block.


Is there anyone selling an IMX334 module for use with SBCs at a
sensible price? I'm always up for tinkering with sensors, even if I'm
not meant to be!

  Dave

> --
> Kieran
>
>
> >
> > --
> > Regards,
> >
> > Sakari Ailus
>

RE: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p resolutions

[Shravan.Chippa]

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Wednesday, March 5, 2025 6:30 PM
> To: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>; shravan Chippa - I35088
> <Shravan.Chippa@microchip.com>; mchehab@kernel.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> I30718 <Praveen.Kumar@microchip.com>
> Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for 720p and 480p
> resolutions
> 
> [You don't often get email from dave.stevenson@raspberrypi.com. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Kieran, Sakari, Shravan, and Tarang
> 
> On Wed, 5 Mar 2025 at 12:17, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Sakari Ailus (2025-03-05 11:50:53)
> > > Hi Kieran, Shravan,
> > >
> > > On Wed, Mar 05, 2025 at 10:45:42AM +0000, Kieran Bingham wrote:
> > > > Quoting Shravan.Chippa@microchip.com (2025-03-05 10:22:12)
> > > > > Hi Kieran
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > Sent: Wednesday, March 5, 2025 3:05 PM
> > > > > > To: mchehab@kernel.org; sakari.ailus@linux.intel.com; shravan
> > > > > > Chippa -
> > > > > > I35088 <Shravan.Chippa@microchip.com>
> > > > > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > > Conor Dooley
> > > > > > - M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez
> > > > > > Alanis -
> > > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen
> > > > > > Kumar -
> > > > > > I30718 <Praveen.Kumar@microchip.com>; shravan Chippa - I35088
> > > > > > <Shravan.Chippa@microchip.com>
> > > > > > Subject: Re: [PATCH V7 4/4] media: i2c: imx334: add modes for
> > > > > > 720p and 480p resolutions
> > > > > >
> > > > > > EXTERNAL EMAIL: Do not click links or open attachments unless
> > > > > > you know the content is safe
> > > > > >
> > > > > > Quoting shravan kumar (2025-03-05 05:14:42)
> > > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > >
> > > > > > > Added support for 1280x720@30 and 640x480@30 resolutions
> 
> Silly question - why is the frame rate specified here? Is that the minimum,
> maximum, or default framerate, as we have V4L2_CID_VBLANK for varying
> the frame rate.

Yes, default is 30fps, max upto 60fps

Thanks,
Shravan

> 
> > > > > > >
> > > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/imx334.c | 66
> > > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 66 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > > > b/drivers/media/i2c/imx334.c index
> > > > > > > a7c0bd38c9b8..8cd1eecd0143 100644
> > > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > > @@ -314,6 +314,46 @@ static const struct imx334_reg
> > > > > > common_mode_regs[] = {
> > > > > > >         {0x3002, 0x00},
> > > > > > >  };
> > > > > > >
> > > > > > > +/* Sensor mode registers for 640x480@30fps */ static const
> > > > > > > +struct imx334_reg mode_640x480_regs[] = {
> > > > > > > +       {0x302c, 0x70},
> > > > > > > +       {0x302d, 0x06},
> > > > > >
> > > > > > These two are a single 16 bit register HTRIMMING_START = 1648
> > > > > >
> > > > > > > +       {0x302e, 0x80},
> > > > > > > +       {0x302f, 0x02},
> > > > > >
> > > > > > These two are a single 16 bit register HNUM = 640
> > > > > >
> > > > > > > +       {0x3074, 0x48},
> > > > > > > +       {0x3075, 0x07},
> > > > > >
> > > > > > These two are a single 16 bit (well, 12 bit value)
> > > > > > AREA3_ST_ADR_1 = 1864
> > > > > >
> > > > > > > +       {0x308e, 0x49},
> > > > > > > +       {0x308f, 0x07},
> > > > > >
> > > > > > These two are a single 16 bit register AREA3_ST_ADR_2 = 1865
> > > > > >
> > > > > > > +       {0x3076, 0xe0},
> > > > > > > +       {0x3077, 0x01},
> > > > > >
> > > > > > These two are a single 16 bit register AREA3_WIDTH_1 = 480
> > > > > >
> > > > > > > +       {0x3090, 0xe0},
> > > > > > > +       {0x3091, 0x01},
> > > > > >
> > > > > > These two are a single 16 bit register AREA3_WIDTH_2 = 480
> > > > > >
> > > > > > > +       {0x3308, 0xe0},
> > > > > > > +       {0x3309, 0x01},
> > > > > >
> > > > > > These two are a single 16 bit register Y_OUT_SIZE
> > > > > >
> > > > > > Don't you think
> > > > > >         { Y_OUT_SIZE, 480 },
> > > > > >
> > > > > > Is so much more readable and easier to comprehend and maintain?
> > > > > >
> > > > > >
> > > > > > > +       {0x30d8, 0x30},
> > > > > > > +       {0x30d9, 0x0b},
> > > > > >
> > > > > > These two are a single 16 bit register UNREAD_ED_ADR = 2864
> > > > > >
> > > > > > > +};
> > > > > >
> > > > > > I'm still sad that we can all know the names of all these
> > > > > > registers and yet this is writing new tables of hex values.
> > > > >
> > > > > Do you want me use call like bellow API with register names:
> > > > > CCI_REG16_LE(0x30d8);
> > > > > cci_write();
> > > > > cci_multi_reg_write();
> > > > > devm_cci_regmap_init_i2c();
> > > >
> > > >
> > > > Yes please, I would want that very much. I'm not very good at
> > > > reading and storing hex values in my head! That's why I broke down
> > > > the above registers to strings and decimal values.
> > >
> > > I agree, it'd be good to do these while changing the driver now. I
> > > think it could be done after adding the new modes, now that the
> > > patches already exist.
> >
> > That's ok with me! Either way I'm happy to see the drivers are getting
> > cleaned up!
> >
> > >
> > > >
> > > >
> > > > The discussions at
> > > >
> https://lore.kernel.org/all/PH0PR11MB5611FD22CF6E12F7226FA9C081E12
> > > > @PH0PR11MB5611.namprd11.prod.outlook.com/
> > > > reports the full datasheet, and I stated:
> > > >
> > > >
> > > > > > > This is an enormous amount of duplicated data that could be
> > > > > > > factored out.
> > > > > > >
> > > > > > > These are also /very/ common against the existing mode
> > > > > > > register tables too.
> > > > > > >
> > > > > > > I think several things need to happen in this driver:
> > > > > > >
> > > > > > >  1. It should be converted to use the CCI helpers.
> > > > > > >  2. Whereever identifiable, the register names should be
> > > > > > > used instead of
> > > > > > >     just the addresses.
> > > > > > >  3. The common factors of these tables should be de-duplicated.
> > > > > > >
> > > > > > > In your additions you only have differences in the following
> > > > > > > registers from those entire tables:
> > > >
> > > > You replied with
> > > >
> > > > > >
> > > > > > I will try to optimize camera resolution array register value
> > > > > > usage by writing common register array values.
> > > >
> > > > which you have done (point 3), and is great progress in the series.
> > > >
> > > > Further down in the thread I stated:
> > > >
> > > >
> > > > > > >  4. And ideally - the differences which determine the modes
> > > > > > > should be
> > > > > > >     factored out to calculations so that we are not writing
> > > > > > > out large
> > > > > > >     tables just to write a parameterised frame size.
> > > > > > >
> > > > > > >
> > > > > > > I would beleive that at least steps 1 and 3 would be
> > > > > > > achievable, 2 and 4 would depend upon access to the datasheet.
> > > > > > >
> > > >
> > > >
> > > > I still believe steps 1 is important to this development. You have
> > > > done step 3 I think. And now both step 2 and (later) step 4 are
> > > > possible as we have the datasheet.
> > >
> > > In this case the datasheet doesn't appear to be documenting the PLL.
> >
> > :-(
> >
> > But at least we will have enough to handle all the vmax/and cropping
> > more directly.
> 
> I haven't fully analysed the datasheet, but isn't this just another Sony Starvis
> sensor, working in the same way as imx415 and imx290?
> 
> I don't think you can reasonably treat it as generic PLLs without effectively
> reverse engineering the hardware.
> INCK gets handled by the registers as specified in "INCK Setting" to get the
> source clocks consistent regardless of INCK.
> Source clock vs link frequency registers need to be teased out of the "INCK
> Setting Register" table. Quick analysis:
> - BCWAIT_TIME, CP_WAIT, INCKSEL2, INCKSEL3, and INCKSEL4 only change
> with INCK.
> - PLL_IF_GC and SYS_MODE only change with link frequency.
> - INCKSEL1 is the awkward one if there is a need to support the 1188Mbit/s
> data rate, otherwise it only changes with INCK.
> 
> The pixel rate stays the same for all modes (binning may be the exception
> here) HMAX (V4L2_CID_HBLANK) is changed to vary the line time.
> Whilst VMAX is listed as a constant for each mode, it can be changed to alter
> frame rate.
> 
> Link frequency is independent of resolution, but HMAX has to be set to ensure
> that we don't overflow the FIFO between array and CSI block.
> 
> 
> Is there anyone selling an IMX334 module for use with SBCs at a sensible
> price? I'm always up for tinkering with sensors, even if I'm not meant to be!
> 
>   Dave
> 
> > --
> > Kieran
> >
> >
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
> >

Re: [PATCH V7 2/4] media: i2c: imx334: common reg value correction

[Sakari Ailus]

Hi Shravan,

On Wed, Mar 05, 2025 at 10:44:40AM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
> 
> correcting the CPWAIT_TIME value as per the data sheet
> for the link frequency and input clock
> 
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>

I've postponed applying this one once we have a better understanding what
that change actually is and why Tarang would seem to want to revert it.

> ---
>  drivers/media/i2c/imx334.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index db61b298ceb3..0b42bc0470a1 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -191,7 +191,7 @@ static const struct imx334_reg common_mode_regs[] = {
>  	{0x31a0, 0x20},
>  	{0x31a1, 0x0f},
>  	{0x300c, 0x3b},
> -	{0x300d, 0x29},
> +	{0x300d, 0x2a},
>  	{0x314c, 0x29},
>  	{0x314d, 0x01},
>  	{0x315a, 0x06},

-- 
Kind regards,

Sakari Ailus

Re: [PATCH V7 2/4] media: i2c: imx334: common reg value correction

[Tarang Raval]

Hi Sakari,

> On Wed, Mar 05, 2025 at 10:44:40AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > correcting the CPWAIT_TIME value as per the data sheet
> > for the link frequency and input clock
> >
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> 
> I've postponed applying this one once we have a better understanding what
> that change actually is and why Tarang would seem to want to revert it.

No, please recheck; I am not reverting this patch.

You can go ahead and apply this patch.

Best Regards,
Tarang 

> > ---
> >  drivers/media/i2c/imx334.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index db61b298ceb3..0b42bc0470a1 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -191,7 +191,7 @@ static const struct imx334_reg common_mode_regs[] = {
> >       {0x31a0, 0x20},
> >       {0x31a1, 0x0f},
> >       {0x300c, 0x3b},
> > -     {0x300d, 0x29},
> > +     {0x300d, 0x2a},
> >       {0x314c, 0x29},
> >       {0x314d, 0x01},
> >       {0x315a, 0x06},
> 
> --
> Kind regards,
> 
> Sakari Ailus