Hi Mirela,

Thanks a lot for your patch/series.

On Mar 05, 2025 at 11:43:57 +0200, Mirela Rabulea wrote:

Add a v4l2 subdevice driver for the Omnivision OX05B1S RGB-IR sensor.

The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an
active array size of 2592 x 1944.

The following features are supported for OX05B1S:
- Manual exposure an gain control support
- vblank/hblank control support
- Supported resolution: 2592 x 1944 @ 30fps (SGRBG10)

Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---
Changes in v4:
	Switch to Y media bus codes. The CFA pattern control will be implemented when patches get merged, or maybe separatelly as RFC?
	Add pixel_rate member to mode struct, remove fps member. We do not have information how to calculate the pixel rate from the PLL parameters that can be made public.
	Use register macros for the registers that are documented. User register group macros, where individual registers are not documented
	Remove some uneeded local variable initialisations
	Fix extra/missing spaces
	Add missing ending \n
	Use return -ENODEV & return 0 to ease reading
	Rename retval to ret in probe for consistency
	Use devm_mutex_init instead of mutex_init
	Replace more dev_err's with dev_err_probe
	Constify more structs
	Remove some unneded ending commas after a terminator
	Fix smatch error in ox05b1s_s_ctrl: error: typename in expression
	Fix a seeries of smatch warnings like: warning: symbol 'ovx5b_init_setting_2592x1944' was not declared. Should it be static?
	Shorten some more lines to 80 columns

Changes in v3:
	Use helpers from v4l2-cci.h (drop ox05b1s_write_reg, ox05b1s_read_reg, ox05b1s_set_hts/vts/exp/analog_gain, ox05b1s_regmap_config)
	Don't hardcode timing registers: remove timing registers x_output_size/y_output_size from register configuration list, add them to ox05b1s_apply_current_mode
	Remove HTS,VTS from register config list as they are written by HBLANK and VBLANK controls through __v4l2_ctrl_handler_setup
	ox05b1s register config cleaning (remove all registers that were at their default value, and more, keep only what seems mandatory to be able to stream)
	Use const for ox05b1s_supported_modes
	Device should be silent on success, use dev_dbg.
	Drop unneeded {}
	Fixed an error introduced in v2 in ox05b1s_nearest_size (set_fmt for 4k BGGR12 mode was stuck)
	Fix an issue in ox05b1s_set_fmt, the format was saved in subdev state only for _TRY, save it also for _ACTIVE

Changes in v2:
	Use dev_err_probe for missing clock, since it is now required property, and use NULL for devm_clk_get (no name for single clock), remove check for non NULL sensor->sensor_clk
	Remove dev_err message for devm_regmap_init_i2c allocation error
	Added spaces inside brackets, wrap lines to 80
	Remove some redundant initializations
	Add regulators
	Make "sizes" a pointer
		Use struct v4l2_area instead of u32[2] array
		Remove the count for supported_modes[] and supported_codes[], instead use sentinel element at the end
		Consequently, update ox05b1s_enum_mbus_code, ox05b1s_enum_frame_size, ox05b1s_nearest_size, ox05b1s_find_code, to not use the count
	Remove .h files for modes, however did not move this code in the driver file but added a separate c file for all supported modes
	Refactor register lists to allow multiple register arrays per mode
	Use GPL-2.0-only instead of GPL-2.0

 drivers/media/i2c/Kconfig                 |   1 +
 drivers/media/i2c/Makefile                |   1 +
 drivers/media/i2c/ox05b1s/Kconfig         |  10 +
 drivers/media/i2c/ox05b1s/Makefile        |   2 +
 drivers/media/i2c/ox05b1s/ox05b1s.h       |  22 +
 drivers/media/i2c/ox05b1s/ox05b1s_mipi.c  | 951 ++++++++++++++++++++++
 drivers/media/i2c/ox05b1s/ox05b1s_modes.c |  77 ++
 7 files changed, 1064 insertions(+)
 create mode 100644 drivers/media/i2c/ox05b1s/Kconfig
 create mode 100644 drivers/media/i2c/ox05b1s/Makefile
 create mode 100644 drivers/media/i2c/ox05b1s/ox05b1s.h
 create mode 100644 drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
 create mode 100644 drivers/media/i2c/ox05b1s/ox05b1s_modes.c

[snip]

+#define OX05B1S_REG_CHIP_ID		CCI_REG24(0x300a)
+#define OX05B1S_REG_TIMING_HTS		CCI_REG16(0x380c)
+#define OX05B1S_REG_TIMING_VTS		CCI_REG16(0x380e)
+#define OX05B1S_REG_EXPOSURE		CCI_REG16(0x3501)
+#define OX05B1S_REG_GAIN		CCI_REG16(0x3508)

There is a non-trivial overlap of registers between this driver and 
ov9282.c which supports OV9281/OV9282 (1MP Mono).

There are two other Omnivision sensors, OV2311 (2MP Mono) and OV2312 
(2MP 4x4 RGB-IR Bayer) with an even larger register overlap with OX05B1S 
and OS08A20. Unfortunately those two have separate downstream drivers in 
RPi and TI linux downstream trees respectively, and haven't yet been 
posted upstream.

It would be ideal to have a single driver for all of these Omnivision 
sensors, or if not, at least a common C module that can implement the 
shared functionality like gain, exposure, blanking (vts & hts) in a 
single place, as this will make maintenance much easier.

My question here to you and the maintainers is, would it be okay to use 
this driver as a baseline to integrate all these different sensors 
together? And secondly, would you like to take a look at supporting 
ov9282, so the other driver can be dropped?

Anyway thanks again for your series, hopefully this will give a good 
starting point for upstreaming OV2311 and OV2312 soon.

Thanks,
Jai

+#define OX05B1S_REG_X_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OX05B1S_REG_Y_OUTPUT_SIZE	CCI_REG16(0x380a)
+

[snip]

-- 
Thanks,
Jai