Re: [PATCH v4 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor driver

[Jai Luthra]

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@xxxxxxx>
> ---
> 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]
> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c 
> b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
> new file mode 100644
> index 000000000000..1026216ddd5b
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
> @@ -0,0 +1,951 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * A V4L2 driver for Omnivision OX05B1S RGB-IR camera.
> + * Copyright (C) 2024, NXP
> + *
> + * Inspired from Sony imx219, imx290, imx214 and imx334 camera drivers
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/v4l2-cci.h>
> +#include <media/mipi-csi2.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "ox05b1s.h"
> +
> +#define OX05B1S_SENS_PAD_SOURCE	0
> +#define OX05B1S_SENS_PADS_NUM	1
> +
> +#define OX05B1S_REG_SW_STB		CCI_REG8(0x0100)
> +#define OX05B1S_REG_SW_RST		CCI_REG8(0x0103)

> +#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

Attachment: signature.asc
Description: PGP signature