Re: [PATCH v4 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor driver
From: Mirela Rabulea
Date: Wed Mar 26 2025 - 11:36:57 EST
Hi Jai,
On 25.03.2025 18:02, Jai Luthra wrote:
On Mar 24, 2025 at 17:32:01 +0200, Mirela Rabulea wrote:
Hi Jai and all,
On Mar 19, 2025 at 16:40:30 +0530, 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 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.
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 ;)
Same here :)
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.
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.
Sure, I had some experience with supporting OV2312 and OX05B1S in the
downstream TI linux tree, and while they share the registers for
exposure and gain, there are some other differences in features, aside
from the 2MP vs 5MP resolution.
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?
While it would be ideal to have OV2312 support within this driver if
there is a significant register overlap, it might still require some
effort, as TI's downstream drivers for the RGBIR sensors capture two
streams with different exposure, gain and IR flash values, and different
MIPI CSI virtual channels, using the group hold functionality. Which
IIUC may be quite different from what your patches implement, and will
require adding streams/routing support so the userspace can configure.
You are not alone on this ;)
We have an internal solution for adding streams/routing support to this
driver, we used it both for ox05b1s (AB mode with 2 pixel streams on
separate virtual channels) and for os08a20 (staggered hdr mode with 2
pixel streams on separate virtual channels), and also a separate stream
for embedded data (same VC but a different mipi data type). I did not
post these patches because of 2 reasons:
1. We are waiting for internal pads to be accepted, and possibly the
common raw sensor model
2. There are issues with the individual control (exposure, analog and
digital gain) per exposure/context, with the current available standard
controls. This is an entire topic on its own, maybe I should start a
separate discussion thread on that.
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.
I agree, having a single maintainer would not be feasible given
different sensor modules may have incompatible connectors. But yes it
should be okay to provide a T-By tag or a Nack on the shared driver if a
patch breaks your particular hardware or usecase, similar to how other
popular sensor drivers are maintained like IMX219 or OV5640.
Sounds ok to me, we cannot guarantee to test the other sensors, but
having T-by tag from other users will hopefully cover it.
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.
For OV2312 I have used this FPDLink module [1] with the Arducam V3Link
board [2] that connects to the EVM using 22-pin FFC MIPI-CSI connector
that is pin-compatible with the RPi5 connector.
[1]: https://leopardimaging.com/wp-content/uploads/2024/03/LI-OV2312-FPDLINKIII_Datasheet.pdf
[2]: https://www.arducam.com/downloads/datasheet/Arducam_V3Link_Datasheet.pdf
Thanks, we don't have any ser/des involved in the ox05b1s/os08a20 case,
if we will ever get into the position to try ov2312, probably we will
look for some solution for imx95-15x15 board, on the RPi connector
(22-pin), cannot tell if it will work, one can never tell what may go
wrong.
Regards,
Mirela
PS: Your mail client broke the quotations on your reply. I have fixed
those here, but might be a good idea to double-check your client
settings.
Sorry about that, I may have edited the draft from both Thunderbird and
Outlook. Hope this will be ok, sending from Thunderbird now as plain
text only.
Regards,
Mirela