[PATCH v3 00/17] media: i2c: os05b10: Refactor driver and Add new features

[PATCH v3 00/17] media: i2c: os05b10: Refactor driver and Add new features

[Tarang Raval]

This series improves the OS05B10 sensor driver with cleanup,
refactoring, additional controls, and support for new modes.

Patch 1 Use pm_runtime_get_if_active() when applying controls.

Patches 2–3 perform initial cleanup and refactoring. Unused
group-hold programming is removed, register definitions are
introduced, and the initialization tables are reorganized by
splitting common and mode-specific registers.

Patches 5–7 add additional V4L2 controls including digital gain,
horizontal/vertical flip, and test pattern support.

Patches 8–10 extend the driver with 12-bit RAW support, update
pixel rate handling when switching between 10-bit and 12-bit
modes, and add new operating modes including 1080p and 2x2
binned 720p.

Patches 11–13/17 improve control handling and internal logic by
keeping vblank and exposure in sync on mode changes, updating
the active format before adjusting framing controls, and
renaming the vmax variable used by the VBLANK control. Remove unused
controls.

Patch 14 adds support for 2-lane MIPI CSI-2 operation.

Patch 15 fix negative hblank calculation.

Patch 16 Enable runtime PM autosuspend

All patches were tested on the Debix Model A board running
Linux kernel v7.0-rc2.

Changelog:

v2 -> v3

- Patch 1/17: Drop Fixes:/Cc: stable tags; use "if (!pm_runtime_get_if_active(...))"
- Patch 3/17: Drop unused HDR define; use GAIN_DEFAULT macro;
  DIGITAL_GAIN_STEP 16 -> 1 (tested, not in datasheet)
- Patch 4/17: Drop unrelated cosmetic reordering
- Patch 6/17: Use OS05B10_MIRROR/OS05B10_FLIP macros instead of GENMASK
- Patch 7/17: Clarify 0x5080 removal in commit msg; fix test_pattern_menu[]
  casing
- Patch 8/17: Merge codes_12/codes_10 into one table; handle NULL mode list
- Add new Patch 16/17 & 17/17: Enable runtime PM autosuspend; remove unused
  control fields/simplify

Link to v2: https://lore.kernel.org/linux-media/20260325114404.95188-1-tarang.raval@siliconsignals.io/T/#t

v1 -> v2
- Added a separate patch to fix the use of pm_runtime_get_if_active().
- Modified patch 11 Remove unneccery exposure range update now just modifiyng one time only instead of twice.
- Add One patch for fix negative hblank calculation

Link to v1: https://lore.kernel.org/linux-media/20260306123304.76722-1-tarang.raval@siliconsignals.io/T/#t

Tarang Raval (17):
  media: i2c: os05b10: Use pm_runtime_get_if_active() when applying
    controls
  media: i2c: os05b10: drop unused group-hold programming
  media: i2c: os05b10: add register definitions and use them in init
    table
  media: i2c: os05b10: split common and mode-specific init registers
  media: i2c: os05b10: add V4L2 digital gain control
  media: i2c: os05b10: Add H/V flip support
  media: i2c: os05b10: Add test pattern options
  media: i2c: os05b10: add 12-bit RAW mode support
  media: i2c: os05b10: update pixel rate on 10/12-bit mode switch
  media: i2c: os05b10: Add 1080p and 2x2 binning 720p modes
  media: i2c: os05b10: keep vblank and exposure range in sync on mode
    switch
  media: i2c: os05b10: Update active format before adjusting framing
    controls
  media: i2c: os05b10: Rename vmax variable in VBLANK control
  media: i2c: os05b10: add 2-lane support
  media: i2c: os05b10: fix negative hblank calculation
  media: i2c: os05b10: Enable runtime PM autosuspend
  media: i2c: os05b10: remove unused control fields, simplify error
    handling

 drivers/media/i2c/os05b10.c | 744 +++++++++++++++++++++++++++---------
 1 file changed, 570 insertions(+), 174 deletions(-)

--
2.34.1

[PATCH v3 01/17] media: i2c: os05b10: Use pm_runtime_get_if_active() when applying controls

[Tarang Raval]

os05b10_set_ctrl() currently uses pm_runtime_get_if_in_use() to decide
whether controls should be applied to hardware.

Use pm_runtime_get_if_active() instead so controls are applied whenever
the device is runtime-active, regardless of the current usage count.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
---
 drivers/media/i2c/os05b10.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index e0453c988e4a..3e0440220c07 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -531,7 +531,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 			return ret;
 	}
 
-	if (pm_runtime_get_if_in_use(os05b10->dev) == 0)
+	if (!pm_runtime_get_if_active(os05b10->dev))
 		return 0;
 
 	switch (ctrl->id) {
-- 
2.34.1

Re: [PATCH v3 01/17] media: i2c: os05b10: Use pm_runtime_get_if_active() when applying controls

[Jai Luthra]

Quoting Tarang Raval (2026-07-19 01:38:52)
> os05b10_set_ctrl() currently uses pm_runtime_get_if_in_use() to decide
> whether controls should be applied to hardware.
> 
> Use pm_runtime_get_if_active() instead so controls are applied whenever
> the device is runtime-active, regardless of the current usage count.

What was your motivation behind this change?

This is a bit wasteful, because this driver unconditionally calls
__v4l2_ctrl_handler_setup() in enable_streams(), along with writing the
whole register table again, without checking if the device was active or
sleeping. So updating some control registers in set_ctrl() seems like doing
double work IMHO.

Thanks,
    Jai

> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> ---
>  drivers/media/i2c/os05b10.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index e0453c988e4a..3e0440220c07 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -531,7 +531,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>                         return ret;
>         }
>  
> -       if (pm_runtime_get_if_in_use(os05b10->dev) == 0)
> +       if (!pm_runtime_get_if_active(os05b10->dev))
>                 return 0;
>  
>         switch (ctrl->id) {
> -- 
> 2.34.1
> 
>

Re: [PATCH v3 01/17] media: i2c: os05b10: Use pm_runtime_get_if_active() when applying controls

[Tarang Raval]

Hi Jai,                                                                        
                                                                               
> Quoting Tarang Raval (2026-07-19 01:38:52)                                   
> > os05b10_set_ctrl() currently uses pm_runtime_get_if_in_use() to decide     
> > whether controls should be applied to hardware.                            
> >                                                                            
> > Use pm_runtime_get_if_active() instead so controls are applied whenever    
> > the device is runtime-active, regardless of the current usage count.       
>                                                                              
> What was your motivation behind this change?                                 
>                                                                              
> This is a bit wasteful, because this driver unconditionally calls            
> __v4l2_ctrl_handler_setup() in enable_streams(), along with writing the      
> whole register table again, without checking if the device was active or     
> sleeping. So updating some control registers in set_ctrl() seems like doing  
> double work IMHO.                                                            
                                                                               
I will drop this patch in v4.                                                  
                                                                               
Based on our previous discussion on this topic during the IMX678 driver 
series, I agree that this change is unnecessary.                                                      
                                                                               
Best Regards,                                                                  
Tarang                                                                         

[PATCH v3 02/17] media: i2c: os05b10: drop unused group-hold programming

[Tarang Raval]

Register table included group-hold (0x3208) sequences for
groups 6/7/8/9 that only stage alternative tuning values in SRAM but are
never launched by the driver. Remove these group-hold blocks.

Also remove a duplicate register entry for 0x37bf.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
---
 drivers/media/i2c/os05b10.c | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 3e0440220c07..84621370d1da 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -250,7 +250,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x37ab), 0x0e },
 	{ CCI_REG8(0x37ac), 0xa0 },
 	{ CCI_REG8(0x37be), 0x0a },
-	{ CCI_REG8(0x37bf), 0x05 },
 	{ CCI_REG8(0x37bb), 0x02 },
 	{ CCI_REG8(0x37bf), 0x05 },
 	{ CCI_REG8(0x37c2), 0x04 },
@@ -415,42 +414,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x5820), 0x00 },
 	{ CCI_REG8(0x5821), 0x00 },
 	{ CCI_REG8(0x3222), 0x03 },
-	{ CCI_REG8(0x3208), 0x06 },
-	{ CCI_REG8(0x3701), 0x1d },
-	{ CCI_REG8(0x37ab), 0x01 },
-	{ CCI_REG8(0x3790), 0x21 },
-	{ CCI_REG8(0x38be), 0x00 },
-	{ CCI_REG8(0x3791), 0x5a },
-	{ CCI_REG8(0x37bf), 0x1c },
-	{ CCI_REG8(0x3610), 0x37 },
-	{ CCI_REG8(0x3208), 0x16 },
-	{ CCI_REG8(0x3208), 0x07 },
-	{ CCI_REG8(0x3701), 0x1d },
-	{ CCI_REG8(0x37ab), 0x0e },
-	{ CCI_REG8(0x3790), 0x21 },
-	{ CCI_REG8(0x38be), 0x00 },
-	{ CCI_REG8(0x3791), 0x5a },
-	{ CCI_REG8(0x37bf), 0x0a },
-	{ CCI_REG8(0x3610), 0x87 },
-	{ CCI_REG8(0x3208), 0x17 },
-	{ CCI_REG8(0x3208), 0x08 },
-	{ CCI_REG8(0x3701), 0x1d },
-	{ CCI_REG8(0x37ab), 0x0e },
-	{ CCI_REG8(0x3790), 0x21 },
-	{ CCI_REG8(0x38be), 0x00 },
-	{ CCI_REG8(0x3791), 0x5a },
-	{ CCI_REG8(0x37bf), 0x0a },
-	{ CCI_REG8(0x3610), 0x87 },
-	{ CCI_REG8(0x3208), 0x18 },
-	{ CCI_REG8(0x3208), 0x09 },
-	{ CCI_REG8(0x3701), 0x1d },
-	{ CCI_REG8(0x37ab), 0x0e },
-	{ CCI_REG8(0x3790), 0x28 },
-	{ CCI_REG8(0x38be), 0x00 },
-	{ CCI_REG8(0x3791), 0x63 },
-	{ CCI_REG8(0x37bf), 0x0a },
-	{ CCI_REG8(0x3610), 0x87 },
-	{ CCI_REG8(0x3208), 0x19 },
 };
 
 struct os05b10 {
-- 
2.34.1

[PATCH v3 03/17] media: i2c: os05b10: add register definitions and use them in init table

[Tarang Raval]

Define named register macros for OS05B10 and replace raw register
addresses in the common initialization array with the new definitions.
This improves readability and maintainability without changing
functionality.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 110 +++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 40 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 84621370d1da..96395cd7a2ca 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -38,6 +38,20 @@
 #define OS05B10_MODE_STANDBY		0x00
 #define OS05B10_MODE_STREAMING		0x01
 
+#define OS05B10_REG_PLL_CTRL_01		CCI_REG8(0x0301)
+#define OS05B10_REG_PLL_CTRL_03		CCI_REG8(0x0303)
+#define OS05B10_REG_PLL_CTRL_05		CCI_REG8(0x0305)
+#define OS05B10_REG_PLL_CTRL_06		CCI_REG8(0x0306)
+#define OS05B10_REG_PLL_CTRL_25		CCI_REG8(0x0325)
+
+#define OS05B10_REG_MIPI_SC_CTRL	CCI_REG8(0x3016)
+#define OS05B10_4_LANE_MODE		0x72
+#define OS05B10_2_LANE_MODE		0x32
+
+#define OS05B10_REG_MIPI_SC_CTRL_1	CCI_REG8(0x3022)
+#define OS05B10_10BIT_MODE		0x01
+#define OS05B10_12BIT_MODE		0x61
+
 #define OS05B10_REG_EXPOSURE		CCI_REG24(0x3500)
 #define OS05B10_EXPOSURE_MIN		2
 #define OS05B10_EXPOSURE_STEP		1
@@ -49,11 +63,41 @@
 #define OS05B10_ANALOG_GAIN_STEP	1
 #define OS05B10_ANALOG_GAIN_DEFAULT	0x80
 
+#define OS05B10_REG_DIGITAL_GAIN	CCI_REG16(0x350a)
+#define OS05B10_DIGITAL_GAIN_MIN	0x400
+#define OS05B10_DIGITAL_GAIN_MAX	0x3fff
+#define OS05B10_DIGITAL_GAIN_STEP	1
+#define OS05B10_DIGITAL_GAIN_DEFAULT	0x400
+
+#define OS05B10_REG_ANALOG_GAIN_SHORT	CCI_REG16(0x350c)
+#define OS05B10_REG_DIGITAL_GAIN_SHORT	CCI_REG16(0x350e)
+#define OS05B10_REG_EXPOSURE_SHORT	CCI_REG24(0x3510)
+
+#define OS05B10_REG_X_ADDR_START	CCI_REG16(0x3800)
+#define OS05B10_REG_Y_ADDR_START	CCI_REG16(0x3802)
+#define OS05B10_REG_X_ADDR_END		CCI_REG16(0x3804)
+#define OS05B10_REG_Y_ADDR_END		CCI_REG16(0x3806)
+#define OS05B10_REG_X_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OS05B10_REG_Y_OUTPUT_SIZE	CCI_REG16(0x380a)
+
 #define OS05B10_REG_HTS			CCI_REG16(0x380c)
 
 #define OS05B10_REG_VTS			CCI_REG16(0x380e)
 #define OS05B10_VTS_MAX			0x7fff
 
+#define OS05B10_REG_ISP_X_WIN		CCI_REG16(0x3810)
+#define OS05B10_REG_ISP_Y_WIN		CCI_REG16(0x3812)
+#define OS05B10_REG_X_INC_ODD		CCI_REG8(0x3814)
+#define OS05B10_REG_X_INC_EVEN		CCI_REG8(0x3815)
+#define OS05B10_REG_Y_INC_ODD		CCI_REG8(0x3816)
+#define OS05B10_REG_Y_INC_EVEN		CCI_REG8(0x3817)
+
+#define OS05B10_REG_FORMAT1		CCI_REG8(0x3820)
+#define OS05B10_MIRROR			BIT(3)
+#define OS05B10_FLIP			GENMASK(5, 4)
+
+#define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
+
 #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
 
 static const struct v4l2_rect os05b10_native_area = {
@@ -77,30 +121,25 @@ static const char * const os05b10_supply_name[] = {
 };
 
 static const struct cci_reg_sequence os05b10_common_regs[] = {
-	{ CCI_REG8(0x0301), 0x44 },
-	{ CCI_REG8(0x0303), 0x02 },
-	{ CCI_REG8(0x0305), 0x32 },
-	{ CCI_REG8(0x0306), 0x00 },
-	{ CCI_REG8(0x0325), 0x3b },
+	{ OS05B10_REG_PLL_CTRL_01, 0x44 },
+	{ OS05B10_REG_PLL_CTRL_03, 0x02 },
+	{ OS05B10_REG_PLL_CTRL_05, 0x32 },
+	{ OS05B10_REG_PLL_CTRL_06, 0x00 },
+	{ OS05B10_REG_PLL_CTRL_25, 0x3b },
 	{ CCI_REG8(0x3002), 0x21 },
-	{ CCI_REG8(0x3016), 0x72 },
+	{ OS05B10_REG_MIPI_SC_CTRL, 0x72 },
 	{ CCI_REG8(0x301e), 0xb4 },
 	{ CCI_REG8(0x301f), 0xd0 },
 	{ CCI_REG8(0x3021), 0x03 },
-	{ CCI_REG8(0x3022), 0x01 },
+	{ OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
 	{ CCI_REG8(0x3107), 0xa1 },
 	{ CCI_REG8(0x3108), 0x7d },
 	{ CCI_REG8(0x3109), 0xfc },
 	{ CCI_REG8(0x3503), 0x88 },
-	{ CCI_REG8(0x350a), 0x04 },
-	{ CCI_REG8(0x350b), 0x00 },
-	{ CCI_REG8(0x350c), 0x00 },
-	{ CCI_REG8(0x350d), 0x80 },
-	{ CCI_REG8(0x350e), 0x04 },
-	{ CCI_REG8(0x350f), 0x00 },
-	{ CCI_REG8(0x3510), 0x00 },
-	{ CCI_REG8(0x3511), 0x00 },
-	{ CCI_REG8(0x3512), 0x20 },
+	{ OS05B10_REG_DIGITAL_GAIN, 0x0400 },
+	{ OS05B10_REG_ANALOG_GAIN_SHORT, 0x0080 },
+	{ OS05B10_REG_DIGITAL_GAIN_SHORT, OS05B10_DIGITAL_GAIN_DEFAULT },
+	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
 	{ CCI_REG8(0x3600), 0x4d },
 	{ CCI_REG8(0x3601), 0x08 },
 	{ CCI_REG8(0x3610), 0x87 },
@@ -274,34 +313,25 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x37f5), 0x00 },
 	{ CCI_REG8(0x37f6), 0x00 },
 	{ CCI_REG8(0x37f7), 0x00 },
-	{ CCI_REG8(0x3800), 0x01 },
-	{ CCI_REG8(0x3801), 0x30 },
-	{ CCI_REG8(0x3802), 0x00 },
-	{ CCI_REG8(0x3803), 0x00 },
-	{ CCI_REG8(0x3804), 0x0b },
-	{ CCI_REG8(0x3805), 0x5f },
-	{ CCI_REG8(0x3806), 0x07 },
-	{ CCI_REG8(0x3807), 0xa7 },
-	{ CCI_REG8(0x3808), 0x0a },
-	{ CCI_REG8(0x3809), 0x20 },
-	{ CCI_REG8(0x380a), 0x07 },
-	{ CCI_REG8(0x380b), 0x98 },
-	{ CCI_REG8(0x380c), 0x06 },
-	{ CCI_REG8(0x380d), 0xd0 },
-	{ CCI_REG8(0x3810), 0x00 },
-	{ CCI_REG8(0x3811), 0x08 },
-	{ CCI_REG8(0x3812), 0x00 },
-	{ CCI_REG8(0x3813), 0x08 },
-	{ CCI_REG8(0x3814), 0x01 },
-	{ CCI_REG8(0x3815), 0x01 },
-	{ CCI_REG8(0x3816), 0x01 },
-	{ CCI_REG8(0x3817), 0x01 },
+	{ OS05B10_REG_X_ADDR_START, 0x0130 },
+	{ OS05B10_REG_Y_ADDR_START, 0x0000 },
+	{ OS05B10_REG_X_ADDR_END, 0x0b5f },
+	{ OS05B10_REG_Y_ADDR_END, 0x07a7 },
+	{ OS05B10_REG_X_OUTPUT_SIZE, 0x0a20 },
+	{ OS05B10_REG_Y_OUTPUT_SIZE, 0x0798 },
+	{ OS05B10_REG_HTS, 0x06d0 },
+	{ OS05B10_REG_ISP_X_WIN, 0x0008 },
+	{ OS05B10_REG_ISP_Y_WIN, 0x0008 },
+	{ OS05B10_REG_X_INC_ODD, 0x01 },
+	{ OS05B10_REG_X_INC_EVEN, 0x01 },
+	{ OS05B10_REG_Y_INC_ODD, 0x01 },
+	{ OS05B10_REG_Y_INC_EVEN, 0x01 },
 	{ CCI_REG8(0x3818), 0x00 },
 	{ CCI_REG8(0x3819), 0x00 },
 	{ CCI_REG8(0x381a), 0x00 },
 	{ CCI_REG8(0x381b), 0x01 },
-	{ CCI_REG8(0x3820), 0x88 },
-	{ CCI_REG8(0x3821), 0x00 },
+	{ OS05B10_REG_FORMAT1, 0x88 },
+	{ OS05B10_REG_FORMAT2, 0x00 },
 	{ CCI_REG8(0x3822), 0x12 },
 	{ CCI_REG8(0x3823), 0x08 },
 	{ CCI_REG8(0x3824), 0x00 },
-- 
2.34.1

Re: [PATCH v3 03/17] media: i2c: os05b10: add register definitions and use them in init table

[Mehdi Djait]

Hi Tarang,

On Sun, Jul 19, 2026 at 01:38:54AM +0530, Tarang Raval wrote:
> Define named register macros for OS05B10 and replace raw register
> addresses in the common initialization array with the new definitions.
> This improves readability and maintainability without changing
> functionality.
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 03/17] media: i2c: os05b10: add register definitions and use them in init table

[Vladimir Zapolskiy]

Hi Tarang.

On 7/18/26 23:08, Tarang Raval wrote:
> Define named register macros for OS05B10 and replace raw register
> addresses in the common initialization array with the new definitions.
> This improves readability and maintainability without changing
> functionality.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 110 +++++++++++++++++++++++-------------
>   1 file changed, 70 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 84621370d1da..96395cd7a2ca 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -38,6 +38,20 @@
>   #define OS05B10_MODE_STANDBY		0x00
>   #define OS05B10_MODE_STREAMING		0x01
>   
> +#define OS05B10_REG_PLL_CTRL_01		CCI_REG8(0x0301)
> +#define OS05B10_REG_PLL_CTRL_03		CCI_REG8(0x0303)
> +#define OS05B10_REG_PLL_CTRL_05		CCI_REG8(0x0305)
> +#define OS05B10_REG_PLL_CTRL_06		CCI_REG8(0x0306)
> +#define OS05B10_REG_PLL_CTRL_25		CCI_REG8(0x0325)

Here I see a potential to remove a few lines by announcing
a parameterised macro like

#define OS05B10_REG_PLL_CTRL(n)		CCI_REG8(0x0300 + (n))

> +
> +#define OS05B10_REG_MIPI_SC_CTRL	CCI_REG8(0x3016)
> +#define OS05B10_4_LANE_MODE		0x72
> +#define OS05B10_2_LANE_MODE		0x32
> +
> +#define OS05B10_REG_MIPI_SC_CTRL_1	CCI_REG8(0x3022)
> +#define OS05B10_10BIT_MODE		0x01
> +#define OS05B10_12BIT_MODE		0x61
> +
>   #define OS05B10_REG_EXPOSURE		CCI_REG24(0x3500)
>   #define OS05B10_EXPOSURE_MIN		2
>   #define OS05B10_EXPOSURE_STEP		1
> @@ -49,11 +63,41 @@
>   #define OS05B10_ANALOG_GAIN_STEP	1
>   #define OS05B10_ANALOG_GAIN_DEFAULT	0x80
>   
> +#define OS05B10_REG_DIGITAL_GAIN	CCI_REG16(0x350a)
> +#define OS05B10_DIGITAL_GAIN_MIN	0x400
> +#define OS05B10_DIGITAL_GAIN_MAX	0x3fff
> +#define OS05B10_DIGITAL_GAIN_STEP	1
> +#define OS05B10_DIGITAL_GAIN_DEFAULT	0x400

#define OS05B10_DIGITAL_GAIN_DEFAULT	OS05B10_DIGITAL_GAIN_MIN

Or just use OS05B10_DIGITAL_GAIN_MIN whenever default one is needed.

> +
> +#define OS05B10_REG_ANALOG_GAIN_SHORT	CCI_REG16(0x350c)
> +#define OS05B10_REG_DIGITAL_GAIN_SHORT	CCI_REG16(0x350e)
> +#define OS05B10_REG_EXPOSURE_SHORT	CCI_REG24(0x3510)
> +
> +#define OS05B10_REG_X_ADDR_START	CCI_REG16(0x3800)
> +#define OS05B10_REG_Y_ADDR_START	CCI_REG16(0x3802)
> +#define OS05B10_REG_X_ADDR_END		CCI_REG16(0x3804)
> +#define OS05B10_REG_Y_ADDR_END		CCI_REG16(0x3806)
> +#define OS05B10_REG_X_OUTPUT_SIZE	CCI_REG16(0x3808)
> +#define OS05B10_REG_Y_OUTPUT_SIZE	CCI_REG16(0x380a)
> +
>   #define OS05B10_REG_HTS			CCI_REG16(0x380c)
>   
>   #define OS05B10_REG_VTS			CCI_REG16(0x380e)
>   #define OS05B10_VTS_MAX			0x7fff
>   
> +#define OS05B10_REG_ISP_X_WIN		CCI_REG16(0x3810)
> +#define OS05B10_REG_ISP_Y_WIN		CCI_REG16(0x3812)
> +#define OS05B10_REG_X_INC_ODD		CCI_REG8(0x3814)
> +#define OS05B10_REG_X_INC_EVEN		CCI_REG8(0x3815)
> +#define OS05B10_REG_Y_INC_ODD		CCI_REG8(0x3816)
> +#define OS05B10_REG_Y_INC_EVEN		CCI_REG8(0x3817)
> +
> +#define OS05B10_REG_FORMAT1		CCI_REG8(0x3820)
> +#define OS05B10_MIRROR			BIT(3)
> +#define OS05B10_FLIP			GENMASK(5, 4)
> +
> +#define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
> +
>   #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
>   
>   static const struct v4l2_rect os05b10_native_area = {
> @@ -77,30 +121,25 @@ static const char * const os05b10_supply_name[] = {
>   };
>   
>   static const struct cci_reg_sequence os05b10_common_regs[] = {
> -	{ CCI_REG8(0x0301), 0x44 },
> -	{ CCI_REG8(0x0303), 0x02 },
> -	{ CCI_REG8(0x0305), 0x32 },
> -	{ CCI_REG8(0x0306), 0x00 },
> -	{ CCI_REG8(0x0325), 0x3b },
> +	{ OS05B10_REG_PLL_CTRL_01, 0x44 },
> +	{ OS05B10_REG_PLL_CTRL_03, 0x02 },
> +	{ OS05B10_REG_PLL_CTRL_05, 0x32 },
> +	{ OS05B10_REG_PLL_CTRL_06, 0x00 },
> +	{ OS05B10_REG_PLL_CTRL_25, 0x3b },
>   	{ CCI_REG8(0x3002), 0x21 },
> -	{ CCI_REG8(0x3016), 0x72 },
> +	{ OS05B10_REG_MIPI_SC_CTRL, 0x72 },

{ OS05B10_REG_MIPI_SC_CTRL, OS05B10_4_LANE_MODE },

>   	{ CCI_REG8(0x301e), 0xb4 },
>   	{ CCI_REG8(0x301f), 0xd0 },
>   	{ CCI_REG8(0x3021), 0x03 },
> -	{ CCI_REG8(0x3022), 0x01 },
> +	{ OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },

{ OS05B10_REG_MIPI_SC_CTRL_1, OS05B10_10BIT_MODE },

>   	{ CCI_REG8(0x3107), 0xa1 },
>   	{ CCI_REG8(0x3108), 0x7d },
>   	{ CCI_REG8(0x3109), 0xfc },
>   	{ CCI_REG8(0x3503), 0x88 },
> -	{ CCI_REG8(0x350a), 0x04 },
> -	{ CCI_REG8(0x350b), 0x00 },
> -	{ CCI_REG8(0x350c), 0x00 },
> -	{ CCI_REG8(0x350d), 0x80 },
> -	{ CCI_REG8(0x350e), 0x04 },
> -	{ CCI_REG8(0x350f), 0x00 },
> -	{ CCI_REG8(0x3510), 0x00 },
> -	{ CCI_REG8(0x3511), 0x00 },
> -	{ CCI_REG8(0x3512), 0x20 },
> +	{ OS05B10_REG_DIGITAL_GAIN, 0x0400 },

{ OS05B10_REG_DIGITAL_GAIN, OS05B10_DIGITAL_GAIN_DEFAULT }, // or OS05B10_DIGITAL_GAIN_MIN

> +	{ OS05B10_REG_ANALOG_GAIN_SHORT, 0x0080 },
> +	{ OS05B10_REG_DIGITAL_GAIN_SHORT, OS05B10_DIGITAL_GAIN_DEFAULT },
> +	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
>   	{ CCI_REG8(0x3600), 0x4d },
>   	{ CCI_REG8(0x3601), 0x08 },
>   	{ CCI_REG8(0x3610), 0x87 },
> @@ -274,34 +313,25 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x37f5), 0x00 },
>   	{ CCI_REG8(0x37f6), 0x00 },
>   	{ CCI_REG8(0x37f7), 0x00 },
> -	{ CCI_REG8(0x3800), 0x01 },
> -	{ CCI_REG8(0x3801), 0x30 },
> -	{ CCI_REG8(0x3802), 0x00 },
> -	{ CCI_REG8(0x3803), 0x00 },
> -	{ CCI_REG8(0x3804), 0x0b },
> -	{ CCI_REG8(0x3805), 0x5f },
> -	{ CCI_REG8(0x3806), 0x07 },
> -	{ CCI_REG8(0x3807), 0xa7 },
> -	{ CCI_REG8(0x3808), 0x0a },
> -	{ CCI_REG8(0x3809), 0x20 },
> -	{ CCI_REG8(0x380a), 0x07 },
> -	{ CCI_REG8(0x380b), 0x98 },
> -	{ CCI_REG8(0x380c), 0x06 },
> -	{ CCI_REG8(0x380d), 0xd0 },
> -	{ CCI_REG8(0x3810), 0x00 },
> -	{ CCI_REG8(0x3811), 0x08 },
> -	{ CCI_REG8(0x3812), 0x00 },
> -	{ CCI_REG8(0x3813), 0x08 },
> -	{ CCI_REG8(0x3814), 0x01 },
> -	{ CCI_REG8(0x3815), 0x01 },
> -	{ CCI_REG8(0x3816), 0x01 },
> -	{ CCI_REG8(0x3817), 0x01 },
> +	{ OS05B10_REG_X_ADDR_START, 0x0130 },
> +	{ OS05B10_REG_Y_ADDR_START, 0x0000 },
> +	{ OS05B10_REG_X_ADDR_END, 0x0b5f },
> +	{ OS05B10_REG_Y_ADDR_END, 0x07a7 },
> +	{ OS05B10_REG_X_OUTPUT_SIZE, 0x0a20 },
> +	{ OS05B10_REG_Y_OUTPUT_SIZE, 0x0798 },
> +	{ OS05B10_REG_HTS, 0x06d0 },
> +	{ OS05B10_REG_ISP_X_WIN, 0x0008 },
> +	{ OS05B10_REG_ISP_Y_WIN, 0x0008 },
> +	{ OS05B10_REG_X_INC_ODD, 0x01 },
> +	{ OS05B10_REG_X_INC_EVEN, 0x01 },
> +	{ OS05B10_REG_Y_INC_ODD, 0x01 },
> +	{ OS05B10_REG_Y_INC_EVEN, 0x01 },
>   	{ CCI_REG8(0x3818), 0x00 },
>   	{ CCI_REG8(0x3819), 0x00 },
>   	{ CCI_REG8(0x381a), 0x00 },
>   	{ CCI_REG8(0x381b), 0x01 },
> -	{ CCI_REG8(0x3820), 0x88 },
> -	{ CCI_REG8(0x3821), 0x00 },
> +	{ OS05B10_REG_FORMAT1, 0x88 },
> +	{ OS05B10_REG_FORMAT2, 0x00 },
>   	{ CCI_REG8(0x3822), 0x12 },
>   	{ CCI_REG8(0x3823), 0x08 },
>   	{ CCI_REG8(0x3824), 0x00 },

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 04/17] media: i2c: os05b10: split common and mode-specific init registers

[Tarang Raval]

Separate common initialization registers from mode-specific settings.
Move resolution-dependent registers into a per-mode register list and
program them during stream enable.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 96 ++++++++++++++++++++++++-------------
 1 file changed, 63 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 96395cd7a2ca..12c4e2610d6f 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -142,16 +142,13 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
 	{ CCI_REG8(0x3600), 0x4d },
 	{ CCI_REG8(0x3601), 0x08 },
-	{ CCI_REG8(0x3610), 0x87 },
 	{ CCI_REG8(0x3611), 0x24 },
 	{ CCI_REG8(0x3614), 0x4c },
-	{ CCI_REG8(0x3620), 0x0c },
 	{ CCI_REG8(0x3632), 0x80 },
 	{ CCI_REG8(0x3633), 0x00 },
 	{ CCI_REG8(0x3636), 0xcc },
 	{ CCI_REG8(0x3637), 0x27 },
 	{ CCI_REG8(0x3660), 0x00 },
-	{ CCI_REG8(0x3662), 0x10 },
 	{ CCI_REG8(0x3665), 0x00 },
 	{ CCI_REG8(0x3666), 0x00 },
 	{ CCI_REG8(0x366a), 0x14 },
@@ -235,7 +232,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x370f), 0x1c },
 	{ CCI_REG8(0x3710), 0x00 },
 	{ CCI_REG8(0x3713), 0x00 },
-	{ CCI_REG8(0x3714), 0x24 },
 	{ CCI_REG8(0x3716), 0x24 },
 	{ CCI_REG8(0x371a), 0x1e },
 	{ CCI_REG8(0x3724), 0x09 },
@@ -244,7 +240,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x3730), 0xe1 },
 	{ CCI_REG8(0x3735), 0x80 },
 	{ CCI_REG8(0x3739), 0x10 },
-	{ CCI_REG8(0x373f), 0xb0 },
 	{ CCI_REG8(0x3740), 0x28 },
 	{ CCI_REG8(0x3741), 0x21 },
 	{ CCI_REG8(0x3742), 0x21 },
@@ -290,8 +285,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x37ac), 0xa0 },
 	{ CCI_REG8(0x37be), 0x0a },
 	{ CCI_REG8(0x37bb), 0x02 },
-	{ CCI_REG8(0x37bf), 0x05 },
-	{ CCI_REG8(0x37c2), 0x04 },
 	{ CCI_REG8(0x37c4), 0x11 },
 	{ CCI_REG8(0x37c5), 0x80 },
 	{ CCI_REG8(0x37c6), 0x14 },
@@ -300,7 +293,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x37cd), 0x17 },
 	{ CCI_REG8(0x37ce), 0x01 },
 	{ CCI_REG8(0x37d8), 0x02 },
-	{ CCI_REG8(0x37d9), 0x08 },
 	{ CCI_REG8(0x37dc), 0x01 },
 	{ CCI_REG8(0x37e0), 0x0c },
 	{ CCI_REG8(0x37e1), 0x20 },
@@ -313,25 +305,10 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x37f5), 0x00 },
 	{ CCI_REG8(0x37f6), 0x00 },
 	{ CCI_REG8(0x37f7), 0x00 },
-	{ OS05B10_REG_X_ADDR_START, 0x0130 },
-	{ OS05B10_REG_Y_ADDR_START, 0x0000 },
-	{ OS05B10_REG_X_ADDR_END, 0x0b5f },
-	{ OS05B10_REG_Y_ADDR_END, 0x07a7 },
-	{ OS05B10_REG_X_OUTPUT_SIZE, 0x0a20 },
-	{ OS05B10_REG_Y_OUTPUT_SIZE, 0x0798 },
-	{ OS05B10_REG_HTS, 0x06d0 },
-	{ OS05B10_REG_ISP_X_WIN, 0x0008 },
-	{ OS05B10_REG_ISP_Y_WIN, 0x0008 },
-	{ OS05B10_REG_X_INC_ODD, 0x01 },
-	{ OS05B10_REG_X_INC_EVEN, 0x01 },
-	{ OS05B10_REG_Y_INC_ODD, 0x01 },
-	{ OS05B10_REG_Y_INC_EVEN, 0x01 },
 	{ CCI_REG8(0x3818), 0x00 },
 	{ CCI_REG8(0x3819), 0x00 },
 	{ CCI_REG8(0x381a), 0x00 },
 	{ CCI_REG8(0x381b), 0x01 },
-	{ OS05B10_REG_FORMAT1, 0x88 },
-	{ OS05B10_REG_FORMAT2, 0x00 },
 	{ CCI_REG8(0x3822), 0x12 },
 	{ CCI_REG8(0x3823), 0x08 },
 	{ CCI_REG8(0x3824), 0x00 },
@@ -341,7 +318,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x3829), 0x03 },
 	{ CCI_REG8(0x382a), 0x00 },
 	{ CCI_REG8(0x382b), 0x00 },
-	{ CCI_REG8(0x3832), 0x08 },
 	{ CCI_REG8(0x3838), 0x00 },
 	{ CCI_REG8(0x3839), 0x00 },
 	{ CCI_REG8(0x383a), 0x00 },
@@ -362,26 +338,19 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x38a7), 0x04 },
 	{ CCI_REG8(0x38b8), 0x02 },
 	{ CCI_REG8(0x3c80), 0x3e },
-	{ CCI_REG8(0x3c86), 0x01 },
 	{ CCI_REG8(0x3c87), 0x02 },
 	{ CCI_REG8(0x389c), 0x00 },
 	{ CCI_REG8(0x3ca2), 0x0c },
 	{ CCI_REG8(0x3d85), 0x1b },
-	{ CCI_REG8(0x3d8c), 0x01 },
 	{ CCI_REG8(0x3d8d), 0xe2 },
 	{ CCI_REG8(0x3f00), 0xcb },
-	{ CCI_REG8(0x3f03), 0x08 },
 	{ CCI_REG8(0x3f9e), 0x07 },
 	{ CCI_REG8(0x3f9f), 0x04 },
 	{ CCI_REG8(0x4000), 0xf3 },
 	{ CCI_REG8(0x4002), 0x00 },
 	{ CCI_REG8(0x4003), 0x40 },
-	{ CCI_REG8(0x4008), 0x02 },
-	{ CCI_REG8(0x4009), 0x0d },
-	{ CCI_REG8(0x400a), 0x01 },
 	{ CCI_REG8(0x400b), 0x00 },
 	{ CCI_REG8(0x4040), 0x00 },
-	{ CCI_REG8(0x4041), 0x07 },
 	{ CCI_REG8(0x4090), 0x14 },
 	{ CCI_REG8(0x40b0), 0x01 },
 	{ CCI_REG8(0x40b1), 0x01 },
@@ -401,7 +370,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x4305), 0x83 },
 	{ CCI_REG8(0x4306), 0x21 },
 	{ CCI_REG8(0x430d), 0x00 },
-	{ CCI_REG8(0x4505), 0xc4 },
 	{ CCI_REG8(0x4506), 0x00 },
 	{ CCI_REG8(0x4507), 0x60 },
 	{ CCI_REG8(0x4803), 0x00 },
@@ -413,7 +381,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x481f), 0x30 },
 	{ CCI_REG8(0x4825), 0x34 },
 	{ CCI_REG8(0x4829), 0x64 },
-	{ CCI_REG8(0x4837), 0x12 },
 	{ CCI_REG8(0x484b), 0x07 },
 	{ CCI_REG8(0x4883), 0x36 },
 	{ CCI_REG8(0x4885), 0x03 },
@@ -446,6 +413,42 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x3222), 0x03 },
 };
 
+static const struct cci_reg_sequence mode_2592_1944_regs[] = {
+	{ OS05B10_REG_X_ADDR_START,	0x0130 },
+	{ OS05B10_REG_Y_ADDR_START,	0x0000 },
+	{ OS05B10_REG_X_ADDR_END,	0x0b5f },
+	{ OS05B10_REG_Y_ADDR_END,	0x07a7 },
+	{ OS05B10_REG_X_OUTPUT_SIZE,	0x0a20 },
+	{ OS05B10_REG_Y_OUTPUT_SIZE,	0x0798 },
+	{ OS05B10_REG_HTS,		0x06d0 },
+	{ OS05B10_REG_ISP_X_WIN,	0x0008 },
+	{ OS05B10_REG_ISP_Y_WIN,	0x0008 },
+	{ OS05B10_REG_X_INC_ODD,	0x01 },
+	{ OS05B10_REG_X_INC_EVEN,	0x01 },
+	{ OS05B10_REG_Y_INC_ODD,	0x01 },
+	{ OS05B10_REG_Y_INC_EVEN,	0x01 },
+	{ OS05B10_REG_FORMAT1,		0x88 },
+	{ OS05B10_REG_FORMAT2,		0x00 },
+	{ CCI_REG8(0x3610), 0x87 },
+	{ CCI_REG8(0x3620), 0x0c },
+	{ CCI_REG8(0x3662), 0x10 },
+	{ CCI_REG8(0x3714), 0x24 },
+	{ CCI_REG8(0x373f), 0xb0 },
+	{ CCI_REG8(0x37bf), 0x05 },
+	{ CCI_REG8(0x37c2), 0x04 },
+	{ CCI_REG8(0x37d9), 0x08 },
+	{ CCI_REG8(0x3832), 0x08 },
+	{ CCI_REG8(0x3c86), 0x01 },
+	{ CCI_REG8(0x3d8c), 0x01 },
+	{ CCI_REG8(0x3f03), 0x08 },
+	{ CCI_REG8(0x4008), 0x02 },
+	{ CCI_REG8(0x4009), 0x0d },
+	{ CCI_REG8(0x400a), 0x01 },
+	{ CCI_REG8(0x4041), 0x07 },
+	{ CCI_REG8(0x4505), 0xc4 },
+	{ CCI_REG8(0x4837), 0x12 },
+};
+
 struct os05b10 {
 	struct device *dev;
 	struct regmap *cci;
@@ -468,6 +471,11 @@ struct os05b10 {
 	u32 data_lanes;
 };
 
+struct os05b10_reg_list {
+	u32 num_of_regs;
+	const struct cci_reg_sequence *regs;
+};
+
 struct os05b10_mode {
 	u32 width;
 	u32 height;
@@ -475,6 +483,7 @@ struct os05b10_mode {
 	u32 hts;
 	u32 exp;
 	u8 bpp;
+	struct os05b10_reg_list reg_list;
 };
 
 static const struct os05b10_mode supported_modes_10bit[] = {
@@ -485,6 +494,10 @@ static const struct os05b10_mode supported_modes_10bit[] = {
 		.hts = 1744,
 		.exp = 1944,
 		.bpp = 10,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
+			.regs = mode_2592_1944_regs,
+		},
 	},
 };
 
@@ -654,8 +667,16 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 				  u32 pad, u64 streams_mask)
 {
 	struct os05b10 *os05b10 = to_os05b10(sd);
+	const struct os05b10_reg_list *reg_list;
+	const struct v4l2_mbus_framefmt *fmt;
+	const struct os05b10_mode *mode;
 	int ret;
 
+	fmt = v4l2_subdev_state_get_format(state, 0);
+	mode = v4l2_find_nearest_size(supported_modes_10bit,
+				      ARRAY_SIZE(supported_modes_10bit), width,
+				      height, fmt->width, fmt->height);
+
 	ret = pm_runtime_resume_and_get(os05b10->dev);
 	if (ret < 0)
 		return ret;
@@ -668,6 +689,15 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 		goto err_rpm_put;
 	}
 
+	/* Write sensor mode registers */
+	reg_list = &mode->reg_list;
+	ret = cci_multi_reg_write(os05b10->cci, reg_list->regs,
+				  reg_list->num_of_regs, NULL);
+	if (ret) {
+		dev_err(os05b10->dev, "fail to write initial registers\n");
+		goto err_rpm_put;
+	}
+
 	/* Apply customized user controls */
 	ret = __v4l2_ctrl_handler_setup(os05b10->sd.ctrl_handler);
 	if (ret)
-- 
2.34.1

Re: [PATCH v3 04/17] media: i2c: os05b10: split common and mode-specific init registers

[Mehdi Djait]

Hi Tarang,

On Sun, Jul 19, 2026 at 01:38:55AM +0530, Tarang Raval wrote:
> Separate common initialization registers from mode-specific settings.
> Move resolution-dependent registers into a per-mode register list and
> program them during stream enable.
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 04/17] media: i2c: os05b10: split common and mode-specific init registers

[Vladimir Zapolskiy]

On 7/18/26 23:08, Tarang Raval wrote:
> Separate common initialization registers from mode-specific settings.
> Move resolution-dependent registers into a per-mode register list and
> program them during stream enable.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 96 ++++++++++++++++++++++++-------------
>   1 file changed, 63 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 96395cd7a2ca..12c4e2610d6f 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -142,16 +142,13 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
>   	{ CCI_REG8(0x3600), 0x4d },
>   	{ CCI_REG8(0x3601), 0x08 },
> -	{ CCI_REG8(0x3610), 0x87 },
>   	{ CCI_REG8(0x3611), 0x24 },
>   	{ CCI_REG8(0x3614), 0x4c },
> -	{ CCI_REG8(0x3620), 0x0c },
>   	{ CCI_REG8(0x3632), 0x80 },
>   	{ CCI_REG8(0x3633), 0x00 },
>   	{ CCI_REG8(0x3636), 0xcc },
>   	{ CCI_REG8(0x3637), 0x27 },
>   	{ CCI_REG8(0x3660), 0x00 },
> -	{ CCI_REG8(0x3662), 0x10 },
>   	{ CCI_REG8(0x3665), 0x00 },
>   	{ CCI_REG8(0x3666), 0x00 },
>   	{ CCI_REG8(0x366a), 0x14 },
> @@ -235,7 +232,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x370f), 0x1c },
>   	{ CCI_REG8(0x3710), 0x00 },
>   	{ CCI_REG8(0x3713), 0x00 },
> -	{ CCI_REG8(0x3714), 0x24 },
>   	{ CCI_REG8(0x3716), 0x24 },
>   	{ CCI_REG8(0x371a), 0x1e },
>   	{ CCI_REG8(0x3724), 0x09 },
> @@ -244,7 +240,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x3730), 0xe1 },
>   	{ CCI_REG8(0x3735), 0x80 },
>   	{ CCI_REG8(0x3739), 0x10 },
> -	{ CCI_REG8(0x373f), 0xb0 },
>   	{ CCI_REG8(0x3740), 0x28 },
>   	{ CCI_REG8(0x3741), 0x21 },
>   	{ CCI_REG8(0x3742), 0x21 },
> @@ -290,8 +285,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x37ac), 0xa0 },
>   	{ CCI_REG8(0x37be), 0x0a },
>   	{ CCI_REG8(0x37bb), 0x02 },
> -	{ CCI_REG8(0x37bf), 0x05 },
> -	{ CCI_REG8(0x37c2), 0x04 },
>   	{ CCI_REG8(0x37c4), 0x11 },
>   	{ CCI_REG8(0x37c5), 0x80 },
>   	{ CCI_REG8(0x37c6), 0x14 },
> @@ -300,7 +293,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x37cd), 0x17 },
>   	{ CCI_REG8(0x37ce), 0x01 },
>   	{ CCI_REG8(0x37d8), 0x02 },
> -	{ CCI_REG8(0x37d9), 0x08 },
>   	{ CCI_REG8(0x37dc), 0x01 },
>   	{ CCI_REG8(0x37e0), 0x0c },
>   	{ CCI_REG8(0x37e1), 0x20 },
> @@ -313,25 +305,10 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x37f5), 0x00 },
>   	{ CCI_REG8(0x37f6), 0x00 },
>   	{ CCI_REG8(0x37f7), 0x00 },
> -	{ OS05B10_REG_X_ADDR_START, 0x0130 },
> -	{ OS05B10_REG_Y_ADDR_START, 0x0000 },
> -	{ OS05B10_REG_X_ADDR_END, 0x0b5f },
> -	{ OS05B10_REG_Y_ADDR_END, 0x07a7 },
> -	{ OS05B10_REG_X_OUTPUT_SIZE, 0x0a20 },
> -	{ OS05B10_REG_Y_OUTPUT_SIZE, 0x0798 },
> -	{ OS05B10_REG_HTS, 0x06d0 },
> -	{ OS05B10_REG_ISP_X_WIN, 0x0008 },
> -	{ OS05B10_REG_ISP_Y_WIN, 0x0008 },
> -	{ OS05B10_REG_X_INC_ODD, 0x01 },
> -	{ OS05B10_REG_X_INC_EVEN, 0x01 },
> -	{ OS05B10_REG_Y_INC_ODD, 0x01 },
> -	{ OS05B10_REG_Y_INC_EVEN, 0x01 },
>   	{ CCI_REG8(0x3818), 0x00 },
>   	{ CCI_REG8(0x3819), 0x00 },
>   	{ CCI_REG8(0x381a), 0x00 },
>   	{ CCI_REG8(0x381b), 0x01 },
> -	{ OS05B10_REG_FORMAT1, 0x88 },
> -	{ OS05B10_REG_FORMAT2, 0x00 },
>   	{ CCI_REG8(0x3822), 0x12 },
>   	{ CCI_REG8(0x3823), 0x08 },
>   	{ CCI_REG8(0x3824), 0x00 },
> @@ -341,7 +318,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x3829), 0x03 },
>   	{ CCI_REG8(0x382a), 0x00 },
>   	{ CCI_REG8(0x382b), 0x00 },
> -	{ CCI_REG8(0x3832), 0x08 },
>   	{ CCI_REG8(0x3838), 0x00 },
>   	{ CCI_REG8(0x3839), 0x00 },
>   	{ CCI_REG8(0x383a), 0x00 },
> @@ -362,26 +338,19 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x38a7), 0x04 },
>   	{ CCI_REG8(0x38b8), 0x02 },
>   	{ CCI_REG8(0x3c80), 0x3e },
> -	{ CCI_REG8(0x3c86), 0x01 },
>   	{ CCI_REG8(0x3c87), 0x02 },
>   	{ CCI_REG8(0x389c), 0x00 },
>   	{ CCI_REG8(0x3ca2), 0x0c },
>   	{ CCI_REG8(0x3d85), 0x1b },
> -	{ CCI_REG8(0x3d8c), 0x01 },
>   	{ CCI_REG8(0x3d8d), 0xe2 },
>   	{ CCI_REG8(0x3f00), 0xcb },
> -	{ CCI_REG8(0x3f03), 0x08 },
>   	{ CCI_REG8(0x3f9e), 0x07 },
>   	{ CCI_REG8(0x3f9f), 0x04 },
>   	{ CCI_REG8(0x4000), 0xf3 },
>   	{ CCI_REG8(0x4002), 0x00 },
>   	{ CCI_REG8(0x4003), 0x40 },
> -	{ CCI_REG8(0x4008), 0x02 },
> -	{ CCI_REG8(0x4009), 0x0d },
> -	{ CCI_REG8(0x400a), 0x01 },
>   	{ CCI_REG8(0x400b), 0x00 },
>   	{ CCI_REG8(0x4040), 0x00 },
> -	{ CCI_REG8(0x4041), 0x07 },
>   	{ CCI_REG8(0x4090), 0x14 },
>   	{ CCI_REG8(0x40b0), 0x01 },
>   	{ CCI_REG8(0x40b1), 0x01 },
> @@ -401,7 +370,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x4305), 0x83 },
>   	{ CCI_REG8(0x4306), 0x21 },
>   	{ CCI_REG8(0x430d), 0x00 },
> -	{ CCI_REG8(0x4505), 0xc4 },
>   	{ CCI_REG8(0x4506), 0x00 },
>   	{ CCI_REG8(0x4507), 0x60 },
>   	{ CCI_REG8(0x4803), 0x00 },
> @@ -413,7 +381,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x481f), 0x30 },
>   	{ CCI_REG8(0x4825), 0x34 },
>   	{ CCI_REG8(0x4829), 0x64 },
> -	{ CCI_REG8(0x4837), 0x12 },
>   	{ CCI_REG8(0x484b), 0x07 },
>   	{ CCI_REG8(0x4883), 0x36 },
>   	{ CCI_REG8(0x4885), 0x03 },
> @@ -446,6 +413,42 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x3222), 0x03 },
>   };
>   
> +static const struct cci_reg_sequence mode_2592_1944_regs[] = {
> +	{ OS05B10_REG_X_ADDR_START,	0x0130 },
> +	{ OS05B10_REG_Y_ADDR_START,	0x0000 },
> +	{ OS05B10_REG_X_ADDR_END,	0x0b5f },
> +	{ OS05B10_REG_Y_ADDR_END,	0x07a7 },
> +	{ OS05B10_REG_X_OUTPUT_SIZE,	0x0a20 },
> +	{ OS05B10_REG_Y_OUTPUT_SIZE,	0x0798 },
> +	{ OS05B10_REG_HTS,		0x06d0 },
> +	{ OS05B10_REG_ISP_X_WIN,	0x0008 },
> +	{ OS05B10_REG_ISP_Y_WIN,	0x0008 },
> +	{ OS05B10_REG_X_INC_ODD,	0x01 },
> +	{ OS05B10_REG_X_INC_EVEN,	0x01 },
> +	{ OS05B10_REG_Y_INC_ODD,	0x01 },
> +	{ OS05B10_REG_Y_INC_EVEN,	0x01 },
> +	{ OS05B10_REG_FORMAT1,		0x88 },
> +	{ OS05B10_REG_FORMAT2,		0x00 },
> +	{ CCI_REG8(0x3610), 0x87 },
> +	{ CCI_REG8(0x3620), 0x0c },
> +	{ CCI_REG8(0x3662), 0x10 },
> +	{ CCI_REG8(0x3714), 0x24 },
> +	{ CCI_REG8(0x373f), 0xb0 },
> +	{ CCI_REG8(0x37bf), 0x05 },
> +	{ CCI_REG8(0x37c2), 0x04 },
> +	{ CCI_REG8(0x37d9), 0x08 },
> +	{ CCI_REG8(0x3832), 0x08 },
> +	{ CCI_REG8(0x3c86), 0x01 },
> +	{ CCI_REG8(0x3d8c), 0x01 },
> +	{ CCI_REG8(0x3f03), 0x08 },
> +	{ CCI_REG8(0x4008), 0x02 },
> +	{ CCI_REG8(0x4009), 0x0d },
> +	{ CCI_REG8(0x400a), 0x01 },
> +	{ CCI_REG8(0x4041), 0x07 },
> +	{ CCI_REG8(0x4505), 0xc4 },
> +	{ CCI_REG8(0x4837), 0x12 },
> +};
> +
>   struct os05b10 {
>   	struct device *dev;
>   	struct regmap *cci;
> @@ -468,6 +471,11 @@ struct os05b10 {
>   	u32 data_lanes;
>   };
>   
> +struct os05b10_reg_list {
> +	u32 num_of_regs;
> +	const struct cci_reg_sequence *regs;
> +};
> +
>   struct os05b10_mode {
>   	u32 width;
>   	u32 height;
> @@ -475,6 +483,7 @@ struct os05b10_mode {
>   	u32 hts;
>   	u32 exp;
>   	u8 bpp;
> +	struct os05b10_reg_list reg_list;
>   };
>   
>   static const struct os05b10_mode supported_modes_10bit[] = {
> @@ -485,6 +494,10 @@ static const struct os05b10_mode supported_modes_10bit[] = {
>   		.hts = 1744,
>   		.exp = 1944,
>   		.bpp = 10,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
> +			.regs = mode_2592_1944_regs,
> +		},
>   	},
>   };
>   
> @@ -654,8 +667,16 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   				  u32 pad, u64 streams_mask)
>   {
>   	struct os05b10 *os05b10 = to_os05b10(sd);
> +	const struct os05b10_reg_list *reg_list;
> +	const struct v4l2_mbus_framefmt *fmt;
> +	const struct os05b10_mode *mode;
>   	int ret;
>   
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +	mode = v4l2_find_nearest_size(supported_modes_10bit,
> +				      ARRAY_SIZE(supported_modes_10bit), width,
> +				      height, fmt->width, fmt->height);
> +
>   	ret = pm_runtime_resume_and_get(os05b10->dev);
>   	if (ret < 0)
>   		return ret;
> @@ -668,6 +689,15 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   		goto err_rpm_put;
>   	}
>   
> +	/* Write sensor mode registers */
> +	reg_list = &mode->reg_list;
> +	ret = cci_multi_reg_write(os05b10->cci, reg_list->regs,
> +				  reg_list->num_of_regs, NULL);
> +	if (ret) {
> +		dev_err(os05b10->dev, "fail to write initial registers\n");
> +		goto err_rpm_put;
> +	}
> +
>   	/* Apply customized user controls */
>   	ret = __v4l2_ctrl_handler_setup(os05b10->sd.ctrl_handler);
>   	if (ret)

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 05/17] media: i2c: os05b10: add V4L2 digital gain control

[Tarang Raval]

Stop programming digital gain in the common register sequence and expose it as
a V4L2_CID_DIGITAL_GAIN control. Initialize the new control and handle writes
in the ctrl callback.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
---
 drivers/media/i2c/os05b10.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 12c4e2610d6f..abf3d2970900 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -138,7 +138,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x3503), 0x88 },
 	{ OS05B10_REG_DIGITAL_GAIN, 0x0400 },
 	{ OS05B10_REG_ANALOG_GAIN_SHORT, 0x0080 },
-	{ OS05B10_REG_DIGITAL_GAIN_SHORT, OS05B10_DIGITAL_GAIN_DEFAULT },
 	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
 	{ CCI_REG8(0x3600), 0x4d },
 	{ CCI_REG8(0x3601), 0x08 },
@@ -549,6 +548,10 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_write(os05b10->cci, OS05B10_REG_ANALOG_GAIN,
 				ctrl->val, NULL);
 		break;
+	case V4L2_CID_DIGITAL_GAIN:
+		ret = cci_write(os05b10->cci, OS05B10_REG_DIGITAL_GAIN,
+				ctrl->val, NULL);
+		break;
 	case V4L2_CID_EXPOSURE:
 		ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
 				ctrl->val, NULL);
@@ -925,7 +928,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	int ret;
 
 	ctrl_hdlr = &os05b10->handler;
-	v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	v4l2_ctrl_handler_init(ctrl_hdlr, 9);
 
 	pixel_rate = os05b10_pixel_rate(os05b10, mode);
 	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
@@ -967,6 +970,10 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 					  OS05B10_ANALOG_GAIN_STEP,
 					  OS05B10_ANALOG_GAIN_DEFAULT);
 
+	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
+			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
+			  OS05B10_DIGITAL_GAIN_STEP, OS05B10_DIGITAL_GAIN_DEFAULT);
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
-- 
2.34.1

Re: [PATCH v3 05/17] media: i2c: os05b10: add V4L2 digital gain control

[Vladimir Zapolskiy]

On 7/18/26 23:08, Tarang Raval wrote:
> Stop programming digital gain in the common register sequence and expose it as
> a V4L2_CID_DIGITAL_GAIN control. Initialize the new control and handle writes
> in the ctrl callback.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> ---
>   drivers/media/i2c/os05b10.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 12c4e2610d6f..abf3d2970900 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -138,7 +138,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x3503), 0x88 },
>   	{ OS05B10_REG_DIGITAL_GAIN, 0x0400 },
>   	{ OS05B10_REG_ANALOG_GAIN_SHORT, 0x0080 },
> -	{ OS05B10_REG_DIGITAL_GAIN_SHORT, OS05B10_DIGITAL_GAIN_DEFAULT },
>   	{ OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
>   	{ CCI_REG8(0x3600), 0x4d },
>   	{ CCI_REG8(0x3601), 0x08 },
> @@ -549,6 +548,10 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   		ret = cci_write(os05b10->cci, OS05B10_REG_ANALOG_GAIN,
>   				ctrl->val, NULL);
>   		break;
> +	case V4L2_CID_DIGITAL_GAIN:
> +		ret = cci_write(os05b10->cci, OS05B10_REG_DIGITAL_GAIN,
> +				ctrl->val, NULL);

OS05B10_REG_DIGITAL_GAIN_SHORT control is removed, but
OS05B10_REG_DIGITAL_GAIN is left in the long sequence and rewritten here.

If OS05B10_REG_DIGITAL_GAIN_SHORT control is just not needed, please
remove it from the long sequence in advance, and then this change will
touch the remaining OS05B10_REG_DIGITAL_GAIN control only.

> +		break;
>   	case V4L2_CID_EXPOSURE:
>   		ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
>   				ctrl->val, NULL);
> @@ -925,7 +928,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   	int ret;
>   
>   	ctrl_hdlr = &os05b10->handler;
> -	v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	v4l2_ctrl_handler_init(ctrl_hdlr, 9);
>   
>   	pixel_rate = os05b10_pixel_rate(os05b10, mode);
>   	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
> @@ -967,6 +970,10 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   					  OS05B10_ANALOG_GAIN_STEP,
>   					  OS05B10_ANALOG_GAIN_DEFAULT);
>   
> +	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
> +			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
> +			  OS05B10_DIGITAL_GAIN_STEP, OS05B10_DIGITAL_GAIN_DEFAULT);
> +
>   	if (ctrl_hdlr->error) {
>   		ret = ctrl_hdlr->error;
>   		dev_err(os05b10->dev, "control init failed (%d)\n", ret);

-- 
Best wishes,
Vladimir

Re: [PATCH v3 05/17] media: i2c: os05b10: add V4L2 digital gain control

[Tarang Raval]

Hi Vladimir,

Thanks for the review.

> On 7/18/26 23:08, Tarang Raval wrote:
> > Stop programming digital gain in the common register sequence and expose it as
> > a V4L2_CID_DIGITAL_GAIN control. Initialize the new control and handle writes
> > in the ctrl callback.
> >
> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>
> > ---
> >   drivers/media/i2c/os05b10.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 12c4e2610d6f..abf3d2970900 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -138,7 +138,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
> >       { CCI_REG8(0x3503), 0x88 },
> >       { OS05B10_REG_DIGITAL_GAIN, 0x0400 },
> >       { OS05B10_REG_ANALOG_GAIN_SHORT, 0x0080 },
> > -     { OS05B10_REG_DIGITAL_GAIN_SHORT, OS05B10_DIGITAL_GAIN_DEFAULT },
> >       { OS05B10_REG_EXPOSURE_SHORT, 0x000020 },
> >       { CCI_REG8(0x3600), 0x4d },
> >       { CCI_REG8(0x3601), 0x08 },
> > @@ -549,6 +548,10 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
> >               ret = cci_write(os05b10->cci, OS05B10_REG_ANALOG_GAIN,
> >                               ctrl->val, NULL);
> >               break;
> > +     case V4L2_CID_DIGITAL_GAIN:
> > +             ret = cci_write(os05b10->cci, OS05B10_REG_DIGITAL_GAIN,
> > +                             ctrl->val, NULL);
>
> OS05B10_REG_DIGITAL_GAIN_SHORT control is removed, but
> OS05B10_REG_DIGITAL_GAIN is left in the long sequence and rewritten here.
>
> If OS05B10_REG_DIGITAL_GAIN_SHORT control is just not needed, please
> remove it from the long sequence in advance, and then this change will
> touch the remaining OS05B10_REG_DIGITAL_GAIN control only.

I made a mistake here. I intended to remove { OS05B10_REG_DIGITAL_GAIN, 0x0400 },
But I accidentally removed the wrong digital gain entry. I'll correct
this in the next revision.

Best Regards,
Tarang

[PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Tarang Raval]

Add HFLIP and VFLIP controls, lock them while streaming,
and update the reported Bayer format based on the flip state.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 56 ++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index abf3d2970900..61c51844a91e 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -96,6 +96,10 @@
 #define OS05B10_MIRROR			BIT(3)
 #define OS05B10_FLIP			GENMASK(5, 4)
 
+#define OS05B10_REG_ANALOG_FLIP         CCI_REG8(0x3716)
+#define OS05B10_FLIP_ENABLE             0x04
+#define OS05B10_FLIP_DISABLE            0x24
+
 #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
 
 #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
@@ -231,7 +235,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x370f), 0x1c },
 	{ CCI_REG8(0x3710), 0x00 },
 	{ CCI_REG8(0x3713), 0x00 },
-	{ CCI_REG8(0x3716), 0x24 },
 	{ CCI_REG8(0x371a), 0x1e },
 	{ CCI_REG8(0x3724), 0x09 },
 	{ CCI_REG8(0x3725), 0xb2 },
@@ -465,6 +468,8 @@ struct os05b10 {
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *vflip;
+	struct v4l2_ctrl *hflip;
 
 	u32 link_freq_index;
 	u32 data_lanes;
@@ -513,6 +518,16 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
 	return container_of_const(sd, struct os05b10, sd);
 };
 
+static u32 os05b10_get_format_code(struct os05b10 *os05b10)
+{
+	static const u32 codes[2][2] = {
+		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
+		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
+	};
+
+	return codes[os05b10->vflip->val][os05b10->hflip->val];
+}
+
 static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
@@ -556,6 +571,20 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
 				ctrl->val, NULL);
 		break;
+	case V4L2_CID_HFLIP:
+	case V4L2_CID_VFLIP:
+		ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
+				      OS05B10_MIRROR | OS05B10_FLIP,
+				      (!os05b10->hflip->val ?
+				       OS05B10_MIRROR : 0) |
+				      (os05b10->vflip->val ?
+				       OS05B10_FLIP : 0), NULL);
+
+		ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
+				 (os05b10->vflip->val == 1) ?
+				 OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
+				 NULL);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -570,10 +599,12 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
+	struct os05b10 *os05b10 = to_os05b10(sd);
+
 	if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
 		return -EINVAL;
 
-	code->code = os05b10_mbus_codes[code->index];
+	code->code = os05b10_get_format_code(os05b10);
 
 	return 0;
 }
@@ -712,6 +743,9 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 	if (ret)
 		goto err_rpm_put;
 
+	__v4l2_ctrl_grab(os05b10->vflip, true);
+	__v4l2_ctrl_grab(os05b10->hflip, true);
+
 	return 0;
 
 err_rpm_put:
@@ -732,6 +766,9 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
 	if (ret)
 		dev_err(os05b10->dev, "failed to set stream off\n");
 
+	__v4l2_ctrl_grab(os05b10->vflip, false);
+	__v4l2_ctrl_grab(os05b10->hflip, false);
+
 	pm_runtime_put(os05b10->dev);
 
 	return 0;
@@ -740,6 +777,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
 static int os05b10_init_state(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *state)
 {
+	struct os05b10 *os05b10 = to_os05b10(sd);
 	struct v4l2_mbus_framefmt *format;
 	const struct os05b10_mode *mode;
 
@@ -747,7 +785,7 @@ static int os05b10_init_state(struct v4l2_subdev *sd,
 	format = v4l2_subdev_state_get_format(state, 0);
 
 	mode = &supported_modes_10bit[0];
-	format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
+	format->code = os05b10_get_format_code(os05b10);
 
 	/* Update image pad formate */
 	format->width = mode->width;
@@ -928,7 +966,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	int ret;
 
 	ctrl_hdlr = &os05b10->handler;
-	v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
 
 	pixel_rate = os05b10_pixel_rate(os05b10, mode);
 	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
@@ -974,6 +1012,16 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
 			  OS05B10_DIGITAL_GAIN_STEP, OS05B10_DIGITAL_GAIN_DEFAULT);
 
+	os05b10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
+					   V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (os05b10->hflip)
+		os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
+	os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
+					   V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (os05b10->vflip)
+		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
-- 
2.34.1

Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Mehdi Djait]

Hi Tarang,

On Sun, Jul 19, 2026 at 01:38:57AM +0530, Tarang Raval wrote:
> Add HFLIP and VFLIP controls, lock them while streaming,
> and update the reported Bayer format based on the flip state.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/os05b10.c | 56 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index abf3d2970900..61c51844a91e 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -96,6 +96,10 @@
>  #define OS05B10_MIRROR			BIT(3)
>  #define OS05B10_FLIP			GENMASK(5, 4)
>  
> +#define OS05B10_REG_ANALOG_FLIP         CCI_REG8(0x3716)
> +#define OS05B10_FLIP_ENABLE             0x04
> +#define OS05B10_FLIP_DISABLE            0x24
> +
>  #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
>  
>  #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
> @@ -231,7 +235,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>  	{ CCI_REG8(0x370f), 0x1c },
>  	{ CCI_REG8(0x3710), 0x00 },
>  	{ CCI_REG8(0x3713), 0x00 },
> -	{ CCI_REG8(0x3716), 0x24 },
>  	{ CCI_REG8(0x371a), 0x1e },
>  	{ CCI_REG8(0x3724), 0x09 },
>  	{ CCI_REG8(0x3725), 0xb2 },
> @@ -465,6 +468,8 @@ struct os05b10 {
>  	struct v4l2_ctrl *vblank;
>  	struct v4l2_ctrl *gain;
>  	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
>  
>  	u32 link_freq_index;
>  	u32 data_lanes;
> @@ -513,6 +518,16 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
>  	return container_of_const(sd, struct os05b10, sd);
>  };
>  
> +static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> +{
> +	static const u32 codes[2][2] = {
> +		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> +		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> +	};
> +
> +	return codes[os05b10->vflip->val][os05b10->hflip->val];
> +}
> +
>  static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> @@ -556,6 +571,20 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
>  				ctrl->val, NULL);
>  		break;
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> +				      OS05B10_MIRROR | OS05B10_FLIP,
> +				      (!os05b10->hflip->val ?
> +				       OS05B10_MIRROR : 0) |
> +				      (os05b10->vflip->val ?
> +				       OS05B10_FLIP : 0), NULL);
> +
> +		ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> +				 (os05b10->vflip->val == 1) ?
> +				 OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> +				 NULL);

I don't know about the ret |= assignment

You can use the last parameter of the cci_*() helpers. That would make
this cleaner and not combine two error codes to produce something weird.

If you pass &ret to cci_write() the helper will check if an error
already occured and return it.

> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Jai Luthra]

Hi Tarang,

Quoting Tarang Raval (2026-07-19 01:38:57)
> Add HFLIP and VFLIP controls, lock them while streaming,
> and update the reported Bayer format based on the flip state.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/os05b10.c | 56 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index abf3d2970900..61c51844a91e 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c

[...]

> @@ -556,6 +571,20 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
>                                 ctrl->val, NULL);
>                 break;
> +       case V4L2_CID_HFLIP:
> +       case V4L2_CID_VFLIP:
> +               ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> +                                     OS05B10_MIRROR | OS05B10_FLIP,
> +                                     (!os05b10->hflip->val ?
> +                                      OS05B10_MIRROR : 0) |
> +                                     (os05b10->vflip->val ?
> +                                      OS05B10_FLIP : 0), NULL);
> +
> +               ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> +                                (os05b10->vflip->val == 1) ?
> +                                OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> +                                NULL);
> +               break;
>         default:
>                 ret = -EINVAL;
>                 break;

Please update the active state format on HFLIP/VFLIP, so the subsequent
G_FMT calls show the correct format.

Many drivers miss this today, I assume because libcamera usually calls
S_FMT on streaming using the output of ENUM_FMT.

Thanks,
    Jai

Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Tarang Raval]

Hi Jai, Mehdi,

Thanks for the review.

> > +       case V4L2_CID_HFLIP:
> > +       case V4L2_CID_VFLIP:
> > +               ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> > +                                     OS05B10_MIRROR | OS05B10_FLIP,
> > +                                     (!os05b10->hflip->val ?
> > +                                      OS05B10_MIRROR : 0) |
> > +                                     (os05b10->vflip->val ?
> > +                                      OS05B10_FLIP : 0), NULL);
> > +
> > +               ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> > +                                (os05b10->vflip->val == 1) ?
> > +                                OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> > +                                NULL);
> > +               break;
> >         default:
> >                 ret = -EINVAL;
> >                 break;
>
> Please update the active state format on HFLIP/VFLIP, so the subsequent
> G_FMT calls show the correct format.
>
> Many drivers miss this today, I assume because libcamera usually calls
> S_FMT on streaming using the output of ENUM_FMT.

Good point. I'll update the active state so subsequent G_FMT calls report
the correct format.

...

> > +
> > +             ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> > +                              (os05b10->vflip->val == 1) ?
> > +                              OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> > +                              NULL);
>
> I don't know about the ret |= assignment
>
> You can use the last parameter of the cci_*() helpers. That would make
> this cleaner and not combine two error codes to produce something weird.
>
> If you pass &ret to cci_write() the helper will check if an error
> already occured and return it.

Makes sense. I'll update this in v4.

Best Regards,
Tarang

Re: [PATCH v3 06/17] media: i2c: os05b10: Add H/V flip support

[Vladimir Zapolskiy]

On 7/18/26 23:08, Tarang Raval wrote:
> Add HFLIP and VFLIP controls, lock them while streaming,
> and update the reported Bayer format based on the flip state.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 56 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index abf3d2970900..61c51844a91e 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -96,6 +96,10 @@
>   #define OS05B10_MIRROR			BIT(3)
>   #define OS05B10_FLIP			GENMASK(5, 4)
>   
> +#define OS05B10_REG_ANALOG_FLIP         CCI_REG8(0x3716)
> +#define OS05B10_FLIP_ENABLE             0x04
> +#define OS05B10_FLIP_DISABLE            0x24
> +
>   #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
>   
>   #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
> @@ -231,7 +235,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x370f), 0x1c },
>   	{ CCI_REG8(0x3710), 0x00 },
>   	{ CCI_REG8(0x3713), 0x00 },
> -	{ CCI_REG8(0x3716), 0x24 },
>   	{ CCI_REG8(0x371a), 0x1e },
>   	{ CCI_REG8(0x3724), 0x09 },
>   	{ CCI_REG8(0x3725), 0xb2 },
> @@ -465,6 +468,8 @@ struct os05b10 {
>   	struct v4l2_ctrl *vblank;
>   	struct v4l2_ctrl *gain;
>   	struct v4l2_ctrl *exposure;
> +	struct v4l2_ctrl *vflip;
> +	struct v4l2_ctrl *hflip;
>   
>   	u32 link_freq_index;
>   	u32 data_lanes;
> @@ -513,6 +518,16 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
>   	return container_of_const(sd, struct os05b10, sd);
>   };
>   
> +static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> +{
> +	static const u32 codes[2][2] = {
> +		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> +		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> +	};
> +
> +	return codes[os05b10->vflip->val][os05b10->hflip->val];
> +}
> +
>   static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> @@ -556,6 +571,20 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   		ret = cci_write(os05b10->cci, OS05B10_REG_EXPOSURE,
>   				ctrl->val, NULL);
>   		break;
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		ret = cci_update_bits(os05b10->cci, OS05B10_REG_FORMAT1,
> +				      OS05B10_MIRROR | OS05B10_FLIP,
> +				      (!os05b10->hflip->val ?
> +				       OS05B10_MIRROR : 0) |
> +				      (os05b10->vflip->val ?
> +				       OS05B10_FLIP : 0), NULL);
> +
> +		ret |= cci_write(os05b10->cci, OS05B10_REG_ANALOG_FLIP,
> +				 (os05b10->vflip->val == 1) ?
> +				 OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
> +				 NULL);

Above please follow the review comment given by Mehdi.

It's expected to see

cci_update_bits(<...>, &ret);
cci_write(<...>, &ret);

> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> @@ -570,10 +599,12 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>   				  struct v4l2_subdev_state *sd_state,
>   				  struct v4l2_subdev_mbus_code_enum *code)
>   {
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +
>   	if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
>   		return -EINVAL;
>   
> -	code->code = os05b10_mbus_codes[code->index];
> +	code->code = os05b10_get_format_code(os05b10);
>   
>   	return 0;
>   }
> @@ -712,6 +743,9 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   	if (ret)
>   		goto err_rpm_put;
>   
> +	__v4l2_ctrl_grab(os05b10->vflip, true);
> +	__v4l2_ctrl_grab(os05b10->hflip, true);
> +
>   	return 0;
>   
>   err_rpm_put:
> @@ -732,6 +766,9 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
>   	if (ret)
>   		dev_err(os05b10->dev, "failed to set stream off\n");
>   
> +	__v4l2_ctrl_grab(os05b10->vflip, false);
> +	__v4l2_ctrl_grab(os05b10->hflip, false);
> +
>   	pm_runtime_put(os05b10->dev);
>   
>   	return 0;
> @@ -740,6 +777,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
>   static int os05b10_init_state(struct v4l2_subdev *sd,
>   			      struct v4l2_subdev_state *state)
>   {
> +	struct os05b10 *os05b10 = to_os05b10(sd);
>   	struct v4l2_mbus_framefmt *format;
>   	const struct os05b10_mode *mode;
>   
> @@ -747,7 +785,7 @@ static int os05b10_init_state(struct v4l2_subdev *sd,
>   	format = v4l2_subdev_state_get_format(state, 0);
>   
>   	mode = &supported_modes_10bit[0];
> -	format->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +	format->code = os05b10_get_format_code(os05b10);
>   
>   	/* Update image pad formate */
>   	format->width = mode->width;
> @@ -928,7 +966,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   	int ret;
>   
>   	ctrl_hdlr = &os05b10->handler;
> -	v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> +	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
>   
>   	pixel_rate = os05b10_pixel_rate(os05b10, mode);
>   	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
> @@ -974,6 +1012,16 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
>   			  OS05B10_DIGITAL_GAIN_STEP, OS05B10_DIGITAL_GAIN_DEFAULT);
>   
> +	os05b10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> +					   V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (os05b10->hflip)
> +		os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> +	os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> +					   V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (os05b10->vflip)
> +		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +

As a micro optimization againt the error path, setting of these flags can be
done at the end of the function, for instance check how it's done in ov2680.c

>   	if (ctrl_hdlr->error) {
>   		ret = ctrl_hdlr->error;
>   		dev_err(os05b10->dev, "control init failed (%d)\n", ret);

-- 
Best wishes,
Vladimir

[PATCH v3 07/17] media: i2c: os05b10: Add test pattern options

[Tarang Raval]

Add V4L2_CID_TEST_PATTERN support with multiple sensor test-pattern modes
and program them via register 0x5080. Drop the fixed 0x5080 setting from
the common register sequence so the pattern is selected only through the
control.

Removing the default write of 0x04 to register 0x5080 does not change the
sensor's default behaviour. The value 0x04 leaves the test pattern enable
bit (bit 7) cleared, so the sensor remains in normal streaming mode. It
only programs the color bar style field, which is ignored unless a test
pattern is enabled.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 55 +++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 61c51844a91e..4e177eacc815 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -102,6 +102,17 @@
 
 #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
 
+#define OS05B10_REG_PRE_ISP_20_0       CCI_REG8(0x5080)
+#define OS05B10_DISABLED               0x00
+#define OS05B10_COLOR_BAR_1            0x80
+#define OS05B10_COLOR_BAR_2            0x84
+#define OS05B10_COLOR_BAR_3            0x88
+#define OS05B10_COLOR_BAR_4            0x8c
+#define OS05B10_COLOR_SQUARE           0x82
+#define OS05B10_BW_SQUARE              0x92
+#define OS05B10_TRANSPARENT_EFFECT     0xa0
+#define OS05B10_ROLLING_BAR_EFFECT     0xc0
+
 #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
 
 static const struct v4l2_rect os05b10_native_area = {
@@ -395,7 +406,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x5004), 0x00 },
 	{ CCI_REG8(0x5005), 0x0e },
 	{ CCI_REG8(0x5036), 0x00 },
-	{ CCI_REG8(0x5080), 0x04 },
 	{ CCI_REG8(0x5082), 0x00 },
 	{ CCI_REG8(0x5180), 0x00 },
 	{ CCI_REG8(0x5181), 0x10 },
@@ -513,6 +523,30 @@ static const u32 os05b10_mbus_codes[] = {
 	MEDIA_BUS_FMT_SBGGR10_1X10,
 };
 
+static const char * const os05b10_test_pattern_menu[] = {
+	"Disabled",
+	"Colour bar type 1",
+	"Colour bar type 2",
+	"Colour bar type 3",
+	"Colour bar type 4",
+	"Colour square",
+	"Black-white square",
+	"Transparent effect",
+	"Rolling bar effect",
+};
+
+static const int os05b10_tp_val[] = {
+	OS05B10_DISABLED,
+	OS05B10_COLOR_BAR_1,
+	OS05B10_COLOR_BAR_2,
+	OS05B10_COLOR_BAR_3,
+	OS05B10_COLOR_BAR_4,
+	OS05B10_COLOR_SQUARE,
+	OS05B10_BW_SQUARE,
+	OS05B10_TRANSPARENT_EFFECT,
+	OS05B10_ROLLING_BAR_EFFECT,
+};
+
 static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
 {
 	return container_of_const(sd, struct os05b10, sd);
@@ -528,6 +562,15 @@ static u32 os05b10_get_format_code(struct os05b10 *os05b10)
 	return codes[os05b10->vflip->val][os05b10->hflip->val];
 }
 
+static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
+{
+       if (pattern >= ARRAY_SIZE(os05b10_test_pattern_menu))
+               return -EINVAL;
+
+       return cci_write(os05b10->cci, OS05B10_REG_PRE_ISP_20_0,
+                        os05b10_tp_val[pattern], NULL);
+}
+
 static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
@@ -585,6 +628,9 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 				 OS05B10_FLIP_ENABLE : OS05B10_FLIP_DISABLE,
 				 NULL);
 		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = os05b10_update_test_pattern(os05b10, ctrl->val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -966,7 +1012,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	int ret;
 
 	ctrl_hdlr = &os05b10->handler;
-	v4l2_ctrl_handler_init(ctrl_hdlr, 11);
+	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
 
 	pixel_rate = os05b10_pixel_rate(os05b10, mode);
 	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
@@ -1022,6 +1068,11 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	if (os05b10->vflip)
 		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &os05b10_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(os05b10_test_pattern_menu) - 1,
+				     0, 0, os05b10_test_pattern_menu);
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
-- 
2.34.1

Re: [PATCH v3 07/17] media: i2c: os05b10: Add test pattern options

[Mehdi Djait]

Hi Tarang,

On Sun, Jul 19, 2026 at 01:38:58AM +0530, Tarang Raval wrote:
> Add V4L2_CID_TEST_PATTERN support with multiple sensor test-pattern modes
> and program them via register 0x5080. Drop the fixed 0x5080 setting from
> the common register sequence so the pattern is selected only through the
> control.
> 
> Removing the default write of 0x04 to register 0x5080 does not change the
> sensor's default behaviour. The value 0x04 leaves the test pattern enable
> bit (bit 7) cleared, so the sensor remains in normal streaming mode. It
> only programs the color bar style field, which is ignored unless a test
> pattern is enabled.
> 

Reviewed-by: Mehdi Djait <mehdi.djait@linux.intel.com>

> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/os05b10.c | 55 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 61c51844a91e..4e177eacc815 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -102,6 +102,17 @@
>  
>  #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
>  
> +#define OS05B10_REG_PRE_ISP_20_0       CCI_REG8(0x5080)
> +#define OS05B10_DISABLED               0x00
> +#define OS05B10_COLOR_BAR_1            0x80
> +#define OS05B10_COLOR_BAR_2            0x84
> +#define OS05B10_COLOR_BAR_3            0x88
> +#define OS05B10_COLOR_BAR_4            0x8c
> +#define OS05B10_COLOR_SQUARE           0x82
> +#define OS05B10_BW_SQUARE              0x92
> +#define OS05B10_TRANSPARENT_EFFECT     0xa0
> +#define OS05B10_ROLLING_BAR_EFFECT     0xc0
> +
>  #define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
>  
>  static const struct v4l2_rect os05b10_native_area = {
> @@ -395,7 +406,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>  	{ CCI_REG8(0x5004), 0x00 },
>  	{ CCI_REG8(0x5005), 0x0e },
>  	{ CCI_REG8(0x5036), 0x00 },
> -	{ CCI_REG8(0x5080), 0x04 },
>  	{ CCI_REG8(0x5082), 0x00 },
>  	{ CCI_REG8(0x5180), 0x00 },
>  	{ CCI_REG8(0x5181), 0x10 },
> @@ -513,6 +523,30 @@ static const u32 os05b10_mbus_codes[] = {
>  	MEDIA_BUS_FMT_SBGGR10_1X10,
>  };
>  
> +static const char * const os05b10_test_pattern_menu[] = {
> +	"Disabled",
> +	"Colour bar type 1",

nit: make it color to be consistent with the rest of the driver ?

> +	"Colour bar type 2",
> +	"Colour bar type 3",
> +	"Colour bar type 4",
> +	"Colour square",
> +	"Black-white square",
> +	"Transparent effect",
> +	"Rolling bar effect",
> +};

--
Kind Regards
Mehdi Djait

Re: [PATCH v3 07/17] media: i2c: os05b10: Add test pattern options

[Vladimir Zapolskiy]

On 7/18/26 23:08, Tarang Raval wrote:
> Add V4L2_CID_TEST_PATTERN support with multiple sensor test-pattern modes
> and program them via register 0x5080. Drop the fixed 0x5080 setting from
> the common register sequence so the pattern is selected only through the
> control.
> 
> Removing the default write of 0x04 to register 0x5080 does not change the
> sensor's default behaviour. The value 0x04 leaves the test pattern enable
> bit (bit 7) cleared, so the sensor remains in normal streaming mode. It
> only programs the color bar style field, which is ignored unless a test
> pattern is enabled.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 55 +++++++++++++++++++++++++++++++++++--
>   1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 61c51844a91e..4e177eacc815 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -102,6 +102,17 @@
>   
>   #define OS05B10_REG_FORMAT2		CCI_REG8(0x3821)
>   
> +#define OS05B10_REG_PRE_ISP_20_0       CCI_REG8(0x5080)
> +#define OS05B10_DISABLED               0x00
> +#define OS05B10_COLOR_BAR_1            0x80
> +#define OS05B10_COLOR_BAR_2            0x84
> +#define OS05B10_COLOR_BAR_3            0x88
> +#define OS05B10_COLOR_BAR_4            0x8c
> +#define OS05B10_COLOR_SQUARE           0x82
> +#define OS05B10_BW_SQUARE              0x92
> +#define OS05B10_TRANSPARENT_EFFECT     0xa0
> +#define OS05B10_ROLLING_BAR_EFFECT     0xc0

I'd rather split the values into 0x80 "enabled" mask and a specific
mode value, but this is also good as it is.

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 08/17] media: i2c: os05b10: add 12-bit RAW mode support

[Tarang Raval]

Expose a 12-bit Bayer output option in the OS05B10 V4L2 sub-device driver.

Add a 12-bit mode table alongside the existing 10-bit mode, extend the
enumerated mbus codes to include RAW12, and select the correct mode table
based on the requested mbus format in enum_frame_size and stream enable.

Also move OS05B10_REG_MIPI_SC_CTRL_1 programming out of the common register
list and program it at stream-on depending on the selected mode bpp (10/12).

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 112 ++++++++++++++++++++++++++++++------
 1 file changed, 96 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 4e177eacc815..e11a3c308299 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -146,7 +146,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
 	{ CCI_REG8(0x301e), 0xb4 },
 	{ CCI_REG8(0x301f), 0xd0 },
 	{ CCI_REG8(0x3021), 0x03 },
-	{ OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
 	{ CCI_REG8(0x3107), 0xa1 },
 	{ CCI_REG8(0x3108), 0x7d },
 	{ CCI_REG8(0x3109), 0xfc },
@@ -500,6 +499,21 @@ struct os05b10_mode {
 	struct os05b10_reg_list reg_list;
 };
 
+static const struct os05b10_mode supported_modes_12bit[] = {
+	{
+		.width = 2592,
+		.height = 1944,
+		.vts = 2007,
+		.hts = 1744,
+		.exp = 1900,
+		.bpp = 12,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
+			.regs = mode_2592_1944_regs,
+		},
+	},
+};
+
 static const struct os05b10_mode supported_modes_10bit[] = {
 	{
 		.width = 2592,
@@ -521,6 +535,7 @@ static const s64 link_frequencies[] = {
 
 static const u32 os05b10_mbus_codes[] = {
 	MEDIA_BUS_FMT_SBGGR10_1X10,
+	MEDIA_BUS_FMT_SBGGR12_1X12,
 };
 
 static const char * const os05b10_test_pattern_menu[] = {
@@ -552,14 +567,20 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
 	return container_of_const(sd, struct os05b10, sd);
 };
 
-static u32 os05b10_get_format_code(struct os05b10 *os05b10)
+static u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
 {
-	static const u32 codes[2][2] = {
-		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
-		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
+	static const u32 codes[2][2][2] = {
+		{	/* 10 bpp */
+			{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
+			{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
+		},
+		{	/* 12 bpp */
+			{ MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12 },
+			{ MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12 },
+		},
 	};
 
-	return codes[os05b10->vflip->val][os05b10->hflip->val];
+	return codes[bpp == 12][os05b10->vflip->val][os05b10->hflip->val];
 }
 
 static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
@@ -571,6 +592,34 @@ static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
                         os05b10_tp_val[pattern], NULL);
 }
 
+static int get_mode_table(struct os05b10 *os05b10, unsigned int code,
+			  const struct os05b10_mode **mode_list,
+			  unsigned int *num_modes)
+{
+	switch (code) {
+	case MEDIA_BUS_FMT_SBGGR12_1X12:
+	case MEDIA_BUS_FMT_SGBRG12_1X12:
+	case MEDIA_BUS_FMT_SGRBG12_1X12:
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		*mode_list = supported_modes_12bit;
+		*num_modes = ARRAY_SIZE(supported_modes_12bit);
+		return 0;
+
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
+	case MEDIA_BUS_FMT_SGBRG10_1X10:
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		*mode_list = supported_modes_10bit;
+		*num_modes = ARRAY_SIZE(supported_modes_10bit);
+		return 0;
+
+	default:
+		dev_err(os05b10->dev,
+			"Unsupported media bus format: %#x\n", code);
+		return -EINVAL;
+	}
+}
+
 static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
@@ -650,8 +699,8 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
 		return -EINVAL;
 
-	code->code = os05b10_get_format_code(os05b10);
-
+	code->code = os05b10_get_format_code(os05b10,
+					     (code->index == 1) ? 12 : 10);
 	return 0;
 }
 
@@ -684,11 +733,20 @@ static int os05b10_set_pad_format(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_format *fmt)
 {
-	const struct os05b10_mode *mode = &supported_modes_10bit[0];
 	struct os05b10 *os05b10 = to_os05b10(sd);
+	const struct os05b10_mode *mode_list;
 	struct v4l2_mbus_framefmt *format;
+	const struct os05b10_mode *mode;
+	unsigned int num_modes;
 	int ret;
 
+	ret = get_mode_table(os05b10, fmt->format.code, &mode_list, &num_modes);
+	if (ret)
+		return ret;
+
+	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+				      fmt->format.width, fmt->format.height);
+
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
 	fmt->format.field = V4L2_FIELD_NONE;
@@ -731,12 +789,21 @@ static int os05b10_enum_frame_size(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_frame_size_enum *fse)
 {
-	if (fse->index >= ARRAY_SIZE(supported_modes_10bit))
+	struct os05b10 *os05b10 = to_os05b10(sd);
+	const struct os05b10_mode *mode_list;
+	unsigned int num_modes;
+	int ret;
+
+	ret = get_mode_table(os05b10, fse->code, &mode_list, &num_modes);
+	if (ret)
+		return ret;
+
+	if (fse->index >= num_modes)
 		return -EINVAL;
 
-	fse->min_width = supported_modes_10bit[fse->index].width;
+	fse->min_width = mode_list[fse->index].width;
 	fse->max_width = fse->min_width;
-	fse->min_height = supported_modes_10bit[fse->index].height;
+	fse->min_height = mode_list[fse->index].height;
 	fse->max_height = fse->min_height;
 
 	return 0;
@@ -749,13 +816,18 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 	struct os05b10 *os05b10 = to_os05b10(sd);
 	const struct os05b10_reg_list *reg_list;
 	const struct v4l2_mbus_framefmt *fmt;
+	const struct os05b10_mode *mode_list;
 	const struct os05b10_mode *mode;
+	unsigned int num_modes;
 	int ret;
 
 	fmt = v4l2_subdev_state_get_format(state, 0);
-	mode = v4l2_find_nearest_size(supported_modes_10bit,
-				      ARRAY_SIZE(supported_modes_10bit), width,
-				      height, fmt->width, fmt->height);
+	ret = get_mode_table(os05b10, fmt->code, &mode_list, &num_modes);
+	if (ret)
+		return ret;
+
+	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+				      fmt->width, fmt->height);
 
 	ret = pm_runtime_resume_and_get(os05b10->dev);
 	if (ret < 0)
@@ -769,6 +841,14 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 		goto err_rpm_put;
 	}
 
+	ret = cci_write(os05b10->cci, OS05B10_REG_MIPI_SC_CTRL_1,
+			(mode->bpp == 12) ? OS05B10_12BIT_MODE :
+			OS05B10_10BIT_MODE, NULL);
+	if (ret) {
+		dev_err(os05b10->dev, "failed to write pixel bit registers\n");
+		goto err_rpm_put;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &mode->reg_list;
 	ret = cci_multi_reg_write(os05b10->cci, reg_list->regs,
@@ -831,7 +911,7 @@ static int os05b10_init_state(struct v4l2_subdev *sd,
 	format = v4l2_subdev_state_get_format(state, 0);
 
 	mode = &supported_modes_10bit[0];
-	format->code = os05b10_get_format_code(os05b10);
+	format->code = os05b10_get_format_code(os05b10, 10);
 
 	/* Update image pad formate */
 	format->width = mode->width;
-- 
2.34.1

Re: [PATCH v3 08/17] media: i2c: os05b10: add 12-bit RAW mode support

[Vladimir Zapolskiy]

On 7/18/26 23:08, Tarang Raval wrote:
> Expose a 12-bit Bayer output option in the OS05B10 V4L2 sub-device driver.
> 
> Add a 12-bit mode table alongside the existing 10-bit mode, extend the
> enumerated mbus codes to include RAW12, and select the correct mode table
> based on the requested mbus format in enum_frame_size and stream enable.
> 
> Also move OS05B10_REG_MIPI_SC_CTRL_1 programming out of the common register
> list and program it at stream-on depending on the selected mode bpp (10/12).
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 112 ++++++++++++++++++++++++++++++------
>   1 file changed, 96 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 4e177eacc815..e11a3c308299 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -146,7 +146,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>   	{ CCI_REG8(0x301e), 0xb4 },
>   	{ CCI_REG8(0x301f), 0xd0 },
>   	{ CCI_REG8(0x3021), 0x03 },
> -	{ OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
>   	{ CCI_REG8(0x3107), 0xa1 },
>   	{ CCI_REG8(0x3108), 0x7d },
>   	{ CCI_REG8(0x3109), 0xfc },
> @@ -500,6 +499,21 @@ struct os05b10_mode {
>   	struct os05b10_reg_list reg_list;
>   };
>   
> +static const struct os05b10_mode supported_modes_12bit[] = {
> +	{
> +		.width = 2592,
> +		.height = 1944,
> +		.vts = 2007,
> +		.hts = 1744,

It's unusual to see .hts < .width and .vts < .height. Will is cause
errors in hblank/vblank computations in os05b10_set_framing_limits()?

> +		.exp = 1900,
> +		.bpp = 12,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
> +			.regs = mode_2592_1944_regs,
> +		},
> +	},
> +};
> +
>   static const struct os05b10_mode supported_modes_10bit[] = {
>   	{
>   		.width = 2592,
> @@ -521,6 +535,7 @@ static const s64 link_frequencies[] = {
>   
>   static const u32 os05b10_mbus_codes[] = {
>   	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
>   };
>   
>   static const char * const os05b10_test_pattern_menu[] = {
> @@ -552,14 +567,20 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
>   	return container_of_const(sd, struct os05b10, sd);
>   };
>   
> -static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> +static u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
>   {
> -	static const u32 codes[2][2] = {
> -		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> -		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> +	static const u32 codes[2][2][2] = {
> +		{	/* 10 bpp */
> +			{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> +			{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> +		},
> +		{	/* 12 bpp */
> +			{ MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> +			{ MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> +		},
>   	};
>   
> -	return codes[os05b10->vflip->val][os05b10->hflip->val];
> +	return codes[bpp == 12][os05b10->vflip->val][os05b10->hflip->val];
>   }
>   
>   static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
> @@ -571,6 +592,34 @@ static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
>                           os05b10_tp_val[pattern], NULL);
>   }
>   
> +static int get_mode_table(struct os05b10 *os05b10, unsigned int code,
> +			  const struct os05b10_mode **mode_list,
> +			  unsigned int *num_modes)
> +{
> +	switch (code) {
> +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +		*mode_list = supported_modes_12bit;
> +		*num_modes = ARRAY_SIZE(supported_modes_12bit);
> +		return 0;
> +
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		*mode_list = supported_modes_10bit;
> +		*num_modes = ARRAY_SIZE(supported_modes_10bit);
> +		return 0;
> +
> +	default:
> +		dev_err(os05b10->dev,
> +			"Unsupported media bus format: %#x\n", code);
> +		return -EINVAL;
> +	}
> +}
> +
>   static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> @@ -650,8 +699,8 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>   	if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
>   		return -EINVAL;
>   
> -	code->code = os05b10_get_format_code(os05b10);
> -
> +	code->code = os05b10_get_format_code(os05b10,
> +					     (code->index == 1) ? 12 : 10);

os05b10_mbus_codes[code->index] == MEDIA_BUS_FMT_SBGGR12_1X12 is more
verbose, but seems to be a better and more reliable check.

Probably a simple inline function to get bpp from code->index can be added.

Another option is to change the second argument of os05b10_get_format_code()
from bpp to just media bus format, so you can write

     os05b10_get_format_code(os05b10, os05b10_mbus_codes[code->index]);

>   	return 0;
>   }
>   
> @@ -684,11 +733,20 @@ static int os05b10_set_pad_format(struct v4l2_subdev *sd,
>   				  struct v4l2_subdev_state *sd_state,
>   				  struct v4l2_subdev_format *fmt)
>   {
> -	const struct os05b10_mode *mode = &supported_modes_10bit[0];
>   	struct os05b10 *os05b10 = to_os05b10(sd);
> +	const struct os05b10_mode *mode_list;
>   	struct v4l2_mbus_framefmt *format;
> +	const struct os05b10_mode *mode;
> +	unsigned int num_modes;
>   	int ret;
>   
> +	ret = get_mode_table(os05b10, fmt->format.code, &mode_list, &num_modes);
> +	if (ret)
> +		return ret;
> +
> +	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> +				      fmt->format.width, fmt->format.height);
> +
>   	fmt->format.width = mode->width;
>   	fmt->format.height = mode->height;
>   	fmt->format.field = V4L2_FIELD_NONE;
> @@ -731,12 +789,21 @@ static int os05b10_enum_frame_size(struct v4l2_subdev *sd,
>   				   struct v4l2_subdev_state *sd_state,
>   				   struct v4l2_subdev_frame_size_enum *fse)
>   {
> -	if (fse->index >= ARRAY_SIZE(supported_modes_10bit))
> +	struct os05b10 *os05b10 = to_os05b10(sd);
> +	const struct os05b10_mode *mode_list;
> +	unsigned int num_modes;
> +	int ret;
> +
> +	ret = get_mode_table(os05b10, fse->code, &mode_list, &num_modes);
> +	if (ret)
> +		return ret;
> +
> +	if (fse->index >= num_modes)
>   		return -EINVAL;
>   
> -	fse->min_width = supported_modes_10bit[fse->index].width;
> +	fse->min_width = mode_list[fse->index].width;
>   	fse->max_width = fse->min_width;
> -	fse->min_height = supported_modes_10bit[fse->index].height;
> +	fse->min_height = mode_list[fse->index].height;
>   	fse->max_height = fse->min_height;
>   
>   	return 0;
> @@ -749,13 +816,18 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   	struct os05b10 *os05b10 = to_os05b10(sd);
>   	const struct os05b10_reg_list *reg_list;
>   	const struct v4l2_mbus_framefmt *fmt;
> +	const struct os05b10_mode *mode_list;
>   	const struct os05b10_mode *mode;
> +	unsigned int num_modes;
>   	int ret;
>   
>   	fmt = v4l2_subdev_state_get_format(state, 0);
> -	mode = v4l2_find_nearest_size(supported_modes_10bit,
> -				      ARRAY_SIZE(supported_modes_10bit), width,
> -				      height, fmt->width, fmt->height);
> +	ret = get_mode_table(os05b10, fmt->code, &mode_list, &num_modes);
> +	if (ret)
> +		return ret;
> +
> +	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> +				      fmt->width, fmt->height);
>   
>   	ret = pm_runtime_resume_and_get(os05b10->dev);
>   	if (ret < 0)
> @@ -769,6 +841,14 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   		goto err_rpm_put;
>   	}
>   
> +	ret = cci_write(os05b10->cci, OS05B10_REG_MIPI_SC_CTRL_1,
> +			(mode->bpp == 12) ? OS05B10_12BIT_MODE :
> +			OS05B10_10BIT_MODE, NULL);
> +	if (ret) {
> +		dev_err(os05b10->dev, "failed to write pixel bit registers\n");
> +		goto err_rpm_put;
> +	}
> +
>   	/* Write sensor mode registers */
>   	reg_list = &mode->reg_list;
>   	ret = cci_multi_reg_write(os05b10->cci, reg_list->regs,
> @@ -831,7 +911,7 @@ static int os05b10_init_state(struct v4l2_subdev *sd,
>   	format = v4l2_subdev_state_get_format(state, 0);
>   
>   	mode = &supported_modes_10bit[0];
> -	format->code = os05b10_get_format_code(os05b10);
> +	format->code = os05b10_get_format_code(os05b10, 10);

If the type of the second argument of os05b10_get_format_code() is changed,
it'd be needed to change it to a clearer

       os05b10_get_format_code(os05b10, os05b10_mbus_codes[0]);

>   
>   	/* Update image pad formate */
>   	format->width = mode->width;

-- 
Best wishes,
Vladimir

Re: [PATCH v3 08/17] media: i2c: os05b10: add 12-bit RAW mode support

[Tarang Raval]

Hi Vladimir,

Thanks for the review.

> On 7/18/26 23:08, Tarang Raval wrote:
> > Expose a 12-bit Bayer output option in the OS05B10 V4L2 sub-device driver.
> >
> > Add a 12-bit mode table alongside the existing 10-bit mode, extend the
> > enumerated mbus codes to include RAW12, and select the correct mode table
> > based on the requested mbus format in enum_frame_size and stream enable.
> >
> > Also move OS05B10_REG_MIPI_SC_CTRL_1 programming out of the common register
> > list and program it at stream-on depending on the selected mode bpp (10/12).
> >
> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > ---
> >   drivers/media/i2c/os05b10.c | 112 ++++++++++++++++++++++++++++++------
> >   1 file changed, 96 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 4e177eacc815..e11a3c308299 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -146,7 +146,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
> >       { CCI_REG8(0x301e), 0xb4 },
> >       { CCI_REG8(0x301f), 0xd0 },
> >       { CCI_REG8(0x3021), 0x03 },
> > -     { OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
> >       { CCI_REG8(0x3107), 0xa1 },
> >       { CCI_REG8(0x3108), 0x7d },
> >       { CCI_REG8(0x3109), 0xfc },
> > @@ -500,6 +499,21 @@ struct os05b10_mode {
> >       struct os05b10_reg_list reg_list;
> >   };
> >
> > +static const struct os05b10_mode supported_modes_12bit[] = {
> > +     {
> > +             .width = 2592,
> > +             .height = 1944,
> > +             .vts = 2007,
> > +             .hts = 1744,
>
> It's unusual to see .hts < .width and .vts < .height. Will is cause
> errors in hblank/vblank computations in os05b10_set_framing_limits()?

Negative hblank is accepted by the framework, so no, I'm not seeing any
errors from it.

But you're right that it doesn't make logical/physical sense. I got a
similar comment from Jai on patch 15/17 about this.

I'll update it accordingly, the datasheet doesn't have any information
about the HTS register's unit/clock domain, so I'm doing some testing on
hardware first and will update this once I have concrete numbers.

> > +             .exp = 1900,
> > +             .bpp = 12,
> > +             .reg_list = {
> > +                     .num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
> > +                     .regs = mode_2592_1944_regs,
> > +             },
> > +     },
> > +};
> > +
> >   static const struct os05b10_mode supported_modes_10bit[] = {
> >       {
> >               .width = 2592,
> > @@ -521,6 +535,7 @@ static const s64 link_frequencies[] = {
> >
> >   static const u32 os05b10_mbus_codes[] = {
> >       MEDIA_BUS_FMT_SBGGR10_1X10,
> > +     MEDIA_BUS_FMT_SBGGR12_1X12,
> >   };
> >
> >   static const char * const os05b10_test_pattern_menu[] = {
> > @@ -552,14 +567,20 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
> >       return container_of_const(sd, struct os05b10, sd);
> >   };
> >
> > -static u32 os05b10_get_format_code(struct os05b10 *os05b10)
> > +static u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
> >   {
> > -     static const u32 codes[2][2] = {
> > -             { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> > -             { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> > +     static const u32 codes[2][2][2] = {
> > +             {       /* 10 bpp */
> > +                     { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> > +                     { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> > +             },
> > +             {       /* 12 bpp */
> > +                     { MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> > +                     { MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> > +             },
> >       };
> >
> > -     return codes[os05b10->vflip->val][os05b10->hflip->val];
> > +     return codes[bpp == 12][os05b10->vflip->val][os05b10->hflip->val];
> >   }
> >
> >   static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
> > @@ -571,6 +592,34 @@ static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
> >                           os05b10_tp_val[pattern], NULL);
> >   }
> >
> > +static int get_mode_table(struct os05b10 *os05b10, unsigned int code,
> > +                       const struct os05b10_mode **mode_list,
> > +                       unsigned int *num_modes)
> > +{
> > +     switch (code) {
> > +     case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +     case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +     case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +     case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +             *mode_list = supported_modes_12bit;
> > +             *num_modes = ARRAY_SIZE(supported_modes_12bit);
> > +             return 0;
> > +
> > +     case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +     case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +     case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +     case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +             *mode_list = supported_modes_10bit;
> > +             *num_modes = ARRAY_SIZE(supported_modes_10bit);
> > +             return 0;
> > +
> > +     default:
> > +             dev_err(os05b10->dev,
> > +                     "Unsupported media bus format: %#x\n", code);
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> >   static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
> >   {
> >       struct os05b10 *os05b10 = container_of_const(ctrl->handler,
> > @@ -650,8 +699,8 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
> >       if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
> >               return -EINVAL;
> >
> > -     code->code = os05b10_get_format_code(os05b10);
> > -
> > +     code->code = os05b10_get_format_code(os05b10,
> > +                                          (code->index == 1) ? 12 : 10);
>
> os05b10_mbus_codes[code->index] == MEDIA_BUS_FMT_SBGGR12_1X12 is more
> verbose, but seems to be a better and more reliable check.
>
> Probably a simple inline function to get bpp from code->index can be added.
>
> Another option is to change the second argument of os05b10_get_format_code()
> from bpp to just media bus format, so you can write
>
>      os05b10_get_format_code(os05b10, os05b10_mbus_codes[code->index]);

Okay, I will update the os05b10_get_format_code 2nd argument.

Best Regards,
Tarang

Re: [PATCH v3 08/17] media: i2c: os05b10: add 12-bit RAW mode support

[Vladimir Zapolskiy]

On 7/24/26 17:30, Tarang Raval wrote:
> Hi Vladimir,
> 
> Thanks for the review.
> 
>> On 7/18/26 23:08, Tarang Raval wrote:
>>> Expose a 12-bit Bayer output option in the OS05B10 V4L2 sub-device driver.
>>>
>>> Add a 12-bit mode table alongside the existing 10-bit mode, extend the
>>> enumerated mbus codes to include RAW12, and select the correct mode table
>>> based on the requested mbus format in enum_frame_size and stream enable.
>>>
>>> Also move OS05B10_REG_MIPI_SC_CTRL_1 programming out of the common register
>>> list and program it at stream-on depending on the selected mode bpp (10/12).
>>>
>>> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
>>> ---
>>>    drivers/media/i2c/os05b10.c | 112 ++++++++++++++++++++++++++++++------
>>>    1 file changed, 96 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
>>> index 4e177eacc815..e11a3c308299 100644
>>> --- a/drivers/media/i2c/os05b10.c
>>> +++ b/drivers/media/i2c/os05b10.c
>>> @@ -146,7 +146,6 @@ static const struct cci_reg_sequence os05b10_common_regs[] = {
>>>        { CCI_REG8(0x301e), 0xb4 },
>>>        { CCI_REG8(0x301f), 0xd0 },
>>>        { CCI_REG8(0x3021), 0x03 },
>>> -     { OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
>>>        { CCI_REG8(0x3107), 0xa1 },
>>>        { CCI_REG8(0x3108), 0x7d },
>>>        { CCI_REG8(0x3109), 0xfc },
>>> @@ -500,6 +499,21 @@ struct os05b10_mode {
>>>        struct os05b10_reg_list reg_list;
>>>    };
>>>
>>> +static const struct os05b10_mode supported_modes_12bit[] = {
>>> +     {
>>> +             .width = 2592,
>>> +             .height = 1944,
>>> +             .vts = 2007,
>>> +             .hts = 1744,
>>
>> It's unusual to see .hts < .width and .vts < .height. Will is cause
>> errors in hblank/vblank computations in os05b10_set_framing_limits()?
> 
> Negative hblank is accepted by the framework, so no, I'm not seeing any
> errors from it.
> 

It could be so, but in os05b10_set_framing_limits() hblank/vblank types
are unsigned (and that's correct), so there is a likely unwanted
assigment of a negative value to an unsigned variable. It's better
to correct it.

> But you're right that it doesn't make logical/physical sense. I got a
> similar comment from Jai on patch 15/17 about this.
> 
> I'll update it accordingly, the datasheet doesn't have any information
> about the HTS register's unit/clock domain, so I'm doing some testing on
> hardware first and will update this once I have concrete numbers.
> 
>>> +             .exp = 1900,
>>> +             .bpp = 12,
>>> +             .reg_list = {
>>> +                     .num_of_regs = ARRAY_SIZE(mode_2592_1944_regs),
>>> +                     .regs = mode_2592_1944_regs,
>>> +             },
>>> +     },
>>> +};
>>> +
>>>    static const struct os05b10_mode supported_modes_10bit[] = {
>>>        {
>>>                .width = 2592,
>>> @@ -521,6 +535,7 @@ static const s64 link_frequencies[] = {
>>>
>>>    static const u32 os05b10_mbus_codes[] = {
>>>        MEDIA_BUS_FMT_SBGGR10_1X10,
>>> +     MEDIA_BUS_FMT_SBGGR12_1X12,
>>>    };
>>>
>>>    static const char * const os05b10_test_pattern_menu[] = {
>>> @@ -552,14 +567,20 @@ static inline struct os05b10 *to_os05b10(struct v4l2_subdev *sd)
>>>        return container_of_const(sd, struct os05b10, sd);
>>>    };
>>>
>>> -static u32 os05b10_get_format_code(struct os05b10 *os05b10)
>>> +static u32 os05b10_get_format_code(struct os05b10 *os05b10, u8 bpp)
>>>    {
>>> -     static const u32 codes[2][2] = {
>>> -             { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
>>> -             { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
>>> +     static const u32 codes[2][2][2] = {
>>> +             {       /* 10 bpp */
>>> +                     { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
>>> +                     { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
>>> +             },
>>> +             {       /* 12 bpp */
>>> +                     { MEDIA_BUS_FMT_SBGGR12_1X12, MEDIA_BUS_FMT_SGBRG12_1X12 },
>>> +                     { MEDIA_BUS_FMT_SGRBG12_1X12, MEDIA_BUS_FMT_SRGGB12_1X12 },
>>> +             },
>>>        };
>>>
>>> -     return codes[os05b10->vflip->val][os05b10->hflip->val];
>>> +     return codes[bpp == 12][os05b10->vflip->val][os05b10->hflip->val];
>>>    }
>>>
>>>    static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
>>> @@ -571,6 +592,34 @@ static int os05b10_update_test_pattern(struct os05b10 *os05b10, u32 pattern)
>>>                            os05b10_tp_val[pattern], NULL);
>>>    }
>>>
>>> +static int get_mode_table(struct os05b10 *os05b10, unsigned int code,
>>> +                       const struct os05b10_mode **mode_list,
>>> +                       unsigned int *num_modes)
>>> +{
>>> +     switch (code) {
>>> +     case MEDIA_BUS_FMT_SBGGR12_1X12:
>>> +     case MEDIA_BUS_FMT_SGBRG12_1X12:
>>> +     case MEDIA_BUS_FMT_SGRBG12_1X12:
>>> +     case MEDIA_BUS_FMT_SRGGB12_1X12:
>>> +             *mode_list = supported_modes_12bit;
>>> +             *num_modes = ARRAY_SIZE(supported_modes_12bit);
>>> +             return 0;
>>> +
>>> +     case MEDIA_BUS_FMT_SBGGR10_1X10:
>>> +     case MEDIA_BUS_FMT_SGBRG10_1X10:
>>> +     case MEDIA_BUS_FMT_SGRBG10_1X10:
>>> +     case MEDIA_BUS_FMT_SRGGB10_1X10:
>>> +             *mode_list = supported_modes_10bit;
>>> +             *num_modes = ARRAY_SIZE(supported_modes_10bit);
>>> +             return 0;
>>> +
>>> +     default:
>>> +             dev_err(os05b10->dev,
>>> +                     "Unsupported media bus format: %#x\n", code);
>>> +             return -EINVAL;
>>> +     }
>>> +}
>>> +
>>>    static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>>>    {
>>>        struct os05b10 *os05b10 = container_of_const(ctrl->handler,
>>> @@ -650,8 +699,8 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>>>        if (code->index >= ARRAY_SIZE(os05b10_mbus_codes))
>>>                return -EINVAL;
>>>
>>> -     code->code = os05b10_get_format_code(os05b10);
>>> -
>>> +     code->code = os05b10_get_format_code(os05b10,
>>> +                                          (code->index == 1) ? 12 : 10);
>>
>> os05b10_mbus_codes[code->index] == MEDIA_BUS_FMT_SBGGR12_1X12 is more
>> verbose, but seems to be a better and more reliable check.
>>
>> Probably a simple inline function to get bpp from code->index can be added.
>>
>> Another option is to change the second argument of os05b10_get_format_code()
>> from bpp to just media bus format, so you can write
>>
>>       os05b10_get_format_code(os05b10, os05b10_mbus_codes[code->index]);
> 
> Okay, I will update the os05b10_get_format_code 2nd argument.
> 

Hopefully it's not a big disadvantage, a bit of complexity will be moved
to function body from the caller's side. Thank you!

-- 
Best wishes,
Vladimir

[PATCH v3 09/17] media: i2c: os05b10: update pixel rate on 10/12-bit mode switch

[Tarang Raval]

After adding 12-bit RAW support, the pixel rate depends on the selected
mode bpp. Store the V4L2_CID_PIXEL_RATE control pointer and update its
range/value when the mode changes so 10/12-bit switching reports the
correct pixel rate.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 41 ++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index e11a3c308299..3fd788bd8b20 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -473,6 +473,7 @@ struct os05b10 {
 	/* V4L2 Controls */
 	struct v4l2_ctrl_handler handler;
 	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *gain;
@@ -704,12 +705,35 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
+			      const struct os05b10_mode *mode)
+{
+	u64 link_freq = link_frequencies[os05b10->link_freq_index];
+	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
+
+	dev_dbg(os05b10->dev,
+		"link_freq=%llu bpp=%u lanes=%u pixel_rate=%llu\n",
+		link_freq, mode->bpp, os05b10->data_lanes, pixel_rate);
+
+	return pixel_rate;
+}
+
 static int os05b10_set_framing_limits(struct os05b10 *os05b10,
 				      const struct os05b10_mode *mode)
 {
+	u64 pixel_rate = os05b10_pixel_rate(os05b10, mode);
 	u32 hblank, vblank, vblank_max, max_exp;
 	int ret;
 
+	ret = __v4l2_ctrl_modify_range(os05b10->pixel_rate, pixel_rate,
+				       pixel_rate, 1, pixel_rate);
+	if (ret)
+		return ret;
+
+	ret = __v4l2_ctrl_s_ctrl_int64(os05b10->pixel_rate, pixel_rate);
+	if (ret)
+		return ret;
+
 	hblank = mode->hts - mode->width;
 	ret = __v4l2_ctrl_modify_range(os05b10->hblank, hblank, hblank, 1,
 				       hblank);
@@ -1070,18 +1094,6 @@ static int os05b10_parse_endpoint(struct os05b10 *os05b10)
 	return ret;
 }
 
-static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
-			      const struct os05b10_mode *mode)
-{
-	u64 link_freq = link_frequencies[os05b10->link_freq_index];
-	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
-
-	dev_dbg(os05b10->dev,
-		"link_freq=%llu bpp=%u lanes=%u pixel_rate=%llu\n",
-		link_freq, mode->bpp, os05b10->data_lanes, pixel_rate);
-
-	return pixel_rate;
-}
 
 static int os05b10_init_controls(struct os05b10 *os05b10)
 {
@@ -1095,8 +1107,9 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
 
 	pixel_rate = os05b10_pixel_rate(os05b10, mode);
-	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
-			  pixel_rate, pixel_rate, 1, pixel_rate);
+	os05b10->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
+						V4L2_CID_PIXEL_RATE, pixel_rate,
+						pixel_rate, 1, pixel_rate);
 
 	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
 						    V4L2_CID_LINK_FREQ,
-- 
2.34.1

Re: [PATCH v3 09/17] media: i2c: os05b10: update pixel rate on 10/12-bit mode switch

[Jai Luthra]

Quoting Tarang Raval (2026-07-19 01:39:00)
> After adding 12-bit RAW support, the pixel rate depends on the selected
> mode bpp. Store the V4L2_CID_PIXEL_RATE control pointer and update its
> range/value when the mode changes so 10/12-bit switching reports the
> correct pixel rate.
> 

Link frequency (output bus rate) and Pixel rate (sensor internal readout)
can often be independent of each other. It would be good to verify that in
the sensor documentation.

A quick test would be different bpp and the same HTS/VTS and frame size
registers if you get the same frame rate, it's likely that the sensor's
internal PIXEL_RATE doesn't change due to bpp.

> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>

Thanks,
    Jai

Re: [PATCH v3 09/17] media: i2c: os05b10: update pixel rate on 10/12-bit mode switch

[Jai Luthra]

Quoting Jai Luthra (2026-07-22 11:47:49)
> Quoting Tarang Raval (2026-07-19 01:39:00)
> > After adding 12-bit RAW support, the pixel rate depends on the selected
> > mode bpp. Store the V4L2_CID_PIXEL_RATE control pointer and update its
> > range/value when the mode changes so 10/12-bit switching reports the
> > correct pixel rate.
> > 
> 
> Link frequency (output bus rate) and Pixel rate (sensor internal readout)
> can often be independent of each other. It would be good to verify that in
> the sensor documentation.
> 
> A quick test would be different bpp and the same HTS/VTS and frame size
> registers if you get the same frame rate, it's likely that the sensor's
> internal PIXEL_RATE doesn't change due to bpp.
> 

If this sensor indeed operates on different pixel rates just because of the
output width, I would suggest you squash this to the prior patch which
introduced the 12 bit support.

> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> 
> Thanks,
>     Jai

Re: [PATCH v3 09/17] media: i2c: os05b10: update pixel rate on 10/12-bit mode switch

[Tarang Raval]

Hi Jai,

Thanks for the review.

> Quoting Jai Luthra (2026-07-22 11:47:49)
> > Quoting Tarang Raval (2026-07-19 01:39:00)
> > > After adding 12-bit RAW support, the pixel rate depends on the selected
> > > mode bpp. Store the V4L2_CID_PIXEL_RATE control pointer and update its
> > > range/value when the mode changes so 10/12-bit switching reports the
> > > correct pixel rate.
> > >
> >
> > Link frequency (output bus rate) and Pixel rate (sensor internal readout)
> > can often be independent of each other. It would be good to verify that in
> > the sensor documentation.
> >
> > A quick test would be different bpp and the same HTS/VTS and frame size
> > registers if you get the same frame rate, it's likely that the sensor's
> > internal PIXEL_RATE doesn't change due to bpp.
> >
>
> If this sensor indeed operates on different pixel rates just because of the
> output width, I would suggest you squash this to the prior patch which
> introduced the 12 bit support.

I ran the quick test you suggested.

Both 10-bit and 12-bit were tested at 2592x1944, using the same HTS
(shared register list), and I explicitly forced VTS to be identical
between the two run

Measured actual streamed frame rate with v4l2-ctl --stream-mmap in both
cases:

  10-bit, 2592x1944, vts=2006: 30.01 fps
  12-bit, 2592x1944, vts=2006: 30.01 fps

Frame rate is identical. So it looks like the sensor's internal readout
timing doesn't depend on bit depth at all, HTS/VTS alone determine the
real frame rate, and bpp only affects how pixels are packed onto the MIPI
bus, not how fast they're read out.

Best Regards,
Tarang

Re: [PATCH v3 09/17] media: i2c: os05b10: update pixel rate on 10/12-bit mode switch

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> After adding 12-bit RAW support, the pixel rate depends on the selected
> mode bpp. Store the V4L2_CID_PIXEL_RATE control pointer and update its
> range/value when the mode changes so 10/12-bit switching reports the
> correct pixel rate.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 41 ++++++++++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index e11a3c308299..3fd788bd8b20 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -473,6 +473,7 @@ struct os05b10 {
>   	/* V4L2 Controls */
>   	struct v4l2_ctrl_handler handler;
>   	struct v4l2_ctrl *link_freq;
> +	struct v4l2_ctrl *pixel_rate;
>   	struct v4l2_ctrl *hblank;
>   	struct v4l2_ctrl *vblank;
>   	struct v4l2_ctrl *gain;
> @@ -704,12 +705,35 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>   	return 0;
>   }
>   
> +static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
> +			      const struct os05b10_mode *mode)
> +{
> +	u64 link_freq = link_frequencies[os05b10->link_freq_index];
> +	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
> +
> +	dev_dbg(os05b10->dev,
> +		"link_freq=%llu bpp=%u lanes=%u pixel_rate=%llu\n",
> +		link_freq, mode->bpp, os05b10->data_lanes, pixel_rate);
> +
> +	return pixel_rate;
> +}
> +
>   static int os05b10_set_framing_limits(struct os05b10 *os05b10,
>   				      const struct os05b10_mode *mode)
>   {
> +	u64 pixel_rate = os05b10_pixel_rate(os05b10, mode);
>   	u32 hblank, vblank, vblank_max, max_exp;
>   	int ret;
>   
> +	ret = __v4l2_ctrl_modify_range(os05b10->pixel_rate, pixel_rate,
> +				       pixel_rate, 1, pixel_rate);
> +	if (ret)
> +		return ret;
> +
> +	ret = __v4l2_ctrl_s_ctrl_int64(os05b10->pixel_rate, pixel_rate);
> +	if (ret)
> +		return ret;
> +
>   	hblank = mode->hts - mode->width;
>   	ret = __v4l2_ctrl_modify_range(os05b10->hblank, hblank, hblank, 1,
>   				       hblank);
> @@ -1070,18 +1094,6 @@ static int os05b10_parse_endpoint(struct os05b10 *os05b10)
>   	return ret;
>   }
>   
> -static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
> -			      const struct os05b10_mode *mode)
> -{
> -	u64 link_freq = link_frequencies[os05b10->link_freq_index];
> -	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
> -
> -	dev_dbg(os05b10->dev,
> -		"link_freq=%llu bpp=%u lanes=%u pixel_rate=%llu\n",
> -		link_freq, mode->bpp, os05b10->data_lanes, pixel_rate);
> -
> -	return pixel_rate;
> -}
>   
>   static int os05b10_init_controls(struct os05b10 *os05b10)
>   {
> @@ -1095,8 +1107,9 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
>   
>   	pixel_rate = os05b10_pixel_rate(os05b10, mode);
> -	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_PIXEL_RATE,
> -			  pixel_rate, pixel_rate, 1, pixel_rate);
> +	os05b10->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> +						V4L2_CID_PIXEL_RATE, pixel_rate,
> +						pixel_rate, 1, pixel_rate);
>   
>   	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
>   						    V4L2_CID_LINK_FREQ,

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 10/17] media: i2c: os05b10: Add 1080p and 2x2 binning 720p modes

[Tarang Raval]

Add support for 1920x1080 and 1280x720 resolutions.
The 1280x720 mode uses 2x2 binning.

Both 10-bit and 12-bit pixel formats are supported.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 120 ++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 3fd788bd8b20..c54b466656aa 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -460,6 +460,78 @@ static const struct cci_reg_sequence mode_2592_1944_regs[] = {
 	{ CCI_REG8(0x4837), 0x12 },
 };
 
+static const struct cci_reg_sequence mode_1920_1080_regs[] = {
+	{ OS05B10_REG_X_ADDR_START,	0x0280 },
+	{ OS05B10_REG_Y_ADDR_START,	0x01b4 },
+	{ OS05B10_REG_X_ADDR_END,	0x0a0f },
+	{ OS05B10_REG_Y_ADDR_END,	0x05f3 },
+	{ OS05B10_REG_X_OUTPUT_SIZE,	0x0780 },
+	{ OS05B10_REG_Y_OUTPUT_SIZE,	0x0438 },
+	{ OS05B10_REG_HTS,		0x06d0 },
+	{ OS05B10_REG_ISP_X_WIN,	0x0008 },
+	{ OS05B10_REG_ISP_Y_WIN,	0x0008 },
+	{ OS05B10_REG_X_INC_ODD,	0x01 },
+	{ OS05B10_REG_X_INC_EVEN,	0x01 },
+	{ OS05B10_REG_Y_INC_ODD,	0x01 },
+	{ OS05B10_REG_Y_INC_EVEN,	0x01 },
+	{ OS05B10_REG_FORMAT1,		0x88 },
+	{ OS05B10_REG_FORMAT2,		0x00 },
+	{ CCI_REG8(0x3610), 0x87 },
+	{ CCI_REG8(0x3620), 0x0c },
+	{ CCI_REG8(0x3662), 0x10 },
+	{ CCI_REG8(0x3714), 0x24 },
+	{ CCI_REG8(0x373f), 0xb0 },
+	{ CCI_REG8(0x37bf), 0x05 },
+	{ CCI_REG8(0x37c2), 0x04 },
+	{ CCI_REG8(0x37d9), 0x08 },
+	{ CCI_REG8(0x3832), 0x08 },
+	{ CCI_REG8(0x3c86), 0x03 },
+	{ CCI_REG8(0x3d8c), 0x71 },
+	{ CCI_REG8(0x3f03), 0x08 },
+	{ CCI_REG8(0x4008), 0x02 },
+	{ CCI_REG8(0x4009), 0x0d },
+	{ CCI_REG8(0x400a), 0x02 },
+	{ CCI_REG8(0x4041), 0x07 },
+	{ CCI_REG8(0x4505), 0xc4 },
+	{ CCI_REG8(0x4837), 0x0d },
+};
+
+static const struct cci_reg_sequence mode_1280_720_regs[] = {
+	{ OS05B10_REG_X_ADDR_START,	0x0140 },
+	{ OS05B10_REG_Y_ADDR_START,	0x00fc },
+	{ OS05B10_REG_X_ADDR_END,	0x0b4f },
+	{ OS05B10_REG_Y_ADDR_END,	0x06ab },
+	{ OS05B10_REG_X_OUTPUT_SIZE,	0x0500 },
+	{ OS05B10_REG_Y_OUTPUT_SIZE,	0x02d0 },
+	{ OS05B10_REG_HTS,		0x0368 },
+	{ OS05B10_REG_ISP_X_WIN,	0x0004 },
+	{ OS05B10_REG_ISP_Y_WIN,	0x0004 },
+	{ OS05B10_REG_X_INC_ODD,	0x03 },
+	{ OS05B10_REG_X_INC_EVEN,	0x01 },
+	{ OS05B10_REG_Y_INC_ODD,	0x03 },
+	{ OS05B10_REG_Y_INC_EVEN,	0x01 },
+	{ OS05B10_REG_FORMAT1,		0x8b },
+	{ OS05B10_REG_FORMAT2,		0x00 },
+	{ CCI_REG8(0x3610), 0x57 },
+	{ CCI_REG8(0x3620), 0x01 },
+	{ CCI_REG8(0x3662), 0x08 },
+	{ CCI_REG8(0x3714), 0x28 },
+	{ CCI_REG8(0x373f), 0xa0 },
+	{ CCI_REG8(0x37bf), 0x05 },
+	{ CCI_REG8(0x37c2), 0x14 },
+	{ CCI_REG8(0x37d9), 0x04 },
+	{ CCI_REG8(0x3832), 0x00 },
+	{ CCI_REG8(0x3c86), 0x03 },
+	{ CCI_REG8(0x3d8c), 0x71 },
+	{ CCI_REG8(0x3f03), 0x1d },
+	{ CCI_REG8(0x4008), 0x01 },
+	{ CCI_REG8(0x4009), 0x06 },
+	{ CCI_REG8(0x400a), 0x02 },
+	{ CCI_REG8(0x4041), 0x03 },
+	{ CCI_REG8(0x4505), 0xe4 },
+	{ CCI_REG8(0x4837), 0x0d },
+};
+
 struct os05b10 {
 	struct device *dev;
 	struct regmap *cci;
@@ -513,6 +585,30 @@ static const struct os05b10_mode supported_modes_12bit[] = {
 			.regs = mode_2592_1944_regs,
 		},
 	},
+	{	/* 40 fps */
+		.width = 1920,
+		.height = 1080,
+		.vts = 1504,
+		.hts = 1744,
+		.exp = 1472,
+		.bpp = 12,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
+			.regs = mode_1920_1080_regs,
+		},
+	},
+	{	/* 2x2 binning 120 fps */
+		.width = 1280,
+		.height = 720,
+		.vts = 1003,
+		.hts = 872,
+		.exp = 970,
+		.bpp = 12,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280_720_regs),
+			.regs = mode_1280_720_regs,
+		},
+	},
 };
 
 static const struct os05b10_mode supported_modes_10bit[] = {
@@ -528,6 +624,30 @@ static const struct os05b10_mode supported_modes_10bit[] = {
 			.regs = mode_2592_1944_regs,
 		},
 	},
+	{	/* 40 fps */
+		.width = 1920,
+		.height = 1080,
+		.vts = 1504,
+		.hts = 1744,
+		.exp = 1472,
+		.bpp = 10,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920_1080_regs),
+			.regs = mode_1920_1080_regs,
+		},
+	},
+	{	/* 2x2 binning 120 fps */
+		.width = 1280,
+		.height = 720,
+		.vts = 1003,
+		.hts = 872,
+		.exp = 970,
+		.bpp = 10,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1280_720_regs),
+			.regs = mode_1280_720_regs,
+		},
+	},
 };
 
 static const s64 link_frequencies[] = {
-- 
2.34.1

Re: [PATCH v3 10/17] media: i2c: os05b10: Add 1080p and 2x2 binning 720p modes

[Jai Luthra]

Hi Tarang,

Quoting Tarang Raval (2026-07-19 01:39:01)
> Add support for 1920x1080 and 1280x720 resolutions.
> The 1280x720 mode uses 2x2 binning.
> 
> Both 10-bit and 12-bit pixel formats are supported.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/os05b10.c | 120 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 3fd788bd8b20..c54b466656aa 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -460,6 +460,78 @@ static const struct cci_reg_sequence mode_2592_1944_regs[] = {
>         { CCI_REG8(0x4837), 0x12 },
>  };
>  
> +static const struct cci_reg_sequence mode_1920_1080_regs[] = {
> +       { OS05B10_REG_X_ADDR_START,     0x0280 },
> +       { OS05B10_REG_Y_ADDR_START,     0x01b4 },
> +       { OS05B10_REG_X_ADDR_END,       0x0a0f },
> +       { OS05B10_REG_Y_ADDR_END,       0x05f3 },
> +       { OS05B10_REG_X_OUTPUT_SIZE,    0x0780 },
> +       { OS05B10_REG_Y_OUTPUT_SIZE,    0x0438 },
> +       { OS05B10_REG_HTS,              0x06d0 },
> +       { OS05B10_REG_ISP_X_WIN,        0x0008 },
> +       { OS05B10_REG_ISP_Y_WIN,        0x0008 },
> +       { OS05B10_REG_X_INC_ODD,        0x01 },
> +       { OS05B10_REG_X_INC_EVEN,       0x01 },
> +       { OS05B10_REG_Y_INC_ODD,        0x01 },
> +       { OS05B10_REG_Y_INC_EVEN,       0x01 },

Mode-based sensor drivers are the last option, especially if the sensor has
known registers for configuring the crop size, which seems to me is
possible here?

There's the new common raw sensor model in the works too, which can even
handle separate userspace configuration for analogue and digital crop,
subsampling (INC_ODD/INC_EVEN), binning and output size.

Could you please explore that? Adding hard-coded modes would make this
driver a mess when we move to the new model.

Thanks,
    Jai

Re: [PATCH v3 10/17] media: i2c: os05b10: Add 1080p and 2x2 binning 720p modes

[Tarang Raval]

Hi Jai,

Thanks for the feedback.

> Quoting Tarang Raval (2026-07-19 01:39:01)
> > Add support for 1920x1080 and 1280x720 resolutions.
> > The 1280x720 mode uses 2x2 binning.
> >
> > Both 10-bit and 12-bit pixel formats are supported.
> >
> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > ---
> >  drivers/media/i2c/os05b10.c | 120 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 3fd788bd8b20..c54b466656aa 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -460,6 +460,78 @@ static const struct cci_reg_sequence mode_2592_1944_regs[] = {
> >         { CCI_REG8(0x4837), 0x12 },
> >  };
> >
> > +static const struct cci_reg_sequence mode_1920_1080_regs[] = {
> > +       { OS05B10_REG_X_ADDR_START,     0x0280 },
> > +       { OS05B10_REG_Y_ADDR_START,     0x01b4 },
> > +       { OS05B10_REG_X_ADDR_END,       0x0a0f },
> > +       { OS05B10_REG_Y_ADDR_END,       0x05f3 },
> > +       { OS05B10_REG_X_OUTPUT_SIZE,    0x0780 },
> > +       { OS05B10_REG_Y_OUTPUT_SIZE,    0x0438 },
> > +       { OS05B10_REG_HTS,              0x06d0 },
> > +       { OS05B10_REG_ISP_X_WIN,        0x0008 },
> > +       { OS05B10_REG_ISP_Y_WIN,        0x0008 },
> > +       { OS05B10_REG_X_INC_ODD,        0x01 },
> > +       { OS05B10_REG_X_INC_EVEN,       0x01 },
> > +       { OS05B10_REG_Y_INC_ODD,        0x01 },
> > +       { OS05B10_REG_Y_INC_EVEN,       0x01 },
>
> Mode-based sensor drivers are the last option, especially if the sensor has
> known registers for configuring the crop size, which seems to me is
> possible here?
>
> There's the new common raw sensor model in the works too, which can even
> handle separate userspace configuration for analogue and digital crop,
> subsampling (INC_ODD/INC_EVEN), binning and output size.
>
> Could you please explore that? Adding hard-coded modes would make this
> driver a mess when we move to the new model.

You're right that mode tables aren't a great long term fit here. Looking at
the datasheet again, the analog crop (X/Y_ADDR_START/END), subsampling
(X/Y_INC_ODD/EVEN), digital crop (ISP_X/Y_WIN) and output size
(X/Y_OUTPUT_SIZE) are all independent registers, so this sensor should map
onto the new common raw sensor model reasonably well once it's ready.

I've actually started looking into that approach for os05b10, but it's
going to need its own series to do properly, and I don't want to redesign
the driver against something that's still moving. So my plan is to get this
series in as-is for now, and once the raw sensor model work is finalized,
follow up with a separate series that reworks os05b10 around it and cleans
up the mode handling. Feels like it'll be less churn for everyone that way
than trying to do both at once.

With that said, I'd like to go ahead with the current approach for this
series, and pick this up separately once that lands.

Best Regards,
Tarang

[PATCH v3 11/17] media: i2c: os05b10: keep vblank and exposure range in sync on mode switch

[Tarang Raval]

Update vblank through the control path on mode changes so exposure
limits and default values are recalculated consistently from the
active mode.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index c54b466656aa..d124db74766c 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -745,13 +745,23 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
 						     struct os05b10, handler);
+	const struct os05b10_mode *mode_list;
 	struct v4l2_subdev_state *state;
+	const struct os05b10_mode *mode;
 	struct v4l2_mbus_framefmt *fmt;
+	unsigned int num_modes;
 	int vmax, ret;
 
 	state = v4l2_subdev_get_locked_active_state(&os05b10->sd);
 	fmt = v4l2_subdev_state_get_format(state, 0);
 
+	ret = get_mode_table(os05b10, fmt->code, &mode_list, &num_modes);
+	if (ret)
+		return ret;
+
+	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
+				      fmt->width, fmt->height);
+
 	if (ctrl->id == V4L2_CID_VBLANK) {
 		/* Honour the VBLANK limits when setting exposure. */
 		s64 max = fmt->height + ctrl->val - OS05B10_EXPOSURE_MARGIN;
@@ -759,7 +769,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = __v4l2_ctrl_modify_range(os05b10->exposure,
 					       os05b10->exposure->minimum, max,
 					       os05b10->exposure->step,
-					       os05b10->exposure->default_value);
+					       mode->exp);
 		if (ret)
 			return ret;
 	}
@@ -842,7 +852,7 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
 				      const struct os05b10_mode *mode)
 {
 	u64 pixel_rate = os05b10_pixel_rate(os05b10, mode);
-	u32 hblank, vblank, vblank_max, max_exp;
+	u32 hblank, vblank, vblank_max;
 	int ret;
 
 	ret = __v4l2_ctrl_modify_range(os05b10->pixel_rate, pixel_rate,
@@ -867,10 +877,7 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
 	if (ret)
 		return ret;
 
-	max_exp = mode->vts - OS05B10_EXPOSURE_MARGIN;
-	return __v4l2_ctrl_modify_range(os05b10->exposure,
-					OS05B10_EXPOSURE_MIN, max_exp,
-					OS05B10_EXPOSURE_STEP, mode->exp);
+	return __v4l2_ctrl_s_ctrl(os05b10->vblank, vblank);
 }
 
 static int os05b10_set_pad_format(struct v4l2_subdev *sd,
-- 
2.34.1

Re: [PATCH v3 11/17] media: i2c: os05b10: keep vblank and exposure range in sync on mode switch

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> Update vblank through the control path on mode changes so exposure
> limits and default values are recalculated consistently from the
> active mode.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index c54b466656aa..d124db74766c 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -745,13 +745,23 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct os05b10 *os05b10 = container_of_const(ctrl->handler,
>   						     struct os05b10, handler);
> +	const struct os05b10_mode *mode_list;
>   	struct v4l2_subdev_state *state;
> +	const struct os05b10_mode *mode;
>   	struct v4l2_mbus_framefmt *fmt;
> +	unsigned int num_modes;
>   	int vmax, ret;
>   
>   	state = v4l2_subdev_get_locked_active_state(&os05b10->sd);
>   	fmt = v4l2_subdev_state_get_format(state, 0);
>   
> +	ret = get_mode_table(os05b10, fmt->code, &mode_list, &num_modes);
> +	if (ret)
> +		return ret;
> +
> +	mode = v4l2_find_nearest_size(mode_list, num_modes, width, height,
> +				      fmt->width, fmt->height);
> +
>   	if (ctrl->id == V4L2_CID_VBLANK) {
>   		/* Honour the VBLANK limits when setting exposure. */
>   		s64 max = fmt->height + ctrl->val - OS05B10_EXPOSURE_MARGIN;
> @@ -759,7 +769,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   		ret = __v4l2_ctrl_modify_range(os05b10->exposure,
>   					       os05b10->exposure->minimum, max,
>   					       os05b10->exposure->step,
> -					       os05b10->exposure->default_value);
> +					       mode->exp);
>   		if (ret)
>   			return ret;
>   	}
> @@ -842,7 +852,7 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
>   				      const struct os05b10_mode *mode)
>   {
>   	u64 pixel_rate = os05b10_pixel_rate(os05b10, mode);
> -	u32 hblank, vblank, vblank_max, max_exp;
> +	u32 hblank, vblank, vblank_max;
>   	int ret;
>   
>   	ret = __v4l2_ctrl_modify_range(os05b10->pixel_rate, pixel_rate,
> @@ -867,10 +877,7 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
>   	if (ret)
>   		return ret;
>   
> -	max_exp = mode->vts - OS05B10_EXPOSURE_MARGIN;
> -	return __v4l2_ctrl_modify_range(os05b10->exposure,
> -					OS05B10_EXPOSURE_MIN, max_exp,
> -					OS05B10_EXPOSURE_STEP, mode->exp);
> +	return __v4l2_ctrl_s_ctrl(os05b10->vblank, vblank);
>   }
>   
>   static int os05b10_set_pad_format(struct v4l2_subdev *sd,

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 12/17] media: i2c: os05b10: Update active format before adjusting framing controls

[Tarang Raval]

os05b10_set_pad_format() calls os05b10_set_framing_limits() before updating
the ACTIVE format. As a result, the VBLANK control handler uses the old
height when recalculating exposure limits, causing -ERANGE when switching
to a larger resolution.

Update the ACTIVE format before adjusting framing controls so control
callbacks use the correct dimensions.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index d124db74766c..82a96081fe21 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -907,14 +907,14 @@ static int os05b10_set_pad_format(struct v4l2_subdev *sd,
 
 	format = v4l2_subdev_state_get_format(sd_state, 0);
 
+	*format = fmt->format;
+
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ret = os05b10_set_framing_limits(os05b10, mode);
 		if (ret)
 			return ret;
 	}
 
-	*format = fmt->format;
-
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v3 12/17] media: i2c: os05b10: Update active format before adjusting framing controls

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> os05b10_set_pad_format() calls os05b10_set_framing_limits() before updating
> the ACTIVE format. As a result, the VBLANK control handler uses the old
> height when recalculating exposure limits, causing -ERANGE when switching
> to a larger resolution.
> 
> Update the ACTIVE format before adjusting framing controls so control
> callbacks use the correct dimensions.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index d124db74766c..82a96081fe21 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -907,14 +907,14 @@ static int os05b10_set_pad_format(struct v4l2_subdev *sd,
>   
>   	format = v4l2_subdev_state_get_format(sd_state, 0);
>   
> +	*format = fmt->format;
> +
>   	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>   		ret = os05b10_set_framing_limits(os05b10, mode);
>   		if (ret)
>   			return ret;
>   	}
>   
> -	*format = fmt->format;
> -
>   	return 0;
>   }
>   

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 13/17] media: i2c: os05b10: Rename vmax variable in VBLANK control

[Tarang Raval]

The VBLANK control programs the sensor VTS register. Rename the local
variable from vmax to vts to match the register meaning and improve
readability.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 82a96081fe21..876186b22f5f 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -750,7 +750,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 	const struct os05b10_mode *mode;
 	struct v4l2_mbus_framefmt *fmt;
 	unsigned int num_modes;
-	int vmax, ret;
+	int vts, ret;
 
 	state = v4l2_subdev_get_locked_active_state(&os05b10->sd);
 	fmt = v4l2_subdev_state_get_format(state, 0);
@@ -779,8 +779,8 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_VBLANK:
-		vmax = fmt->height + ctrl->val;
-		ret = cci_write(os05b10->cci, OS05B10_REG_VTS, vmax, NULL);
+		vts = fmt->height + ctrl->val;
+		ret = cci_write(os05b10->cci, OS05B10_REG_VTS, vts, NULL);
 		break;
 	case V4L2_CID_ANALOGUE_GAIN:
 		ret = cci_write(os05b10->cci, OS05B10_REG_ANALOG_GAIN,
-- 
2.34.1

Re: [PATCH v3 13/17] media: i2c: os05b10: Rename vmax variable in VBLANK control

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> The VBLANK control programs the sensor VTS register. Rename the local
> variable from vmax to vts to match the register meaning and improve
> readability.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 82a96081fe21..876186b22f5f 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -750,7 +750,7 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   	const struct os05b10_mode *mode;
>   	struct v4l2_mbus_framefmt *fmt;
>   	unsigned int num_modes;
> -	int vmax, ret;
> +	int vts, ret;
>   
>   	state = v4l2_subdev_get_locked_active_state(&os05b10->sd);
>   	fmt = v4l2_subdev_state_get_format(state, 0);
> @@ -779,8 +779,8 @@ static int os05b10_set_ctrl(struct v4l2_ctrl *ctrl)
>   
>   	switch (ctrl->id) {
>   	case V4L2_CID_VBLANK:
> -		vmax = fmt->height + ctrl->val;
> -		ret = cci_write(os05b10->cci, OS05B10_REG_VTS, vmax, NULL);
> +		vts = fmt->height + ctrl->val;
> +		ret = cci_write(os05b10->cci, OS05B10_REG_VTS, vts, NULL);
>   		break;
>   	case V4L2_CID_ANALOGUE_GAIN:
>   		ret = cci_write(os05b10->cci, OS05B10_REG_ANALOG_GAIN,

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

-- 
Best wishes,
Vladimir

[PATCH v3 14/17] media: i2c: os05b10: add 2-lane support

[Tarang Raval]

Add support for 2-lane.

Update link-frequency handling to select 750 MHz for 2-lane and 600 MHz for
4-lane, and adjust pixel rate computation accordingly. Extend endpoint
parsing to accept 2 or 4 data lanes.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 94 ++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 876186b22f5f..6d91a9660660 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -113,7 +113,8 @@
 #define OS05B10_TRANSPARENT_EFFECT     0xa0
 #define OS05B10_ROLLING_BAR_EFFECT     0xc0
 
-#define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
+#define OS05B10_LINK_FREQ_4LANE                (600 * HZ_PER_MHZ)
+#define OS05B10_LINK_FREQ_2LANE                (750 * HZ_PER_MHZ)
 
 static const struct v4l2_rect os05b10_native_area = {
 	.top = 0,
@@ -136,13 +137,7 @@ static const char * const os05b10_supply_name[] = {
 };
 
 static const struct cci_reg_sequence os05b10_common_regs[] = {
-	{ OS05B10_REG_PLL_CTRL_01, 0x44 },
-	{ OS05B10_REG_PLL_CTRL_03, 0x02 },
-	{ OS05B10_REG_PLL_CTRL_05, 0x32 },
-	{ OS05B10_REG_PLL_CTRL_06, 0x00 },
-	{ OS05B10_REG_PLL_CTRL_25, 0x3b },
 	{ CCI_REG8(0x3002), 0x21 },
-	{ OS05B10_REG_MIPI_SC_CTRL, 0x72 },
 	{ CCI_REG8(0x301e), 0xb4 },
 	{ CCI_REG8(0x301f), 0xd0 },
 	{ CCI_REG8(0x3021), 0x03 },
@@ -532,6 +527,24 @@ static const struct cci_reg_sequence mode_1280_720_regs[] = {
 	{ CCI_REG8(0x4837), 0x0d },
 };
 
+static const struct cci_reg_sequence os05b10_2lane_regs[] = {
+	{ OS05B10_REG_PLL_CTRL_01,	0x44 },
+	{ OS05B10_REG_PLL_CTRL_03,	0x02 },
+	{ OS05B10_REG_PLL_CTRL_05,	0x64 },
+	{ OS05B10_REG_PLL_CTRL_06,	0x00 },
+	{ OS05B10_REG_PLL_CTRL_25,	0x3b },
+	{ OS05B10_REG_MIPI_SC_CTRL,	OS05B10_2_LANE_MODE },
+};
+
+static const struct cci_reg_sequence os05b10_4lane_regs[] = {
+	{ OS05B10_REG_PLL_CTRL_01,	0x44 },
+	{ OS05B10_REG_PLL_CTRL_03,	0x02 },
+	{ OS05B10_REG_PLL_CTRL_05,	0x32 },
+	{ OS05B10_REG_PLL_CTRL_06,	0x00 },
+	{ OS05B10_REG_PLL_CTRL_25,	0x3b },
+	{ OS05B10_REG_MIPI_SC_CTRL,	OS05B10_4_LANE_MODE },
+};
+
 struct os05b10 {
 	struct device *dev;
 	struct regmap *cci;
@@ -650,8 +663,12 @@ static const struct os05b10_mode supported_modes_10bit[] = {
 	},
 };
 
-static const s64 link_frequencies[] = {
-	OS05B10_LINK_FREQ_600MHZ,
+static const s64 link_frequencies_4lane[] = {
+	OS05B10_LINK_FREQ_4LANE,
+};
+
+static const s64 link_frequencies_2lane[] = {
+	OS05B10_LINK_FREQ_2LANE,
 };
 
 static const u32 os05b10_mbus_codes[] = {
@@ -838,7 +855,9 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
 static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
 			      const struct os05b10_mode *mode)
 {
-	u64 link_freq = link_frequencies[os05b10->link_freq_index];
+	u64 link_freq = (os05b10->data_lanes == 2) ?
+			link_frequencies_2lane[os05b10->link_freq_index] :
+			link_frequencies_4lane[os05b10->link_freq_index];
 	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
 
 	dev_dbg(os05b10->dev,
@@ -983,6 +1002,17 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 	ret = pm_runtime_resume_and_get(os05b10->dev);
 	if (ret < 0)
 		return ret;
+	/* Set pll & mipi lane configuration */
+	if (os05b10->data_lanes == 2)
+		cci_multi_reg_write(os05b10->cci, os05b10_2lane_regs,
+				    ARRAY_SIZE(os05b10_2lane_regs), &ret);
+	else
+		cci_multi_reg_write(os05b10->cci, os05b10_4lane_regs,
+				    ARRAY_SIZE(os05b10_4lane_regs), &ret);
+	if (ret) {
+		dev_err(os05b10->dev, "failed to write pll & mipi lane registers\n");
+		goto err_rpm_put;
+	}
 
 	/* Write common registers */
 	ret = cci_multi_reg_write(os05b10->cci, os05b10_common_regs,
@@ -1195,22 +1225,39 @@ static int os05b10_parse_endpoint(struct os05b10 *os05b10)
 	if (ret)
 		return ret;
 
-	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4 &&
+	    bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
 		ret = dev_err_probe(os05b10->dev, -EINVAL,
-				    "only 4 data lanes are supported\n");
+				    "4 and 2 data lanes are supported\n");
 		goto error_out;
 	}
 
 	os05b10->data_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
 
-	ret = v4l2_link_freq_to_bitmap(os05b10->dev, bus_cfg.link_frequencies,
-				       bus_cfg.nr_of_link_frequencies,
-				       link_frequencies,
-				       ARRAY_SIZE(link_frequencies),
-				       &link_freq_bitmap);
-	if (ret) {
-		dev_err(os05b10->dev, "only 600MHz frequency is available\n");
-		goto error_out;
+	if (os05b10->data_lanes == 2) {
+		ret = v4l2_link_freq_to_bitmap(os05b10->dev,
+					       bus_cfg.link_frequencies,
+					       bus_cfg.nr_of_link_frequencies,
+					       link_frequencies_2lane,
+					       ARRAY_SIZE(link_frequencies_2lane),
+					       &link_freq_bitmap);
+		if (ret) {
+			dev_err(os05b10->dev,
+				"For 2 lane 750MHz frequency is available\n");
+			goto error_out;
+		}
+	} else {
+		ret = v4l2_link_freq_to_bitmap(os05b10->dev,
+					       bus_cfg.link_frequencies,
+					       bus_cfg.nr_of_link_frequencies,
+					       link_frequencies_4lane,
+					       ARRAY_SIZE(link_frequencies_4lane),
+					       &link_freq_bitmap);
+		if (ret) {
+			dev_err(os05b10->dev,
+				"For 4 lane 600MHz frequency is available\n");
+			goto error_out;
+		}
 	}
 
 	os05b10->link_freq_index = __ffs(link_freq_bitmap);
@@ -1240,10 +1287,11 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 
 	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
 						    V4L2_CID_LINK_FREQ,
-						    ARRAY_SIZE(link_frequencies) - 1,
+						    ARRAY_SIZE(link_frequencies_4lane) - 1,
 						    os05b10->link_freq_index,
-						    link_frequencies);
-
+						    (os05b10->data_lanes == 2) ?
+						    link_frequencies_2lane :
+						    link_frequencies_4lane);
 	if (os05b10->link_freq)
 		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-- 
2.34.1

Re: [PATCH v3 14/17] media: i2c: os05b10: add 2-lane support

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> Add support for 2-lane.
> 
> Update link-frequency handling to select 750 MHz for 2-lane and 600 MHz for
> 4-lane, and adjust pixel rate computation accordingly. Extend endpoint
> parsing to accept 2 or 4 data lanes.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 94 ++++++++++++++++++++++++++++---------
>   1 file changed, 71 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 876186b22f5f..6d91a9660660 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -113,7 +113,8 @@
>   #define OS05B10_TRANSPARENT_EFFECT     0xa0
>   #define OS05B10_ROLLING_BAR_EFFECT     0xc0
>   
> -#define OS05B10_LINK_FREQ_600MHZ	(600 * HZ_PER_MHZ)
> +#define OS05B10_LINK_FREQ_4LANE                (600 * HZ_PER_MHZ)
> +#define OS05B10_LINK_FREQ_2LANE                (750 * HZ_PER_MHZ)

I believe the macro name shall be kept as is, frequency is not specific
to the number of lanes, it's driver implementation/limitation specifics.

>   
>   static const struct v4l2_rect os05b10_native_area = {
>   	.top = 0,
> @@ -136,13 +137,7 @@ static const char * const os05b10_supply_name[] = {
>   };
>   
>   static const struct cci_reg_sequence os05b10_common_regs[] = {
> -	{ OS05B10_REG_PLL_CTRL_01, 0x44 },
> -	{ OS05B10_REG_PLL_CTRL_03, 0x02 },
> -	{ OS05B10_REG_PLL_CTRL_05, 0x32 },
> -	{ OS05B10_REG_PLL_CTRL_06, 0x00 },
> -	{ OS05B10_REG_PLL_CTRL_25, 0x3b },
>   	{ CCI_REG8(0x3002), 0x21 },
> -	{ OS05B10_REG_MIPI_SC_CTRL, 0x72 },
>   	{ CCI_REG8(0x301e), 0xb4 },
>   	{ CCI_REG8(0x301f), 0xd0 },
>   	{ CCI_REG8(0x3021), 0x03 },
> @@ -532,6 +527,24 @@ static const struct cci_reg_sequence mode_1280_720_regs[] = {
>   	{ CCI_REG8(0x4837), 0x0d },
>   };
>   
> +static const struct cci_reg_sequence os05b10_2lane_regs[] = {
> +	{ OS05B10_REG_PLL_CTRL_01,	0x44 },
> +	{ OS05B10_REG_PLL_CTRL_03,	0x02 },
> +	{ OS05B10_REG_PLL_CTRL_05,	0x64 },
> +	{ OS05B10_REG_PLL_CTRL_06,	0x00 },
> +	{ OS05B10_REG_PLL_CTRL_25,	0x3b },
> +	{ OS05B10_REG_MIPI_SC_CTRL,	OS05B10_2_LANE_MODE },
> +};
> +
> +static const struct cci_reg_sequence os05b10_4lane_regs[] = {
> +	{ OS05B10_REG_PLL_CTRL_01,	0x44 },
> +	{ OS05B10_REG_PLL_CTRL_03,	0x02 },
> +	{ OS05B10_REG_PLL_CTRL_05,	0x32 },
> +	{ OS05B10_REG_PLL_CTRL_06,	0x00 },
> +	{ OS05B10_REG_PLL_CTRL_25,	0x3b },
> +	{ OS05B10_REG_MIPI_SC_CTRL,	OS05B10_4_LANE_MODE },
> +};
> +
>   struct os05b10 {
>   	struct device *dev;
>   	struct regmap *cci;
> @@ -650,8 +663,12 @@ static const struct os05b10_mode supported_modes_10bit[] = {
>   	},
>   };
>   
> -static const s64 link_frequencies[] = {
> -	OS05B10_LINK_FREQ_600MHZ,
> +static const s64 link_frequencies_4lane[] = {
> +	OS05B10_LINK_FREQ_4LANE,
> +};
> +
> +static const s64 link_frequencies_2lane[] = {
> +	OS05B10_LINK_FREQ_2LANE,
>   };

Please just use macro with frequency in its name, or plainly use the intended
frequencies here.

>   
>   static const u32 os05b10_mbus_codes[] = {
> @@ -838,7 +855,9 @@ static int os05b10_enum_mbus_code(struct v4l2_subdev *sd,
>   static u64 os05b10_pixel_rate(struct os05b10 *os05b10,
>   			      const struct os05b10_mode *mode)
>   {
> -	u64 link_freq = link_frequencies[os05b10->link_freq_index];
> +	u64 link_freq = (os05b10->data_lanes == 2) ?
> +			link_frequencies_2lane[os05b10->link_freq_index] :
> +			link_frequencies_4lane[os05b10->link_freq_index];
>   	u64 pixel_rate = div_u64(link_freq * 2 * os05b10->data_lanes, mode->bpp);
>   
>   	dev_dbg(os05b10->dev,
> @@ -983,6 +1002,17 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>   	ret = pm_runtime_resume_and_get(os05b10->dev);
>   	if (ret < 0)
>   		return ret;
> +	/* Set pll & mipi lane configuration */
> +	if (os05b10->data_lanes == 2)
> +		cci_multi_reg_write(os05b10->cci, os05b10_2lane_regs,
> +				    ARRAY_SIZE(os05b10_2lane_regs), &ret);
> +	else
> +		cci_multi_reg_write(os05b10->cci, os05b10_4lane_regs,
> +				    ARRAY_SIZE(os05b10_4lane_regs), &ret);
> +	if (ret) {
> +		dev_err(os05b10->dev, "failed to write pll & mipi lane registers\n");
> +		goto err_rpm_put;
> +	}
>   
>   	/* Write common registers */
>   	ret = cci_multi_reg_write(os05b10->cci, os05b10_common_regs,
> @@ -1195,22 +1225,39 @@ static int os05b10_parse_endpoint(struct os05b10 *os05b10)
>   	if (ret)
>   		return ret;
>   
> -	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4 &&
> +	    bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
>   		ret = dev_err_probe(os05b10->dev, -EINVAL,
> -				    "only 4 data lanes are supported\n");
> +				    "4 and 2 data lanes are supported\n");
>   		goto error_out;
>   	}
>   
>   	os05b10->data_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
>   
> -	ret = v4l2_link_freq_to_bitmap(os05b10->dev, bus_cfg.link_frequencies,
> -				       bus_cfg.nr_of_link_frequencies,
> -				       link_frequencies,
> -				       ARRAY_SIZE(link_frequencies),
> -				       &link_freq_bitmap);
> -	if (ret) {
> -		dev_err(os05b10->dev, "only 600MHz frequency is available\n");
> -		goto error_out;
> +	if (os05b10->data_lanes == 2) {
> +		ret = v4l2_link_freq_to_bitmap(os05b10->dev,
> +					       bus_cfg.link_frequencies,
> +					       bus_cfg.nr_of_link_frequencies,
> +					       link_frequencies_2lane,
> +					       ARRAY_SIZE(link_frequencies_2lane),
> +					       &link_freq_bitmap);
> +		if (ret) {
> +			dev_err(os05b10->dev,
> +				"For 2 lane 750MHz frequency is available\n");
> +			goto error_out;
> +		}
> +	} else {
> +		ret = v4l2_link_freq_to_bitmap(os05b10->dev,
> +					       bus_cfg.link_frequencies,
> +					       bus_cfg.nr_of_link_frequencies,
> +					       link_frequencies_4lane,
> +					       ARRAY_SIZE(link_frequencies_4lane),
> +					       &link_freq_bitmap);
> +		if (ret) {
> +			dev_err(os05b10->dev,
> +				"For 4 lane 600MHz frequency is available\n");
> +			goto error_out;
> +		}
>   	}
>   
>   	os05b10->link_freq_index = __ffs(link_freq_bitmap);
> @@ -1240,10 +1287,11 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   
>   	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
>   						    V4L2_CID_LINK_FREQ,
> -						    ARRAY_SIZE(link_frequencies) - 1,
> +						    ARRAY_SIZE(link_frequencies_4lane) - 1,
>   						    os05b10->link_freq_index,
> -						    link_frequencies);
> -
> +						    (os05b10->data_lanes == 2) ?
> +						    link_frequencies_2lane :
> +						    link_frequencies_4lane);
>   	if (os05b10->link_freq)
>   		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>   

-- 
Best wishes,
Vladimir

[PATCH v3 15/17] media: i2c: os05b10: fix negative hblank calculation

[Tarang Raval]

hblank is computed as "mode->hts - mode->width", but both are u32.
For every mode hts < width (e.g. 1744 vs 2592 for 2592x1944), so the
subtraction wraps in unsigned 32-bit arithmetic instead of going
negative. That wrapped value ends up in the control's s64 min/max,
which v4l2-compliance flags as out of range:

    fail: v4l2-test-controls.cpp(413): returned control value out of range
    fail: v4l2-test-controls.cpp(476): invalid control 009e0902

and v4l2-ctl shows:

    horizontal_blanking 0x009e0902 (int): min=4294966448 max=4294966448
    step=1 default=4294966448 value=-848 flags=read-only, has-min-max

The same bug exists in os05b10_init_controls(), which sets the
control's default before os05b10_set_framing_limits() runs.

Fix both sites by casting to a signed type before subtracting, so the
real (possibly negative) value is computed directly.

Fixes: 3aa9296a23ec4 ("media: i2c: add os05b10 image sensor driver")
Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 6d91a9660660..59097eb96561 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -871,7 +871,8 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
 				      const struct os05b10_mode *mode)
 {
 	u64 pixel_rate = os05b10_pixel_rate(os05b10, mode);
-	u32 hblank, vblank, vblank_max;
+	u32 vblank, vblank_max;
+	s32 hblank;
 	int ret;
 
 	ret = __v4l2_ctrl_modify_range(os05b10->pixel_rate, pixel_rate,
@@ -883,7 +884,7 @@ static int os05b10_set_framing_limits(struct os05b10 *os05b10,
 	if (ret)
 		return ret;
 
-	hblank = mode->hts - mode->width;
+	hblank = (s32)mode->hts - (s32)mode->width;
 	ret = __v4l2_ctrl_modify_range(os05b10->hblank, hblank, hblank, 1,
 				       hblank);
 	if (ret)
@@ -1272,9 +1273,10 @@ static int os05b10_parse_endpoint(struct os05b10 *os05b10)
 static int os05b10_init_controls(struct os05b10 *os05b10)
 {
 	const struct os05b10_mode *mode = &supported_modes_10bit[0];
-	u64 hblank_def, vblank_def, exp_max, pixel_rate;
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
+	u64 vblank_def, exp_max, pixel_rate;
+	s64 hblank_def;
 	int ret;
 
 	ctrl_hdlr = &os05b10->handler;
@@ -1295,7 +1297,7 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	if (os05b10->link_freq)
 		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
-	hblank_def = mode->hts - mode->width;
+	hblank_def = (s64)mode->hts -(s64)mode->width;
 	os05b10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
 					    hblank_def, hblank_def,
 					    1, hblank_def);
-- 
2.34.1

Re: [PATCH v3 15/17] media: i2c: os05b10: fix negative hblank calculation

[Jai Luthra]

Quoting Tarang Raval (2026-07-19 01:39:06)
> hblank is computed as "mode->hts - mode->width", but both are u32.
> For every mode hts < width (e.g. 1744 vs 2592 for 2592x1944), so the
> subtraction wraps in unsigned 32-bit arithmetic instead of going
> negative. That wrapped value ends up in the control's s64 min/max,
> which v4l2-compliance flags as out of range:
> 
>     fail: v4l2-test-controls.cpp(413): returned control value out of range
>     fail: v4l2-test-controls.cpp(476): invalid control 009e0902
> 
> and v4l2-ctl shows:
> 
>     horizontal_blanking 0x009e0902 (int): min=4294966448 max=4294966448
>     step=1 default=4294966448 value=-848 flags=read-only, has-min-max
> 
> The same bug exists in os05b10_init_controls(), which sets the
> control's default before os05b10_set_framing_limits() runs.
> 
> Fix both sites by casting to a signed type before subtracting, so the
> real (possibly negative) value is computed directly.
> 

While the framework accepts negative blankings, IMHO they physically don't
make much sense.

Many sensors scale their HTS register by some constant factor, so increment
of 1 in HTS does not mean 1 pixel on a line, but 4 or 8 or n pixels.
This also affects the internal "pixel rate" for framerate calculation.

Please look at imx283, imx678 etc.

Thanks,
    Jai

> Fixes: 3aa9296a23ec4 ("media: i2c: add os05b10 image sensor driver")
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>

Re: [PATCH v3 15/17] media: i2c: os05b10: fix negative hblank calculation

[Tarang Raval]

Hi Jai,

Thanks for the review.

> Quoting Tarang Raval (2026-07-19 01:39:06)
> > hblank is computed as "mode->hts - mode->width", but both are u32.
> > For every mode hts < width (e.g. 1744 vs 2592 for 2592x1944), so the
> > subtraction wraps in unsigned 32-bit arithmetic instead of going
> > negative. That wrapped value ends up in the control's s64 min/max,
> > which v4l2-compliance flags as out of range:
> >
> >     fail: v4l2-test-controls.cpp(413): returned control value out of range
> >     fail: v4l2-test-controls.cpp(476): invalid control 009e0902
> >
> > and v4l2-ctl shows:
> >
> >     horizontal_blanking 0x009e0902 (int): min=4294966448 max=4294966448
> >     step=1 default=4294966448 value=-848 flags=read-only, has-min-max
> >
> > The same bug exists in os05b10_init_controls(), which sets the
> > control's default before os05b10_set_framing_limits() runs.
> >
> > Fix both sites by casting to a signed type before subtracting, so the
> > real (possibly negative) value is computed directly.
> >
>
> While the framework accepts negative blankings, IMHO they physically don't
> make much sense.
>
> Many sensors scale their HTS register by some constant factor, so increment
> of 1 in HTS does not mean 1 pixel on a line, but 4 or 8 or n pixels.
> This also affects the internal "pixel rate" for framerate calculation.
>
> Please look at imx283, imx678 etc.

Sure, I'll look into this properly.

Before that, I want to flag something, I went through the OS05B10 datasheet
carefully, and it doesn't explicitly state what clock domain HTS is
counted in, or what its actual unit is relative to the output pixel clock.
The register description just says "Total horizontal timing size" with no
further detail.

That said, looking at the clock scheme section (PLL1 driving separate SCLK
and PCLK dividers) and the register defaults themselves (default output
width is larger than the default HTS value), it does look like HTS isn't
counted in output-pixel units directly, so there's likely a scale factor
between the two, similar to imx283 or imx678's HMAX.

I don't have vendor support to get the exact PLL divider table to confirm
this precisely, so I'll try to derive the real value empirically on
hardware and cross-check it across the supported resolutions before
sending a proper fix.

Best Regards,
Tarang

[PATCH v3 16/17] media: i2c: os05b10: Enable runtime PM autosuspend

[Tarang Raval]

Use pm_runtime_put_autosuspend() instead of pm_runtime_put()
to allow autosuspend. Set a 1000ms autosuspend delay.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index 59097eb96561..b4dd3137ad5c 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -1057,7 +1057,7 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
 	return 0;
 
 err_rpm_put:
-	pm_runtime_put(os05b10->dev);
+	pm_runtime_put_autosuspend(os05b10->dev);
 
 	return ret;
 }
@@ -1077,7 +1077,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
 	__v4l2_ctrl_grab(os05b10->vflip, false);
 	__v4l2_ctrl_grab(os05b10->hflip, false);
 
-	pm_runtime_put(os05b10->dev);
+	pm_runtime_put_autosuspend(os05b10->dev);
 
 	return 0;
 }
@@ -1455,6 +1455,8 @@ static int os05b10_probe(struct i2c_client *client)
 
 	pm_runtime_set_active(os05b10->dev);
 	pm_runtime_enable(os05b10->dev);
+	pm_runtime_set_autosuspend_delay(os05b10->dev, 1000);
+	pm_runtime_use_autosuspend(os05b10->dev);
 
 	ret = v4l2_async_register_subdev_sensor(&os05b10->sd);
 	if (ret < 0) {
-- 
2.34.1

Re: [PATCH v3 16/17] media: i2c: os05b10: Enable runtime PM autosuspend

[Jai Luthra]

Hi Tarang,

Quoting Tarang Raval (2026-07-19 01:39:07)
> Use pm_runtime_put_autosuspend() instead of pm_runtime_put()
> to allow autosuspend. Set a 1000ms autosuspend delay.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>  drivers/media/i2c/os05b10.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index 59097eb96561..b4dd3137ad5c 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -1057,7 +1057,7 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
>         return 0;
>  
>  err_rpm_put:
> -       pm_runtime_put(os05b10->dev);
> +       pm_runtime_put_autosuspend(os05b10->dev);
>  
>         return ret;
>  }
> @@ -1077,7 +1077,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
>         __v4l2_ctrl_grab(os05b10->vflip, false);
>         __v4l2_ctrl_grab(os05b10->hflip, false);
>  
> -       pm_runtime_put(os05b10->dev);
> +       pm_runtime_put_autosuspend(os05b10->dev);
>  
>         return 0;
>  }
> @@ -1455,6 +1455,8 @@ static int os05b10_probe(struct i2c_client *client)
>  
>         pm_runtime_set_active(os05b10->dev);
>         pm_runtime_enable(os05b10->dev);
> +       pm_runtime_set_autosuspend_delay(os05b10->dev, 1000);
> +       pm_runtime_use_autosuspend(os05b10->dev);

These two calls are easier to understand (for both humans and sashiko)
moved to the end of the probe, when you're done configuring the sensor and
call pm_runtime_idle().

Thanks,
    Jai

>  
>         ret = v4l2_async_register_subdev_sensor(&os05b10->sd);
>         if (ret < 0) {
> -- 
> 2.34.1
> 
>

Re: [PATCH v3 16/17] media: i2c: os05b10: Enable runtime PM autosuspend

[Tarang Raval]

Hi Jai,

Thanks for the review.

> Quoting Tarang Raval (2026-07-19 01:39:07)
> > Use pm_runtime_put_autosuspend() instead of pm_runtime_put()
> > to allow autosuspend. Set a 1000ms autosuspend delay.
> >
> > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> > ---
> >  drivers/media/i2c/os05b10.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 59097eb96561..b4dd3137ad5c 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -1057,7 +1057,7 @@ static int os05b10_enable_streams(struct v4l2_subdev *sd,
> >         return 0;
> >
> >  err_rpm_put:
> > -       pm_runtime_put(os05b10->dev);
> > +       pm_runtime_put_autosuspend(os05b10->dev);
> >
> >         return ret;
> >  }
> > @@ -1077,7 +1077,7 @@ static int os05b10_disable_streams(struct v4l2_subdev *sd,
> >         __v4l2_ctrl_grab(os05b10->vflip, false);
> >         __v4l2_ctrl_grab(os05b10->hflip, false);
> >
> > -       pm_runtime_put(os05b10->dev);
> > +       pm_runtime_put_autosuspend(os05b10->dev);
> >
> >         return 0;
> >  }
> > @@ -1455,6 +1455,8 @@ static int os05b10_probe(struct i2c_client *client)
> >
> >         pm_runtime_set_active(os05b10->dev);
> >         pm_runtime_enable(os05b10->dev);
> > +       pm_runtime_set_autosuspend_delay(os05b10->dev, 1000);
> > +       pm_runtime_use_autosuspend(os05b10->dev);
>
> These two calls are easier to understand (for both humans and sashiko)
> moved to the end of the probe, when you're done configuring the sensor and
> call pm_runtime_idle().

Sure, I will move this just before pm_runtime_idle.

By the way, I'm not sure why, but sashiko isn't reviewing my patches.
I'm not getting any mail from sashiko on any of my patches from this
series. Even though I recently sent one patch on int3472, on that
patch also sashiko had no comments. That's strange :)

Best Regards,
Tarang

Re: [PATCH v3 16/17] media: i2c: os05b10: Enable runtime PM autosuspend

[Jai Luthra]

Hi Tarang,

Quoting Tarang Raval (2026-07-24 12:32:15)
> Hi Jai,
> 
> Thanks for the review.
> 
[...]
> By the way, I'm not sure why, but sashiko isn't reviewing my patches.
> I'm not getting any mail from sashiko on any of my patches from this
> series. Even though I recently sent one patch on int3472, on that
> patch also sashiko had no comments. That's strange :)

Sashiko was enabled on the main list only for a short period of time, now
the reviews are opt-in by joining the media-ci@ list (see Mauro's latest
                                                      announcement [1])

Or you can access it on sashiko.dev:
https://sashiko.dev/#/patchset/20260718200912.16001-1-tarang.raval%40siliconsignals.io

[1]: https://lore.kernel.org/linux-media/20260722142757.020b70e1@foz.lan/

> 
> Best Regards,
> Tarang

Thanks,
    Jai

[PATCH v3 17/17] media: i2c: os05b10: remove unused control fields, simplify error handling

[Tarang Raval]

link_freq and gain don't need to be stored in struct os05b10; make
them local.

Parse the fwnode properties up front and register them before the
single ctrl_hdlr->error check, so all controls are covered by one
check and the per-control NULL guards before setting flags can be
dropped.

Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
---
 drivers/media/i2c/os05b10.c | 55 +++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
index b4dd3137ad5c..c355ac197eb2 100644
--- a/drivers/media/i2c/os05b10.c
+++ b/drivers/media/i2c/os05b10.c
@@ -557,11 +557,9 @@ struct os05b10 {
 
 	/* V4L2 Controls */
 	struct v4l2_ctrl_handler handler;
-	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *vblank;
-	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *exposure;
 	struct v4l2_ctrl *vflip;
 	struct v4l2_ctrl *hflip;
@@ -1276,9 +1274,14 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	u64 vblank_def, exp_max, pixel_rate;
+	struct v4l2_ctrl *link_freq;
 	s64 hblank_def;
 	int ret;
 
+	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
+	if (ret)
+		return ret;
+
 	ctrl_hdlr = &os05b10->handler;
 	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
 
@@ -1287,22 +1290,18 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 						V4L2_CID_PIXEL_RATE, pixel_rate,
 						pixel_rate, 1, pixel_rate);
 
-	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
-						    V4L2_CID_LINK_FREQ,
-						    ARRAY_SIZE(link_frequencies_4lane) - 1,
-						    os05b10->link_freq_index,
-						    (os05b10->data_lanes == 2) ?
-						    link_frequencies_2lane :
-						    link_frequencies_4lane);
-	if (os05b10->link_freq)
-		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
+					   V4L2_CID_LINK_FREQ,
+					   ARRAY_SIZE(link_frequencies_4lane) - 1,
+					   os05b10->link_freq_index,
+					   (os05b10->data_lanes == 2) ?
+					   link_frequencies_2lane :
+					   link_frequencies_4lane);
 
 	hblank_def = (s64)mode->hts -(s64)mode->width;
 	os05b10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
 					    hblank_def, hblank_def,
 					    1, hblank_def);
-	if (os05b10->hblank)
-		os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	vblank_def = mode->vts - mode->height;
 	os05b10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
@@ -1317,12 +1316,10 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 					      exp_max, OS05B10_EXPOSURE_STEP,
 					      mode->exp);
 
-	os05b10->gain = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
-					  V4L2_CID_ANALOGUE_GAIN,
-					  OS05B10_ANALOG_GAIN_MIN,
-					  OS05B10_ANALOG_GAIN_MAX,
-					  OS05B10_ANALOG_GAIN_STEP,
-					  OS05B10_ANALOG_GAIN_DEFAULT);
+	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
+			  V4L2_CID_ANALOGUE_GAIN, OS05B10_ANALOG_GAIN_MIN,
+			  OS05B10_ANALOG_GAIN_MAX, OS05B10_ANALOG_GAIN_STEP,
+			  OS05B10_ANALOG_GAIN_DEFAULT);
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
 			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
@@ -1330,33 +1327,31 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
 
 	os05b10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
 					   V4L2_CID_HFLIP, 0, 1, 1, 0);
-	if (os05b10->hflip)
-		os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
 					   V4L2_CID_VFLIP, 0, 1, 1, 0);
-	if (os05b10->vflip)
-		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &os05b10_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
 				     ARRAY_SIZE(os05b10_test_pattern_menu) - 1,
 				     0, 0, os05b10_test_pattern_menu);
 
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
 	if (ctrl_hdlr->error) {
 		ret = ctrl_hdlr->error;
 		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
 		goto error;
 	}
 
-	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
-	if (ret)
-		goto error;
 
-	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
-					      &props);
-	if (ret)
-		goto error;
+	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+	os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	os05b10->sd.ctrl_handler = ctrl_hdlr;
 
-- 
2.34.1

Re: [PATCH v3 17/17] media: i2c: os05b10: remove unused control fields, simplify error handling

[Vladimir Zapolskiy]

On 7/18/26 23:09, Tarang Raval wrote:
> link_freq and gain don't need to be stored in struct os05b10; make
> them local.
> 
> Parse the fwnode properties up front and register them before the
> single ctrl_hdlr->error check, so all controls are covered by one
> check and the per-control NULL guards before setting flags can be
> dropped.
> 
> Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
>   drivers/media/i2c/os05b10.c | 55 +++++++++++++++++--------------------
>   1 file changed, 25 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> index b4dd3137ad5c..c355ac197eb2 100644
> --- a/drivers/media/i2c/os05b10.c
> +++ b/drivers/media/i2c/os05b10.c
> @@ -557,11 +557,9 @@ struct os05b10 {
>   
>   	/* V4L2 Controls */
>   	struct v4l2_ctrl_handler handler;
> -	struct v4l2_ctrl *link_freq;
>   	struct v4l2_ctrl *pixel_rate;
>   	struct v4l2_ctrl *hblank;
>   	struct v4l2_ctrl *vblank;
> -	struct v4l2_ctrl *gain;
>   	struct v4l2_ctrl *exposure;
>   	struct v4l2_ctrl *vflip;
>   	struct v4l2_ctrl *hflip;
> @@ -1276,9 +1274,14 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   	struct v4l2_fwnode_device_properties props;
>   	struct v4l2_ctrl_handler *ctrl_hdlr;
>   	u64 vblank_def, exp_max, pixel_rate;
> +	struct v4l2_ctrl *link_freq;
>   	s64 hblank_def;
>   	int ret;
>   
> +	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
> +	if (ret)
> +		return ret;
> +
>   	ctrl_hdlr = &os05b10->handler;
>   	v4l2_ctrl_handler_init(ctrl_hdlr, 12);
>   
> @@ -1287,22 +1290,18 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   						V4L2_CID_PIXEL_RATE, pixel_rate,
>   						pixel_rate, 1, pixel_rate);
>   
> -	os05b10->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
> -						    V4L2_CID_LINK_FREQ,
> -						    ARRAY_SIZE(link_frequencies_4lane) - 1,
> -						    os05b10->link_freq_index,
> -						    (os05b10->data_lanes == 2) ?
> -						    link_frequencies_2lane :
> -						    link_frequencies_4lane);
> -	if (os05b10->link_freq)
> -		os05b10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &os05b10_ctrl_ops,
> +					   V4L2_CID_LINK_FREQ,
> +					   ARRAY_SIZE(link_frequencies_4lane) - 1,
> +					   os05b10->link_freq_index,
> +					   (os05b10->data_lanes == 2) ?
> +					   link_frequencies_2lane :
> +					   link_frequencies_4lane);
>   
>   	hblank_def = (s64)mode->hts -(s64)mode->width;
>   	os05b10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
>   					    hblank_def, hblank_def,
>   					    1, hblank_def);
> -	if (os05b10->hblank)
> -		os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>   
>   	vblank_def = mode->vts - mode->height;
>   	os05b10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> @@ -1317,12 +1316,10 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   					      exp_max, OS05B10_EXPOSURE_STEP,
>   					      mode->exp);
>   
> -	os05b10->gain = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> -					  V4L2_CID_ANALOGUE_GAIN,
> -					  OS05B10_ANALOG_GAIN_MIN,
> -					  OS05B10_ANALOG_GAIN_MAX,
> -					  OS05B10_ANALOG_GAIN_STEP,
> -					  OS05B10_ANALOG_GAIN_DEFAULT);
> +	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
> +			  V4L2_CID_ANALOGUE_GAIN, OS05B10_ANALOG_GAIN_MIN,
> +			  OS05B10_ANALOG_GAIN_MAX, OS05B10_ANALOG_GAIN_STEP,
> +			  OS05B10_ANALOG_GAIN_DEFAULT);
>   
>   	v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops, V4L2_CID_DIGITAL_GAIN,
>   			  OS05B10_DIGITAL_GAIN_MIN, OS05B10_DIGITAL_GAIN_MAX,
> @@ -1330,33 +1327,31 @@ static int os05b10_init_controls(struct os05b10 *os05b10)
>   
>   	os05b10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
>   					   V4L2_CID_HFLIP, 0, 1, 1, 0);
> -	if (os05b10->hflip)
> -		os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>   
>   	os05b10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os05b10_ctrl_ops,
>   					   V4L2_CID_VFLIP, 0, 1, 1, 0);
> -	if (os05b10->vflip)
> -		os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>   
>   	v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &os05b10_ctrl_ops,
>   				     V4L2_CID_TEST_PATTERN,
>   				     ARRAY_SIZE(os05b10_test_pattern_menu) - 1,
>   				     0, 0, os05b10_test_pattern_menu);
>   
> +	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
> +					      &props);
> +	if (ret)
> +		goto error;
> +
>   	if (ctrl_hdlr->error) {
>   		ret = ctrl_hdlr->error;
>   		dev_err(os05b10->dev, "control init failed (%d)\n", ret);
>   		goto error;
>   	}
>   
> -	ret = v4l2_fwnode_device_parse(os05b10->dev, &props);
> -	if (ret)
> -		goto error;
>   
> -	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &os05b10_ctrl_ops,
> -					      &props);
> -	if (ret)
> -		goto error;
> +	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	os05b10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	os05b10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +	os05b10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>   
>   	os05b10->sd.ctrl_handler = ctrl_hdlr;
>   

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>


I would suggest to reorganize and reorder changes in the changeset, moving
simple changes like this one to the very top. Also it might be possible to
merge a number of trivial changes earlier, this will save time and efforts.

-- 
Best wishes,
Vladimir