Re: [PATCH v4 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor driver
From: Mirela Rabulea
Date: Mon Mar 24 2025 - 11:33:13 EST
Hi Jai and all,
On 19.03.2025 13:10, Jai Luthra wrote:
Hi Mirela,
Thanks a lot for your patch/series.
On Mar 05, 2025 at 11:43:57 +0200, Mirela Rabulea wrote:
Add a v4l2subdevice driver for theOmnivision OX05B1S RGB-IR sensor.
TheOmnivision 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> <mailto:mirela.rabulea@xxxxxxx>
---
Changes in v4:
Switch to Y media bus codes. The CFA pattern control will be implemented when patches get merged, or maybeseparatelly as RFC?
Addpixel_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 someuneeded local variableinitialisations
Fix extra/missing spaces
Add missing ending \n
Use return -ENODEV & return 0 to ease reading
Renameretval to ret in probe for consistency
Usedevm_mutex_init instead ofmutex_init
Replace moredev_err's withdev_err_probe
Constify more structs
Remove someunneded ending commas after a terminator
Fixsmatch error in ox05b1s_s_ctrl: error:typename in expression
Fix aseeries ofsmatch 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 registersx_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, usedev_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 insubdev state only for _TRY, save it also for _ACTIVE
Changes in v2:
Usedev_err_probe for missing clock, since it is now required property, and use NULL fordevm_clk_get (no name for single clock), remove check fornon NULL sensor->sensor_clk
Removedev_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 forsupported_modes[] andsupported_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 forOmnivision 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_SOURCE0
+#define OX05B1S_SENS_PADS_NUM1
+
+#define OX05B1S_REG_SW_STBCCI_REG8(0x0100)
+#define OX05B1S_REG_SW_RSTCCI_REG8(0x0103)
+#define OX05B1S_REG_CHIP_IDCCI_REG24(0x300a)
+#define OX05B1S_REG_TIMING_HTSCCI_REG16(0x380c)
+#define OX05B1S_REG_TIMING_VTSCCI_REG16(0x380e)
+#define OX05B1S_REG_EXPOSURECCI_REG16(0x3501)
+#define OX05B1S_REG_GAINCCI_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 otherOmnivision 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 TIlinux downstream trees respectively, and haven't yet been
posted upstream.
Thanks for sharing this information, I was unaware. The question of how
much similarity should two sensors share, in order to stay in the same
driver, was also on my mind for some time, and I’d be glad to hear more
opinions on it ;)
It would be ideal to have a single driver for all of theseOmnivision
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.
I would need to get more information on the sensors you mentioned in
order to issue an informed opinion. So far, with the OX05B1S and
OS08A20, I have found some small differences regarding exposure and
digital gain registers, so the overlap is not perfect, I expect it will
also not be a perfect overlap with the other sensors you mentioned.
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?
For the first question, I don't know what to say, and I cannot tell if
we are far or close for this patch-set to be accepted. Also, I am unsure
about how maintenance would go on a driver claiming to support a
multitude of sensors, who could test them all, whenever something
changes? Are you thinking to add ov2311/12 as other compatibles to this
driver?
I agree there is a great deal of similarity shared across many raw,
mode-based sensor drivers, and it sounds good to have some common
framework. Some steps were done with the common raw sensor model. I
would definitely also like to hear more expert opinions on this.
For the second question, as of now, we do not have any of the sensors
you mentioned, unfortunately. I could help in the future to test patches
for this driver on the sensors that we already have, but cannot make any
promises for what I do not have, best effort if we find these sensors in
a form factor that will work for our boards.
Anyway thanks again for your series, hopefully this will give a good
starting point for upstreaming OV2311 and OV2312 soon.
I would like to know more about the OV2312 (RGB-Ir) sensor and if it has
many similarities with OX05B1S. What hardware are you using to test
this sensor? And what interface to connect the sensor? We are working
with MIPI-CSI on most imx boards, and also RPI on imx93.
Regards,
Mirela
Thanks,
Jai
+#define OX05B1S_REG_X_OUTPUT_SIZECCI_REG16(0x3808)
+#define OX05B1S_REG_Y_OUTPUT_SIZECCI_REG16(0x380a)
+
[snip]