Message-ID: <pzbbtaltu7wcrsjvjg6n2x33uwm3us4uwpykektc7xlj47s7pz@odqzjc64db2i>
Date: Tue, 25 Mar 2025 21:32:33 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Mirela Rabulea <mirela.rabulea@....com>
Cc: "mchehab@...nel.org" <mchehab@...nel.org>, 
	"sakari.ailus@...ux.intel.com" <sakari.ailus@...ux.intel.com>, "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>, 
	"laurent.pinchart+renesas@...asonboard.com" <laurent.pinchart+renesas@...asonboard.com>, "robh@...nel.org" <robh@...nel.org>, 
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "bryan.odonoghue@...aro.org" <bryan.odonoghue@...aro.org>, 
	Laurentiu Palcu <laurentiu.palcu@....com>, Robert Chiras <robert.chiras@....com>, 
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	LnxRevLi <LnxRevLi@....com>, 
	"kieran.bingham@...asonboard.com" <kieran.bingham@...asonboard.com>, "hdegoede@...hat.com" <hdegoede@...hat.com>, 
	"dave.stevenson@...pberrypi.com" <dave.stevenson@...pberrypi.com>, "mike.rudenko@...il.com" <mike.rudenko@...il.com>, 
	"alain.volmat@...s.st.com" <alain.volmat@...s.st.com>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"conor+dt@...nel.org" <conor+dt@...nel.org>, 
	"alexander.stein@...tq-group.com" <alexander.stein@...tq-group.com>, "umang.jain@...asonboard.com" <umang.jain@...asonboard.com>, 
	"zhi.mao@...iatek.com" <zhi.mao@...iatek.com>, "festevam@...x.de" <festevam@...x.de>, 
	Julien Vuillaumier <julien.vuillaumier@....com>, "devarsht@...com" <devarsht@...com>, 
	"r-donadkar@...com" <r-donadkar@...com>, Oliver Brown <oliver.brown@....com>
Subject: Re: [PATCH v4 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor
 driver

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

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

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

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

-- 
Thanks,
Jai

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists