[PATCH 0/6] media: imx335: Vflip, active state and binning support

[PATCH 0/6] media: imx335: Vflip, active state and binning support

[Jai Luthra]

Hi,

This series adds support for 2x2 binning mode for Sony IMX335, along
with some improvements like using subdev active state, and fixing the
native pixel array width to match the datasheet.

These changes are done on top of a couple of already reviewed patches
from Umang's series from last year that added support for vertical flip [1]

[1]: https://lore.kernel.org/all/20240830062639.72947-1-umang.jain@ideasonboard.com/

There is some WIP to make the resolution freely configurable:
 - [x] Add support for a cropped mode (1944x1100)
 - [ ] Fix exposure timing limits when cropping
 - [ ] Add support for a cropped binned mode
 - [ ] Make it freely configurable (depends on new control for binning)

Available in the branch here:
https://github.com/jailuthra/linux/commits/imx335_binning

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Jai Luthra (4):
      media: imx335: Update the native pixel array width
      media: imx335: Update HBLANK range on mode change
      media: imx355: Use subdev active state
      media: imx335: Support 2x2 binning

Umang Jain (2):
      media: imx335: Rectify name of mode struct
      media: imx335: Support vertical flip

 drivers/media/i2c/imx335.c | 463 +++++++++++++++++++++++++++++++--------------
 1 file changed, 324 insertions(+), 139 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250606-imx335_binning-d43dce921b0a

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH 1/6] media: imx335: Rectify name of mode struct

[Jai Luthra]

From: Umang Jain <umang.jain@ideasonboard.com>

In commit 81495a59baeb ("media: imx335: Fix active area height
discrepency") the height for the mode struct was rectified to '1944'.
However, the name of mode struct is still reflecting to '1940'. Update
it.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 9b4db4cd4929caf83596e93d2ac3de2f54e892b0..33f92a3062c14251498cc65f14cc34cff6179f78 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -252,7 +252,7 @@ static const int imx335_tpg_val[] = {
 };
 
 /* Sensor mode registers */
-static const struct cci_reg_sequence mode_2592x1940_regs[] = {
+static const struct cci_reg_sequence mode_2592x1944_regs[] = {
 	{ IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
 	{ IMX335_REG_MASTER_MODE, 0x00 },
 	{ IMX335_REG_WINMODE, 0x04 },
@@ -416,8 +416,8 @@ static const struct imx335_mode supported_mode = {
 	.vblank_max = 133060,
 	.pclk = 396000000,
 	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
-		.regs = mode_2592x1940_regs,
+		.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
+		.regs = mode_2592x1944_regs,
 	},
 };
 

-- 
2.50.1

[PATCH 2/6] media: imx335: Support vertical flip

[Jai Luthra]

From: Umang Jain <umang.jain@ideasonboard.com>

Support vertical flip by setting REG_VREVERSE.
Additional registers also needs to be set per mode, according
to the readout direction (normal/inverted) as mentioned in the
data sheet.

Since the register IMX335_REG_AREA3_ST_ADR_1 is based on the
flip (and is set via vflip related registers), it has been
moved out of the 2592x1944 mode regs.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 71 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 33f92a3062c14251498cc65f14cc34cff6179f78..6369bdbd2b09ba1f89c143cdf6be061820f2d051 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -56,6 +56,9 @@
 #define IMX335_AGAIN_STEP		1
 #define IMX335_AGAIN_DEFAULT		0
 
+/* Vertical flip */
+#define IMX335_REG_VREVERSE		CCI_REG8(0x304f)
+
 #define IMX335_REG_TPG_TESTCLKEN	CCI_REG8(0x3148)
 
 #define IMX335_REG_INCLKSEL1		CCI_REG16_LE(0x314c)
@@ -155,6 +158,8 @@ static const char * const imx335_supply_name[] = {
  * @vblank_max: Maximum vertical blanking in lines
  * @pclk: Sensor pixel clock
  * @reg_list: Register list for sensor mode
+ * @vflip_normal: Register list vflip (normal readout)
+ * @vflip_inverted: Register list vflip (inverted readout)
  */
 struct imx335_mode {
 	u32 width;
@@ -166,6 +171,8 @@ struct imx335_mode {
 	u32 vblank_max;
 	u64 pclk;
 	struct imx335_reg_list reg_list;
+	struct imx335_reg_list vflip_normal;
+	struct imx335_reg_list vflip_inverted;
 };
 
 /**
@@ -183,6 +190,7 @@ struct imx335_mode {
  * @pclk_ctrl: Pointer to pixel clock control
  * @hblank_ctrl: Pointer to horizontal blanking control
  * @vblank_ctrl: Pointer to vertical blanking control
+ * @vflip: Pointer to vertical flip control
  * @exp_ctrl: Pointer to exposure control
  * @again_ctrl: Pointer to analog gain control
  * @vblank: Vertical blanking in lines
@@ -207,6 +215,7 @@ struct imx335 {
 	struct v4l2_ctrl *pclk_ctrl;
 	struct v4l2_ctrl *hblank_ctrl;
 	struct v4l2_ctrl *vblank_ctrl;
+	struct v4l2_ctrl *vflip;
 	struct {
 		struct v4l2_ctrl *exp_ctrl;
 		struct v4l2_ctrl *again_ctrl;
@@ -259,7 +268,6 @@ static const struct cci_reg_sequence mode_2592x1944_regs[] = {
 	{ IMX335_REG_HTRIMMING_START, 48 },
 	{ IMX335_REG_HNUM, 2592 },
 	{ IMX335_REG_Y_OUT_SIZE, 1944 },
-	{ IMX335_REG_AREA3_ST_ADR_1, 176 },
 	{ IMX335_REG_AREA3_WIDTH_1, 3928 },
 	{ IMX335_REG_OPB_SIZE_V, 0 },
 	{ IMX335_REG_XVS_XHS_DRV, 0x00 },
@@ -333,6 +341,26 @@ static const struct cci_reg_sequence mode_2592x1944_regs[] = {
 	{ CCI_REG8(0x3a00), 0x00 },
 };
 
+static const struct cci_reg_sequence mode_2592x1944_vflip_normal[] = {
+	{ IMX335_REG_AREA3_ST_ADR_1, 176 },
+
+	/* Undocumented V-Flip related registers on Page 55 of datasheet. */
+	{ CCI_REG8(0x3081), 0x02, },
+	{ CCI_REG8(0x3083), 0x02, },
+	{ CCI_REG16_LE(0x30b6), 0x00 },
+	{ CCI_REG16_LE(0x3116), 0x08 },
+};
+
+static const struct cci_reg_sequence mode_2592x1944_vflip_inverted[] = {
+	{ IMX335_REG_AREA3_ST_ADR_1, 4112 },
+
+	/* Undocumented V-Flip related registers on Page 55 of datasheet. */
+	{ CCI_REG8(0x3081), 0xfe, },
+	{ CCI_REG8(0x3083), 0xfe, },
+	{ CCI_REG16_LE(0x30b6), 0x1fa },
+	{ CCI_REG16_LE(0x3116), 0x002 },
+};
+
 static const struct cci_reg_sequence raw10_framefmt_regs[] = {
 	{ IMX335_REG_ADBIT, 0x00 },
 	{ IMX335_REG_MDBIT, 0x00 },
@@ -419,6 +447,14 @@ static const struct imx335_mode supported_mode = {
 		.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
 		.regs = mode_2592x1944_regs,
 	},
+	.vflip_normal = {
+		.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
+		.regs = mode_2592x1944_vflip_normal,
+	},
+	.vflip_inverted = {
+		.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
+		.regs = mode_2592x1944_vflip_inverted,
+	},
 };
 
 /**
@@ -492,6 +528,26 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
 	return ret;
 }
 
+static int imx335_update_vertical_flip(struct imx335 *imx335, u32 vflip)
+{
+	int ret = 0;
+
+	if (vflip)
+		cci_multi_reg_write(imx335->cci,
+				    imx335->cur_mode->vflip_inverted.regs,
+				    imx335->cur_mode->vflip_inverted.num_of_regs,
+				    &ret);
+	else
+		cci_multi_reg_write(imx335->cci,
+				    imx335->cur_mode->vflip_normal.regs,
+				    imx335->cur_mode->vflip_normal.num_of_regs,
+				    &ret);
+	if (ret)
+		return ret;
+
+	return cci_write(imx335->cci, IMX335_REG_VREVERSE, vflip, NULL);
+}
+
 static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
 {
 	int ret = 0;
@@ -593,6 +649,10 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = imx335_update_exp_gain(imx335, exposure, analog_gain);
 
+		break;
+	case V4L2_CID_VFLIP:
+		ret = imx335_update_vertical_flip(imx335, ctrl->val);
+
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		ret = imx335_update_test_pattern(imx335, ctrl->val);
@@ -1176,7 +1236,7 @@ static int imx335_init_controls(struct imx335 *imx335)
 		return ret;
 
 	/* v4l2_fwnode_device_properties can add two more controls */
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -1211,6 +1271,13 @@ static int imx335_init_controls(struct imx335 *imx335)
 
 	v4l2_ctrl_cluster(2, &imx335->exp_ctrl);
 
+	imx335->vflip = v4l2_ctrl_new_std(ctrl_hdlr,
+					  &imx335_ctrl_ops,
+					  V4L2_CID_VFLIP,
+					  0, 1, 1, 0);
+	if (imx335->vflip)
+		imx335->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
 	imx335->vblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 						&imx335_ctrl_ops,
 						V4L2_CID_VBLANK,

-- 
2.50.1

[PATCH 3/6] media: imx335: Update the native pixel array width

[Jai Luthra]

The sensor datasheet reports actual total number of pixels as 2696x2044.

This becomes important for supporting 2x2 binning modes that can go
beyond the current maximum pixel array width set here.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 6369bdbd2b09ba1f89c143cdf6be061820f2d051..dbf2db4bf9cbfd792ff5865264c6f465eb79a43b 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -124,10 +124,10 @@
 #define IMX335_NUM_DATA_LANES	4
 
 /* IMX335 native and active pixel array size. */
-#define IMX335_NATIVE_WIDTH		2616U
-#define IMX335_NATIVE_HEIGHT		1964U
-#define IMX335_PIXEL_ARRAY_LEFT		12U
-#define IMX335_PIXEL_ARRAY_TOP		12U
+#define IMX335_NATIVE_WIDTH		2696U
+#define IMX335_NATIVE_HEIGHT		2044U
+#define IMX335_PIXEL_ARRAY_LEFT		52U
+#define IMX335_PIXEL_ARRAY_TOP		50U
 #define IMX335_PIXEL_ARRAY_WIDTH	2592U
 #define IMX335_PIXEL_ARRAY_HEIGHT	1944U
 

-- 
2.50.1

Re: [PATCH 3/6] media: imx335: Update the native pixel array width

[Kieran Bingham]

Quoting Jai Luthra (2025-08-13 08:20:34)
> The sensor datasheet reports actual total number of pixels as 2696x2044.

Err ...

Page 2 of the IMX335LQN-D datasheet I have says:

Total number of pixels: 2704 (H) x 2104 (V) approx 5.69 M pixels
Number of Effective Pixels: 2616 (H) x 1964 (V) approx 5.14 M pixels
Number of Active Pixels: 2616 (H) x 1964 (V) approx 5.04 M pixels

Where does 2696x2044 come from ?


Argh - then on page 8 - indeed it says
Total Number of pixels 2696(H) x 2044(V) = 5.51M


In imx283.c I've moved to defining these windows as a v4l2_rect. I find
that's a nicer way to convey the rectangles specifically instead of
through #defines and then they can be directly used to report crop
rectangles:

i.e.:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/drivers/media/i2c/imx283.c:

/* IMX283 native and active pixel array size. */
static const struct v4l2_rect imx283_native_area = {
	.top = 0,
	.left = 0,
	.width = 5592,
	.height = 3710,
};

static const struct v4l2_rect imx283_active_area = {
	.top = 40,
	.left = 108,
	.width = 5472,
	.height = 3648,
};

Not required - but just an idea (that obviously I like :D)


> 
> This becomes important for supporting 2x2 binning modes that can go
> beyond the current maximum pixel array width set here.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 6369bdbd2b09ba1f89c143cdf6be061820f2d051..dbf2db4bf9cbfd792ff5865264c6f465eb79a43b 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -124,10 +124,10 @@
>  #define IMX335_NUM_DATA_LANES  4
>  
>  /* IMX335 native and active pixel array size. */
> -#define IMX335_NATIVE_WIDTH            2616U
> -#define IMX335_NATIVE_HEIGHT           1964U
> -#define IMX335_PIXEL_ARRAY_LEFT                12U
> -#define IMX335_PIXEL_ARRAY_TOP         12U
> +#define IMX335_NATIVE_WIDTH            2696U

The all scan mode on page 56 doesn't show these at all.
Just 12 + 2592 + 12

But I think that's the datasheet being inconsistent/restrictive about
what it says an all scan mode could be.

It would be interesting to make a 'super resolution' output mode that
can transmit every pixel possible but not required for this series
development just for fun tests.

I'm torn here - as the datasheet changes it's points of reference to
make it's "all scan mode" essentially start at 0 which is clearly not
correct against the 'native' positions.

So ... I think I'm just going to say I think we don't believe the
datasheet as we *know* there are more pixels and we are using them so :


> +#define IMX335_NATIVE_HEIGHT           2044U
> +#define IMX335_PIXEL_ARRAY_LEFT                52U
> +#define IMX335_PIXEL_ARRAY_TOP         50U

I see you have taken the '80' extra pixels on both width and height and
divided half before and half after centering the window. I have no
information to say if it's position is otherwise so I think this is all
we can do:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  #define IMX335_PIXEL_ARRAY_WIDTH       2592U
>  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
>  
> 
> -- 
> 2.50.1
>

Re: [PATCH 3/6] media: imx335: Update the native pixel array width

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 4480 bytes --]

Quoting Kieran Bingham (2025-08-13 15:45:59)
> Quoting Jai Luthra (2025-08-13 08:20:34)
> > The sensor datasheet reports actual total number of pixels as 2696x2044.
> 
> Err ...
> 
> Page 2 of the IMX335LQN-D datasheet I have says:
> 
> Total number of pixels: 2704 (H) x 2104 (V) approx 5.69 M pixels
> Number of Effective Pixels: 2616 (H) x 1964 (V) approx 5.14 M pixels
> Number of Active Pixels: 2616 (H) x 1964 (V) approx 5.04 M pixels
> 
> Where does 2696x2044 come from ?
> 
> 
> Argh - then on page 8 - indeed it says
> Total Number of pixels 2696(H) x 2044(V) = 5.51M
> 

Yeah, I don't know which one to use, but the page 8 seems "safer".

> 
> In imx283.c I've moved to defining these windows as a v4l2_rect. I find
> that's a nicer way to convey the rectangles specifically instead of
> through #defines and then they can be directly used to report crop
> rectangles:
> 
> i.e.:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/drivers/media/i2c/imx283.c:
> 
> /* IMX283 native and active pixel array size. */
> static const struct v4l2_rect imx283_native_area = {
>         .top = 0,
>         .left = 0,
>         .width = 5592,
>         .height = 3710,
> };
> 
> static const struct v4l2_rect imx283_active_area = {
>         .top = 40,
>         .left = 108,
>         .width = 5472,
>         .height = 3648,
> };
> 
> Not required - but just an idea (that obviously I like :D)
> 

Ah that is indeed quite nice, will do the same in v2.

> 
> > 
> > This becomes important for supporting 2x2 binning modes that can go
> > beyond the current maximum pixel array width set here.
> > 
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index 6369bdbd2b09ba1f89c143cdf6be061820f2d051..dbf2db4bf9cbfd792ff5865264c6f465eb79a43b 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -124,10 +124,10 @@
> >  #define IMX335_NUM_DATA_LANES  4
> >  
> >  /* IMX335 native and active pixel array size. */
> > -#define IMX335_NATIVE_WIDTH            2616U
> > -#define IMX335_NATIVE_HEIGHT           1964U
> > -#define IMX335_PIXEL_ARRAY_LEFT                12U
> > -#define IMX335_PIXEL_ARRAY_TOP         12U
> > +#define IMX335_NATIVE_WIDTH            2696U
> 
> The all scan mode on page 56 doesn't show these at all.
> Just 12 + 2592 + 12
> 
> But I think that's the datasheet being inconsistent/restrictive about
> what it says an all scan mode could be.
> 
> It would be interesting to make a 'super resolution' output mode that
> can transmit every pixel possible but not required for this series
> development just for fun tests.
> 

Yeah I think datasheet is being quite restrictive, and even the +12 on both
sides for "color processing" makes it very difficult to parse what the
actual resolution is on the wire, and what they expect to be used post ISP
processing.

The all-pixel scan mode table given in the datasheet is actually different
from what we actually use (we use WINMODE=4 i.e. cropped) for the 2592x1944
mode in driver, as the datasheet settings would give us 2592+24 = 2616
pixels per line.

I didn't want to change the existing mode in this series though. Making the
driver freely-configurable will fix all of this (hopefully).

> I'm torn here - as the datasheet changes it's points of reference to
> make it's "all scan mode" essentially start at 0 which is clearly not
> correct against the 'native' positions.
> 
> So ... I think I'm just going to say I think we don't believe the
> datasheet as we *know* there are more pixels and we are using them so :
> 
> 
> > +#define IMX335_NATIVE_HEIGHT           2044U
> > +#define IMX335_PIXEL_ARRAY_LEFT                52U
> > +#define IMX335_PIXEL_ARRAY_TOP         50U
> 
> I see you have taken the '80' extra pixels on both width and height and
> divided half before and half after centering the window. I have no
> information to say if it's position is otherwise so I think this is all
> we can do:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >  #define IMX335_PIXEL_ARRAY_WIDTH       2592U
> >  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
> >  
> > 
> > -- 
> > 2.50.1
> >

Thanks,
Jai

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEETeDYGOXVdejUWq/FQ96R+SSacUUFAmikI/wACgkQQ96R+SSa
cUUhrg/8CaJH8DpYJ0aYicgshRm6ZP3Isy9DSxNCy4UC9iKY3+/mNu1N2gewyP3M
zRYujQIhE74IAxh/WaSyn/UdEiyJ2NIzu5fhmAGkN9jEV5ZRhTSW0pAum+WzpMsv
PRgF4dLnkxWon6xoVoxGIIV7kU+dyfWhLy6KO8ES5mWbR0grtx5z7MbNSyeNdXsR
QXgTRUwhSeIfR759Juh5/ZueubFPnC7eYyNjNXgjoRNGHnIDCgK6kze3ZoVUCh4Y
gXniEfuzZNGm87N6TY4HkSUtPyowemUeV9JiwXzMS1hSQ1AqFvYcdYhO1i/CS2Sj
u/YOz4SnON+fB8YVihSoSas5xGWdfmc/qyEI4DpqwrfYH5t0TQJeTruEl4X7EzSc
y4an+wV6o+UzaW12Z58qQLQEHHYYREViNNDnoJaPjApr7NbPWGXE7BzlK+a89Ym/
nI907+jNB7fjCF+HNYQO0lnjbIFDA+l4yzbNe6RZZBN5u19KYPo67Goa7nVQvaPH
64HzZhCR6Ir8K8oIr5oXWWfOQJsUVCLHeDe2jNdrm2mIV2O2NtNrcEd43cucw0ef
bhakI8+TqLup9W/AckBQhR+dQURg/GvZtF/YoCeaInzSXa4ebbn8MIpa76n0jC99
HSn1aHO9Ry3unDS8JOng5bzByphjGDVEEJ+CmLV2DMHT9+o0PVE=
=14ti
-----END PGP SIGNATURE-----

[PATCH 4/6] media: imx335: Update HBLANK range on mode change

[Jai Luthra]

While switching modes, updating to a different value of HBLANK isn't
sufficient, as this is a read-only control with a single allowed value,
and thus hblank_min == hblank_max == hblank of the default mode.

So to correctly update the user-facing value of the HBLANK parameter,
which is necessary for correct framerate calculation, update the whole
range when switching modes.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index dbf2db4bf9cbfd792ff5865264c6f465eb79a43b..c61a6952f828fd8b945746ae2f53f5517e98c410 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -485,7 +485,8 @@ static int imx335_update_controls(struct imx335 *imx335,
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(imx335->hblank_ctrl, mode->hblank);
+	ret = __v4l2_ctrl_modify_range(imx335->hblank_ctrl, mode->hblank,
+				       mode->hblank, 1, mode->hblank);
 	if (ret)
 		return ret;
 

-- 
2.50.1

Re: [PATCH 4/6] media: imx335: Update HBLANK range on mode change

[Kieran Bingham]

Quoting Jai Luthra (2025-08-13 08:20:35)
> While switching modes, updating to a different value of HBLANK isn't
> sufficient, as this is a read-only control with a single allowed value,
> and thus hblank_min == hblank_max == hblank of the default mode.
> 
> So to correctly update the user-facing value of the HBLANK parameter,
> which is necessary for correct framerate calculation, update the whole
> range when switching modes.
> 

Seems reasonable to me.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index dbf2db4bf9cbfd792ff5865264c6f465eb79a43b..c61a6952f828fd8b945746ae2f53f5517e98c410 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -485,7 +485,8 @@ static int imx335_update_controls(struct imx335 *imx335,
>         if (ret)
>                 return ret;
>  
> -       ret = __v4l2_ctrl_s_ctrl(imx335->hblank_ctrl, mode->hblank);
> +       ret = __v4l2_ctrl_modify_range(imx335->hblank_ctrl, mode->hblank,
> +                                      mode->hblank, 1, mode->hblank);
>         if (ret)
>                 return ret;
>  
> 
> -- 
> 2.50.1
>

[PATCH 5/6] media: imx355: Use subdev active state

[Jai Luthra]

Port the driver to use the subdev active state. This simplifies locking,
and makes it easier to support different crop sizes for binned modes, by
storing the crop rectangle inside the subdev state.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 125 +++++++++++++++------------------------------
 1 file changed, 41 insertions(+), 84 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index c61a6952f828fd8b945746ae2f53f5517e98c410..df1535927f481de3a0d043ac9be24b9336ea8f7f 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -196,7 +196,6 @@ struct imx335_mode {
  * @vblank: Vertical blanking in lines
  * @lane_mode: Mode for number of connected data lanes
  * @cur_mode: Pointer to current selected sensor mode
- * @mutex: Mutex for serializing sensor controls
  * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
  * @cur_mbus_code: Currently selected media bus format code
  */
@@ -223,7 +222,6 @@ struct imx335 {
 	u32 vblank;
 	u32 lane_mode;
 	const struct imx335_mode *cur_mode;
-	struct mutex mutex;
 	unsigned long link_freq_bitmap;
 	u32 cur_mbus_code;
 };
@@ -758,36 +756,6 @@ static void imx335_fill_pad_format(struct imx335 *imx335,
 	fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
-/**
- * imx335_get_pad_format() - Get subdevice pad format
- * @sd: pointer to imx335 V4L2 sub-device structure
- * @sd_state: V4L2 sub-device configuration
- * @fmt: V4L2 sub-device format need to be set
- *
- * Return: 0 if successful, error code otherwise.
- */
-static int imx335_get_pad_format(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
-				 struct v4l2_subdev_format *fmt)
-{
-	struct imx335 *imx335 = to_imx335(sd);
-
-	mutex_lock(&imx335->mutex);
-
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *framefmt;
-
-		framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
-		fmt->format = *framefmt;
-	} else {
-		imx335_fill_pad_format(imx335, imx335->cur_mode, fmt);
-	}
-
-	mutex_unlock(&imx335->mutex);
-
-	return 0;
-}
-
 /**
  * imx335_set_pad_format() - Set subdevice pad format
  * @sd: pointer to imx335 V4L2 sub-device structure
@@ -801,12 +769,12 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_format *fmt)
 {
 	struct imx335 *imx335 = to_imx335(sd);
+	struct v4l2_mbus_framefmt *format;
 	const struct imx335_mode *mode;
 	int i, ret = 0;
 
-	mutex_lock(&imx335->mutex);
-
 	mode = &supported_mode;
+
 	for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
 		if (imx335_mbus_codes[i] == fmt->format.code)
 			imx335->cur_mbus_code = imx335_mbus_codes[i];
@@ -814,19 +782,15 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
 
 	imx335_fill_pad_format(imx335, mode, fmt);
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *framefmt;
+	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
+	*format = fmt->format;
 
-		framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
-		*framefmt = fmt->format;
-	} else {
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ret = imx335_update_controls(imx335, mode);
 		if (!ret)
 			imx335->cur_mode = mode;
 	}
 
-	mutex_unlock(&imx335->mutex);
-
 	return ret;
 }
 
@@ -846,12 +810,10 @@ static int imx335_init_state(struct v4l2_subdev *sd,
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
 	imx335_fill_pad_format(imx335, &supported_mode, &fmt);
 
-	mutex_lock(&imx335->mutex);
 	__v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
 				 __fls(imx335->link_freq_bitmap),
 				 ~(imx335->link_freq_bitmap),
 				 __ffs(imx335->link_freq_bitmap));
-	mutex_unlock(&imx335->mutex);
 
 	return imx335_set_pad_format(sd, sd_state, &fmt);
 }
@@ -919,13 +881,17 @@ static int imx335_start_streaming(struct imx335 *imx335)
 	const struct imx335_reg_list *reg_list;
 	int ret;
 
+	ret = pm_runtime_resume_and_get(imx335->dev);
+	if (ret < 0)
+		return ret;
+
 	/* Setup PLL */
 	reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
 	ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
 				  reg_list->num_of_regs, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "%s failed to set plls\n", __func__);
-		return ret;
+		goto err_rpm_put;
 	}
 
 	/* Write sensor mode registers */
@@ -934,27 +900,27 @@ static int imx335_start_streaming(struct imx335 *imx335)
 				  reg_list->num_of_regs, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "fail to write initial registers\n");
-		return ret;
+		goto err_rpm_put;
 	}
 
 	ret = imx335_set_framefmt(imx335);
 	if (ret) {
 		dev_err(imx335->dev, "%s failed to set frame format: %d\n",
 			__func__, ret);
-		return ret;
+		goto err_rpm_put;
 	}
 
 	/* Configure lanes */
 	ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
 			imx335->lane_mode, NULL);
 	if (ret)
-		return ret;
+		goto err_rpm_put;
 
 	/* Setup handler will write actual exposure and gain */
 	ret =  __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
 	if (ret) {
 		dev_err(imx335->dev, "fail to setup handler\n");
-		return ret;
+		goto err_rpm_put;
 	}
 
 	/* Start streaming */
@@ -962,25 +928,29 @@ static int imx335_start_streaming(struct imx335 *imx335)
 			IMX335_MODE_STREAMING, NULL);
 	if (ret) {
 		dev_err(imx335->dev, "fail to start streaming\n");
-		return ret;
+		goto err_rpm_put;
 	}
 
 	/* Initial regulator stabilization period */
 	usleep_range(18000, 20000);
 
 	return 0;
+
+err_rpm_put:
+	pm_runtime_put(imx335->dev);
+
+	return ret;
 }
 
 /**
  * imx335_stop_streaming() - Stop sensor stream
  * @imx335: pointer to imx335 device
- *
- * Return: 0 if successful, error code otherwise.
  */
-static int imx335_stop_streaming(struct imx335 *imx335)
+static void imx335_stop_streaming(struct imx335 *imx335)
 {
-	return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
-			 IMX335_MODE_STANDBY, NULL);
+	cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
+		  IMX335_MODE_STANDBY, NULL);
+	pm_runtime_put(imx335->dev);
 }
 
 /**
@@ -993,31 +963,18 @@ static int imx335_stop_streaming(struct imx335 *imx335)
 static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct imx335 *imx335 = to_imx335(sd);
-	int ret;
+	struct v4l2_subdev_state *state;
+	int ret = 0;
 
-	mutex_lock(&imx335->mutex);
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable) {
-		ret = pm_runtime_resume_and_get(imx335->dev);
-		if (ret)
-			goto error_unlock;
-
 		ret = imx335_start_streaming(imx335);
-		if (ret)
-			goto error_power_off;
 	} else {
 		imx335_stop_streaming(imx335);
-		pm_runtime_put(imx335->dev);
 	}
 
-	mutex_unlock(&imx335->mutex);
-
-	return 0;
-
-error_power_off:
-	pm_runtime_put(imx335->dev);
-error_unlock:
-	mutex_unlock(&imx335->mutex);
+	v4l2_subdev_unlock_state(state);
 
 	return ret;
 }
@@ -1146,7 +1103,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
 	.enum_frame_size = imx335_enum_frame_size,
 	.get_selection = imx335_get_selection,
 	.set_selection = imx335_get_selection,
-	.get_fmt = imx335_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx335_set_pad_format,
 };
 
@@ -1241,9 +1198,6 @@ static int imx335_init_controls(struct imx335 *imx335)
 	if (ret)
 		return ret;
 
-	/* Serialize controls with sensor device */
-	ctrl_hdlr->lock = &imx335->mutex;
-
 	/* Initialize exposure and gain */
 	lpfr = mode->vblank + mode->height;
 	imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
@@ -1363,12 +1317,10 @@ static int imx335_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	mutex_init(&imx335->mutex);
-
 	ret = imx335_power_on(imx335->dev);
 	if (ret) {
 		dev_err(imx335->dev, "failed to power-on the sensor\n");
-		goto error_mutex_destroy;
+		return ret;
 	}
 
 	/* Check module identity */
@@ -1401,11 +1353,18 @@ static int imx335_probe(struct i2c_client *client)
 		goto error_handler_free;
 	}
 
+	imx335->sd.state_lock = imx335->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&imx335->sd);
+	if (ret < 0) {
+		dev_err(imx335->dev, "subdev init error\n");
+		goto error_media_entity;
+	}
+
 	ret = v4l2_async_register_subdev_sensor(&imx335->sd);
 	if (ret < 0) {
 		dev_err(imx335->dev,
 			"failed to register async subdev: %d\n", ret);
-		goto error_media_entity;
+		goto error_subdev_cleanup;
 	}
 
 	pm_runtime_set_active(imx335->dev);
@@ -1414,14 +1373,14 @@ static int imx335_probe(struct i2c_client *client)
 
 	return 0;
 
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&imx335->sd);
 error_media_entity:
 	media_entity_cleanup(&imx335->sd.entity);
 error_handler_free:
 	v4l2_ctrl_handler_free(imx335->sd.ctrl_handler);
 error_power_off:
 	imx335_power_off(imx335->dev);
-error_mutex_destroy:
-	mutex_destroy(&imx335->mutex);
 
 	return ret;
 }
@@ -1435,9 +1394,9 @@ static int imx335_probe(struct i2c_client *client)
 static void imx335_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct imx335 *imx335 = to_imx335(sd);
 
 	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 
@@ -1445,8 +1404,6 @@ static void imx335_remove(struct i2c_client *client)
 	if (!pm_runtime_status_suspended(&client->dev))
 		imx335_power_off(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-
-	mutex_destroy(&imx335->mutex);
 }
 
 static const struct dev_pm_ops imx335_pm_ops = {

-- 
2.50.1

Re: [PATCH 5/6] media: imx355: Use subdev active state

[Kieran Bingham]

Quoting Jai Luthra (2025-08-13 08:20:36)
> Port the driver to use the subdev active state. This simplifies locking,
> and makes it easier to support different crop sizes for binned modes, by
> storing the crop rectangle inside the subdev state.
> 

Has this patch inadverntently squashed subdev active state and some
runtime power / start/stop stream changes together?

It looks like there's some interesting additional development in there
but I don't think that's related to subdev active state ?

--
Kieran

> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 125 +++++++++++++++------------------------------
>  1 file changed, 41 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index c61a6952f828fd8b945746ae2f53f5517e98c410..df1535927f481de3a0d043ac9be24b9336ea8f7f 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -196,7 +196,6 @@ struct imx335_mode {
>   * @vblank: Vertical blanking in lines
>   * @lane_mode: Mode for number of connected data lanes
>   * @cur_mode: Pointer to current selected sensor mode
> - * @mutex: Mutex for serializing sensor controls
>   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
>   * @cur_mbus_code: Currently selected media bus format code
>   */
> @@ -223,7 +222,6 @@ struct imx335 {
>         u32 vblank;
>         u32 lane_mode;
>         const struct imx335_mode *cur_mode;
> -       struct mutex mutex;
>         unsigned long link_freq_bitmap;
>         u32 cur_mbus_code;
>  };
> @@ -758,36 +756,6 @@ static void imx335_fill_pad_format(struct imx335 *imx335,
>         fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
>  }
>  
> -/**
> - * imx335_get_pad_format() - Get subdevice pad format
> - * @sd: pointer to imx335 V4L2 sub-device structure
> - * @sd_state: V4L2 sub-device configuration
> - * @fmt: V4L2 sub-device format need to be set
> - *
> - * Return: 0 if successful, error code otherwise.
> - */
> -static int imx335_get_pad_format(struct v4l2_subdev *sd,
> -                                struct v4l2_subdev_state *sd_state,
> -                                struct v4l2_subdev_format *fmt)
> -{
> -       struct imx335 *imx335 = to_imx335(sd);
> -
> -       mutex_lock(&imx335->mutex);
> -
> -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               struct v4l2_mbus_framefmt *framefmt;
> -
> -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> -               fmt->format = *framefmt;
> -       } else {
> -               imx335_fill_pad_format(imx335, imx335->cur_mode, fmt);
> -       }
> -
> -       mutex_unlock(&imx335->mutex);
> -
> -       return 0;
> -}
> -
>  /**
>   * imx335_set_pad_format() - Set subdevice pad format
>   * @sd: pointer to imx335 V4L2 sub-device structure
> @@ -801,12 +769,12 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_format *fmt)
>  {
>         struct imx335 *imx335 = to_imx335(sd);
> +       struct v4l2_mbus_framefmt *format;
>         const struct imx335_mode *mode;
>         int i, ret = 0;
>  
> -       mutex_lock(&imx335->mutex);
> -
>         mode = &supported_mode;
> +
>         for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
>                 if (imx335_mbus_codes[i] == fmt->format.code)
>                         imx335->cur_mbus_code = imx335_mbus_codes[i];
> @@ -814,19 +782,15 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>  
>         imx335_fill_pad_format(imx335, mode, fmt);
>  
> -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -               struct v4l2_mbus_framefmt *framefmt;
> +       format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> +       *format = fmt->format;
>  
> -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> -               *framefmt = fmt->format;
> -       } else {
> +       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                 ret = imx335_update_controls(imx335, mode);
>                 if (!ret)
>                         imx335->cur_mode = mode;
>         }
>  
> -       mutex_unlock(&imx335->mutex);
> -
>         return ret;
>  }
>  
> @@ -846,12 +810,10 @@ static int imx335_init_state(struct v4l2_subdev *sd,
>         fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>         imx335_fill_pad_format(imx335, &supported_mode, &fmt);
>  
> -       mutex_lock(&imx335->mutex);
>         __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
>                                  __fls(imx335->link_freq_bitmap),
>                                  ~(imx335->link_freq_bitmap),
>                                  __ffs(imx335->link_freq_bitmap));
> -       mutex_unlock(&imx335->mutex);
>  
>         return imx335_set_pad_format(sd, sd_state, &fmt);
>  }
> @@ -919,13 +881,17 @@ static int imx335_start_streaming(struct imx335 *imx335)
>         const struct imx335_reg_list *reg_list;
>         int ret;
>  
> +       ret = pm_runtime_resume_and_get(imx335->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         /* Setup PLL */
>         reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
>         ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
>                                   reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set plls\n", __func__);
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Write sensor mode registers */
> @@ -934,27 +900,27 @@ static int imx335_start_streaming(struct imx335 *imx335)
>                                   reg_list->num_of_regs, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to write initial registers\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         ret = imx335_set_framefmt(imx335);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set frame format: %d\n",
>                         __func__, ret);
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Configure lanes */
>         ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
>                         imx335->lane_mode, NULL);
>         if (ret)
> -               return ret;
> +               goto err_rpm_put;
>  
>         /* Setup handler will write actual exposure and gain */
>         ret =  __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to setup handler\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Start streaming */
> @@ -962,25 +928,29 @@ static int imx335_start_streaming(struct imx335 *imx335)
>                         IMX335_MODE_STREAMING, NULL);
>         if (ret) {
>                 dev_err(imx335->dev, "fail to start streaming\n");
> -               return ret;
> +               goto err_rpm_put;
>         }
>  
>         /* Initial regulator stabilization period */
>         usleep_range(18000, 20000);
>  
>         return 0;
> +
> +err_rpm_put:
> +       pm_runtime_put(imx335->dev);
> +
> +       return ret;
>  }
>  
>  /**
>   * imx335_stop_streaming() - Stop sensor stream
>   * @imx335: pointer to imx335 device
> - *
> - * Return: 0 if successful, error code otherwise.
>   */
> -static int imx335_stop_streaming(struct imx335 *imx335)
> +static void imx335_stop_streaming(struct imx335 *imx335)
>  {
> -       return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> -                        IMX335_MODE_STANDBY, NULL);
> +       cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> +                 IMX335_MODE_STANDBY, NULL);
> +       pm_runtime_put(imx335->dev);
>  }
>  
>  /**
> @@ -993,31 +963,18 @@ static int imx335_stop_streaming(struct imx335 *imx335)
>  static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>         struct imx335 *imx335 = to_imx335(sd);
> -       int ret;
> +       struct v4l2_subdev_state *state;
> +       int ret = 0;
>  
> -       mutex_lock(&imx335->mutex);
> +       state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>         if (enable) {
> -               ret = pm_runtime_resume_and_get(imx335->dev);
> -               if (ret)
> -                       goto error_unlock;
> -
>                 ret = imx335_start_streaming(imx335);
> -               if (ret)
> -                       goto error_power_off;
>         } else {
>                 imx335_stop_streaming(imx335);
> -               pm_runtime_put(imx335->dev);
>         }
>  
> -       mutex_unlock(&imx335->mutex);
> -
> -       return 0;
> -
> -error_power_off:
> -       pm_runtime_put(imx335->dev);
> -error_unlock:
> -       mutex_unlock(&imx335->mutex);
> +       v4l2_subdev_unlock_state(state);
>  
>         return ret;
>  }
> @@ -1146,7 +1103,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
>         .enum_frame_size = imx335_enum_frame_size,
>         .get_selection = imx335_get_selection,
>         .set_selection = imx335_get_selection,
> -       .get_fmt = imx335_get_pad_format,
> +       .get_fmt = v4l2_subdev_get_fmt,
>         .set_fmt = imx335_set_pad_format,
>  };
>  
> @@ -1241,9 +1198,6 @@ static int imx335_init_controls(struct imx335 *imx335)
>         if (ret)
>                 return ret;
>  
> -       /* Serialize controls with sensor device */
> -       ctrl_hdlr->lock = &imx335->mutex;
> -
>         /* Initialize exposure and gain */
>         lpfr = mode->vblank + mode->height;
>         imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> @@ -1363,12 +1317,10 @@ static int imx335_probe(struct i2c_client *client)
>                 return ret;
>         }
>  
> -       mutex_init(&imx335->mutex);
> -
>         ret = imx335_power_on(imx335->dev);
>         if (ret) {
>                 dev_err(imx335->dev, "failed to power-on the sensor\n");
> -               goto error_mutex_destroy;
> +               return ret;
>         }
>  
>         /* Check module identity */
> @@ -1401,11 +1353,18 @@ static int imx335_probe(struct i2c_client *client)
>                 goto error_handler_free;
>         }
>  
> +       imx335->sd.state_lock = imx335->ctrl_handler.lock;
> +       ret = v4l2_subdev_init_finalize(&imx335->sd);
> +       if (ret < 0) {
> +               dev_err(imx335->dev, "subdev init error\n");
> +               goto error_media_entity;
> +       }
> +
>         ret = v4l2_async_register_subdev_sensor(&imx335->sd);
>         if (ret < 0) {
>                 dev_err(imx335->dev,
>                         "failed to register async subdev: %d\n", ret);
> -               goto error_media_entity;
> +               goto error_subdev_cleanup;
>         }
>  
>         pm_runtime_set_active(imx335->dev);
> @@ -1414,14 +1373,14 @@ static int imx335_probe(struct i2c_client *client)
>  
>         return 0;
>  
> +error_subdev_cleanup:
> +       v4l2_subdev_cleanup(&imx335->sd);
>  error_media_entity:
>         media_entity_cleanup(&imx335->sd.entity);
>  error_handler_free:
>         v4l2_ctrl_handler_free(imx335->sd.ctrl_handler);
>  error_power_off:
>         imx335_power_off(imx335->dev);
> -error_mutex_destroy:
> -       mutex_destroy(&imx335->mutex);
>  
>         return ret;
>  }
> @@ -1435,9 +1394,9 @@ static int imx335_probe(struct i2c_client *client)
>  static void imx335_remove(struct i2c_client *client)
>  {
>         struct v4l2_subdev *sd = i2c_get_clientdata(client);
> -       struct imx335 *imx335 = to_imx335(sd);
>  
>         v4l2_async_unregister_subdev(sd);
> +       v4l2_subdev_cleanup(sd);
>         media_entity_cleanup(&sd->entity);
>         v4l2_ctrl_handler_free(sd->ctrl_handler);
>  
> @@ -1445,8 +1404,6 @@ static void imx335_remove(struct i2c_client *client)
>         if (!pm_runtime_status_suspended(&client->dev))
>                 imx335_power_off(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> -
> -       mutex_destroy(&imx335->mutex);
>  }
>  
>  static const struct dev_pm_ops imx335_pm_ops = {
> 
> -- 
> 2.50.1
>

Re: [PATCH 5/6] media: imx355: Use subdev active state

[Jai Luthra]

Quoting Kieran Bingham (2025-08-13 15:54:56)
> Quoting Jai Luthra (2025-08-13 08:20:36)
> > Port the driver to use the subdev active state. This simplifies locking,
> > and makes it easier to support different crop sizes for binned modes, by
> > storing the crop rectangle inside the subdev state.
> > 
> 
> Has this patch inadverntently squashed subdev active state and some
> runtime power / start/stop stream changes together?
> 
> It looks like there's some interesting additional development in there
> but I don't think that's related to subdev active state ?
> 

Oops, good catch, I will split the runtime PM related changes in a separate
patch in v2 of this series.

> --
> Kieran
> 
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 125 +++++++++++++++------------------------------
> >  1 file changed, 41 insertions(+), 84 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index c61a6952f828fd8b945746ae2f53f5517e98c410..df1535927f481de3a0d043ac9be24b9336ea8f7f 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -196,7 +196,6 @@ struct imx335_mode {
> >   * @vblank: Vertical blanking in lines
> >   * @lane_mode: Mode for number of connected data lanes
> >   * @cur_mode: Pointer to current selected sensor mode
> > - * @mutex: Mutex for serializing sensor controls
> >   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> >   * @cur_mbus_code: Currently selected media bus format code
> >   */
> > @@ -223,7 +222,6 @@ struct imx335 {
> >         u32 vblank;
> >         u32 lane_mode;
> >         const struct imx335_mode *cur_mode;
> > -       struct mutex mutex;
> >         unsigned long link_freq_bitmap;
> >         u32 cur_mbus_code;
> >  };
> > @@ -758,36 +756,6 @@ static void imx335_fill_pad_format(struct imx335 *imx335,
> >         fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> >  }
> >  
> > -/**
> > - * imx335_get_pad_format() - Get subdevice pad format
> > - * @sd: pointer to imx335 V4L2 sub-device structure
> > - * @sd_state: V4L2 sub-device configuration
> > - * @fmt: V4L2 sub-device format need to be set
> > - *
> > - * Return: 0 if successful, error code otherwise.
> > - */
> > -static int imx335_get_pad_format(struct v4l2_subdev *sd,
> > -                                struct v4l2_subdev_state *sd_state,
> > -                                struct v4l2_subdev_format *fmt)
> > -{
> > -       struct imx335 *imx335 = to_imx335(sd);
> > -
> > -       mutex_lock(&imx335->mutex);
> > -
> > -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -               struct v4l2_mbus_framefmt *framefmt;
> > -
> > -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> > -               fmt->format = *framefmt;
> > -       } else {
> > -               imx335_fill_pad_format(imx335, imx335->cur_mode, fmt);
> > -       }
> > -
> > -       mutex_unlock(&imx335->mutex);
> > -
> > -       return 0;
> > -}
> > -
> >  /**
> >   * imx335_set_pad_format() - Set subdevice pad format
> >   * @sd: pointer to imx335 V4L2 sub-device structure
> > @@ -801,12 +769,12 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
> >                                  struct v4l2_subdev_format *fmt)
> >  {
> >         struct imx335 *imx335 = to_imx335(sd);
> > +       struct v4l2_mbus_framefmt *format;
> >         const struct imx335_mode *mode;
> >         int i, ret = 0;
> >  
> > -       mutex_lock(&imx335->mutex);
> > -
> >         mode = &supported_mode;
> > +
> >         for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
> >                 if (imx335_mbus_codes[i] == fmt->format.code)
> >                         imx335->cur_mbus_code = imx335_mbus_codes[i];
> > @@ -814,19 +782,15 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
> >  
> >         imx335_fill_pad_format(imx335, mode, fmt);
> >  
> > -       if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -               struct v4l2_mbus_framefmt *framefmt;
> > +       format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> > +       *format = fmt->format;
> >  
> > -               framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> > -               *framefmt = fmt->format;
> > -       } else {
> > +       if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >                 ret = imx335_update_controls(imx335, mode);
> >                 if (!ret)
> >                         imx335->cur_mode = mode;
> >         }
> >  
> > -       mutex_unlock(&imx335->mutex);
> > -
> >         return ret;
> >  }
> >  
> > @@ -846,12 +810,10 @@ static int imx335_init_state(struct v4l2_subdev *sd,
> >         fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> >         imx335_fill_pad_format(imx335, &supported_mode, &fmt);
> >  
> > -       mutex_lock(&imx335->mutex);
> >         __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
> >                                  __fls(imx335->link_freq_bitmap),
> >                                  ~(imx335->link_freq_bitmap),
> >                                  __ffs(imx335->link_freq_bitmap));
> > -       mutex_unlock(&imx335->mutex);
> >  
> >         return imx335_set_pad_format(sd, sd_state, &fmt);
> >  }
> > @@ -919,13 +881,17 @@ static int imx335_start_streaming(struct imx335 *imx335)
> >         const struct imx335_reg_list *reg_list;
> >         int ret;
> >  
> > +       ret = pm_runtime_resume_and_get(imx335->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> >         /* Setup PLL */
> >         reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
> >         ret = cci_multi_reg_write(imx335->cci, reg_list->regs,
> >                                   reg_list->num_of_regs, NULL);
> >         if (ret) {
> >                 dev_err(imx335->dev, "%s failed to set plls\n", __func__);
> > -               return ret;
> > +               goto err_rpm_put;
> >         }
> >  
> >         /* Write sensor mode registers */
> > @@ -934,27 +900,27 @@ static int imx335_start_streaming(struct imx335 *imx335)
> >                                   reg_list->num_of_regs, NULL);
> >         if (ret) {
> >                 dev_err(imx335->dev, "fail to write initial registers\n");
> > -               return ret;
> > +               goto err_rpm_put;
> >         }
> >  
> >         ret = imx335_set_framefmt(imx335);
> >         if (ret) {
> >                 dev_err(imx335->dev, "%s failed to set frame format: %d\n",
> >                         __func__, ret);
> > -               return ret;
> > +               goto err_rpm_put;
> >         }
> >  
> >         /* Configure lanes */
> >         ret = cci_write(imx335->cci, IMX335_REG_LANEMODE,
> >                         imx335->lane_mode, NULL);
> >         if (ret)
> > -               return ret;
> > +               goto err_rpm_put;
> >  
> >         /* Setup handler will write actual exposure and gain */
> >         ret =  __v4l2_ctrl_handler_setup(imx335->sd.ctrl_handler);
> >         if (ret) {
> >                 dev_err(imx335->dev, "fail to setup handler\n");
> > -               return ret;
> > +               goto err_rpm_put;
> >         }
> >  
> >         /* Start streaming */
> > @@ -962,25 +928,29 @@ static int imx335_start_streaming(struct imx335 *imx335)
> >                         IMX335_MODE_STREAMING, NULL);
> >         if (ret) {
> >                 dev_err(imx335->dev, "fail to start streaming\n");
> > -               return ret;
> > +               goto err_rpm_put;
> >         }
> >  
> >         /* Initial regulator stabilization period */
> >         usleep_range(18000, 20000);
> >  
> >         return 0;
> > +
> > +err_rpm_put:
> > +       pm_runtime_put(imx335->dev);
> > +
> > +       return ret;
> >  }
> >  
> >  /**
> >   * imx335_stop_streaming() - Stop sensor stream
> >   * @imx335: pointer to imx335 device
> > - *
> > - * Return: 0 if successful, error code otherwise.
> >   */
> > -static int imx335_stop_streaming(struct imx335 *imx335)
> > +static void imx335_stop_streaming(struct imx335 *imx335)
> >  {
> > -       return cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> > -                        IMX335_MODE_STANDBY, NULL);
> > +       cci_write(imx335->cci, IMX335_REG_MODE_SELECT,
> > +                 IMX335_MODE_STANDBY, NULL);
> > +       pm_runtime_put(imx335->dev);
> >  }
> >  
> >  /**
> > @@ -993,31 +963,18 @@ static int imx335_stop_streaming(struct imx335 *imx335)
> >  static int imx335_set_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >         struct imx335 *imx335 = to_imx335(sd);
> > -       int ret;
> > +       struct v4l2_subdev_state *state;
> > +       int ret = 0;
> >  
> > -       mutex_lock(&imx335->mutex);
> > +       state = v4l2_subdev_lock_and_get_active_state(sd);
> >  
> >         if (enable) {
> > -               ret = pm_runtime_resume_and_get(imx335->dev);
> > -               if (ret)
> > -                       goto error_unlock;
> > -
> >                 ret = imx335_start_streaming(imx335);
> > -               if (ret)
> > -                       goto error_power_off;
> >         } else {
> >                 imx335_stop_streaming(imx335);
> > -               pm_runtime_put(imx335->dev);
> >         }
> >  
> > -       mutex_unlock(&imx335->mutex);
> > -
> > -       return 0;
> > -
> > -error_power_off:
> > -       pm_runtime_put(imx335->dev);
> > -error_unlock:
> > -       mutex_unlock(&imx335->mutex);
> > +       v4l2_subdev_unlock_state(state);
> >  
> >         return ret;
> >  }
> > @@ -1146,7 +1103,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = {
> >         .enum_frame_size = imx335_enum_frame_size,
> >         .get_selection = imx335_get_selection,
> >         .set_selection = imx335_get_selection,
> > -       .get_fmt = imx335_get_pad_format,
> > +       .get_fmt = v4l2_subdev_get_fmt,
> >         .set_fmt = imx335_set_pad_format,
> >  };
> >  
> > @@ -1241,9 +1198,6 @@ static int imx335_init_controls(struct imx335 *imx335)
> >         if (ret)
> >                 return ret;
> >  
> > -       /* Serialize controls with sensor device */
> > -       ctrl_hdlr->lock = &imx335->mutex;
> > -
> >         /* Initialize exposure and gain */
> >         lpfr = mode->vblank + mode->height;
> >         imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> > @@ -1363,12 +1317,10 @@ static int imx335_probe(struct i2c_client *client)
> >                 return ret;
> >         }
> >  
> > -       mutex_init(&imx335->mutex);
> > -
> >         ret = imx335_power_on(imx335->dev);
> >         if (ret) {
> >                 dev_err(imx335->dev, "failed to power-on the sensor\n");
> > -               goto error_mutex_destroy;
> > +               return ret;
> >         }
> >  
> >         /* Check module identity */
> > @@ -1401,11 +1353,18 @@ static int imx335_probe(struct i2c_client *client)
> >                 goto error_handler_free;
> >         }
> >  
> > +       imx335->sd.state_lock = imx335->ctrl_handler.lock;
> > +       ret = v4l2_subdev_init_finalize(&imx335->sd);
> > +       if (ret < 0) {
> > +               dev_err(imx335->dev, "subdev init error\n");
> > +               goto error_media_entity;
> > +       }
> > +
> >         ret = v4l2_async_register_subdev_sensor(&imx335->sd);
> >         if (ret < 0) {
> >                 dev_err(imx335->dev,
> >                         "failed to register async subdev: %d\n", ret);
> > -               goto error_media_entity;
> > +               goto error_subdev_cleanup;
> >         }
> >  
> >         pm_runtime_set_active(imx335->dev);
> > @@ -1414,14 +1373,14 @@ static int imx335_probe(struct i2c_client *client)
> >  
> >         return 0;
> >  
> > +error_subdev_cleanup:
> > +       v4l2_subdev_cleanup(&imx335->sd);
> >  error_media_entity:
> >         media_entity_cleanup(&imx335->sd.entity);
> >  error_handler_free:
> >         v4l2_ctrl_handler_free(imx335->sd.ctrl_handler);
> >  error_power_off:
> >         imx335_power_off(imx335->dev);
> > -error_mutex_destroy:
> > -       mutex_destroy(&imx335->mutex);
> >  
> >         return ret;
> >  }
> > @@ -1435,9 +1394,9 @@ static int imx335_probe(struct i2c_client *client)
> >  static void imx335_remove(struct i2c_client *client)
> >  {
> >         struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > -       struct imx335 *imx335 = to_imx335(sd);
> >  
> >         v4l2_async_unregister_subdev(sd);
> > +       v4l2_subdev_cleanup(sd);
> >         media_entity_cleanup(&sd->entity);
> >         v4l2_ctrl_handler_free(sd->ctrl_handler);
> >  
> > @@ -1445,8 +1404,6 @@ static void imx335_remove(struct i2c_client *client)
> >         if (!pm_runtime_status_suspended(&client->dev))
> >                 imx335_power_off(&client->dev);
> >         pm_runtime_set_suspended(&client->dev);
> > -
> > -       mutex_destroy(&imx335->mutex);
> >  }
> >  
> >  static const struct dev_pm_ops imx335_pm_ops = {
> > 
> > -- 
> > 2.50.1
> >

Re: [PATCH 5/6] media: imx355: Use subdev active state

[Sakari Ailus]

Hi Jai,

Thanks for the set.

On Wed, Aug 13, 2025 at 12:50:36PM +0530, Jai Luthra wrote:
> Port the driver to use the subdev active state. This simplifies locking,
> and makes it easier to support different crop sizes for binned modes, by
> storing the crop rectangle inside the subdev state.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>

It'd be nice to switch to {enable,disable}_streams, too. Just an idea. :-)

-- 
Sakari Ailus

Re: [PATCH 5/6] media: imx355: Use subdev active state

[Jai Luthra]

Hi Sakari,

Quoting Sakari Ailus (2025-08-18 18:44:35)
> Hi Jai,
> 
> Thanks for the set.
> 
> On Wed, Aug 13, 2025 at 12:50:36PM +0530, Jai Luthra wrote:
> > Port the driver to use the subdev active state. This simplifies locking,
> > and makes it easier to support different crop sizes for binned modes, by
> > storing the crop rectangle inside the subdev state.
> > 
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> 
> It'd be nice to switch to {enable,disable}_streams, too. Just an idea. :-)
> 

Indeed, will give it an attempt before v2 :)

> -- 
> Sakari Ailus

[PATCH 6/6] media: imx335: Support 2x2 binning

[Jai Luthra]

Introduce 2x2 binning mode (1312x972@60fps). Since there are multiple
modes now, use v4l2_find_nearest_size() to select the appropriate mode
in .set_fmt().

For 2x2 binning the minimum shutter value supported is 17 instead of 9.
This effects the maximum allowed exposure time, and if not enforced then
the sensor produces very dark frames when the minimum shutter limit is
violated.

Lastly, update the crop size reported to the userspace. When trying 2x2
binning with the datasheet suggested pixel array size (i.e. 2592 / 2 =>
1296), on some platforms (Raspberry Pi 5) artefacts are introduced on
the right edge of the output image. Instead, using a higher width of
1312 works fine on all platforms.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 274 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 217 insertions(+), 57 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index df1535927f481de3a0d043ac9be24b9336ea8f7f..24d26bc6260bf60ca73df81fe398121ac9371b42 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -35,6 +35,7 @@
 
 /* Lines per frame */
 #define IMX335_REG_VMAX			CCI_REG24_LE(0x3030)
+#define IMX335_REG_HMAX			CCI_REG16_LE(0x3034)
 
 #define IMX335_REG_OPB_SIZE_V		CCI_REG8(0x304c)
 #define IMX335_REG_ADBIT		CCI_REG8(0x3050)
@@ -42,10 +43,13 @@
 
 #define IMX335_REG_SHUTTER		CCI_REG24_LE(0x3058)
 #define IMX335_EXPOSURE_MIN		1
-#define IMX335_EXPOSURE_OFFSET		9
+#define IMX335_SHUTTER_MIN		9
+#define IMX335_SHUTTER_MIN_BINNED	17
 #define IMX335_EXPOSURE_STEP		1
 #define IMX335_EXPOSURE_DEFAULT		0x0648
 
+#define IMX335_REG_AREA2_WIDTH_1	CCI_REG16_LE(0x3072)
+
 #define IMX335_REG_AREA3_ST_ADR_1	CCI_REG16_LE(0x3074)
 #define IMX335_REG_AREA3_WIDTH_1	CCI_REG16_LE(0x3076)
 
@@ -126,9 +130,9 @@
 /* IMX335 native and active pixel array size. */
 #define IMX335_NATIVE_WIDTH		2696U
 #define IMX335_NATIVE_HEIGHT		2044U
-#define IMX335_PIXEL_ARRAY_LEFT		52U
+#define IMX335_PIXEL_ARRAY_LEFT		36U
 #define IMX335_PIXEL_ARRAY_TOP		50U
-#define IMX335_PIXEL_ARRAY_WIDTH	2592U
+#define IMX335_PIXEL_ARRAY_WIDTH	2624U
 #define IMX335_PIXEL_ARRAY_HEIGHT	1944U
 
 /**
@@ -147,8 +151,14 @@ static const char * const imx335_supply_name[] = {
 	"dvdd", /* Digital Core (1.2V) supply */
 };
 
+enum imx335_scan_mode {
+	IMX335_ALL_PIXEL,
+	IMX335_2_2_BINNING,
+};
+
 /**
  * struct imx335_mode - imx335 sensor mode structure
+ * @scan_mode: Configuration scan mode (All pixel / 2x2Binning)
  * @width: Frame width
  * @height: Frame height
  * @code: Format code
@@ -162,6 +172,7 @@ static const char * const imx335_supply_name[] = {
  * @vflip_inverted: Register list vflip (inverted readout)
  */
 struct imx335_mode {
+	enum imx335_scan_mode scan_mode;
 	u32 width;
 	u32 height;
 	u32 code;
@@ -263,12 +274,33 @@ static const struct cci_reg_sequence mode_2592x1944_regs[] = {
 	{ IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
 	{ IMX335_REG_MASTER_MODE, 0x00 },
 	{ IMX335_REG_WINMODE, 0x04 },
+	{ IMX335_REG_HMAX, 550 },
 	{ IMX335_REG_HTRIMMING_START, 48 },
 	{ IMX335_REG_HNUM, 2592 },
 	{ IMX335_REG_Y_OUT_SIZE, 1944 },
+	{ IMX335_REG_AREA2_WIDTH_1, 40 },
 	{ IMX335_REG_AREA3_WIDTH_1, 3928 },
 	{ IMX335_REG_OPB_SIZE_V, 0 },
 	{ IMX335_REG_XVS_XHS_DRV, 0x00 },
+};
+
+static const struct cci_reg_sequence mode_1312x972_regs[] = {
+	{ IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
+	{ IMX335_REG_MASTER_MODE, 0x00 },
+	{ IMX335_REG_WINMODE, 0x01 },
+	{ IMX335_REG_HMAX, 275 },
+	{ IMX335_REG_HTRIMMING_START, 48 },
+	{ IMX335_REG_HNUM, 2600 },
+	{ IMX335_REG_Y_OUT_SIZE, 972 },
+	{ IMX335_REG_AREA2_WIDTH_1, 48 },
+	{ IMX335_REG_AREA3_WIDTH_1, 3936 },
+	{ IMX335_REG_OPB_SIZE_V, 0 },
+	{ IMX335_REG_XVS_XHS_DRV, 0x00 },
+	{ CCI_REG8(0x3300), 1 }, /* TCYCLE */
+	{ CCI_REG8(0x3199), 0x30 }, /* HADD/VADD */
+};
+
+static const struct cci_reg_sequence imx335_common_regs[] = {
 	{ CCI_REG8(0x3288), 0x21 },
 	{ CCI_REG8(0x328a), 0x02 },
 	{ CCI_REG8(0x3414), 0x05 },
@@ -359,16 +391,72 @@ static const struct cci_reg_sequence mode_2592x1944_vflip_inverted[] = {
 	{ CCI_REG16_LE(0x3116), 0x002 },
 };
 
-static const struct cci_reg_sequence raw10_framefmt_regs[] = {
-	{ IMX335_REG_ADBIT, 0x00 },
-	{ IMX335_REG_MDBIT, 0x00 },
-	{ IMX335_REG_ADBIT1, 0x1ff },
+static const struct cci_reg_sequence mode_1312x972_vflip_normal[] = {
+	{ IMX335_REG_AREA3_ST_ADR_1, 176 },
+
+	/* Undocumented */
+	{ CCI_REG8(0x3078), 0x04 },
+	{ CCI_REG8(0x3079), 0xfd },
+	{ CCI_REG8(0x307a), 0x04 },
+	{ CCI_REG8(0x307b), 0xfe },
+	{ CCI_REG8(0x307c), 0x04 },
+	{ CCI_REG8(0x307d), 0xfb },
+	{ CCI_REG8(0x307e), 0x04 },
+	{ CCI_REG8(0x307f), 0x02 },
+	{ CCI_REG8(0x3080), 0x04, },
+	{ CCI_REG8(0x3081), 0xfd, },
+	{ CCI_REG8(0x3082), 0x04, },
+	{ CCI_REG8(0x3083), 0xfe, },
+	{ CCI_REG8(0x3084), 0x04, },
+	{ CCI_REG8(0x3085), 0xfb, },
+	{ CCI_REG8(0x3086), 0x04, },
+	{ CCI_REG8(0x3087), 0x02, },
+	{ CCI_REG8(0x30a4), 0x77, },
+	{ CCI_REG8(0x30a8), 0x20, },
+	{ CCI_REG8(0x30a9), 0x00, },
+	{ CCI_REG8(0x30ac), 0x08, },
+	{ CCI_REG8(0x30ad), 0x08, },
+	{ CCI_REG8(0x30b0), 0x20, },
+	{ CCI_REG8(0x30b1), 0x00, },
+	{ CCI_REG8(0x30b4), 0x10, },
+	{ CCI_REG8(0x30b5), 0x10, },
+	{ CCI_REG16_LE(0x30b6), 0x00 },
+	{ CCI_REG16_LE(0x3112), 0x10 },
+	{ CCI_REG16_LE(0x3116), 0x10 },
 };
 
-static const struct cci_reg_sequence raw12_framefmt_regs[] = {
-	{ IMX335_REG_ADBIT, 0x01 },
-	{ IMX335_REG_MDBIT, 0x01 },
-	{ IMX335_REG_ADBIT1, 0x47 },
+static const struct cci_reg_sequence mode_1312x972_vflip_inverted[] = {
+	{ IMX335_REG_AREA3_ST_ADR_1, 4112 },
+
+	/* Undocumented */
+	{ CCI_REG8(0x3078), 0x04 },
+	{ CCI_REG8(0x3079), 0xfd },
+	{ CCI_REG8(0x307a), 0x04 },
+	{ CCI_REG8(0x307b), 0xfe },
+	{ CCI_REG8(0x307c), 0x04 },
+	{ CCI_REG8(0x307d), 0xfb },
+	{ CCI_REG8(0x307e), 0x04 },
+	{ CCI_REG8(0x307f), 0x02 },
+	{ CCI_REG8(0x3080), 0xfc, },
+	{ CCI_REG8(0x3081), 0x05, },
+	{ CCI_REG8(0x3082), 0xfc, },
+	{ CCI_REG8(0x3083), 0x02, },
+	{ CCI_REG8(0x3084), 0xfc, },
+	{ CCI_REG8(0x3085), 0x03, },
+	{ CCI_REG8(0x3086), 0xfc, },
+	{ CCI_REG8(0x3087), 0xfe, },
+	{ CCI_REG8(0x30a4), 0x77, },
+	{ CCI_REG8(0x30a8), 0x20, },
+	{ CCI_REG8(0x30a9), 0x00, },
+	{ CCI_REG8(0x30ac), 0x08, },
+	{ CCI_REG8(0x30ad), 0x78, },
+	{ CCI_REG8(0x30b0), 0x20, },
+	{ CCI_REG8(0x30b1), 0x00, },
+	{ CCI_REG8(0x30b4), 0x10, },
+	{ CCI_REG8(0x30b5), 0x70, },
+	{ CCI_REG16_LE(0x30b6), 0x01f2 },
+	{ CCI_REG16_LE(0x3112), 0x10 },
+	{ CCI_REG16_LE(0x3116), 0x02 },
 };
 
 static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
@@ -433,25 +521,49 @@ static const u32 imx335_mbus_codes[] = {
 };
 
 /* Supported sensor mode configurations */
-static const struct imx335_mode supported_mode = {
-	.width = 2592,
-	.height = 1944,
-	.hblank = 342,
-	.vblank = 2556,
-	.vblank_min = 2556,
-	.vblank_max = 133060,
-	.pclk = 396000000,
-	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
-		.regs = mode_2592x1944_regs,
-	},
-	.vflip_normal = {
-		.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
-		.regs = mode_2592x1944_vflip_normal,
-	},
-	.vflip_inverted = {
-		.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
-		.regs = mode_2592x1944_vflip_inverted,
+static const struct imx335_mode supported_modes[] = {
+	{
+		.scan_mode = IMX335_ALL_PIXEL,
+		.width = 2592,
+		.height = 1944,
+		.hblank = 342,
+		.vblank = 2556,
+		.vblank_min = 2556,
+		.vblank_max = 133060,
+		.pclk = 396000000,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
+			.regs = mode_2592x1944_regs,
+		},
+		.vflip_normal = {
+			.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
+			.regs = mode_2592x1944_vflip_normal,
+		},
+		.vflip_inverted = {
+			.num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
+			.regs = mode_2592x1944_vflip_inverted,
+		}
+	}, {
+		.scan_mode = IMX335_2_2_BINNING,
+		.width = 1312,
+		.height = 972,
+		.hblank = 155,
+		.vblank = 3528,
+		.vblank_min = 3528,
+		.vblank_max = 133060,
+		.pclk = 396000000,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1312x972_regs),
+			.regs = mode_1312x972_regs,
+		},
+		.vflip_normal = {
+			.num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_normal),
+			.regs = mode_1312x972_vflip_normal,
+		},
+		.vflip_inverted = {
+			.num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_inverted),
+			.regs = mode_1312x972_vflip_inverted,
+		},
 	},
 };
 
@@ -608,18 +720,22 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	/* Propagate change of current control to all related controls */
 	if (ctrl->id == V4L2_CID_VBLANK) {
+		u32 shutter_min = IMX335_SHUTTER_MIN;
+		u32 lpfr;
+
 		imx335->vblank = imx335->vblank_ctrl->val;
+		lpfr = imx335->vblank + imx335->cur_mode->height;
 
 		dev_dbg(imx335->dev, "Received vblank %u, new lpfr %u\n",
-			imx335->vblank,
-			imx335->vblank + imx335->cur_mode->height);
+			imx335->vblank, lpfr);
+
+		if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
+			shutter_min = IMX335_SHUTTER_MIN_BINNED;
 
 		ret = __v4l2_ctrl_modify_range(imx335->exp_ctrl,
 					       IMX335_EXPOSURE_MIN,
-					       imx335->vblank +
-					       imx335->cur_mode->height -
-					       IMX335_EXPOSURE_OFFSET,
-					       1, IMX335_EXPOSURE_DEFAULT);
+					       lpfr - shutter_min, 1,
+					       IMX335_EXPOSURE_DEFAULT);
 		if (ret)
 			return ret;
 	}
@@ -719,17 +835,16 @@ static int imx335_enum_frame_size(struct v4l2_subdev *sd,
 	struct imx335 *imx335 = to_imx335(sd);
 	u32 code;
 
-	/* Only a single supported_mode available. */
-	if (fsize->index > 0)
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
 	code = imx335_get_format_code(imx335, fsize->code);
 	if (fsize->code != code)
 		return -EINVAL;
 
-	fsize->min_width = supported_mode.width;
+	fsize->min_width = supported_modes[fsize->index].width;
 	fsize->max_width = fsize->min_width;
-	fsize->min_height = supported_mode.height;
+	fsize->min_height = supported_modes[fsize->index].height;
 	fsize->max_height = fsize->min_height;
 
 	return 0;
@@ -771,9 +886,13 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
 	struct imx335 *imx335 = to_imx335(sd);
 	struct v4l2_mbus_framefmt *format;
 	const struct imx335_mode *mode;
+	struct v4l2_rect *crop;
 	int i, ret = 0;
 
-	mode = &supported_mode;
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
 
 	for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
 		if (imx335_mbus_codes[i] == fmt->format.code)
@@ -785,6 +904,16 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
 	format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
 	*format = fmt->format;
 
+	crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
+	crop->width = fmt->format.width;
+	crop->height = fmt->format.height;
+	if (mode->scan_mode == IMX335_2_2_BINNING) {
+		crop->width *= 2;
+		crop->height *= 2;
+	}
+	crop->left = (IMX335_NATIVE_WIDTH - crop->width) / 2;
+	crop->top = (IMX335_NATIVE_HEIGHT - crop->height) / 2;
+
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ret = imx335_update_controls(imx335, mode);
 		if (!ret)
@@ -808,7 +937,7 @@ static int imx335_init_state(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	imx335_fill_pad_format(imx335, &supported_mode, &fmt);
+	imx335_fill_pad_format(imx335, &supported_modes[0], &fmt);
 
 	__v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
 				 __fls(imx335->link_freq_bitmap),
@@ -831,6 +960,11 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_selection *sel)
 {
 	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
+
+		return 0;
+
 	case V4L2_SEL_TGT_NATIVE_SIZE:
 		sel->r.top = 0;
 		sel->r.left = 0;
@@ -839,7 +973,6 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
 
 		return 0;
 
-	case V4L2_SEL_TGT_CROP:
 	case V4L2_SEL_TGT_CROP_DEFAULT:
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		sel->r.top = IMX335_PIXEL_ARRAY_TOP;
@@ -855,19 +988,35 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
 
 static int imx335_set_framefmt(struct imx335 *imx335)
 {
-	switch (imx335->cur_mbus_code) {
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
-					   ARRAY_SIZE(raw10_framefmt_regs),
-					   NULL);
-
-	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
-					   ARRAY_SIZE(raw12_framefmt_regs),
-					   NULL);
+	/*
+	 * In the all-pixel scan mode the AD conversion shall match the output
+	 * bit width requested.
+	 *
+	 * However, when 2/2 binning is enabled, the AD conversion is always
+	 * 10-bit, so we ensure ADBIT is clear and ADBIT1 is assigned 0x1ff.
+	 * That's as much as the documentation gives us...
+	 */
+	int ret = 0;
+	u8 bpp = imx335->cur_mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10 ? 10 : 12;
+	u8 ad_conv = bpp;
+
+	/* Start with the output mode */
+	cci_write(imx335->cci, IMX335_REG_MDBIT, bpp == 12, &ret);
+
+	/* Enforce 10 bit AD on binning modes */
+	if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
+		ad_conv = 10;
+
+	/* AD Conversion configuration */
+	if (ad_conv == 10) {
+		cci_write(imx335->cci, IMX335_REG_ADBIT, 0x00, &ret);
+		cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x1ff, &ret);
+	} else { /* 12 bit AD Conversion */
+		cci_write(imx335->cci, IMX335_REG_ADBIT, 0x01, &ret);
+		cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x47, &ret);
 	}
 
-	return -EINVAL;
+	return ret;
 }
 
 /**
@@ -903,6 +1052,14 @@ static int imx335_start_streaming(struct imx335 *imx335)
 		goto err_rpm_put;
 	}
 
+	/* Write sensor common registers */
+	ret = cci_multi_reg_write(imx335->cci, imx335_common_regs,
+				  ARRAY_SIZE(imx335_common_regs), NULL);
+	if (ret) {
+		dev_err(imx335->dev, "fail to write initial registers\n");
+		goto err_rpm_put;
+	}
+
 	ret = imx335_set_framefmt(imx335);
 	if (ret) {
 		dev_err(imx335->dev, "%s failed to set frame format: %d\n",
@@ -1186,7 +1343,7 @@ static int imx335_init_controls(struct imx335 *imx335)
 	struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
 	const struct imx335_mode *mode = imx335->cur_mode;
 	struct v4l2_fwnode_device_properties props;
-	u32 lpfr;
+	u32 lpfr, shutter_min;
 	int ret;
 
 	ret = v4l2_fwnode_device_parse(imx335->dev, &props);
@@ -1200,11 +1357,14 @@ static int imx335_init_controls(struct imx335 *imx335)
 
 	/* Initialize exposure and gain */
 	lpfr = mode->vblank + mode->height;
+	shutter_min = IMX335_SHUTTER_MIN;
+	if (mode->scan_mode == IMX335_2_2_BINNING)
+		shutter_min = IMX335_SHUTTER_MIN_BINNED;
 	imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 					     &imx335_ctrl_ops,
 					     V4L2_CID_EXPOSURE,
 					     IMX335_EXPOSURE_MIN,
-					     lpfr - IMX335_EXPOSURE_OFFSET,
+					     lpfr - shutter_min,
 					     IMX335_EXPOSURE_STEP,
 					     IMX335_EXPOSURE_DEFAULT);
 
@@ -1331,7 +1491,7 @@ static int imx335_probe(struct i2c_client *client)
 	}
 
 	/* Set default mode to max resolution */
-	imx335->cur_mode = &supported_mode;
+	imx335->cur_mode = &supported_modes[0];
 	imx335->cur_mbus_code = imx335_mbus_codes[0];
 	imx335->vblank = imx335->cur_mode->vblank;
 

-- 
2.50.1

Re: [PATCH 6/6] media: imx335: Support 2x2 binning

[Kieran Bingham]

Quoting Jai Luthra (2025-08-13 08:20:37)
> Introduce 2x2 binning mode (1312x972@60fps). Since there are multiple
> modes now, use v4l2_find_nearest_size() to select the appropriate mode
> in .set_fmt().
> 
> For 2x2 binning the minimum shutter value supported is 17 instead of 9.
> This effects the maximum allowed exposure time, and if not enforced then
> the sensor produces very dark frames when the minimum shutter limit is
> violated.
> 
> Lastly, update the crop size reported to the userspace. When trying 2x2
> binning with the datasheet suggested pixel array size (i.e. 2592 / 2 =>
> 1296), on some platforms (Raspberry Pi 5) artefacts are introduced on

It was visible on i.MX8MP too. I look forward to testing this update on
my platform!

> the right edge of the output image. Instead, using a higher width of
> 1312 works fine on all platforms.
> 
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 274 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 217 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index df1535927f481de3a0d043ac9be24b9336ea8f7f..24d26bc6260bf60ca73df81fe398121ac9371b42 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -35,6 +35,7 @@
>  
>  /* Lines per frame */
>  #define IMX335_REG_VMAX                        CCI_REG24_LE(0x3030)
> +#define IMX335_REG_HMAX                        CCI_REG16_LE(0x3034)
>  
>  #define IMX335_REG_OPB_SIZE_V          CCI_REG8(0x304c)
>  #define IMX335_REG_ADBIT               CCI_REG8(0x3050)
> @@ -42,10 +43,13 @@
>  
>  #define IMX335_REG_SHUTTER             CCI_REG24_LE(0x3058)
>  #define IMX335_EXPOSURE_MIN            1
> -#define IMX335_EXPOSURE_OFFSET         9
> +#define IMX335_SHUTTER_MIN             9
> +#define IMX335_SHUTTER_MIN_BINNED      17
>  #define IMX335_EXPOSURE_STEP           1
>  #define IMX335_EXPOSURE_DEFAULT                0x0648
>  
> +#define IMX335_REG_AREA2_WIDTH_1       CCI_REG16_LE(0x3072)
> +

Ohh what's the distinction between Area2 and Area3 ? (I should probably
jsut open the datasheet to answer that myself ...)

>  #define IMX335_REG_AREA3_ST_ADR_1      CCI_REG16_LE(0x3074)
>  #define IMX335_REG_AREA3_WIDTH_1       CCI_REG16_LE(0x3076)
>  
> @@ -126,9 +130,9 @@
>  /* IMX335 native and active pixel array size. */
>  #define IMX335_NATIVE_WIDTH            2696U
>  #define IMX335_NATIVE_HEIGHT           2044U
> -#define IMX335_PIXEL_ARRAY_LEFT                52U
> +#define IMX335_PIXEL_ARRAY_LEFT                36U
>  #define IMX335_PIXEL_ARRAY_TOP         50U
> -#define IMX335_PIXEL_ARRAY_WIDTH       2592U
> +#define IMX335_PIXEL_ARRAY_WIDTH       2624U
>  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U

Should these changes have been in the earlier "Update the native pixel
array width" patch?

>  
>  /**
> @@ -147,8 +151,14 @@ static const char * const imx335_supply_name[] = {
>         "dvdd", /* Digital Core (1.2V) supply */
>  };
>  
> +enum imx335_scan_mode {
> +       IMX335_ALL_PIXEL,
> +       IMX335_2_2_BINNING,
> +};
> +
>  /**
>   * struct imx335_mode - imx335 sensor mode structure
> + * @scan_mode: Configuration scan mode (All pixel / 2x2Binning)
>   * @width: Frame width
>   * @height: Frame height
>   * @code: Format code
> @@ -162,6 +172,7 @@ static const char * const imx335_supply_name[] = {
>   * @vflip_inverted: Register list vflip (inverted readout)
>   */
>  struct imx335_mode {
> +       enum imx335_scan_mode scan_mode;
>         u32 width;
>         u32 height;
>         u32 code;
> @@ -263,12 +274,33 @@ static const struct cci_reg_sequence mode_2592x1944_regs[] = {
>         { IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
>         { IMX335_REG_MASTER_MODE, 0x00 },
>         { IMX335_REG_WINMODE, 0x04 },
> +       { IMX335_REG_HMAX, 550 },

Is this more related to the media: imx335: Update HBLANK range on mode
change patch too?

Or perhaps distinct on it's own? If this isn't the default value perhaps
it needs a separate commit message explaining it.

If HMAX is only 550 - is it in some clock specific unit ? But I'm not
too worried about that until we want variable HBLANK.


>         { IMX335_REG_HTRIMMING_START, 48 },
>         { IMX335_REG_HNUM, 2592 },
>         { IMX335_REG_Y_OUT_SIZE, 1944 },
> +       { IMX335_REG_AREA2_WIDTH_1, 40 },
>         { IMX335_REG_AREA3_WIDTH_1, 3928 },
>         { IMX335_REG_OPB_SIZE_V, 0 },
>         { IMX335_REG_XVS_XHS_DRV, 0x00 },
> +};
> +
> +static const struct cci_reg_sequence mode_1312x972_regs[] = {
> +       { IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
> +       { IMX335_REG_MASTER_MODE, 0x00 },
> +       { IMX335_REG_WINMODE, 0x01 },
> +       { IMX335_REG_HMAX, 275 },
> +       { IMX335_REG_HTRIMMING_START, 48 },
> +       { IMX335_REG_HNUM, 2600 },
> +       { IMX335_REG_Y_OUT_SIZE, 972 },
> +       { IMX335_REG_AREA2_WIDTH_1, 48 },
> +       { IMX335_REG_AREA3_WIDTH_1, 3936 },
> +       { IMX335_REG_OPB_SIZE_V, 0 },
> +       { IMX335_REG_XVS_XHS_DRV, 0x00 },
> +       { CCI_REG8(0x3300), 1 }, /* TCYCLE */
> +       { CCI_REG8(0x3199), 0x30 }, /* HADD/VADD */
> +};
> +
> +static const struct cci_reg_sequence imx335_common_regs[] = {
>         { CCI_REG8(0x3288), 0x21 },
>         { CCI_REG8(0x328a), 0x02 },
>         { CCI_REG8(0x3414), 0x05 },
> @@ -359,16 +391,72 @@ static const struct cci_reg_sequence mode_2592x1944_vflip_inverted[] = {
>         { CCI_REG16_LE(0x3116), 0x002 },
>  };
>  
> -static const struct cci_reg_sequence raw10_framefmt_regs[] = {
> -       { IMX335_REG_ADBIT, 0x00 },
> -       { IMX335_REG_MDBIT, 0x00 },
> -       { IMX335_REG_ADBIT1, 0x1ff },
> +static const struct cci_reg_sequence mode_1312x972_vflip_normal[] = {
> +       { IMX335_REG_AREA3_ST_ADR_1, 176 },
> +
> +       /* Undocumented */
> +       { CCI_REG8(0x3078), 0x04 },
> +       { CCI_REG8(0x3079), 0xfd },
> +       { CCI_REG8(0x307a), 0x04 },
> +       { CCI_REG8(0x307b), 0xfe },
> +       { CCI_REG8(0x307c), 0x04 },
> +       { CCI_REG8(0x307d), 0xfb },
> +       { CCI_REG8(0x307e), 0x04 },
> +       { CCI_REG8(0x307f), 0x02 },

That set are common to both modes.

> +       { CCI_REG8(0x3080), 0x04, },
> +       { CCI_REG8(0x3081), 0xfd, },
> +       { CCI_REG8(0x3082), 0x04, },
> +       { CCI_REG8(0x3083), 0xfe, },
> +       { CCI_REG8(0x3084), 0x04, },
> +       { CCI_REG8(0x3085), 0xfb, },
> +       { CCI_REG8(0x3086), 0x04, },
> +       { CCI_REG8(0x3087), 0x02, },

These are a suspicious mix of switching between 0xfc,0xfd,
0x02,0x03,0x04,0x05... but I haven't spent enough time to analyse what
the pattern or underlying meaning could be yet.

> +       { CCI_REG8(0x30a4), 0x77, },
> +       { CCI_REG8(0x30a8), 0x20, },
> +       { CCI_REG8(0x30a9), 0x00, },
> +       { CCI_REG8(0x30ac), 0x08, },
> +       { CCI_REG8(0x30ad), 0x08, },
> +       { CCI_REG8(0x30b0), 0x20, },
> +       { CCI_REG8(0x30b1), 0x00, },
> +       { CCI_REG8(0x30b4), 0x10, },
> +       { CCI_REG8(0x30b5), 0x10, },

It's a lot of churn for a flipped scan out ... But codifying just the
differences without knowing the underlying meaning of those registers
probably isn't worth the effort at the moment.

So I think keeping these tables is unfortunately the sanest thing to do
for now.

> +       { CCI_REG16_LE(0x30b6), 0x00 },
> +       { CCI_REG16_LE(0x3112), 0x10 },
> +       { CCI_REG16_LE(0x3116), 0x10 },
>  };
>  
> -static const struct cci_reg_sequence raw12_framefmt_regs[] = {
> -       { IMX335_REG_ADBIT, 0x01 },
> -       { IMX335_REG_MDBIT, 0x01 },
> -       { IMX335_REG_ADBIT1, 0x47 },
> +static const struct cci_reg_sequence mode_1312x972_vflip_inverted[] = {
> +       { IMX335_REG_AREA3_ST_ADR_1, 4112 },
> +
> +       /* Undocumented */
> +       { CCI_REG8(0x3078), 0x04 },
> +       { CCI_REG8(0x3079), 0xfd },
> +       { CCI_REG8(0x307a), 0x04 },
> +       { CCI_REG8(0x307b), 0xfe },
> +       { CCI_REG8(0x307c), 0x04 },
> +       { CCI_REG8(0x307d), 0xfb },
> +       { CCI_REG8(0x307e), 0x04 },
> +       { CCI_REG8(0x307f), 0x02 },
> +       { CCI_REG8(0x3080), 0xfc, },
> +       { CCI_REG8(0x3081), 0x05, },
> +       { CCI_REG8(0x3082), 0xfc, },
> +       { CCI_REG8(0x3083), 0x02, },
> +       { CCI_REG8(0x3084), 0xfc, },
> +       { CCI_REG8(0x3085), 0x03, },
> +       { CCI_REG8(0x3086), 0xfc, },
> +       { CCI_REG8(0x3087), 0xfe, },
> +       { CCI_REG8(0x30a4), 0x77, },
> +       { CCI_REG8(0x30a8), 0x20, },
> +       { CCI_REG8(0x30a9), 0x00, },
> +       { CCI_REG8(0x30ac), 0x08, },
> +       { CCI_REG8(0x30ad), 0x78, },
> +       { CCI_REG8(0x30b0), 0x20, },
> +       { CCI_REG8(0x30b1), 0x00, },
> +       { CCI_REG8(0x30b4), 0x10, },
> +       { CCI_REG8(0x30b5), 0x70, },

> +       { CCI_REG16_LE(0x30b6), 0x01f2 },
> +       { CCI_REG16_LE(0x3112), 0x10 },
> +       { CCI_REG16_LE(0x3116), 0x02 },

More of a discussion point ... 
but should we write these as 0x0010 ? / 0x002 ?

>  };
>  
>  static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
> @@ -433,25 +521,49 @@ static const u32 imx335_mbus_codes[] = {
>  };
>  
>  /* Supported sensor mode configurations */
> -static const struct imx335_mode supported_mode = {
> -       .width = 2592,
> -       .height = 1944,
> -       .hblank = 342,
> -       .vblank = 2556,
> -       .vblank_min = 2556,
> -       .vblank_max = 133060,
> -       .pclk = 396000000,
> -       .reg_list = {
> -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> -               .regs = mode_2592x1944_regs,
> -       },
> -       .vflip_normal = {
> -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
> -               .regs = mode_2592x1944_vflip_normal,
> -       },
> -       .vflip_inverted = {
> -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
> -               .regs = mode_2592x1944_vflip_inverted,
> +static const struct imx335_mode supported_modes[] = {
> +       {
> +               .scan_mode = IMX335_ALL_PIXEL,
> +               .width = 2592,
> +               .height = 1944,
> +               .hblank = 342,
> +               .vblank = 2556,
> +               .vblank_min = 2556,
> +               .vblank_max = 133060,
> +               .pclk = 396000000,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> +                       .regs = mode_2592x1944_regs,
> +               },
> +               .vflip_normal = {
> +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
> +                       .regs = mode_2592x1944_vflip_normal,
> +               },
> +               .vflip_inverted = {
> +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
> +                       .regs = mode_2592x1944_vflip_inverted,
> +               }
> +       }, {
> +               .scan_mode = IMX335_2_2_BINNING,
> +               .width = 1312,
> +               .height = 972,
> +               .hblank = 155,
> +               .vblank = 3528,
> +               .vblank_min = 3528,
> +               .vblank_max = 133060,
> +               .pclk = 396000000,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_regs),
> +                       .regs = mode_1312x972_regs,
> +               },
> +               .vflip_normal = {
> +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_normal),
> +                       .regs = mode_1312x972_vflip_normal,
> +               },
> +               .vflip_inverted = {
> +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_inverted),
> +                       .regs = mode_1312x972_vflip_inverted,
> +               },
>         },
>  };
>  
> @@ -608,18 +720,22 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
>  
>         /* Propagate change of current control to all related controls */
>         if (ctrl->id == V4L2_CID_VBLANK) {
> +               u32 shutter_min = IMX335_SHUTTER_MIN;
> +               u32 lpfr;
> +
>                 imx335->vblank = imx335->vblank_ctrl->val;
> +               lpfr = imx335->vblank + imx335->cur_mode->height;
>  
>                 dev_dbg(imx335->dev, "Received vblank %u, new lpfr %u\n",
> -                       imx335->vblank,
> -                       imx335->vblank + imx335->cur_mode->height);
> +                       imx335->vblank, lpfr);
> +
> +               if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
> +                       shutter_min = IMX335_SHUTTER_MIN_BINNED;
>  
>                 ret = __v4l2_ctrl_modify_range(imx335->exp_ctrl,
>                                                IMX335_EXPOSURE_MIN,
> -                                              imx335->vblank +
> -                                              imx335->cur_mode->height -
> -                                              IMX335_EXPOSURE_OFFSET,
> -                                              1, IMX335_EXPOSURE_DEFAULT);
> +                                              lpfr - shutter_min, 1,
> +                                              IMX335_EXPOSURE_DEFAULT);
>                 if (ret)
>                         return ret;
>         }
> @@ -719,17 +835,16 @@ static int imx335_enum_frame_size(struct v4l2_subdev *sd,
>         struct imx335 *imx335 = to_imx335(sd);
>         u32 code;
>  
> -       /* Only a single supported_mode available. */
> -       if (fsize->index > 0)
> +       if (fsize->index >= ARRAY_SIZE(supported_modes))
>                 return -EINVAL;
>  
>         code = imx335_get_format_code(imx335, fsize->code);
>         if (fsize->code != code)
>                 return -EINVAL;
>  
> -       fsize->min_width = supported_mode.width;
> +       fsize->min_width = supported_modes[fsize->index].width;
>         fsize->max_width = fsize->min_width;
> -       fsize->min_height = supported_mode.height;
> +       fsize->min_height = supported_modes[fsize->index].height;
>         fsize->max_height = fsize->min_height;
>  
>         return 0;
> @@ -771,9 +886,13 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>         struct imx335 *imx335 = to_imx335(sd);
>         struct v4l2_mbus_framefmt *format;
>         const struct imx335_mode *mode;
> +       struct v4l2_rect *crop;
>         int i, ret = 0;
>  
> -       mode = &supported_mode;
> +       mode = v4l2_find_nearest_size(supported_modes,
> +                                     ARRAY_SIZE(supported_modes),
> +                                     width, height,
> +                                     fmt->format.width, fmt->format.height);
>  
>         for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
>                 if (imx335_mbus_codes[i] == fmt->format.code)
> @@ -785,6 +904,16 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
>         format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
>         *format = fmt->format;
>  
> +       crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
> +       crop->width = fmt->format.width;
> +       crop->height = fmt->format.height;
> +       if (mode->scan_mode == IMX335_2_2_BINNING) {
> +               crop->width *= 2;
> +               crop->height *= 2;
> +       }
> +       crop->left = (IMX335_NATIVE_WIDTH - crop->width) / 2;
> +       crop->top = (IMX335_NATIVE_HEIGHT - crop->height) / 2;
> +
>         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                 ret = imx335_update_controls(imx335, mode);
>                 if (!ret)
> @@ -808,7 +937,7 @@ static int imx335_init_state(struct v4l2_subdev *sd,
>         struct v4l2_subdev_format fmt = { 0 };
>  
>         fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -       imx335_fill_pad_format(imx335, &supported_mode, &fmt);
> +       imx335_fill_pad_format(imx335, &supported_modes[0], &fmt);
>  
>         __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
>                                  __fls(imx335->link_freq_bitmap),
> @@ -831,6 +960,11 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
>                                 struct v4l2_subdev_selection *sel)
>  {
>         switch (sel->target) {
> +       case V4L2_SEL_TGT_CROP:
> +               sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> +
> +               return 0;
> +
>         case V4L2_SEL_TGT_NATIVE_SIZE:
>                 sel->r.top = 0;
>                 sel->r.left = 0;
> @@ -839,7 +973,6 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
>  
>                 return 0;
>  
> -       case V4L2_SEL_TGT_CROP:
>         case V4L2_SEL_TGT_CROP_DEFAULT:
>         case V4L2_SEL_TGT_CROP_BOUNDS:
>                 sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> @@ -855,19 +988,35 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
>  
>  static int imx335_set_framefmt(struct imx335 *imx335)
>  {
> -       switch (imx335->cur_mbus_code) {
> -       case MEDIA_BUS_FMT_SRGGB10_1X10:
> -               return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
> -                                          ARRAY_SIZE(raw10_framefmt_regs),
> -                                          NULL);
> -
> -       case MEDIA_BUS_FMT_SRGGB12_1X12:
> -               return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
> -                                          ARRAY_SIZE(raw12_framefmt_regs),
> -                                          NULL);
> +       /*
> +        * In the all-pixel scan mode the AD conversion shall match the output
> +        * bit width requested.
> +        *
> +        * However, when 2/2 binning is enabled, the AD conversion is always
> +        * 10-bit, so we ensure ADBIT is clear and ADBIT1 is assigned 0x1ff.
> +        * That's as much as the documentation gives us...
> +        */
> +       int ret = 0;
> +       u8 bpp = imx335->cur_mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10 ? 10 : 12;
> +       u8 ad_conv = bpp;
> +
> +       /* Start with the output mode */
> +       cci_write(imx335->cci, IMX335_REG_MDBIT, bpp == 12, &ret);
> +
> +       /* Enforce 10 bit AD on binning modes */
> +       if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
> +               ad_conv = 10;
> +
> +       /* AD Conversion configuration */
> +       if (ad_conv == 10) {
> +               cci_write(imx335->cci, IMX335_REG_ADBIT, 0x00, &ret);
> +               cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x1ff, &ret);
> +       } else { /* 12 bit AD Conversion */
> +               cci_write(imx335->cci, IMX335_REG_ADBIT, 0x01, &ret);
> +               cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x47, &ret);
>         }
>  
> -       return -EINVAL;
> +       return ret;
>  }
>  
>  /**
> @@ -903,6 +1052,14 @@ static int imx335_start_streaming(struct imx335 *imx335)
>                 goto err_rpm_put;
>         }
>  
> +       /* Write sensor common registers */
> +       ret = cci_multi_reg_write(imx335->cci, imx335_common_regs,
> +                                 ARRAY_SIZE(imx335_common_regs), NULL);
> +       if (ret) {
> +               dev_err(imx335->dev, "fail to write initial registers\n");
> +               goto err_rpm_put;
> +       }
> +
>         ret = imx335_set_framefmt(imx335);
>         if (ret) {
>                 dev_err(imx335->dev, "%s failed to set frame format: %d\n",
> @@ -1186,7 +1343,7 @@ static int imx335_init_controls(struct imx335 *imx335)
>         struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
>         const struct imx335_mode *mode = imx335->cur_mode;
>         struct v4l2_fwnode_device_properties props;
> -       u32 lpfr;
> +       u32 lpfr, shutter_min;
>         int ret;
>  
>         ret = v4l2_fwnode_device_parse(imx335->dev, &props);
> @@ -1200,11 +1357,14 @@ static int imx335_init_controls(struct imx335 *imx335)
>  
>         /* Initialize exposure and gain */
>         lpfr = mode->vblank + mode->height;
> +       shutter_min = IMX335_SHUTTER_MIN;
> +       if (mode->scan_mode == IMX335_2_2_BINNING)
> +               shutter_min = IMX335_SHUTTER_MIN_BINNED;
>         imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
>                                              &imx335_ctrl_ops,
>                                              V4L2_CID_EXPOSURE,
>                                              IMX335_EXPOSURE_MIN,
> -                                            lpfr - IMX335_EXPOSURE_OFFSET,
> +                                            lpfr - shutter_min,
>                                              IMX335_EXPOSURE_STEP,
>                                              IMX335_EXPOSURE_DEFAULT);
>  
> @@ -1331,7 +1491,7 @@ static int imx335_probe(struct i2c_client *client)
>         }
>  
>         /* Set default mode to max resolution */
> -       imx335->cur_mode = &supported_mode;
> +       imx335->cur_mode = &supported_modes[0];
>         imx335->cur_mbus_code = imx335_mbus_codes[0];
>         imx335->vblank = imx335->cur_mode->vblank;
>  
> 
> -- 
> 2.50.1
>

Re: [PATCH 6/6] media: imx335: Support 2x2 binning

[Jai Luthra]

[-- Attachment #1: Type: text/plain, Size: 23905 bytes --]

Hi Kieran,

Thanks for the review!

Quoting Kieran Bingham (2025-08-13 15:26:11)
> Quoting Jai Luthra (2025-08-13 08:20:37)
> > Introduce 2x2 binning mode (1312x972@60fps). Since there are multiple
> > modes now, use v4l2_find_nearest_size() to select the appropriate mode
> > in .set_fmt().
> > 
> > For 2x2 binning the minimum shutter value supported is 17 instead of 9.
> > This effects the maximum allowed exposure time, and if not enforced then
> > the sensor produces very dark frames when the minimum shutter limit is
> > violated.
> > 
> > Lastly, update the crop size reported to the userspace. When trying 2x2
> > binning with the datasheet suggested pixel array size (i.e. 2592 / 2 =>
> > 1296), on some platforms (Raspberry Pi 5) artefacts are introduced on
> 
> It was visible on i.MX8MP too. I look forward to testing this update on
> my platform!
>
> > the right edge of the output image. Instead, using a higher width of
> > 1312 works fine on all platforms.
> > 
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx335.c | 274 +++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 217 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> > index df1535927f481de3a0d043ac9be24b9336ea8f7f..24d26bc6260bf60ca73df81fe398121ac9371b42 100644
> > --- a/drivers/media/i2c/imx335.c
> > +++ b/drivers/media/i2c/imx335.c
> > @@ -35,6 +35,7 @@
> >  
> >  /* Lines per frame */
> >  #define IMX335_REG_VMAX                        CCI_REG24_LE(0x3030)
> > +#define IMX335_REG_HMAX                        CCI_REG16_LE(0x3034)
> >  
> >  #define IMX335_REG_OPB_SIZE_V          CCI_REG8(0x304c)
> >  #define IMX335_REG_ADBIT               CCI_REG8(0x3050)
> > @@ -42,10 +43,13 @@
> >  
> >  #define IMX335_REG_SHUTTER             CCI_REG24_LE(0x3058)
> >  #define IMX335_EXPOSURE_MIN            1
> > -#define IMX335_EXPOSURE_OFFSET         9
> > +#define IMX335_SHUTTER_MIN             9
> > +#define IMX335_SHUTTER_MIN_BINNED      17
> >  #define IMX335_EXPOSURE_STEP           1
> >  #define IMX335_EXPOSURE_DEFAULT                0x0648
> >  
> > +#define IMX335_REG_AREA2_WIDTH_1       CCI_REG16_LE(0x3072)
> > +
> 
> Ohh what's the distinction between Area2 and Area3 ? (I should probably
> jsut open the datasheet to answer that myself ...)
> 

From what I understood, Area3 is for the active cropping region, and Area2
is for the OB (Optical black) region. The tables recommend different values
for this register (48 for binning, 40 for all-pixel) and the diagrams on
pg. 56 and 59 show the vertical effective OB reduce by 8 from normal to 2x2
binned mode.

Why a smaller OB region is needed for binned mode is something I don't
understand.

> >  #define IMX335_REG_AREA3_ST_ADR_1      CCI_REG16_LE(0x3074)
> >  #define IMX335_REG_AREA3_WIDTH_1       CCI_REG16_LE(0x3076)
> >  
> > @@ -126,9 +130,9 @@
> >  /* IMX335 native and active pixel array size. */
> >  #define IMX335_NATIVE_WIDTH            2696U
> >  #define IMX335_NATIVE_HEIGHT           2044U
> > -#define IMX335_PIXEL_ARRAY_LEFT                52U
> > +#define IMX335_PIXEL_ARRAY_LEFT                36U
> >  #define IMX335_PIXEL_ARRAY_TOP         50U
> > -#define IMX335_PIXEL_ARRAY_WIDTH       2592U
> > +#define IMX335_PIXEL_ARRAY_WIDTH       2624U
> >  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
> 
> Should these changes have been in the earlier "Update the native pixel
> array width" patch?
> 

Well, I wasn't sure if it is okay to update the CROP_BOUNDS/CROP_DEFAULT
selection targets before the driver actually supports a mode with a bigger
crop rectangle - which is introduced by this patch.

Does userspace/libcamera require the cropping bounds for some calculation?

> >  
> >  /**
> > @@ -147,8 +151,14 @@ static const char * const imx335_supply_name[] = {
> >         "dvdd", /* Digital Core (1.2V) supply */
> >  };
> >  
> > +enum imx335_scan_mode {
> > +       IMX335_ALL_PIXEL,
> > +       IMX335_2_2_BINNING,
> > +};
> > +
> >  /**
> >   * struct imx335_mode - imx335 sensor mode structure
> > + * @scan_mode: Configuration scan mode (All pixel / 2x2Binning)
> >   * @width: Frame width
> >   * @height: Frame height
> >   * @code: Format code
> > @@ -162,6 +172,7 @@ static const char * const imx335_supply_name[] = {
> >   * @vflip_inverted: Register list vflip (inverted readout)
> >   */
> >  struct imx335_mode {
> > +       enum imx335_scan_mode scan_mode;
> >         u32 width;
> >         u32 height;
> >         u32 code;
> > @@ -263,12 +274,33 @@ static const struct cci_reg_sequence mode_2592x1944_regs[] = {
> >         { IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
> >         { IMX335_REG_MASTER_MODE, 0x00 },
> >         { IMX335_REG_WINMODE, 0x04 },
> > +       { IMX335_REG_HMAX, 550 },
> 
> Is this more related to the media: imx335: Update HBLANK range on mode
> change patch too?
> 
> Or perhaps distinct on it's own? If this isn't the default value perhaps
> it needs a separate commit message explaining it.
> 

I believe this change is related, but distinct from the userspace hblank
control update in that previous patch.

The 1312x972 mode sets HMAX = 275 for 60fps operation. The default mode
(and default power on value) is HMAX = 550 for 30fps operation.

> If HMAX is only 550 - is it in some clock specific unit ? But I'm not
> too worried about that until we want variable HBLANK.

Yes, HMAX is some kind of a clock unit for horizontal period as the
datasheet mentions on pg. 70 that 1 XHS period = HMAX / 74.25us

Will look more into it if/when we want variable hblank.

> 
> 
> >         { IMX335_REG_HTRIMMING_START, 48 },
> >         { IMX335_REG_HNUM, 2592 },
> >         { IMX335_REG_Y_OUT_SIZE, 1944 },
> > +       { IMX335_REG_AREA2_WIDTH_1, 40 },
> >         { IMX335_REG_AREA3_WIDTH_1, 3928 },
> >         { IMX335_REG_OPB_SIZE_V, 0 },
> >         { IMX335_REG_XVS_XHS_DRV, 0x00 },
> > +};
> > +
> > +static const struct cci_reg_sequence mode_1312x972_regs[] = {
> > +       { IMX335_REG_MODE_SELECT, IMX335_MODE_STANDBY },
> > +       { IMX335_REG_MASTER_MODE, 0x00 },
> > +       { IMX335_REG_WINMODE, 0x01 },
> > +       { IMX335_REG_HMAX, 275 },
> > +       { IMX335_REG_HTRIMMING_START, 48 },
> > +       { IMX335_REG_HNUM, 2600 },
> > +       { IMX335_REG_Y_OUT_SIZE, 972 },
> > +       { IMX335_REG_AREA2_WIDTH_1, 48 },
> > +       { IMX335_REG_AREA3_WIDTH_1, 3936 },
> > +       { IMX335_REG_OPB_SIZE_V, 0 },
> > +       { IMX335_REG_XVS_XHS_DRV, 0x00 },
> > +       { CCI_REG8(0x3300), 1 }, /* TCYCLE */
> > +       { CCI_REG8(0x3199), 0x30 }, /* HADD/VADD */
> > +};
> > +
> > +static const struct cci_reg_sequence imx335_common_regs[] = {
> >         { CCI_REG8(0x3288), 0x21 },
> >         { CCI_REG8(0x328a), 0x02 },
> >         { CCI_REG8(0x3414), 0x05 },
> > @@ -359,16 +391,72 @@ static const struct cci_reg_sequence mode_2592x1944_vflip_inverted[] = {
> >         { CCI_REG16_LE(0x3116), 0x002 },
> >  };
> >  
> > -static const struct cci_reg_sequence raw10_framefmt_regs[] = {
> > -       { IMX335_REG_ADBIT, 0x00 },
> > -       { IMX335_REG_MDBIT, 0x00 },
> > -       { IMX335_REG_ADBIT1, 0x1ff },
> > +static const struct cci_reg_sequence mode_1312x972_vflip_normal[] = {
> > +       { IMX335_REG_AREA3_ST_ADR_1, 176 },
> > +
> > +       /* Undocumented */
> > +       { CCI_REG8(0x3078), 0x04 },
> > +       { CCI_REG8(0x3079), 0xfd },
> > +       { CCI_REG8(0x307a), 0x04 },
> > +       { CCI_REG8(0x307b), 0xfe },
> > +       { CCI_REG8(0x307c), 0x04 },
> > +       { CCI_REG8(0x307d), 0xfb },
> > +       { CCI_REG8(0x307e), 0x04 },
> > +       { CCI_REG8(0x307f), 0x02 },
> 
> That set are common to both modes.

Will fix in v2.

> 
> > +       { CCI_REG8(0x3080), 0x04, },
> > +       { CCI_REG8(0x3081), 0xfd, },
> > +       { CCI_REG8(0x3082), 0x04, },
> > +       { CCI_REG8(0x3083), 0xfe, },
> > +       { CCI_REG8(0x3084), 0x04, },
> > +       { CCI_REG8(0x3085), 0xfb, },
> > +       { CCI_REG8(0x3086), 0x04, },
> > +       { CCI_REG8(0x3087), 0x02, },
> 
> These are a suspicious mix of switching between 0xfc,0xfd,
> 0x02,0x03,0x04,0x05... but I haven't spent enough time to analyse what
> the pattern or underlying meaning could be yet.
> 
> > +       { CCI_REG8(0x30a4), 0x77, },
> > +       { CCI_REG8(0x30a8), 0x20, },
> > +       { CCI_REG8(0x30a9), 0x00, },
> > +       { CCI_REG8(0x30ac), 0x08, },
> > +       { CCI_REG8(0x30ad), 0x08, },
> > +       { CCI_REG8(0x30b0), 0x20, },
> > +       { CCI_REG8(0x30b1), 0x00, },
> > +       { CCI_REG8(0x30b4), 0x10, },
> > +       { CCI_REG8(0x30b5), 0x10, },
> 
> It's a lot of churn for a flipped scan out ... But codifying just the
> differences without knowing the underlying meaning of those registers
> probably isn't worth the effort at the moment.
> 
> So I think keeping these tables is unfortunately the sanest thing to do
> for now.
> 

Yeah unfortunately we don't even know the names of these registers, just
the values to use :(

> > +       { CCI_REG16_LE(0x30b6), 0x00 },
> > +       { CCI_REG16_LE(0x3112), 0x10 },
> > +       { CCI_REG16_LE(0x3116), 0x10 },
> >  };
> >  
> > -static const struct cci_reg_sequence raw12_framefmt_regs[] = {
> > -       { IMX335_REG_ADBIT, 0x01 },
> > -       { IMX335_REG_MDBIT, 0x01 },
> > -       { IMX335_REG_ADBIT1, 0x47 },
> > +static const struct cci_reg_sequence mode_1312x972_vflip_inverted[] = {
> > +       { IMX335_REG_AREA3_ST_ADR_1, 4112 },
> > +
> > +       /* Undocumented */
> > +       { CCI_REG8(0x3078), 0x04 },
> > +       { CCI_REG8(0x3079), 0xfd },
> > +       { CCI_REG8(0x307a), 0x04 },
> > +       { CCI_REG8(0x307b), 0xfe },
> > +       { CCI_REG8(0x307c), 0x04 },
> > +       { CCI_REG8(0x307d), 0xfb },
> > +       { CCI_REG8(0x307e), 0x04 },
> > +       { CCI_REG8(0x307f), 0x02 },
> > +       { CCI_REG8(0x3080), 0xfc, },
> > +       { CCI_REG8(0x3081), 0x05, },
> > +       { CCI_REG8(0x3082), 0xfc, },
> > +       { CCI_REG8(0x3083), 0x02, },
> > +       { CCI_REG8(0x3084), 0xfc, },
> > +       { CCI_REG8(0x3085), 0x03, },
> > +       { CCI_REG8(0x3086), 0xfc, },
> > +       { CCI_REG8(0x3087), 0xfe, },
> > +       { CCI_REG8(0x30a4), 0x77, },
> > +       { CCI_REG8(0x30a8), 0x20, },
> > +       { CCI_REG8(0x30a9), 0x00, },
> > +       { CCI_REG8(0x30ac), 0x08, },
> > +       { CCI_REG8(0x30ad), 0x78, },
> > +       { CCI_REG8(0x30b0), 0x20, },
> > +       { CCI_REG8(0x30b1), 0x00, },
> > +       { CCI_REG8(0x30b4), 0x10, },
> > +       { CCI_REG8(0x30b5), 0x70, },
> 
> > +       { CCI_REG16_LE(0x30b6), 0x01f2 },
> > +       { CCI_REG16_LE(0x3112), 0x10 },
> > +       { CCI_REG16_LE(0x3116), 0x02 },
> 
> More of a discussion point ... 
> but should we write these as 0x0010 ? / 0x002 ?

Sounds better to me, will fix in v2.

> 
> >  };
> >  
> >  static const struct cci_reg_sequence mipi_data_rate_1188Mbps[] = {
> > @@ -433,25 +521,49 @@ static const u32 imx335_mbus_codes[] = {
> >  };
> >  
> >  /* Supported sensor mode configurations */
> > -static const struct imx335_mode supported_mode = {
> > -       .width = 2592,
> > -       .height = 1944,
> > -       .hblank = 342,
> > -       .vblank = 2556,
> > -       .vblank_min = 2556,
> > -       .vblank_max = 133060,
> > -       .pclk = 396000000,
> > -       .reg_list = {
> > -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> > -               .regs = mode_2592x1944_regs,
> > -       },
> > -       .vflip_normal = {
> > -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
> > -               .regs = mode_2592x1944_vflip_normal,
> > -       },
> > -       .vflip_inverted = {
> > -               .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
> > -               .regs = mode_2592x1944_vflip_inverted,
> > +static const struct imx335_mode supported_modes[] = {
> > +       {
> > +               .scan_mode = IMX335_ALL_PIXEL,
> > +               .width = 2592,
> > +               .height = 1944,
> > +               .hblank = 342,
> > +               .vblank = 2556,
> > +               .vblank_min = 2556,
> > +               .vblank_max = 133060,
> > +               .pclk = 396000000,
> > +               .reg_list = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_regs),
> > +                       .regs = mode_2592x1944_regs,
> > +               },
> > +               .vflip_normal = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_normal),
> > +                       .regs = mode_2592x1944_vflip_normal,
> > +               },
> > +               .vflip_inverted = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_2592x1944_vflip_inverted),
> > +                       .regs = mode_2592x1944_vflip_inverted,
> > +               }
> > +       }, {
> > +               .scan_mode = IMX335_2_2_BINNING,
> > +               .width = 1312,
> > +               .height = 972,
> > +               .hblank = 155,
> > +               .vblank = 3528,
> > +               .vblank_min = 3528,
> > +               .vblank_max = 133060,
> > +               .pclk = 396000000,
> > +               .reg_list = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_regs),
> > +                       .regs = mode_1312x972_regs,
> > +               },
> > +               .vflip_normal = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_normal),
> > +                       .regs = mode_1312x972_vflip_normal,
> > +               },
> > +               .vflip_inverted = {
> > +                       .num_of_regs = ARRAY_SIZE(mode_1312x972_vflip_inverted),
> > +                       .regs = mode_1312x972_vflip_inverted,
> > +               },
> >         },
> >  };
> >  
> > @@ -608,18 +720,22 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
> >  
> >         /* Propagate change of current control to all related controls */
> >         if (ctrl->id == V4L2_CID_VBLANK) {
> > +               u32 shutter_min = IMX335_SHUTTER_MIN;
> > +               u32 lpfr;
> > +
> >                 imx335->vblank = imx335->vblank_ctrl->val;
> > +               lpfr = imx335->vblank + imx335->cur_mode->height;
> >  
> >                 dev_dbg(imx335->dev, "Received vblank %u, new lpfr %u\n",
> > -                       imx335->vblank,
> > -                       imx335->vblank + imx335->cur_mode->height);
> > +                       imx335->vblank, lpfr);
> > +
> > +               if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
> > +                       shutter_min = IMX335_SHUTTER_MIN_BINNED;
> >  
> >                 ret = __v4l2_ctrl_modify_range(imx335->exp_ctrl,
> >                                                IMX335_EXPOSURE_MIN,
> > -                                              imx335->vblank +
> > -                                              imx335->cur_mode->height -
> > -                                              IMX335_EXPOSURE_OFFSET,
> > -                                              1, IMX335_EXPOSURE_DEFAULT);
> > +                                              lpfr - shutter_min, 1,
> > +                                              IMX335_EXPOSURE_DEFAULT);
> >                 if (ret)
> >                         return ret;
> >         }
> > @@ -719,17 +835,16 @@ static int imx335_enum_frame_size(struct v4l2_subdev *sd,
> >         struct imx335 *imx335 = to_imx335(sd);
> >         u32 code;
> >  
> > -       /* Only a single supported_mode available. */
> > -       if (fsize->index > 0)
> > +       if (fsize->index >= ARRAY_SIZE(supported_modes))
> >                 return -EINVAL;
> >  
> >         code = imx335_get_format_code(imx335, fsize->code);
> >         if (fsize->code != code)
> >                 return -EINVAL;
> >  
> > -       fsize->min_width = supported_mode.width;
> > +       fsize->min_width = supported_modes[fsize->index].width;
> >         fsize->max_width = fsize->min_width;
> > -       fsize->min_height = supported_mode.height;
> > +       fsize->min_height = supported_modes[fsize->index].height;
> >         fsize->max_height = fsize->min_height;
> >  
> >         return 0;
> > @@ -771,9 +886,13 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
> >         struct imx335 *imx335 = to_imx335(sd);
> >         struct v4l2_mbus_framefmt *format;
> >         const struct imx335_mode *mode;
> > +       struct v4l2_rect *crop;
> >         int i, ret = 0;
> >  
> > -       mode = &supported_mode;
> > +       mode = v4l2_find_nearest_size(supported_modes,
> > +                                     ARRAY_SIZE(supported_modes),
> > +                                     width, height,
> > +                                     fmt->format.width, fmt->format.height);
> >  
> >         for (i = 0; i < ARRAY_SIZE(imx335_mbus_codes); i++) {
> >                 if (imx335_mbus_codes[i] == fmt->format.code)
> > @@ -785,6 +904,16 @@ static int imx335_set_pad_format(struct v4l2_subdev *sd,
> >         format = v4l2_subdev_state_get_format(sd_state, fmt->pad);
> >         *format = fmt->format;
> >  
> > +       crop = v4l2_subdev_state_get_crop(sd_state, fmt->pad);
> > +       crop->width = fmt->format.width;
> > +       crop->height = fmt->format.height;
> > +       if (mode->scan_mode == IMX335_2_2_BINNING) {
> > +               crop->width *= 2;
> > +               crop->height *= 2;
> > +       }
> > +       crop->left = (IMX335_NATIVE_WIDTH - crop->width) / 2;
> > +       crop->top = (IMX335_NATIVE_HEIGHT - crop->height) / 2;
> > +
> >         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >                 ret = imx335_update_controls(imx335, mode);
> >                 if (!ret)
> > @@ -808,7 +937,7 @@ static int imx335_init_state(struct v4l2_subdev *sd,
> >         struct v4l2_subdev_format fmt = { 0 };
> >  
> >         fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > -       imx335_fill_pad_format(imx335, &supported_mode, &fmt);
> > +       imx335_fill_pad_format(imx335, &supported_modes[0], &fmt);
> >  
> >         __v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
> >                                  __fls(imx335->link_freq_bitmap),
> > @@ -831,6 +960,11 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
> >                                 struct v4l2_subdev_selection *sel)
> >  {
> >         switch (sel->target) {
> > +       case V4L2_SEL_TGT_CROP:
> > +               sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> > +
> > +               return 0;
> > +
> >         case V4L2_SEL_TGT_NATIVE_SIZE:
> >                 sel->r.top = 0;
> >                 sel->r.left = 0;
> > @@ -839,7 +973,6 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
> >  
> >                 return 0;
> >  
> > -       case V4L2_SEL_TGT_CROP:
> >         case V4L2_SEL_TGT_CROP_DEFAULT:
> >         case V4L2_SEL_TGT_CROP_BOUNDS:
> >                 sel->r.top = IMX335_PIXEL_ARRAY_TOP;
> > @@ -855,19 +988,35 @@ static int imx335_get_selection(struct v4l2_subdev *sd,
> >  
> >  static int imx335_set_framefmt(struct imx335 *imx335)
> >  {
> > -       switch (imx335->cur_mbus_code) {
> > -       case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -               return cci_multi_reg_write(imx335->cci, raw10_framefmt_regs,
> > -                                          ARRAY_SIZE(raw10_framefmt_regs),
> > -                                          NULL);
> > -
> > -       case MEDIA_BUS_FMT_SRGGB12_1X12:
> > -               return cci_multi_reg_write(imx335->cci, raw12_framefmt_regs,
> > -                                          ARRAY_SIZE(raw12_framefmt_regs),
> > -                                          NULL);
> > +       /*
> > +        * In the all-pixel scan mode the AD conversion shall match the output
> > +        * bit width requested.
> > +        *
> > +        * However, when 2/2 binning is enabled, the AD conversion is always
> > +        * 10-bit, so we ensure ADBIT is clear and ADBIT1 is assigned 0x1ff.
> > +        * That's as much as the documentation gives us...
> > +        */
> > +       int ret = 0;
> > +       u8 bpp = imx335->cur_mbus_code == MEDIA_BUS_FMT_SRGGB10_1X10 ? 10 : 12;
> > +       u8 ad_conv = bpp;
> > +
> > +       /* Start with the output mode */
> > +       cci_write(imx335->cci, IMX335_REG_MDBIT, bpp == 12, &ret);
> > +
> > +       /* Enforce 10 bit AD on binning modes */
> > +       if (imx335->cur_mode->scan_mode == IMX335_2_2_BINNING)
> > +               ad_conv = 10;
> > +
> > +       /* AD Conversion configuration */
> > +       if (ad_conv == 10) {
> > +               cci_write(imx335->cci, IMX335_REG_ADBIT, 0x00, &ret);
> > +               cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x1ff, &ret);
> > +       } else { /* 12 bit AD Conversion */
> > +               cci_write(imx335->cci, IMX335_REG_ADBIT, 0x01, &ret);
> > +               cci_write(imx335->cci, IMX335_REG_ADBIT1, 0x47, &ret);
> >         }
> >  
> > -       return -EINVAL;
> > +       return ret;
> >  }
> >  
> >  /**
> > @@ -903,6 +1052,14 @@ static int imx335_start_streaming(struct imx335 *imx335)
> >                 goto err_rpm_put;
> >         }
> >  
> > +       /* Write sensor common registers */
> > +       ret = cci_multi_reg_write(imx335->cci, imx335_common_regs,
> > +                                 ARRAY_SIZE(imx335_common_regs), NULL);
> > +       if (ret) {
> > +               dev_err(imx335->dev, "fail to write initial registers\n");
> > +               goto err_rpm_put;
> > +       }
> > +
> >         ret = imx335_set_framefmt(imx335);
> >         if (ret) {
> >                 dev_err(imx335->dev, "%s failed to set frame format: %d\n",
> > @@ -1186,7 +1343,7 @@ static int imx335_init_controls(struct imx335 *imx335)
> >         struct v4l2_ctrl_handler *ctrl_hdlr = &imx335->ctrl_handler;
> >         const struct imx335_mode *mode = imx335->cur_mode;
> >         struct v4l2_fwnode_device_properties props;
> > -       u32 lpfr;
> > +       u32 lpfr, shutter_min;
> >         int ret;
> >  
> >         ret = v4l2_fwnode_device_parse(imx335->dev, &props);
> > @@ -1200,11 +1357,14 @@ static int imx335_init_controls(struct imx335 *imx335)
> >  
> >         /* Initialize exposure and gain */
> >         lpfr = mode->vblank + mode->height;
> > +       shutter_min = IMX335_SHUTTER_MIN;
> > +       if (mode->scan_mode == IMX335_2_2_BINNING)
> > +               shutter_min = IMX335_SHUTTER_MIN_BINNED;
> >         imx335->exp_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> >                                              &imx335_ctrl_ops,
> >                                              V4L2_CID_EXPOSURE,
> >                                              IMX335_EXPOSURE_MIN,
> > -                                            lpfr - IMX335_EXPOSURE_OFFSET,
> > +                                            lpfr - shutter_min,
> >                                              IMX335_EXPOSURE_STEP,
> >                                              IMX335_EXPOSURE_DEFAULT);
> >  
> > @@ -1331,7 +1491,7 @@ static int imx335_probe(struct i2c_client *client)
> >         }
> >  
> >         /* Set default mode to max resolution */
> > -       imx335->cur_mode = &supported_mode;
> > +       imx335->cur_mode = &supported_modes[0];
> >         imx335->cur_mbus_code = imx335_mbus_codes[0];
> >         imx335->vblank = imx335->cur_mode->vblank;
> >  
> > 
> > -- 
> > 2.50.1
> >

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEETeDYGOXVdejUWq/FQ96R+SSacUUFAmikIgYACgkQQ96R+SSa
cUUjpRAAwJ8lABhLGnVH6LQvLaPf06pRHFo/ToeBkEx2s0KhloWR0baieM/OC6fw
e0ImsAACP5vIrUgXziDSKq4Vnl/3Z6ya8gJH7Kos2X4rCn1trbCIcXqOVdOiAMsU
KmvIIOHO+oeI1V5g0D30aeOxTFAXTCoX3HIXVclMd6K1mqxLx9V5Sj5TwXShdOdV
12zItBh87WXvoJEzk1ubC+l303qv8N9sFOKHTuR1uZVgN3RW2yBSV4/ZRRlsCLGx
yxczxU4AszlE2ZACFRSTUdQSewI2hRftJs6PpUyCYQa75D8ESe0O/Gj0tNm4H2xq
0GZnf5LOYaGPw/dEV42kZxB/luIDUU+yqDuM0u3RuOSGkFKXAjPagrE/whKqCfQ5
gpzYKm+F6SMBUaQJM43ekTSov0kWgi+3qAFahMFAZaeloNqLWeIVG9Q56WR3twvn
2g8QRemxT92Lz2Rpc4XskA9eph9wZdEadWRosBjNnTmT193y01F30cp1thFoVcKh
39nUsAe06HLAnDAwZWXnZR/amS6hbt3Z1rDSXeedi/DnXjSRnMvkg5mVC55f6yt5
f888ebGAXrViCrWbkx+UHj4P+w1cHuUpK9vKB7r/L5pbCThrpTfKjDydF8F3UJKA
ykgW3vzfxyodoM2vlyTHZavAkEkfLr838rT5DOOfqaKv2SzDW94=
=CMMe
-----END PGP SIGNATURE-----