[PATCH v3 0/4] media: i2c: Add OX05B1S camera sensor driver

[PATCH v3 0/4] media: i2c: Add OX05B1S camera sensor driver

[Mirela Rabulea]

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)

Support for another sensor, OS08A20, is added as a separate patch, using another compatible.
For OS08a20, HDR mode control is supported, with one HDR mode: staggered HDR with 2 exposures on separate virtual channels. However, for now, only one exposure (VC 0) is accessible via get_frame_desc.

Supported resolutions:
   - 1920 x 1080 @ 60fps (SBGGR10, no HDR)
   - 1920 x 1080 @ 30fps (SBGGR10, HDR)
   - 3840 x 2160 @ 30fps (SBGGR12, no HDR)
   - 3840 x 2160 @ 15fps (SBGGR12, HDR)
   - 3840 x 2160 @ 30fps (SBGGR10, no HDR)
   - 3840 x 2160 @ 15fps (SBGGR10, HDR)

The driver was tested on upstream 6.13-rc7 on imx8mp-evk, but also on nxp tree based on 6.12 on imx95-19x19-evk.

The results of v4l2-compliance test:

root@imx8mpevk:/unit_tests# ./v4l2-compliance -d /dev/video0
v4l2-compliance 1.29.0-5304, 64 bits, 64-bit time_t
v4l2-compliance SHA: 8fb667bc4ec2 2025-01-22 12:37:43

Compliance test for mxc-isi device /dev/video0:

Driver Info:
	Driver name      : mxc-isi
	Card type        : mxc-isi-cap
	Bus info         : platform:32e00000.isi
	Driver version   : 6.13.0
	Capabilities     : 0xa4201000
		Video Capture Multiplanar
		I/O MC
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x24201000
		Video Capture Multiplanar
		I/O MC
		Streaming
		Extended Pix Format
Media Driver Info:
	Driver name      : mxc-isi
	Model            : FSL Capture Media Device
	Serial           : 
	Bus info         : platform:32e00000.isi
	Media version    : 6.13.0
	Hardware revision: 0x00000000 (0)
	Driver version   : 6.13.0
Interface Info:
	ID               : 0x0300000c
	Type             : V4L Video
Entity Info:
	ID               : 0x0000000a (10)
	Name             : mxc_isi.0.capture
	Function         : V4L2 I/O
	Pad 0x0100000b   : 0: Sink
	  Link 0x0200000e: from remote pad 0x1000009 of entity 'mxc_isi.0' (Video Pixel Formatter): Data, Enabled, Immutable

Required ioctls:
	test MC information (see 'Media Driver Info' above): OK
	test VIDIOC_QUERYCAP: OK
	test invalid ioctls: OK

Allow for multiple opens:
	test second /dev/video0 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
	test VIDIOC_QUERYCTRL: OK
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 4 Private Controls: 0

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK (Not Supported)
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test CREATE_BUFS maximum buffers: OK
	test VIDIOC_REMOVE_BUFS: OK
	test VIDIOC_EXPBUF: OK
	test Requests: OK (Not Supported)
	test blocking wait: OK

Total for mxc-isi device /dev/video0: 49, Succeeded: 49, Failed: 0, Warnings: 0


Mirela Rabulea (4):
  dt-bindings: media: i2c: Add OX05B1S sensor
  media: ox05b1s: Add omnivision OX05B1S raw sensor driver
  MAINTAINERS: Add entry for OX05B1S sensor driver
  media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

 .../bindings/media/i2c/ovti,ox05b1s.yaml      |  119 ++
 MAINTAINERS                                   |   10 +
 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           |   26 +
 drivers/media/i2c/ox05b1s/ox05b1s_mipi.c      | 1123 +++++++++++++++++
 drivers/media/i2c/ox05b1s/ox05b1s_modes.c     |  159 +++
 9 files changed, 1451 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
 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

-- 
2.25.1

[PATCH v3 1/4] dt-bindings: media: i2c: Add OX05B1S sensor

[Mirela Rabulea]

Add bindings for Omnivision OX05B1S sensor.
Also add compatible for Omnivision OS08A20 sensor.

Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---

Changes in v3:
	Use unevaluatedProperties: false and drop orientation/rotation
	Drop items and keep alphabetical order in compatible property
	Shorten the description for reset_gpio
	Make the supplies required.
	Use generic node name (camera instead of ox05b1s)

Changes in v2:
	Small updates on description
	Update subject, drop "bindings" and "driver"
	Just one binding patch (squash os08a20 bindings)
	Re-flow to 80 columns.
	Drop clock name (not needed in case of single clock)
	Make the clock required property, strictly from sensor module point of view, it is mandatory (will use a fixed clock for nxp board)
	Add regulators: avdd, dvdd, dovdd
	Add $ref: /schemas/media/video-interface-devices.yaml
	Drop assigned-clock* properties (defined in clocks.yaml)
	Keep "additionalProperties : false" and orientation/rotation (unevaluatedProperties: false was suggested, but only orientation/rotation are needed from video-interface-devices.yaml)
	Include assigned-clock* in the example, for completeness sake (although it was also suggested to omit them)

 .../bindings/media/i2c/ovti,ox05b1s.yaml      | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
new file mode 100644
index 000000000000..9f35b4e67bea
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ox05b1s.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OX05B1S Image Sensor
+
+maintainers:
+  - Mirela Rabulea <mirela.rabulea@nxp.com>
+
+description:
+  The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an active
+  array size of 2592 x 1944. It is programmable through I2C interface.
+  Image data is available via MIPI CSI-2 serial data output.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ovti,os08a20
+      - ovti,ox05b1s
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: Input clock (24 MHz)
+    maxItems: 1
+
+  reset-gpios:
+    description: Active low XSHUTDOWN pin
+    maxItems: 1
+
+  avdd-supply:
+    description: Power for analog circuit (2.8V)
+
+  dovdd-supply:
+    description: Power for I/O circuit (1.8V)
+
+  dvdd-supply:
+    description: Power for digital circuit (1.2V)
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+    description: MIPI CSI-2 transmitter port
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        properties:
+          data-lanes:
+            anyOf:
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+        required:
+          - data-lanes
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+  - avdd-supply
+  - dovdd-supply
+  - dvdd-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@36 {
+            compatible = "ovti,ox05b1s";
+            reg = <0x36>;
+            clocks = <&ox05b1s_clk>;
+
+            assigned-clocks = <&ox05b1s_clk>;
+            assigned-clock-parents = <&ox05b1s_clk_parent>;
+            assigned-clock-rates = <24000000>;
+
+            reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+
+            avdd-supply = <&camera_avdd_2v8>;
+            dovdd-supply = <&camera_dovdd_1v8>;
+            dvdd-supply = <&camera_dvdd_1v2>;
+
+            orientation = <2>;
+            rotation = <0>;
+
+            port {
+                ox05b1s_mipi_0_ep: endpoint {
+                    remote-endpoint = <&mipi_csi0_ep>;
+                    data-lanes = <1 2 3 4>;
+                };
+            };
+        };
+    };
+...
-- 
2.25.1

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

[Mirela Rabulea]

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

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  | 939 ++++++++++++++++++++++
 drivers/media/i2c/ox05b1s/ox05b1s_modes.c |  63 ++
 7 files changed, 1038 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

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 8ba096b8ebca..5cda062c0463 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -700,6 +700,7 @@ config VIDEO_VGXY61
 
 source "drivers/media/i2c/ccs/Kconfig"
 source "drivers/media/i2c/et8ek8/Kconfig"
+source "drivers/media/i2c/ox05b1s/Kconfig"
 
 endif
 
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index fbb988bd067a..028eb08e648c 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -114,6 +114,7 @@ obj-$(CONFIG_VIDEO_OV9282) += ov9282.o
 obj-$(CONFIG_VIDEO_OV9640) += ov9640.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
 obj-$(CONFIG_VIDEO_OV9734) += ov9734.o
+obj-$(CONFIG_VIDEO_OX05B1S) += ox05b1s/
 obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
 obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
 obj-$(CONFIG_VIDEO_RJ54N1) += rj54n1cb0c.o
diff --git a/drivers/media/i2c/ox05b1s/Kconfig b/drivers/media/i2c/ox05b1s/Kconfig
new file mode 100644
index 000000000000..48fcd04b20d5
--- /dev/null
+++ b/drivers/media/i2c/ox05b1s/Kconfig
@@ -0,0 +1,10 @@
+config VIDEO_OX05B1S
+	tristate "OmniVision raw sensor support OX05B1S"
+	depends on OF
+	depends on GPIOLIB
+	select REGMAP_I2C
+	help
+	  This is a Video4Linux2 sensor driver for the Omnivision OX05B1S RGB-IR sensor.
+	  This is a 1/2.5-Inch CMOS image sensor with an active array size of 2592 x 1944.
+	  It is programmable through I2C interface.
+	  The output is on MIPI CSI-2 interface.
diff --git a/drivers/media/i2c/ox05b1s/Makefile b/drivers/media/i2c/ox05b1s/Makefile
new file mode 100644
index 000000000000..0b38dbf98bcd
--- /dev/null
+++ b/drivers/media/i2c/ox05b1s/Makefile
@@ -0,0 +1,2 @@
+ox05b1s-objs := ox05b1s_modes.o ox05b1s_mipi.o
+obj-$(CONFIG_VIDEO_OX05B1S) += ox05b1s.o
diff --git a/drivers/media/i2c/ox05b1s/ox05b1s.h b/drivers/media/i2c/ox05b1s/ox05b1s.h
new file mode 100644
index 000000000000..a893c65894f3
--- /dev/null
+++ b/drivers/media/i2c/ox05b1s/ox05b1s.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024, NXP
+ */
+
+#ifndef OX05B1S_H
+#define OX05B1S_H
+
+#include <linux/types.h>
+
+struct ox05b1s_reg {
+	u32 addr;
+	u32 data;
+};
+
+struct ox05b1s_reglist {
+	struct ox05b1s_reg *regs;
+};
+
+extern struct ox05b1s_reglist ox05b1s_reglist_2592x1944[];
+
+#endif /* OX05B1S_H */
diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
new file mode 100644
index 000000000000..aeea7caa6a15
--- /dev/null
+++ b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
@@ -0,0 +1,939 @@
+// 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)
+#define OX05B1S_REG_X_OUTPUT_SIZE	CCI_REG16(0x3808)
+#define OX05B1S_REG_Y_OUTPUT_SIZE	CCI_REG16(0x380a)
+
+#define client_to_ox05b1s(client)\
+	container_of(i2c_get_clientdata(client), struct ox05b1s, subdev)
+
+struct ox05b1s_sizes {
+	u32	code;
+	const struct v4l2_area *sizes;
+};
+
+struct ox05b1s_plat_data {
+	char				name[20];
+	u32				chip_id;
+	u32				native_width;
+	u32				native_height;
+	u32				active_top;
+	u32				active_left;
+	u32				active_width;
+	u32				active_height;
+	const struct ox05b1s_mode	*supported_modes;
+	u32				default_mode_index;
+	const struct ox05b1s_sizes	*supported_codes;
+};
+
+struct ox05b1s_ctrls {
+	struct v4l2_ctrl_handler handler;
+	struct v4l2_ctrl *link_freq;
+	struct v4l2_ctrl *pixel_rate;
+	struct v4l2_ctrl *hblank;
+	struct v4l2_ctrl *vblank;
+	struct v4l2_ctrl *gain;
+	struct v4l2_ctrl *exposure;
+};
+
+struct ox05b1s_mode {
+	u32 index;
+	u32 width;
+	u32 height;
+	u32 code;
+	u32 bpp;
+	u32 vts; /* default VTS */
+	u32 hts; /* default HTS */
+	u32 exp; /* max exposure */
+	bool h_bin; /* horizontal binning */
+	u32 fps;
+	struct ox05b1s_reglist *reg_data;
+};
+
+/* regulator supplies */
+static const char * const ox05b1s_supply_name[] = {
+	"AVDD",  /* Analog voltage supply, 2.8 volts */
+	"DVDD",  /* Digital I/O voltage supply, 1.8 volts */
+	"DOVDD", /* Digital voltage supply, 1.2 volts */
+};
+
+#define OX05B1S_NUM_SUPPLIES ARRAY_SIZE(ox05b1s_supply_name)
+
+struct ox05b1s {
+	struct i2c_client *i2c_client;
+	struct regmap *regmap;
+	struct gpio_desc *rst_gpio;
+	struct regulator_bulk_data supplies[OX05B1S_NUM_SUPPLIES];
+	struct clk *sensor_clk;
+	const struct ox05b1s_plat_data *model;
+	struct v4l2_subdev subdev;
+	struct media_pad pads[OX05B1S_SENS_PADS_NUM];
+	const struct ox05b1s_mode *mode;
+	struct mutex lock; /* sensor lock */
+	u32 stream_status;
+	struct ox05b1s_ctrls ctrls;
+};
+
+static const struct ox05b1s_mode ox05b1s_supported_modes[] = {
+	{
+		.index		= 0,
+		.width		= 2592,
+		.height		= 1944,
+		.code		= MEDIA_BUS_FMT_SGRBG10_1X10,
+		.bpp		= 10,
+		.vts		= 0x850,
+		.hts		= 0x2f0,
+		.exp		= 0x850 - 8,
+		.h_bin		= false,
+		.fps		= 30,
+		.reg_data	= ox05b1s_reglist_2592x1944,
+	}, {
+		/* sentinel */
+	}
+};
+
+/* keep in sync with ox05b1s_supported_modes */
+static const struct v4l2_area ox05b1s_sgrbg10_sizes[] = {
+	{
+		.width = 2592,
+		.height = 1944,
+	}, {
+		/* sentinel */
+	}
+};
+
+static const struct ox05b1s_sizes ox05b1s_supported_codes[] = {
+	{
+		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
+		.sizes = ox05b1s_sgrbg10_sizes,
+	}, {
+		/* sentinel */
+	}
+};
+
+static int ox05b1s_power_on(struct ox05b1s *sensor)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+	int ret = 0;
+
+	ret = regulator_bulk_enable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulators\n");
+		return ret;
+	}
+
+	/* get out of powerdown and reset */
+	gpiod_set_value_cansleep(sensor->rst_gpio, 0);
+
+	ret = clk_prepare_enable(sensor->sensor_clk);
+	if (ret < 0) {
+		dev_err(dev, "Enable sensor clk fail ret=%d\n", ret);
+		goto reg_off;
+	}
+
+	/* with XVCLK@24MHz, t2 = 6ms required delay for ox05b1s before first SCCB transaction */
+	fsleep(6000);
+
+	return ret;
+
+reg_off:
+	regulator_bulk_disable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
+
+	return ret;
+}
+
+static int ox05b1s_power_off(struct ox05b1s *sensor)
+{
+	gpiod_set_value_cansleep(sensor->rst_gpio, 1);
+
+	/* XVCLK must be active for 512 cycles (0.34 ms at 24MHz) after last SCCB transaction */
+	fsleep(350);
+	clk_disable_unprepare(sensor->sensor_clk);
+
+	regulator_bulk_disable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
+
+	return 0;
+}
+
+static int ox05b1s_runtime_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+
+	return ox05b1s_power_off(sensor);
+}
+
+static int ox05b1s_runtime_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+
+	return ox05b1s_power_on(sensor);
+}
+
+#define OX05B1S_MAX_REG_BULK 16
+static int ox05b1s_write_reg_array(struct ox05b1s *sensor,
+				   struct ox05b1s_reg *reg_array)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+	struct ox05b1s_reg *table = reg_array;
+	u8 vals[OX05B1S_MAX_REG_BULK];
+	int i;
+	int ret;
+
+	while (table->addr) {
+		for (i = 0; i < OX05B1S_MAX_REG_BULK; i++) {
+			if (table[i].addr != (table[0].addr + i))
+				break;
+			vals[i] = table[i].data;
+		}
+		ret = regmap_bulk_write(sensor->regmap, table->addr, vals, i);
+		if (ret) {
+			dev_err(dev, "Failed to write reg addr=%x, count %d\n", table->addr, i);
+			return ret;
+		}
+		table += i;
+	}
+
+	return 0;
+}
+
+static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
+{
+	return &container_of(ctrl->handler, struct ox05b1s,
+			     ctrls.handler)->subdev;
+}
+
+static int ox05b1s_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	u32 w = sensor->mode->width;
+	u32 h = sensor->mode->height;
+	int ret = 0;
+
+	/* apply V4L2 controls values only if power is already up */
+	if (!pm_runtime_get_if_in_use(&client->dev))
+		return 0;
+
+	/* s_ctrl holds sensor lock */
+	switch (ctrl->id) {
+	case V4L2_CID_VBLANK:
+		ret = cci_write(sensor->regmap, OX05B1S_REG_TIMING_VTS,
+				h + ctrl->val, NULL);
+		break;
+	case V4L2_CID_HBLANK:
+		u32 hts = (sensor->mode->h_bin) ?
+			  w + ctrl->val : (w + ctrl->val) / 2;
+		ret = cci_write(sensor->regmap, OX05B1S_REG_TIMING_HTS,
+				hts, NULL);
+		break;
+	case V4L2_CID_PIXEL_RATE:
+		/* Read-only, but we adjust it based on mode. */
+		break;
+	case V4L2_CID_ANALOGUE_GAIN:
+		ret = cci_write(sensor->regmap, OX05B1S_REG_GAIN,
+				ctrl->val, NULL);
+		break;
+	case V4L2_CID_EXPOSURE:
+		ret = cci_write(sensor->regmap, OX05B1S_REG_EXPOSURE,
+				ctrl->val, NULL);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_put(&client->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops ox05b1s_ctrl_ops = {
+	.s_ctrl = ox05b1s_s_ctrl,
+};
+
+/*
+ * MIPI CSI-2 link frequencies.
+ * link_freq = (pixel_rate * bpp) / (2 * data_lanes)
+ */
+static const s64 ox05b1s_csi2_link_freqs[] = {
+	200000000,
+};
+
+/* Link freq for default mode: 1080p RAW10, 4 data lanes 800 Mbps/lane. */
+#define OX05B1S_DEFAULT_LINK_FREQ	0
+
+static int ox05b1s_init_controls(struct ox05b1s *sensor)
+{
+	const struct v4l2_ctrl_ops *ops = &ox05b1s_ctrl_ops;
+	struct ox05b1s_ctrls *ctrls = &sensor->ctrls;
+	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	struct device *dev = &sensor->i2c_client->dev;
+	struct v4l2_fwnode_device_properties props;
+	int ret;
+
+	v4l2_ctrl_handler_init(hdl, 7);
+
+	/* we can use our own mutex for the ctrl lock */
+	hdl->lock = &sensor->lock;
+
+	/* Clock related controls */
+	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops,
+						  V4L2_CID_LINK_FREQ,
+						  ARRAY_SIZE(ox05b1s_csi2_link_freqs) - 1,
+						  OX05B1S_DEFAULT_LINK_FREQ,
+						  ox05b1s_csi2_link_freqs);
+
+	/* mode dependent, actual range set in ox05b1s_update_controls */
+	ctrls->pixel_rate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
+					      0, 0, 1, 0);
+
+	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
+					  0, 0, 1, 0);
+
+	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
+					  0, 0, 1, 0);
+
+	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
+					    0, 0, 1, 0);
+
+	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
+					0, 0xFFFF, 1, 0x80);
+
+	if (hdl->error) {
+		ret = hdl->error;
+		goto free_ctrls;
+	}
+
+	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	ret = v4l2_fwnode_device_parse(dev, &props);
+	if (ret)
+		goto free_ctrls;
+
+	ret = v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
+	if (ret)
+		goto free_ctrls;
+
+	sensor->subdev.ctrl_handler = hdl;
+	return 0;
+
+free_ctrls:
+	dev_err(dev, "Failed to init controls\n");
+	v4l2_ctrl_handler_free(hdl);
+	return ret;
+}
+
+static int ox05b1s_apply_current_mode(struct ox05b1s *sensor);
+
+static int ox05b1s_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	int ret = 0;
+
+	if (enable) {
+		ret = pm_runtime_resume_and_get(&client->dev);
+		if (ret < 0)
+			return ret;
+		ret = ox05b1s_apply_current_mode(sensor);
+		if (!ret)
+			ret = cci_write(sensor->regmap, OX05B1S_REG_SW_STB, 0x01, NULL);
+	} else {
+		ret = cci_write(sensor->regmap, OX05B1S_REG_SW_STB, 0x00, NULL);
+	}
+
+	sensor->stream_status = enable;
+
+	if (!enable || ret) {
+		pm_runtime_mark_last_busy(&sensor->i2c_client->dev);
+		pm_runtime_put_autosuspend(&client->dev);
+	}
+
+	return 0;
+}
+
+static void ox05b1s_update_pad_format(struct ox05b1s *sensor,
+				      const struct ox05b1s_mode *mode,
+				      struct v4l2_mbus_framefmt *fmt)
+{
+	fmt->code = mode->code;
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+}
+
+static int ox05b1s_init_state(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	struct v4l2_mbus_framefmt *format;
+
+	/* Initialize the format. */
+	format = v4l2_subdev_state_get_format(state, 0);
+	ox05b1s_update_pad_format(sensor, &sensor->model->supported_modes[0], format);
+
+	return 0;
+}
+
+static int ox05b1s_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	const struct ox05b1s_sizes *supported_codes = sensor->model->supported_codes;
+	int i = 0;
+
+	while (i++ < code->index && supported_codes->code)
+		supported_codes++;
+	if (!supported_codes->code) /* code->index outside supported_codes[] */
+		return -EINVAL;
+
+	code->code = supported_codes->code;
+
+	return 0;
+}
+
+static int ox05b1s_enum_frame_size(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_state *sd_state,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	const struct ox05b1s_sizes *supported_codes = sensor->model->supported_codes;
+	const struct v4l2_area *sizes;
+	int i = 0;
+
+	if (fse->pad != 0)
+		return -EINVAL;
+
+	while (supported_codes->code) {
+		if (supported_codes->code == fse->code)
+			break;
+		supported_codes++;
+	}
+
+	if (!supported_codes->code) /* fse->code not in supported_codes[] */
+		return -EINVAL;
+
+	sizes = supported_codes->sizes;
+	while (i++ < fse->index && sizes->width)
+		sizes++;
+	if (!sizes->width) /* fse->index outside sizes[] */
+		return -EINVAL;
+
+	fse->min_width = sizes->width;
+	fse->max_width = fse->min_width;
+	fse->min_height = sizes->height;
+	fse->max_height = fse->min_height;
+
+	return 0;
+}
+
+/* Update control ranges based on current streaming mode, needs sensor lock */
+static int ox05b1s_update_controls(struct ox05b1s *sensor)
+{
+	int ret;
+	struct device *dev = &sensor->i2c_client->dev;
+	u32 hts = sensor->mode->hts;
+	u32 hblank;
+	u32 vts = sensor->mode->vts;
+	u32 vblank = vts - sensor->mode->height;
+	u32 fps = sensor->mode->fps;
+	u64 pixel_rate = (sensor->mode->h_bin) ? hts * vts * fps : 2 * hts * vts * fps;
+	u32 min_exp = 8;
+	u32 max_exp = vts - 8;
+
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.pixel_rate, pixel_rate,
+				       pixel_rate, 1, pixel_rate);
+	if (ret) {
+		dev_err(dev, "Modify range for ctrl: pixel_rate %llu-%llu failed\n",
+			pixel_rate, pixel_rate);
+		goto out;
+	}
+
+	if (sensor->mode->h_bin)
+		hblank = hts - sensor->mode->width;
+	else
+		hblank = 2 * hts - sensor->mode->width;
+
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank,
+				       1, hblank);
+	if (ret) {
+		dev_err(dev, "Modify range for ctrl: hblank %u-%u failed\n",
+			hblank, hblank);
+		goto out;
+	}
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.hblank, sensor->ctrls.hblank->default_value);
+
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, 0, vblank * 4,
+				       1, vblank);
+	if (ret) {
+		dev_err(dev, "Modify range for ctrl: vblank %u-%u failed\n",
+			vblank, vblank);
+		goto out;
+	}
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->default_value);
+
+	ret = __v4l2_ctrl_modify_range(sensor->ctrls.exposure, min_exp, max_exp,
+				       1, max_exp / 2);
+	if (ret) {
+		dev_err(dev, "Modify range for ctrl: exposure %u-%u failed\n",
+			min_exp, max_exp);
+		goto out;
+	}
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, sensor->ctrls.exposure->default_value);
+
+out:
+	return ret;
+}
+
+/* needs sensor lock and power on */
+static int ox05b1s_apply_current_mode(struct ox05b1s *sensor)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+	struct ox05b1s_reglist *reg_data = sensor->mode->reg_data;
+	u32 w = sensor->mode->width;
+	u32 h = sensor->mode->height;
+	int ret = 0;
+
+	cci_write(sensor->regmap, OX05B1S_REG_SW_RST, 0x01, NULL);
+
+	while (reg_data->regs) {
+		ret = ox05b1s_write_reg_array(sensor, reg_data->regs);
+		if (ret)
+			goto out;
+		reg_data++;
+	}
+
+	cci_write(sensor->regmap, OX05B1S_REG_X_OUTPUT_SIZE, w, &ret);
+	cci_write(sensor->regmap, OX05B1S_REG_Y_OUTPUT_SIZE, h, &ret);
+	if (ret)
+		goto out;
+
+	/* setup handler will write actual controls into sensor registers */
+	ret =  __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
+
+out:
+	if (ret < 0)
+		dev_err(dev, "Failed to apply mode %dx%d,bpp=%d\n", w, h,
+			sensor->mode->bpp);
+
+	return ret;
+}
+
+/* similar with v4l2_find_nearest_size but filter for mbus code, needs sensor lock */
+static const struct ox05b1s_mode *ox05b1s_nearest_size(const struct ox05b1s_mode *supported_modes,
+						       struct v4l2_subdev_format *fmt)
+{
+	u32 error, min_error = U32_MAX;
+	const struct ox05b1s_mode *best = NULL;
+
+	if (!supported_modes)
+		return NULL;
+
+	for (; supported_modes->width; supported_modes++) {
+		const u32 w = supported_modes->width;
+		const u32 h = supported_modes->height;
+
+		if (supported_modes->code != fmt->format.code)
+			continue;
+
+		error = abs(w - fmt->format.width) + abs(h - fmt->format.height);
+		if (error > min_error)
+			continue;
+
+		min_error = error;
+		best = supported_modes;
+		if (!error)
+			break;
+	}
+
+	return best;
+}
+
+/* get a valid mbus code for the model, either the requested one or the default one */
+static u32 ox05b1s_find_code(const struct ox05b1s_plat_data *model, u32 code)
+{
+	u32 found_code = 0;
+	const struct ox05b1s_sizes *supported_codes = model->supported_codes;
+
+	while (supported_codes->code) {
+		if (supported_codes->code == code) {
+			found_code = code;
+			break;
+		}
+		supported_codes++;
+	}
+
+	if (!supported_codes->code) /* code not in supported_codes[] */
+		found_code = model->supported_codes[model->default_mode_index].code;
+
+	return found_code;
+}
+
+static int ox05b1s_set_fmt(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *state,
+			   struct v4l2_subdev_format *fmt)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	struct device *dev = &sensor->i2c_client->dev;
+	struct v4l2_mbus_framefmt *format;
+	const struct ox05b1s_mode *mode;
+
+	/* if no matching mbus code is found, use the one from the default mode */
+	fmt->format.code = ox05b1s_find_code(sensor->model, fmt->format.code);
+	mode = ox05b1s_nearest_size(sensor->model->supported_modes, fmt);
+
+	fmt->format.width = mode->width;
+	fmt->format.height = mode->height;
+	fmt->format.field = V4L2_FIELD_NONE;
+
+	format = v4l2_subdev_state_get_format(state, 0);
+	*format = fmt->format;
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
+		return 0;
+
+	sensor->mode = mode;
+
+	/* update controls that depend on current mode */
+	ox05b1s_update_controls(sensor);
+
+	dev_dbg(dev, "Set mode index=%d, %d x %d, code=0x%x\n", sensor->mode->index,
+		fmt->format.width, fmt->format.height, fmt->format.code);
+
+	return 0;
+}
+
+static u8 ox05b1s_code2dt(const u32 code)
+{
+	switch (code) {
+	case MEDIA_BUS_FMT_SGRBG10_1X10:
+		return MIPI_CSI2_DT_RAW10;
+	default:
+		return MIPI_CSI2_DT_RAW10;
+	}
+}
+
+static int ox05b1s_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				  struct v4l2_mbus_frame_desc *fd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+	fd->num_entries = 1;
+
+	/* get sensor current code*/
+	mutex_lock(&sensor->lock);
+	fd->entry[0].pixelcode = sensor->mode->code;
+	mutex_unlock(&sensor->lock);
+
+	fd->entry[0].bus.csi2.vc = 0;
+	fd->entry[0].bus.csi2.dt = ox05b1s_code2dt(fd->entry[0].pixelcode);
+
+	return 0;
+}
+
+static int ox05b1s_get_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *sd_state,
+				 struct v4l2_subdev_selection *sel)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = sensor->model->native_width;
+		sel->r.height = sensor->model->native_height;
+		return 0;
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = sensor->model->active_top;
+		sel->r.left = sensor->model->active_left;
+		sel->r.width = sensor->model->active_width;
+		sel->r.height = sensor->model->active_height;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct v4l2_subdev_video_ops ox05b1s_subdev_video_ops = {
+	.s_stream = ox05b1s_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops ox05b1s_subdev_pad_ops = {
+	.set_fmt		= ox05b1s_set_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
+	.get_frame_desc		= ox05b1s_get_frame_desc,
+	.enum_mbus_code		= ox05b1s_enum_mbus_code,
+	.enum_frame_size	= ox05b1s_enum_frame_size,
+	.get_selection		= ox05b1s_get_selection,
+};
+
+static const struct v4l2_subdev_ops ox05b1s_subdev_ops = {
+	.video = &ox05b1s_subdev_video_ops,
+	.pad   = &ox05b1s_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ox05b1s_internal_ops = {
+	.init_state = ox05b1s_init_state,
+};
+
+static void ox05b1s_get_gpios(struct ox05b1s *sensor)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+
+	sensor->rst_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->rst_gpio))
+		dev_warn(dev, "No sensor reset pin available");
+}
+
+static int ox05b1s_get_regulators(struct ox05b1s *sensor)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+	unsigned int i;
+
+	for (i = 0; i < OX05B1S_NUM_SUPPLIES; i++)
+		sensor->supplies[i].supply = ox05b1s_supply_name[i];
+
+	return devm_regulator_bulk_get(dev, OX05B1S_NUM_SUPPLIES, sensor->supplies);
+}
+
+static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
+{
+	struct device *dev = &sensor->i2c_client->dev;
+	u64 chip_id = 0;
+	char *camera_name;
+	int ret = 0;
+
+	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);
+	if (ret) {
+		dev_err(dev, "Camera chip_id read error\n");
+		return -ENODEV;
+	}
+
+	switch (chip_id) {
+	case 0x580542:
+		camera_name = "ox05b1s";
+		break;
+	default:
+		camera_name = "unknown";
+		break;
+	}
+
+	if (chip_id == sensor->model->chip_id) {
+		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n", camera_name, chip_id);
+	} else {
+		dev_err(dev, "Detected %s camera (chip_id=%llx), but expected %s (chip_id=%x)\n",
+			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
+		ret = -ENODEV;
+	}
+
+	return ret;
+}
+
+static int ox05b1s_probe(struct i2c_client *client)
+{
+	int retval;
+	struct device *dev = &client->dev;
+	struct v4l2_subdev *sd;
+	struct ox05b1s *sensor;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
+	if (IS_ERR(sensor->regmap))
+		return PTR_ERR(sensor->regmap);
+
+	sensor->i2c_client = client;
+
+	sensor->model = of_device_get_match_data(dev);
+
+	ox05b1s_get_gpios(sensor);
+
+	/* Get system clock, xvclk */
+	sensor->sensor_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(sensor->sensor_clk))
+		return dev_err_probe(dev, PTR_ERR(sensor->sensor_clk),
+				     "Failed to get xvclk\n");
+
+	retval = ox05b1s_get_regulators(sensor);
+	if (retval)
+		return dev_err_probe(dev, retval, "Failed to get regulators\n");
+
+	sd = &sensor->subdev;
+	v4l2_i2c_subdev_init(sd, client, &ox05b1s_subdev_ops);
+	sd->internal_ops = &ox05b1s_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->dev = &client->dev;
+	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	sensor->pads[OX05B1S_SENS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	retval = media_entity_pads_init(&sd->entity, OX05B1S_SENS_PADS_NUM,
+					sensor->pads);
+	if (retval)
+		goto probe_out;
+
+	mutex_init(&sensor->lock);
+
+	retval = ox05b1s_init_controls(sensor);
+	if (retval)
+		goto probe_err_entity_cleanup;
+
+	/* power on manually */
+	retval = ox05b1s_power_on(sensor);
+	if (retval) {
+		dev_err(dev, "Failed to power on\n");
+		goto probe_err_free_ctrls;
+	}
+
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+
+	retval = ox05b1s_read_chip_id(sensor);
+	if (retval)
+		goto probe_err_pm_runtime;
+
+	v4l2_i2c_subdev_set_name(sd, client, sensor->model->name, NULL);
+
+	/* Centrally managed subdev active state */
+	sd->state_lock = &sensor->lock;
+	retval = v4l2_subdev_init_finalize(sd);
+	if (retval < 0) {
+		dev_err(dev, "Subdev init error: %d\n", retval);
+		goto probe_err_pm_runtime;
+	}
+
+	retval = v4l2_async_register_subdev_sensor(sd);
+	if (retval < 0) {
+		dev_err(&client->dev, "Async register failed, ret=%d\n", retval);
+		goto probe_err_subdev_cleanup;
+	}
+
+	sensor->mode = &sensor->model->supported_modes[0];
+	ox05b1s_update_controls(sensor);
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+
+probe_err_subdev_cleanup:
+	v4l2_subdev_cleanup(sd);
+probe_err_pm_runtime:
+	pm_runtime_put_noidle(dev);
+	pm_runtime_disable(dev);
+	ox05b1s_runtime_suspend(dev);
+probe_err_free_ctrls:
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+probe_err_entity_cleanup:
+	media_entity_cleanup(&sd->entity);
+probe_out:
+	return retval;
+}
+
+static void ox05b1s_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ox05b1s *sensor = client_to_ox05b1s(client);
+	struct device *dev = &client->dev;
+
+	pm_runtime_disable(dev);
+	if (!pm_runtime_status_suspended(dev))
+		ox05b1s_runtime_suspend(dev);
+	pm_runtime_set_suspended(dev);
+	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+	mutex_destroy(&sensor->lock);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ox05b1s_pm_ops, ox05b1s_runtime_suspend,
+				 ox05b1s_runtime_resume, NULL);
+
+static const struct ox05b1s_plat_data ox05b1s_data = {
+	.name			= "ox05b1s",
+	.chip_id		= 0x580542,
+	.native_width		= 2608, /* 8 dummy + 2592 active pixels + 8 dummy */
+	.native_height		= 1960, /* 8 dummy + 1944 active lines + 8 dummy */
+	.active_top		= 8,
+	.active_left		= 8,
+	.active_width		= 2592,
+	.active_height		= 1944,
+	.supported_modes	= ox05b1s_supported_modes,
+	.default_mode_index	= 0,
+	.supported_codes	= ox05b1s_supported_codes,
+};
+
+static const struct of_device_id ox05b1s_of_match[] = {
+	{
+		.compatible = "ovti,ox05b1s",
+		.data = &ox05b1s_data,
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ox05b1s_of_match);
+
+static struct i2c_driver ox05b1s_i2c_driver = {
+	.driver = {
+		.name  = "ox05b1s",
+		.pm = pm_ptr(&ox05b1s_pm_ops),
+		.of_match_table	= ox05b1s_of_match,
+	},
+	.probe	= ox05b1s_probe,
+	.remove = ox05b1s_remove,
+};
+
+module_i2c_driver(ox05b1s_i2c_driver);
+MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
+MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@nxp.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
new file mode 100644
index 000000000000..1f3b822d4482
--- /dev/null
+++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Register configurations for all sensor supported modes
+ * Copyright (C) 2024, NXP
+ * Copyright (C) 2024, Omnivision
+ * Copyright (C) 2024, Verisilicon
+ *
+ */
+
+#include "ox05b1s.h"
+
+/*
+ * Register configuration for Omnivision OX05B1S raw camera
+ * 2592X1944_30FPS_FULL_RGBIr 2592 1944
+ */
+struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {
+	{ 0x0107, 0x01 },
+	{ 0x0307, 0x02 },
+	{ 0x034a, 0x05 },
+	{ 0x040b, 0x5c },
+	{ 0x040c, 0xcd },
+	{ 0x3009, 0x2e },
+	{ 0x3219, 0x08 },
+	{ 0x3684, 0x6d },
+	{ 0x3685, 0x6d },
+	{ 0x3686, 0x6d },
+	{ 0x3687, 0x6d },
+	{ 0x368c, 0x07 },
+	{ 0x368d, 0x07 },
+	{ 0x368e, 0x07 },
+	{ 0x368f, 0x00 },
+	{ 0x3690, 0x04 },
+	{ 0x3691, 0x04 },
+	{ 0x3692, 0x04 },
+	{ 0x3693, 0x04 },
+	{ 0x3698, 0x00 },
+	{ 0x36a0, 0x05 },
+	{ 0x36a2, 0x16 },
+	{ 0x36a3, 0x03 },
+	{ 0x36a4, 0x07 },
+	{ 0x36a5, 0x24 },
+	{ 0x36e3, 0x09 },
+	{ 0x3702, 0x0a },
+	{ 0x3821, 0x04 }, /* mirror */
+	{ 0x3822, 0x10 },
+	{ 0x382b, 0x03 },
+	{ 0x3866, 0x10 },
+	{ 0x386c, 0x46 },
+	{ 0x386d, 0x08 },
+	{ 0x386e, 0x7b },
+	{ 0x4802, 0x00 },
+	{ 0x481b, 0x3c },
+	{ 0x4837, 0x19 },
+	{ /* sentinel*/ },
+};
+
+struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {
+	{
+		.regs = ovx5b_init_setting_2592x1944
+	}, {
+		/* sentinel */
+	}
+};
-- 
2.25.1

[PATCH v3 3/4] MAINTAINERS: Add entry for OX05B1S sensor driver

[Mirela Rabulea]

Add maintainer for Omnivision OX05B1S sensor driver.

Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---

Changes in v3:
	None

Changes in v2:
	None

 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6ff0565d874f..fca5f3f2f2bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17570,6 +17570,16 @@ S:	Maintained
 T:	git git://linuxtv.org/media.git
 F:	drivers/media/i2c/ov9734.c
 
+OMNIVISION OX05B1S SENSOR DRIVER
+M:	Mirela Rabulea <mirela.rabulea@nxp.com>
+R:	Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
+R:	Robert Chiras <robert.chiras@oss.nxp.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ox05b1s.yaml
+F:	drivers/media/i2c/ox05b1s/*
+
 ONBOARD USB HUB DRIVER
 M:	Matthias Kaehlcke <mka@chromium.org>
 L:	linux-usb@vger.kernel.org
-- 
2.25.1

[PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Mirela Rabulea]

This is an 8 megapixel raw10/raw12 sensor with HDR capabilities.
HDR mode control is supported, with one HDR mode: staggered HDR
with 2 exposures on separate virtual channels. However, for now,
only one exposure (VC 0) is accessible via get_frame_desc.

Supported resolutions:
   - 1920 x 1080 @ 60fps (SBGGR10, no HDR)
   - 1920 x 1080 @ 30fps (SBGGR10, HDR)
   - 3840 x 2160 @ 30fps (SBGGR12, no HDR)
   - 3840 x 2160 @ 15fps (SBGGR12, HDR)
   - 3840 x 2160 @ 30fps (SBGGR10, no HDR)
   - 3840 x 2160 @ 15fps (SBGGR10, HDR)

Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
---

Changes in v3:
	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
	Use const for os08a20_supported_modes	
	os08a20 register config cleaning (remove all registers that were at their default value, and more, keep only what seems mandatory to be able to stream)
	Let the 4k 10bit mode by default without hdr, all 3 modes are now by default without hdr, staggered hdr may be enabled via v4l2-ctl for any of them.
	Separate the 10/12 bit register settings into separate lists: os08a20_init_setting_10bit, os08a20_init_setting_12bit
	Update commit description: rearrange supported resolutions and remove 1 duplicate line, state HDR limitation
	Increase a bit the default vts for 1080p, to get exactly 60fps, it was 62.61
	Use regmap_update_bits() directly and remove ox05b1s_regmap_update_bits()

Changes in v2:
	Add spaces inside brackets, wrap lines to 80
	Remove some redundant initialization
	Use a loop in os08a20_enable_staggered_hdr/os08a20_disable_staggered_hdr, for that, add a register settings array for HDR enabling/disabling
	Make "sizes" a pointer
	Remove mode headers, add supported modes in the dedicated c file, ox05b1s_modes.c 
	Refactor register lists, for os08a20 use a common list for all modes, and also specific lists per mode

 drivers/media/i2c/ox05b1s/ox05b1s.h       |   4 +
 drivers/media/i2c/ox05b1s/ox05b1s_mipi.c  | 184 ++++++++++++++++++++++
 drivers/media/i2c/ox05b1s/ox05b1s_modes.c |  96 +++++++++++
 3 files changed, 284 insertions(+)

diff --git a/drivers/media/i2c/ox05b1s/ox05b1s.h b/drivers/media/i2c/ox05b1s/ox05b1s.h
index a893c65894f3..5115060e23cb 100644
--- a/drivers/media/i2c/ox05b1s/ox05b1s.h
+++ b/drivers/media/i2c/ox05b1s/ox05b1s.h
@@ -17,6 +17,10 @@ struct ox05b1s_reglist {
 	struct ox05b1s_reg *regs;
 };
 
+extern struct ox05b1s_reglist os08a20_reglist_4k_10b[];
+extern struct ox05b1s_reglist os08a20_reglist_4k_12b[];
+extern struct ox05b1s_reglist os08a20_reglist_1080p_10b[];
+
 extern struct ox05b1s_reglist ox05b1s_reglist_2592x1944[];
 
 #endif /* OX05B1S_H */
diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
index aeea7caa6a15..7dcba5235926 100644
--- a/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
+++ b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
@@ -40,6 +40,7 @@ struct ox05b1s_sizes {
 	const struct v4l2_area *sizes;
 };
 
+struct ox05b1s;
 struct ox05b1s_plat_data {
 	char				name[20];
 	u32				chip_id;
@@ -52,6 +53,9 @@ struct ox05b1s_plat_data {
 	const struct ox05b1s_mode	*supported_modes;
 	u32				default_mode_index;
 	const struct ox05b1s_sizes	*supported_codes;
+	const char * const		*hdr_modes;
+	u32				hdr_modes_count;
+	int (*set_hdr_mode)(struct ox05b1s *sensor, u32 hdr_mode);
 };
 
 struct ox05b1s_ctrls {
@@ -62,6 +66,7 @@ struct ox05b1s_ctrls {
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *gain;
 	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *hdr_mode;
 };
 
 struct ox05b1s_mode {
@@ -102,6 +107,85 @@ struct ox05b1s {
 	struct ox05b1s_ctrls ctrls;
 };
 
+static const struct ox05b1s_mode os08a20_supported_modes[] = {
+	{
+		/* 1080p BGGR10, no hdr, 60fps */
+		.index		= 0,
+		.width		= 1920,
+		.height		= 1080,
+		.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
+		.bpp		= 10,
+		.vts		= 0x4d3,
+		.hts		= 0x790,
+		.exp		= 0x4d3 - 8,
+		.h_bin		= true,
+		.fps		= 60,
+		.reg_data	= os08a20_reglist_1080p_10b,
+	}, {
+		/* 4k BGGR10, no hdr, 30fps */
+		.index		= 1,
+		.width		= 3840,
+		.height		= 2160,
+		.code		= MEDIA_BUS_FMT_SBGGR10_1X10,
+		.bpp		= 10,
+		.vts		= 0x90a,
+		.hts		= 0x818,
+		.exp		= 0x90a - 8,
+		.h_bin		= false,
+		.fps		= 30,
+		.reg_data	= os08a20_reglist_4k_10b,
+	}, {
+		/* 4k BGGR12, no hdr, 30fps */
+		.index		= 2,
+		.width		= 3840,
+		.height		= 2160,
+		.code		= MEDIA_BUS_FMT_SBGGR12_1X12,
+		.bpp		= 12,
+		.vts		= 0x8f0,
+		.hts		= 0x814,
+		.exp		= 0x8f0 - 8,
+		.h_bin		= false,
+		.fps		= 30,
+		.reg_data	= os08a20_reglist_4k_12b,
+	}, {
+		/* sentinel */
+	}
+};
+
+/* keep in sync with os08a20_supported_modes */
+static const struct v4l2_area os08a20_sbggr10_sizes[] = {
+	{
+		.width = 1920,
+		.height = 1080,
+	}, {
+		.width = 3840,
+		.height = 2160,
+	}, {
+		/* sentinel */
+	}
+};
+
+static const struct v4l2_area os08a20_sbggr12_sizes[] = {
+	{
+		.width = 3840,
+		.height = 2160,
+	}, {
+		/* sentinel */
+	}
+};
+
+static const struct ox05b1s_sizes os08a20_supported_codes[] = {
+	{
+		.code = MEDIA_BUS_FMT_SBGGR10_1X10,
+		.sizes = os08a20_sbggr10_sizes,
+	}, {
+		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
+		.sizes = os08a20_sbggr12_sizes,
+	}, {
+		/* sentinel */
+	}
+};
+
 static const struct ox05b1s_mode ox05b1s_supported_modes[] = {
 	{
 		.index		= 0,
@@ -228,6 +312,62 @@ static int ox05b1s_write_reg_array(struct ox05b1s *sensor,
 	return 0;
 }
 
+static const char * const os08a20_hdr_modes[] = {
+	"NO HDR",		/* No HDR, single exposure */
+	"HDR Staggered",	/* Staggered HDR mode, 2 exposures on separate virtual channels */
+};
+
+static struct ox05b1s_reg os08a20_init_setting_hdr_en[] = {
+	{ 0x3661, BIT(0) }, /* CORE1[0] STG_HDR_ALIGN_EN */
+	{ 0x3821, BIT(5) }, /* FORMAT2[5] STG_HDR_EN */
+	{ 0x4813, BIT(3) }, /* MIPI_CTRL_13[3] */
+	{ 0x486e, BIT(2) }, /* MIPI_CTRL_6E[2] MIPI_VC_ENABLE */
+};
+
+static int os08a20_enable_staggered_hdr(struct ox05b1s *sensor)
+{
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(os08a20_init_setting_hdr_en); i++) {
+		ret = regmap_update_bits(sensor->regmap,
+					 os08a20_init_setting_hdr_en[i].addr,
+					 os08a20_init_setting_hdr_en[i].data,
+					 os08a20_init_setting_hdr_en[i].data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int os08a20_disable_staggered_hdr(struct ox05b1s *sensor)
+{
+	int ret;
+
+	for (int i = 0; i < ARRAY_SIZE(os08a20_init_setting_hdr_en); i++) {
+		ret = regmap_update_bits(sensor->regmap,
+					 os08a20_init_setting_hdr_en[i].addr,
+					 os08a20_init_setting_hdr_en[i].data,
+					 0);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int os08a20_set_hdr_mode(struct ox05b1s *sensor, u32 hdr_mode)
+{
+	switch (hdr_mode) {
+	case 0:
+		return os08a20_disable_staggered_hdr(sensor);
+	case 1:
+		return os08a20_enable_staggered_hdr(sensor);
+	default:
+		return -EINVAL;
+	}
+}
+
 static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 {
 	return &container_of(ctrl->handler, struct ox05b1s,
@@ -270,6 +410,12 @@ static int ox05b1s_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_write(sensor->regmap, OX05B1S_REG_EXPOSURE,
 				ctrl->val, NULL);
 		break;
+	case V4L2_CID_HDR_SENSOR_MODE:
+		if (sensor->model->set_hdr_mode)
+			ret = sensor->model->set_hdr_mode(sensor, ctrl->val);
+		else
+			ret = -EINVAL;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -332,6 +478,13 @@ static int ox05b1s_init_controls(struct ox05b1s *sensor)
 	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
 					0, 0xFFFF, 1, 0x80);
 
+	if (sensor->model->hdr_modes)
+		ctrls->hdr_mode = v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_HDR_SENSOR_MODE,
+							       sensor->model->hdr_modes_count - 1,
+								0, 0, sensor->model->hdr_modes);
+	else
+		ctrls->hdr_mode = NULL;
+
 	if (hdl->error) {
 		ret = hdl->error;
 		goto free_ctrls;
@@ -648,7 +801,10 @@ static u8 ox05b1s_code2dt(const u32 code)
 {
 	switch (code) {
 	case MEDIA_BUS_FMT_SGRBG10_1X10:
+	case MEDIA_BUS_FMT_SBGGR10_1X10:
 		return MIPI_CSI2_DT_RAW10;
+	case MEDIA_BUS_FMT_SBGGR12_1X12:
+		return MIPI_CSI2_DT_RAW12;
 	default:
 		return MIPI_CSI2_DT_RAW10;
 	}
@@ -758,6 +914,9 @@ static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
 	}
 
 	switch (chip_id) {
+	case 0x530841:
+		camera_name = "os08a20";
+		break;
 	case 0x580542:
 		camera_name = "ox05b1s";
 		break;
@@ -900,6 +1059,24 @@ static void ox05b1s_remove(struct i2c_client *client)
 static DEFINE_RUNTIME_DEV_PM_OPS(ox05b1s_pm_ops, ox05b1s_runtime_suspend,
 				 ox05b1s_runtime_resume, NULL);
 
+static const struct ox05b1s_plat_data os08a20_data = {
+	.name			= "os08a20",
+	.chip_id		= 0x530841,
+	.native_width		= 3872, /* 16 dummy + 3840 active pixels + 16 dummy */
+	.native_height		= 2192, /* 16 dummy + 2160 active lines + 16 dummy */
+	.active_top		= 16,
+	.active_left		= 16,
+	.active_width		= 3840,
+	.active_height		= 2160,
+	.supported_modes	= os08a20_supported_modes,
+	.default_mode_index	= 0,
+	.supported_codes	= os08a20_supported_codes,
+	.hdr_modes		= os08a20_hdr_modes,
+	.hdr_modes_count	= ARRAY_SIZE(os08a20_hdr_modes),
+	.set_hdr_mode		= os08a20_set_hdr_mode,
+
+};
+
 static const struct ox05b1s_plat_data ox05b1s_data = {
 	.name			= "ox05b1s",
 	.chip_id		= 0x580542,
@@ -912,9 +1089,16 @@ static const struct ox05b1s_plat_data ox05b1s_data = {
 	.supported_modes	= ox05b1s_supported_modes,
 	.default_mode_index	= 0,
 	.supported_codes	= ox05b1s_supported_codes,
+	.hdr_modes		= NULL,
+	.hdr_modes_count	= 0,
+	.set_hdr_mode		= NULL,
 };
 
 static const struct of_device_id ox05b1s_of_match[] = {
+	{
+		.compatible = "ovti,os08a20",
+		.data = &os08a20_data,
+	},
 	{
 		.compatible = "ovti,ox05b1s",
 		.data = &ox05b1s_data,
diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
index 1f3b822d4482..9cfc55f04a70 100644
--- a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
+++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
@@ -9,6 +9,102 @@
 
 #include "ox05b1s.h"
 
+/* Common register configuration for Omnivision OS08A20 raw camera */
+struct ox05b1s_reg os08a20_init_setting_common[] = {
+	{ 0x3605, 0x50 },
+	{ 0x3610, 0x39 },
+	{ 0x375e, 0x0b },
+	{ 0x5001, 0x42 },
+	{ 0x5005, 0x00 },
+	{ /* sentinel*/ },
+};
+
+/* Common register configuration for Omnivision OS08A20 10 bit */
+struct ox05b1s_reg os08a20_init_setting_10bit[] = {
+	{ 0x031e, 0x09 },
+	{ 0x3609, 0xb5 },
+	{ 0x3660, 0x43 },
+	{ 0x3706, 0x35 },
+	{ 0x370a, 0x00 },
+	{ 0x370b, 0x98 },
+	{ 0x3709, 0x49 },
+	{ /* sentinel*/ },
+};
+
+/* Common register configuration for Omnivision OS08A20 12 bit */
+struct ox05b1s_reg os08a20_init_setting_12bit[] = {
+	{ 0x031e, 0x0a },
+	{ 0x3609, 0xdb },
+	{ 0x3660, 0xd3 },
+	{ 0x3706, 0x6a },
+	{ 0x370a, 0x01 },
+	{ 0x370b, 0x30 },
+	{ 0x3709, 0x48 },
+	{ /* sentinel*/ },
+};
+
+/* Mode specific register configurations for Omnivision OS08A20 raw camera */
+
+/* OS08A20 3840 x 2160 @30fps BGGR10 no more HDR */
+struct ox05b1s_reg os08a20_init_setting_4k_10b[] = {
+	{ 0x3821, 0x04 }, /* mirror */
+	{ 0x4837, 0x10 }, /* PCLK PERIOD */
+	{ /* sentinel*/ },
+};
+
+/* OS08A20 3840 x 2160 @30fps BGGR12 */
+struct ox05b1s_reg os08a20_init_setting_4k_12b[] = {
+	{ 0x3821, 0x04 }, /* mirror */
+	{ 0x4837, 0x10 }, /* PCLK PERIOD */
+	{ /* sentinel*/ },
+};
+
+/* OS08A20 1920 x 1080 @60fps BGGR10 */
+struct ox05b1s_reg os08a20_init_setting_1080p_10b[] = {
+	{ 0x3814, 0x03 }, /* X INC ODD */
+	{ 0x3816, 0x03 }, /* Y INC ODD */
+	{ 0x3820, 0x01 }, /* vertical bining */
+	{ 0x3821, 0x05 }, /* mirror, horizontal bining */
+	{ 0x4837, 0x16 }, /* PCLK PERIOD */
+	{ /* sentinel*/ },
+};
+
+struct ox05b1s_reglist os08a20_reglist_4k_10b[] = {
+	{
+		.regs = os08a20_init_setting_common
+	}, {
+		.regs = os08a20_init_setting_10bit
+	}, {
+		.regs = os08a20_init_setting_4k_10b
+	}, {
+		/* sentinel */
+	}
+};
+
+struct ox05b1s_reglist os08a20_reglist_4k_12b[] = {
+	{
+		.regs = os08a20_init_setting_common
+	}, {
+		.regs = os08a20_init_setting_12bit
+	}, {
+		.regs = os08a20_init_setting_4k_12b
+	}, {
+		/* sentinel */
+	}
+};
+
+struct ox05b1s_reglist os08a20_reglist_1080p_10b[] = {
+	{
+		.regs = os08a20_init_setting_common
+	}, {
+		.regs = os08a20_init_setting_10bit
+	}, {
+		.regs = os08a20_init_setting_1080p_10b
+	}, {
+		/* sentinel */
+	}
+};
+
 /*
  * Register configuration for Omnivision OX05B1S raw camera
  * 2592X1944_30FPS_FULL_RGBIr 2592 1944
-- 
2.25.1

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

[Christophe JAILLET]

Le 24/01/2025 à 01:12, Mirela Rabulea a écrit :
> 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@nxp.com>
> ---

...

> +static int ox05b1s_power_on(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	int ret = 0;

No need to init.

> +
> +	ret = regulator_bulk_enable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulators\n");
> +		return ret;
> +	}
> +
> +	/* get out of powerdown and reset */
> +	gpiod_set_value_cansleep(sensor->rst_gpio, 0);
> +
> +	ret = clk_prepare_enable(sensor->sensor_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "Enable sensor clk fail ret=%d\n", ret);
> +		goto reg_off;
> +	}
> +
> +	/* with XVCLK@24MHz, t2 = 6ms required delay for ox05b1s before first SCCB transaction */
> +	fsleep(6000);
> +
> +	return ret;

return 0;

> +
> +reg_off:
> +	regulator_bulk_disable(OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +
> +	return ret;
> +}

...

> +/* needs sensor lock and power on */
> +static int ox05b1s_apply_current_mode(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	struct ox05b1s_reglist *reg_data = sensor->mode->reg_data;
> +	u32 w = sensor->mode->width;
> +	u32 h = sensor->mode->height;
> +	int ret = 0;
> +
> +	cci_write(sensor->regmap, OX05B1S_REG_SW_RST, 0x01, NULL);
> +
> +	while (reg_data->regs) {
> +		ret = ox05b1s_write_reg_array(sensor, reg_data->regs);
> +		if (ret)
> +			goto out;
> +		reg_data++;
> +	}
> +
> +	cci_write(sensor->regmap, OX05B1S_REG_X_OUTPUT_SIZE, w, &ret);
> +	cci_write(sensor->regmap, OX05B1S_REG_Y_OUTPUT_SIZE, h, &ret);
> +	if (ret)
> +		goto out;
> +
> +	/* setup handler will write actual controls into sensor registers */
> +	ret =  __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);

2 spaces after '='

> +
> +out:
> +	if (ret < 0)
> +		dev_err(dev, "Failed to apply mode %dx%d,bpp=%d\n", w, h,
> +			sensor->mode->bpp);
> +
> +	return ret;
> +}

...

> +static int ox05b1s_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ox05b1s *sensor = client_to_ox05b1s(client);
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->num_entries = 1;
> +
> +	/* get sensor current code*/

Missing space before */

> +	mutex_lock(&sensor->lock);
> +	fd->entry[0].pixelcode = sensor->mode->code;
> +	mutex_unlock(&sensor->lock);
> +
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = ox05b1s_code2dt(fd->entry[0].pixelcode);
> +
> +	return 0;
> +}

...

> +static void ox05b1s_get_gpios(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +
> +	sensor->rst_gpio = devm_gpiod_get_optional(dev, "reset",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(sensor->rst_gpio))
> +		dev_warn(dev, "No sensor reset pin available");

Missing ending \n

> +}
> +
> +static int ox05b1s_get_regulators(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	unsigned int i;
> +
> +	for (i = 0; i < OX05B1S_NUM_SUPPLIES; i++)
> +		sensor->supplies[i].supply = ox05b1s_supply_name[i];
> +
> +	return devm_regulator_bulk_get(dev, OX05B1S_NUM_SUPPLIES, sensor->supplies);
> +}
> +
> +static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	u64 chip_id = 0;
> +	char *camera_name;
> +	int ret = 0;

No need to init.

> +
> +	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);
> +	if (ret) {
> +		dev_err(dev, "Camera chip_id read error\n");
> +		return -ENODEV;
> +	}
> +
> +	switch (chip_id) {
> +	case 0x580542:
> +		camera_name = "ox05b1s";
> +		break;
> +	default:
> +		camera_name = "unknown";
> +		break;
> +	}
> +
> +	if (chip_id == sensor->model->chip_id) {
> +		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n", camera_name, chip_id);
> +	} else {
> +		dev_err(dev, "Detected %s camera (chip_id=%llx), but expected %s (chip_id=%x)\n",
> +			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
> +		ret = -ENODEV;

Could be return -ENODEV;
and return 0; below, to ease reading.

> +	}
> +
> +	return ret;
> +}
> +
> +static int ox05b1s_probe(struct i2c_client *client)
> +{
> +	int retval;

Using just 'ret' would be slighly shorter and more consistent with other 
functions above.

> +	struct device *dev = &client->dev;
> +	struct v4l2_subdev *sd;
> +	struct ox05b1s *sensor;
> +
> +	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> +	if (!sensor)
> +		return -ENOMEM;
> +
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return PTR_ERR(sensor->regmap);
> +
> +	sensor->i2c_client = client;
> +
> +	sensor->model = of_device_get_match_data(dev);
> +
> +	ox05b1s_get_gpios(sensor);
> +
> +	/* Get system clock, xvclk */
> +	sensor->sensor_clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(sensor->sensor_clk))
> +		return dev_err_probe(dev, PTR_ERR(sensor->sensor_clk),
> +				     "Failed to get xvclk\n");
> +
> +	retval = ox05b1s_get_regulators(sensor);
> +	if (retval)
> +		return dev_err_probe(dev, retval, "Failed to get regulators\n");
> +
> +	sd = &sensor->subdev;
> +	v4l2_i2c_subdev_init(sd, client, &ox05b1s_subdev_ops);
> +	sd->internal_ops = &ox05b1s_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->dev = &client->dev;
> +	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +	sensor->pads[OX05B1S_SENS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +	retval = media_entity_pads_init(&sd->entity, OX05B1S_SENS_PADS_NUM,
> +					sensor->pads);
> +	if (retval)
> +		goto probe_out;
> +
> +	mutex_init(&sensor->lock);

This could be devm_mutex_init().
This would slighlty simplify the remove function.
Otherwise, a mutex_destroy() is potentialy missing in the error handling 
of the probe.

> +
> +	retval = ox05b1s_init_controls(sensor);
> +	if (retval)
> +		goto probe_err_entity_cleanup;
> +
> +	/* power on manually */
> +	retval = ox05b1s_power_on(sensor);
> +	if (retval) {
> +		dev_err(dev, "Failed to power on\n");

dev_err_probe() can still be used to be consistent with other error path 
above.

> +		goto probe_err_free_ctrls;
> +	}
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
> +
> +	retval = ox05b1s_read_chip_id(sensor);
> +	if (retval)
> +		goto probe_err_pm_runtime;
> +
> +	v4l2_i2c_subdev_set_name(sd, client, sensor->model->name, NULL);
> +
> +	/* Centrally managed subdev active state */
> +	sd->state_lock = &sensor->lock;
> +	retval = v4l2_subdev_init_finalize(sd);
> +	if (retval < 0) {
> +		dev_err(dev, "Subdev init error: %d\n", retval);

Same

> +		goto probe_err_pm_runtime;
> +	}
> +
> +	retval = v4l2_async_register_subdev_sensor(sd);
> +	if (retval < 0) {
> +		dev_err(&client->dev, "Async register failed, ret=%d\n", retval);

Same

> +		goto probe_err_subdev_cleanup;
> +	}
> +
> +	sensor->mode = &sensor->model->supported_modes[0];
> +	ox05b1s_update_controls(sensor);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +probe_err_subdev_cleanup:
> +	v4l2_subdev_cleanup(sd);
> +probe_err_pm_runtime:
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_disable(dev);
> +	ox05b1s_runtime_suspend(dev);
> +probe_err_free_ctrls:
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +probe_err_entity_cleanup:
> +	media_entity_cleanup(&sd->entity);
> +probe_out:
> +	return retval;
> +}

...

> +static struct i2c_driver ox05b1s_i2c_driver = {
> +	.driver = {
> +		.name  = "ox05b1s",

2 spaces after .name.

> +		.pm = pm_ptr(&ox05b1s_pm_ops),
> +		.of_match_table	= ox05b1s_of_match,
> +	},
> +	.probe	= ox05b1s_probe,
> +	.remove = ox05b1s_remove,
> +};
> +
> +module_i2c_driver(ox05b1s_i2c_driver);
> +MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
> +MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@nxp.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> new file mode 100644
> index 000000000000..1f3b822d4482
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Register configurations for all sensor supported modes
> + * Copyright (C) 2024, NXP
> + * Copyright (C) 2024, Omnivision
> + * Copyright (C) 2024, Verisilicon
> + *
> + */
> +
> +#include "ox05b1s.h"
> +
> +/*
> + * Register configuration for Omnivision OX05B1S raw camera
> + * 2592X1944_30FPS_FULL_RGBIr 2592 1944
> + */
> +struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {

I think this could easily be made a const struct.

> +	{ 0x0107, 0x01 },
> +	{ 0x0307, 0x02 },
> +	{ 0x034a, 0x05 },
> +	{ 0x040b, 0x5c },
> +	{ 0x040c, 0xcd },
> +	{ 0x3009, 0x2e },
> +	{ 0x3219, 0x08 },
> +	{ 0x3684, 0x6d },
> +	{ 0x3685, 0x6d },
> +	{ 0x3686, 0x6d },
> +	{ 0x3687, 0x6d },
> +	{ 0x368c, 0x07 },
> +	{ 0x368d, 0x07 },
> +	{ 0x368e, 0x07 },
> +	{ 0x368f, 0x00 },
> +	{ 0x3690, 0x04 },
> +	{ 0x3691, 0x04 },
> +	{ 0x3692, 0x04 },
> +	{ 0x3693, 0x04 },
> +	{ 0x3698, 0x00 },
> +	{ 0x36a0, 0x05 },
> +	{ 0x36a2, 0x16 },
> +	{ 0x36a3, 0x03 },
> +	{ 0x36a4, 0x07 },
> +	{ 0x36a5, 0x24 },
> +	{ 0x36e3, 0x09 },
> +	{ 0x3702, 0x0a },
> +	{ 0x3821, 0x04 }, /* mirror */
> +	{ 0x3822, 0x10 },
> +	{ 0x382b, 0x03 },
> +	{ 0x3866, 0x10 },
> +	{ 0x386c, 0x46 },
> +	{ 0x386d, 0x08 },
> +	{ 0x386e, 0x7b },
> +	{ 0x4802, 0x00 },
> +	{ 0x481b, 0x3c },
> +	{ 0x4837, 0x19 },
> +	{ /* sentinel*/ },

Nitpick: no need for ending comma after a terminator.

> +};
> +
> +struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {

I think this could easily be made a const struct.

> +	{
> +		.regs = ovx5b_init_setting_2592x1944
> +	}, {
> +		/* sentinel */
> +	}
> +};

Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Christophe JAILLET]

Le 24/01/2025 à 01:12, Mirela Rabulea a écrit :
> This is an 8 megapixel raw10/raw12 sensor with HDR capabilities.
> HDR mode control is supported, with one HDR mode: staggered HDR
> with 2 exposures on separate virtual channels. However, for now,
> only one exposure (VC 0) is accessible via get_frame_desc.
> 
> Supported resolutions:
>     - 1920 x 1080 @ 60fps (SBGGR10, no HDR)
>     - 1920 x 1080 @ 30fps (SBGGR10, HDR)
>     - 3840 x 2160 @ 30fps (SBGGR12, no HDR)
>     - 3840 x 2160 @ 15fps (SBGGR12, HDR)
>     - 3840 x 2160 @ 30fps (SBGGR10, no HDR)
>     - 3840 x 2160 @ 15fps (SBGGR10, HDR)
> 
> Signed-off-by: Mirela Rabulea <mirela.rabulea-3arQi8VN3Tc@public.gmane.org>

...

> +/* Common register configuration for Omnivision OS08A20 raw camera */
> +struct ox05b1s_reg os08a20_init_setting_common[] = {
> +	{ 0x3605, 0x50 },
> +	{ 0x3610, 0x39 },
> +	{ 0x375e, 0x0b },
> +	{ 0x5001, 0x42 },
> +	{ 0x5005, 0x00 },
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
> +
> +/* Common register configuration for Omnivision OS08A20 10 bit */
> +struct ox05b1s_reg os08a20_init_setting_10bit[] = {
> +	{ 0x031e, 0x09 },
> +	{ 0x3609, 0xb5 },
> +	{ 0x3660, 0x43 },
> +	{ 0x3706, 0x35 },
> +	{ 0x370a, 0x00 },
> +	{ 0x370b, 0x98 },
> +	{ 0x3709, 0x49 },
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
> +
> +/* Common register configuration for Omnivision OS08A20 12 bit */
> +struct ox05b1s_reg os08a20_init_setting_12bit[] = {
> +	{ 0x031e, 0x0a },
> +	{ 0x3609, 0xdb },
> +	{ 0x3660, 0xd3 },
> +	{ 0x3706, 0x6a },
> +	{ 0x370a, 0x01 },
> +	{ 0x370b, 0x30 },
> +	{ 0x3709, 0x48 },
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
> +
> +/* Mode specific register configurations for Omnivision OS08A20 raw camera */
> +
> +/* OS08A20 3840 x 2160 @30fps BGGR10 no more HDR */
> +struct ox05b1s_reg os08a20_init_setting_4k_10b[] = {
> +	{ 0x3821, 0x04 }, /* mirror */
> +	{ 0x4837, 0x10 }, /* PCLK PERIOD */
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
> +
> +/* OS08A20 3840 x 2160 @30fps BGGR12 */
> +struct ox05b1s_reg os08a20_init_setting_4k_12b[] = {
> +	{ 0x3821, 0x04 }, /* mirror */
> +	{ 0x4837, 0x10 }, /* PCLK PERIOD */
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
> +
> +/* OS08A20 1920 x 1080 @60fps BGGR10 */
> +struct ox05b1s_reg os08a20_init_setting_1080p_10b[] = {
> +	{ 0x3814, 0x03 }, /* X INC ODD */
> +	{ 0x3816, 0x03 }, /* Y INC ODD */
> +	{ 0x3820, 0x01 }, /* vertical bining */
> +	{ 0x3821, 0x05 }, /* mirror, horizontal bining */
> +	{ 0x4837, 0x16 }, /* PCLK PERIOD */
> +	{ /* sentinel*/ },

No need for ending comma.
The struct could maybe be const.

> +};
 > +
 > +struct ox05b1s_reglist os08a20_reglist_4k_12b[] = {

The struct could maybe be const.

 > +	{
 > +		.regs = os08a20_init_setting_common
 > +	}, {
 > +		.regs = os08a20_init_setting_12bit
 > +	}, {
 > +		.regs = os08a20_init_setting_4k_12b
 > +	}, {
 > +		/* sentinel */
 > +	}
 > +};
 > +
 > +struct ox05b1s_reglist os08a20_reglist_1080p_10b[] = {

The struct could maybe be const.

 > +	{
 > +		.regs = os08a20_init_setting_common
 > +	}, {
 > +		.regs = os08a20_init_setting_10bit
 > +	}, {
 > +		.regs = os08a20_init_setting_1080p_10b
 > +	}, {
 > +		/* sentinel */
 > +	}
 > +};
 > +
 >   /*
 >    * Register configuration for Omnivision OX05B1S raw camera
 >    * 2592X1944_30FPS_FULL_RGBIr 2592 1944

CJ

Re: [PATCH v3 1/4] dt-bindings: media: i2c: Add OX05B1S sensor

[Krzysztof Kozlowski]

On Fri, Jan 24, 2025 at 02:12:40AM +0200, Mirela Rabulea wrote:
> Add bindings for Omnivision OX05B1S sensor.
> Also add compatible for Omnivision OS08A20 sensor.
> 
> Signed-off-by: Mirela Rabulea <mirela.rabulea@nxp.com>
> ---
> 
> Changes in v3:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, Jan 24, 2025 at 02:12:41AM +0200, Mirela Rabulea wrote:
> +
> +static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> +{
> +	struct device *dev = &sensor->i2c_client->dev;
> +	u64 chip_id = 0;
> +	char *camera_name;
> +	int ret = 0;
> +
> +	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);

This suggests these are compatible devices. But in the binding you said
they are not... so your binding is not correct. Express compatibility
with proper fallback.

> +	if (ret) {
> +		dev_err(dev, "Camera chip_id read error\n");
> +		return -ENODEV;
> +	}
> +
> +	switch (chip_id) {
> +	case 0x580542:
> +		camera_name = "ox05b1s";
> +		break;
> +	default:
> +		camera_name = "unknown";
> +		break;
> +	}
> +
> +	if (chip_id == sensor->model->chip_id) {
> +		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n", camera_name, chip_id);
> +	} else {
> +		dev_err(dev, "Detected %s camera (chip_id=%llx), but expected %s (chip_id=%x)\n",
> +			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
> +		ret = -ENODEV;
> +	}
> +
> +	return ret;
> +}

...

> +
> +static const struct of_device_id ox05b1s_of_match[] = {
> +	{
> +		.compatible = "ovti,ox05b1s",

And this is incomplete, according to current binding, so it seems you
wanted to make them compatible just did not write the binding like that?

Confusing.

> +		.data = &ox05b1s_data,
> +	},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ox05b1s_of_match);
> +
> +static struct i2c_driver ox05b1s_i2c_driver = {
> +	.driver = {
> +		.name  = "ox05b1s",
> +		.pm = pm_ptr(&ox05b1s_pm_ops),
> +		.of_match_table	= ox05b1s_of_match,
> +	},
> +	.probe	= ox05b1s_probe,
> +	.remove = ox05b1s_remove,
> +};
> +
> +module_i2c_driver(ox05b1s_i2c_driver);
> +MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
> +MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@nxp.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> new file mode 100644
> index 000000000000..1f3b822d4482
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Register configurations for all sensor supported modes
> + * Copyright (C) 2024, NXP
> + * Copyright (C) 2024, Omnivision
> + * Copyright (C) 2024, Verisilicon
> + *
> + */
> +
> +#include "ox05b1s.h"
> +
> +/*
> + * Register configuration for Omnivision OX05B1S raw camera
> + * 2592X1944_30FPS_FULL_RGBIr 2592 1944
> + */
> +struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {

Why this is not const? I commented last time with the same.

> +	{ 0x0107, 0x01 },
> +	{ 0x0307, 0x02 },
> +	{ 0x034a, 0x05 },
> +	{ 0x040b, 0x5c },
> +	{ 0x040c, 0xcd },
> +	{ 0x3009, 0x2e },
> +	{ 0x3219, 0x08 },
> +	{ 0x3684, 0x6d },
> +	{ 0x3685, 0x6d },
> +	{ 0x3686, 0x6d },
> +	{ 0x3687, 0x6d },
> +	{ 0x368c, 0x07 },
> +	{ 0x368d, 0x07 },
> +	{ 0x368e, 0x07 },
> +	{ 0x368f, 0x00 },
> +	{ 0x3690, 0x04 },
> +	{ 0x3691, 0x04 },
> +	{ 0x3692, 0x04 },
> +	{ 0x3693, 0x04 },
> +	{ 0x3698, 0x00 },
> +	{ 0x36a0, 0x05 },
> +	{ 0x36a2, 0x16 },
> +	{ 0x36a3, 0x03 },
> +	{ 0x36a4, 0x07 },
> +	{ 0x36a5, 0x24 },
> +	{ 0x36e3, 0x09 },
> +	{ 0x3702, 0x0a },
> +	{ 0x3821, 0x04 }, /* mirror */
> +	{ 0x3822, 0x10 },
> +	{ 0x382b, 0x03 },
> +	{ 0x3866, 0x10 },
> +	{ 0x386c, 0x46 },
> +	{ 0x386d, 0x08 },
> +	{ 0x386e, 0x7b },
> +	{ 0x4802, 0x00 },
> +	{ 0x481b, 0x3c },
> +	{ 0x4837, 0x19 },
> +	{ /* sentinel*/ },
> +};
> +
> +struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {

Why not const?

Best regards,
Krzysztof

Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Krzysztof Kozlowski]

On Fri, Jan 24, 2025 at 02:12:43AM +0200, Mirela Rabulea wrote:
> @@ -758,6 +914,9 @@ static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
>  	}
>  
>  	switch (chip_id) {
> +	case 0x530841:
> +		camera_name = "os08a20";
> +		break;

Ah, so here I see missing second device support.

It is still confusing to see that you use here some sort of
autodetection, but in the same time not.


>  	case 0x580542:
>  		camera_name = "ox05b1s";
>  		break;
> @@ -900,6 +1059,24 @@ static void ox05b1s_remove(struct i2c_client *client)
>  static DEFINE_RUNTIME_DEV_PM_OPS(ox05b1s_pm_ops, ox05b1s_runtime_suspend,
>  				 ox05b1s_runtime_resume, NULL);
>  
> +static const struct ox05b1s_plat_data os08a20_data = {
> +	.name			= "os08a20",
> +	.chip_id		= 0x530841,
> +	.native_width		= 3872, /* 16 dummy + 3840 active pixels + 16 dummy */
> +	.native_height		= 2192, /* 16 dummy + 2160 active lines + 16 dummy */
> +	.active_top		= 16,
> +	.active_left		= 16,
> +	.active_width		= 3840,
> +	.active_height		= 2160,
> +	.supported_modes	= os08a20_supported_modes,
> +	.default_mode_index	= 0,
> +	.supported_codes	= os08a20_supported_codes,
> +	.hdr_modes		= os08a20_hdr_modes,
> +	.hdr_modes_count	= ARRAY_SIZE(os08a20_hdr_modes),
> +	.set_hdr_mode		= os08a20_set_hdr_mode,
> +
> +};
> +
>  static const struct ox05b1s_plat_data ox05b1s_data = {
>  	.name			= "ox05b1s",
>  	.chip_id		= 0x580542,
> @@ -912,9 +1089,16 @@ static const struct ox05b1s_plat_data ox05b1s_data = {
>  	.supported_modes	= ox05b1s_supported_modes,
>  	.default_mode_index	= 0,
>  	.supported_codes	= ox05b1s_supported_codes,
> +	.hdr_modes		= NULL,
> +	.hdr_modes_count	= 0,
> +	.set_hdr_mode		= NULL,
>  };
>  
>  static const struct of_device_id ox05b1s_of_match[] = {
> +	{
> +		.compatible = "ovti,os08a20",
> +		.data = &os08a20_data,

And here static configuration of model, not autodetection.


Best regards,
Krzysztof

Re: Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Mirela Rabulea]

Hi Krzysztof,

thanks again for feedback.

On 24.01.2025 10:09, Krzysztof Kozlowski wrote:
> On Fri, Jan 24, 2025 at 02:12:43AM +0200, Mirela Rabulea wrote:
>> @@ -758,6 +914,9 @@ static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
>>        }
>>
>>        switch (chip_id) {
>> +     case 0x530841:
>> +             camera_name = "os08a20";
>> +             break;
> Ah, so here I see missing second device support.
>
> It is still confusing to see that you use here some sort of
> autodetection, but in the same time not.

The two sensors that I included in this driver have some similarities, 
but also differences, for which I used the platform data. I made 
separate patches for the two sensors, such that it is visible how much 
is common/different.  The chip_id reading is for validating that the 
sensor that is actually attached matches the device tree. It happens to 
me sometimes, that I switch the sensors, but forget to switch the dtb, 
and it helps to see which sensor is actually attached. Also, it helps a 
lot when the evaluation board is in some remote lab and I am unsure what 
sensor is attached to it.

I saw most sensor drivers have some kind of identification of the sensor 
module by the means of reading the chip_id register. Some examples with 
multiple compatibles supported and chip_id validation: imx296, ov9650, 
ov772x.

Please let me know what you suggest further.

For your other comments, I did prepare more structures to become const, 
hope I did not miss any, it will be in the next version.

Thanks,

Mirela

>
>
>>        case 0x580542:
>>                camera_name = "ox05b1s";
>>                break;
>> @@ -900,6 +1059,24 @@ static void ox05b1s_remove(struct i2c_client *client)
>>   static DEFINE_RUNTIME_DEV_PM_OPS(ox05b1s_pm_ops, ox05b1s_runtime_suspend,
>>                                 ox05b1s_runtime_resume, NULL);
>>
>> +static const struct ox05b1s_plat_data os08a20_data = {
>> +     .name                   = "os08a20",
>> +     .chip_id                = 0x530841,
>> +     .native_width           = 3872, /* 16 dummy + 3840 active pixels + 16 dummy */
>> +     .native_height          = 2192, /* 16 dummy + 2160 active lines + 16 dummy */
>> +     .active_top             = 16,
>> +     .active_left            = 16,
>> +     .active_width           = 3840,
>> +     .active_height          = 2160,
>> +     .supported_modes        = os08a20_supported_modes,
>> +     .default_mode_index     = 0,
>> +     .supported_codes        = os08a20_supported_codes,
>> +     .hdr_modes              = os08a20_hdr_modes,
>> +     .hdr_modes_count        = ARRAY_SIZE(os08a20_hdr_modes),
>> +     .set_hdr_mode           = os08a20_set_hdr_mode,
>> +
>> +};
>> +
>>   static const struct ox05b1s_plat_data ox05b1s_data = {
>>        .name                   = "ox05b1s",
>>        .chip_id                = 0x580542,
>> @@ -912,9 +1089,16 @@ static const struct ox05b1s_plat_data ox05b1s_data = {
>>        .supported_modes        = ox05b1s_supported_modes,
>>        .default_mode_index     = 0,
>>        .supported_codes        = ox05b1s_supported_codes,
>> +     .hdr_modes              = NULL,
>> +     .hdr_modes_count        = 0,
>> +     .set_hdr_mode           = NULL,
>>   };
>>
>>   static const struct of_device_id ox05b1s_of_match[] = {
>> +     {
>> +             .compatible = "ovti,os08a20",
>> +             .data = &os08a20_data,
> And here static configuration of model, not autodetection.
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 4/4] media: ox05b1s: Add support for Omnivision OS08A20 raw sensor

[Krzysztof Kozlowski]

On 03/02/2025 09:43, Mirela Rabulea wrote:
> Hi Krzysztof,
> 
> thanks again for feedback.
> 
> On 24.01.2025 10:09, Krzysztof Kozlowski wrote:
>> On Fri, Jan 24, 2025 at 02:12:43AM +0200, Mirela Rabulea wrote:
>>> @@ -758,6 +914,9 @@ static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
>>>        }
>>>
>>>        switch (chip_id) {
>>> +     case 0x530841:
>>> +             camera_name = "os08a20";
>>> +             break;
>> Ah, so here I see missing second device support.
>>
>> It is still confusing to see that you use here some sort of
>> autodetection, but in the same time not.
> 
> The two sensors that I included in this driver have some similarities, 
> but also differences, for which I used the platform data. I made 
> separate patches for the two sensors, such that it is visible how much 
> is common/different.  The chip_id reading is for validating that the 
> sensor that is actually attached matches the device tree. It happens to 
> me sometimes, that I switch the sensors, but forget to switch the dtb, 
> and it helps to see which sensor is actually attached. Also, it helps a 
> lot when the evaluation board is in some remote lab and I am unsure what 
> sensor is attached to it.
> 
> I saw most sensor drivers have some kind of identification of the sensor 
> module by the means of reading the chip_id register. Some examples with 
> multiple compatibles supported and chip_id validation: imx296, ov9650, 
> ov772x.
> 
> Please let me know what you suggest further.


If devices are from the same family and support reliable autodetection,
they should be made compatible. At least that's generic approach.


Best regards,
Krzysztof

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

[Laurent Pinchart]

On Fri, Jan 24, 2025 at 09:07:01AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Jan 24, 2025 at 02:12:41AM +0200, Mirela Rabulea wrote:
> > +
> > +static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> > +{
> > +	struct device *dev = &sensor->i2c_client->dev;
> > +	u64 chip_id = 0;
> > +	char *camera_name;
> > +	int ret = 0;
> > +
> > +	ret = cci_read(sensor->regmap, OX05B1S_REG_CHIP_ID, &chip_id, NULL);
> 
> This suggests these are compatible devices. But in the binding you said
> they are not... so your binding is not correct. Express compatibility
> with proper fallback.

Unfortunately we can't do that for camera sensors. It's important for
drivers to be able to identify the camera sensor model without having to
power up the device at probe time, depending on what device the sensor
is integrated in. For instance, with camera sensors found in laptops,
the privacy LED is often connected to the sensor power supply, and a
flashing privacy light when booting scares users.

Reading the ID register at probe time is fine when running on a platform
where the sensor can be powered up when probing, so the driver isn't
wrong doing so. It's also fine for drivers to not implement the no-power
probe right away. DT bindings however need to be ready for this, so a
fallback string can't be used.

> > +	if (ret) {
> > +		dev_err(dev, "Camera chip_id read error\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	switch (chip_id) {
> > +	case 0x580542:
> > +		camera_name = "ox05b1s";
> > +		break;
> > +	default:
> > +		camera_name = "unknown";
> > +		break;
> > +	}
> > +
> > +	if (chip_id == sensor->model->chip_id) {
> > +		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n", camera_name, chip_id);
> > +	} else {
> > +		dev_err(dev, "Detected %s camera (chip_id=%llx), but expected %s (chip_id=%x)\n",
> > +			camera_name, chip_id, sensor->model->name, sensor->model->chip_id);
> > +		ret = -ENODEV;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +
> > +static const struct of_device_id ox05b1s_of_match[] = {
> > +	{
> > +		.compatible = "ovti,ox05b1s",
> 
> And this is incomplete, according to current binding, so it seems you
> wanted to make them compatible just did not write the binding like that?
> 
> Confusing.
> 
> > +		.data = &ox05b1s_data,
> > +	},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ox05b1s_of_match);
> > +
> > +static struct i2c_driver ox05b1s_i2c_driver = {
> > +	.driver = {
> > +		.name  = "ox05b1s",
> > +		.pm = pm_ptr(&ox05b1s_pm_ops),
> > +		.of_match_table	= ox05b1s_of_match,
> > +	},
> > +	.probe	= ox05b1s_probe,
> > +	.remove = ox05b1s_remove,
> > +};
> > +
> > +module_i2c_driver(ox05b1s_i2c_driver);
> > +MODULE_DESCRIPTION("Omnivision OX05B1S MIPI Camera Subdev Driver");
> > +MODULE_AUTHOR("Mirela Rabulea <mirela.rabulea@nxp.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_modes.c b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> > new file mode 100644
> > index 000000000000..1f3b822d4482
> > --- /dev/null
> > +++ b/drivers/media/i2c/ox05b1s/ox05b1s_modes.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Register configurations for all sensor supported modes
> > + * Copyright (C) 2024, NXP
> > + * Copyright (C) 2024, Omnivision
> > + * Copyright (C) 2024, Verisilicon
> > + *
> > + */
> > +
> > +#include "ox05b1s.h"
> > +
> > +/*
> > + * Register configuration for Omnivision OX05B1S raw camera
> > + * 2592X1944_30FPS_FULL_RGBIr 2592 1944
> > + */
> > +struct ox05b1s_reg ovx5b_init_setting_2592x1944[] = {
> 
> Why this is not const? I commented last time with the same.
> 
> > +	{ 0x0107, 0x01 },
> > +	{ 0x0307, 0x02 },
> > +	{ 0x034a, 0x05 },
> > +	{ 0x040b, 0x5c },
> > +	{ 0x040c, 0xcd },
> > +	{ 0x3009, 0x2e },
> > +	{ 0x3219, 0x08 },
> > +	{ 0x3684, 0x6d },
> > +	{ 0x3685, 0x6d },
> > +	{ 0x3686, 0x6d },
> > +	{ 0x3687, 0x6d },
> > +	{ 0x368c, 0x07 },
> > +	{ 0x368d, 0x07 },
> > +	{ 0x368e, 0x07 },
> > +	{ 0x368f, 0x00 },
> > +	{ 0x3690, 0x04 },
> > +	{ 0x3691, 0x04 },
> > +	{ 0x3692, 0x04 },
> > +	{ 0x3693, 0x04 },
> > +	{ 0x3698, 0x00 },
> > +	{ 0x36a0, 0x05 },
> > +	{ 0x36a2, 0x16 },
> > +	{ 0x36a3, 0x03 },
> > +	{ 0x36a4, 0x07 },
> > +	{ 0x36a5, 0x24 },
> > +	{ 0x36e3, 0x09 },
> > +	{ 0x3702, 0x0a },
> > +	{ 0x3821, 0x04 }, /* mirror */
> > +	{ 0x3822, 0x10 },
> > +	{ 0x382b, 0x03 },
> > +	{ 0x3866, 0x10 },
> > +	{ 0x386c, 0x46 },
> > +	{ 0x386d, 0x08 },
> > +	{ 0x386e, 0x7b },
> > +	{ 0x4802, 0x00 },
> > +	{ 0x481b, 0x3c },
> > +	{ 0x4837, 0x19 },
> > +	{ /* sentinel*/ },
> > +};
> > +
> > +struct ox05b1s_reglist ox05b1s_reglist_2592x1944[] = {
> 
> Why not const?
> 
> Best regards,
> Krzysztof
> 

-- 
Regards,

Laurent Pinchart

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

[Markus Elfring]

…
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
> @@ -0,0 +1,939 @@
…
> +static int ox05b1s_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				  struct v4l2_mbus_frame_desc *fd)
> +{
…
> +	/* get sensor current code*/
> +	mutex_lock(&sensor->lock);
> +	fd->entry[0].pixelcode = sensor->mode->code;
> +	mutex_unlock(&sensor->lock);
…

Under which circumstances would you become interested to apply a statement
like “guard(mutex)(&sensor->lock);”?
https://elixir.bootlin.com/linux/v6.13.1/source/include/linux/mutex.h#L201

Regards,
Markus