Message-ID: <9e4e8d6d-f4f0-4332-a30c-ab0429e1526d@nxp.com>
Date: Wed, 26 Mar 2025 17:32:55 +0200
From: Mirela Rabulea <mirela.rabulea@....com>
To: Jai Luthra <jai.luthra@...asonboard.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

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

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

Powered by blists - more mailing lists