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

[Jai Luthra]

On Mar 26, 2025 at 17:32:55 +0200, Mirela Rabulea wrote:
> 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:

Ah that's good to know that you also use AB mode, as combining the 
drivers makes even more sense now.

> 
> 1. We are waiting for internal pads to be accepted, and possibly the common
> raw sensor model

I wasn't aware of the raw sensor model series, will read up more on that 
as that also seems to have an easier way to represent RGB-Ir bayer 
formats.

> 
> 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.
> 

Yes individual controls, be it for internal pads or per-stream, will be 
a requirement for multi-stream sensor drivers. I have proposed a topic 
to discuss this with the rest of the community at the Media Summit being 
held on May 13 in Nice. [1]

Do you have plans to attend the summit? In any case, please feel free to 
start a RFC discussion thread on it :)

One idea is to move the controls in the v4l2_subdev_state, which would 
make it easier to specify pad/stream combinations, but I am not yet sure 
on what the userspace ioctls will look like for that.

[1] https://lore.kernel.org/linux-media/sbhsskq7kzrl5bkbqbijxszz7hfg34pjajgdmw23x7aseztyy3@26zcjwnjkpl4/

> > 
> > > 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.
> > 

Thanks.

> > > > 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.
> > 

Makes sense.

> > >   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.
> 

No problem, this mail looks okay.

> Regards,
> 
> Mirela
> 

Thanks,
Jai

Attachment: signature.asc
Description: PGP signature