Message-ID: <2fc622c4-04a3-4069-93d9-0b2388226fb4@nxp.com>
Date: Mon, 31 Mar 2025 18:42:31 +0300
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: [EXT] Re: [PATCH v4 2/4] media: ox05b1s: Add omnivision OX05B1S
 raw sensor driver

Hi Jai,

On 28.03.2025 10:37, Jai Luthra wrote:
> 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@....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:
> 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/

Thanks for letting me know, yes that is interesting, I will participate, 
if not in person, at least remotely. I will reply to the indicated 
thread, after we decide on our agenda.

Regards,

Mirela

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

Powered by blists - more mailing lists