* [PATCH v3 0/2] media: i2c: Add ov2735 camera sensor driver
@ 2025-07-10 13:10 Hardevsinh Palaniya
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
0 siblings, 2 replies; 11+ messages in thread
From: Hardevsinh Palaniya @ 2025-07-10 13:10 UTC (permalink / raw)
To: sakari.ailus, krzk+dt, kieran.bingham
Cc: dave.stevenson, pratap.nirujogi, laurent.pinchart, tarang.raval,
Hardevsinh Palaniya, Himanshu Bhavani, Mauro Carvalho Chehab,
Rob Herring, Conor Dooley, Hans Verkuil, Hans de Goede,
Bryan O'Donoghue, André Apitzsch, Sylvain Petinot,
Arnd Bergmann, Benjamin Mugnier, Dongcheng Yan, Jingjing Xiong,
Andy Shevchenko, Matthias Fend, Heimir Thor Sverrisson,
linux-media, devicetree, linux-kernel
Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
active array size of 1920 x 1080.
The following features are supported:
- Manual exposure an gain control support.
- vblank/hblank control support.
- Test pattern support control.
- Supported resolution: 1920 x 1080 @ 30fps (SGRBG10).
The driver is tested on mainline branch v6.14-rc6 on IMX8MP Debix Model a.
v2 -> v3
In Patch 1/2:
- Renamed pwdn pin to enable pin.
- Changed supply names to lowercase and added them to required properties.
In Patch 2/2:
- Stored page number in CCI private bits.
- Added helper functions to handle page switching in cci_read() and cci_write().
- Removed ov2735_mbus_codes.
- Corrected control count to 9.
v1 -> v2
- Added necessary header files.
- Corrected indentation.
- Used the ret parameter in cci_write and cci_read functions.
Hardevsinh Palaniya (1):
media: i2c: add ov2735 image sensor driver
Himanshu Bhavani (1):
dt-bindings: media: i2c: Add ov2735 sensor
.../bindings/media/i2c/ovti,ov2735.yaml | 115 +++
MAINTAINERS | 9 +
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/ov2735.c | 962 ++++++++++++++++++
5 files changed, 1097 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
create mode 100644 drivers/media/i2c/ov2735.c
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 13:10 [PATCH v3 0/2] media: i2c: Add ov2735 camera sensor driver Hardevsinh Palaniya
@ 2025-07-10 13:10 ` Hardevsinh Palaniya
2025-07-10 13:33 ` Krzysztof Kozlowski
` (2 more replies)
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
1 sibling, 3 replies; 11+ messages in thread
From: Hardevsinh Palaniya @ 2025-07-10 13:10 UTC (permalink / raw)
To: sakari.ailus, krzk+dt, kieran.bingham
Cc: dave.stevenson, pratap.nirujogi, laurent.pinchart, tarang.raval,
Himanshu Bhavani, Hardevsinh Palaniya, Mauro Carvalho Chehab,
Rob Herring, Conor Dooley, Hans Verkuil, Bryan O'Donoghue,
Ricardo Ribalda, André Apitzsch, Arnd Bergmann,
Dongcheng Yan, Jingjing Xiong, Matthias Fend, Benjamin Mugnier,
Sylvain Petinot, Heimir Thor Sverrisson, Andy Shevchenko,
Hans de Goede, linux-media, devicetree, linux-kernel
From: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Add bindings for Omnivision OV2735 sensor.
Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
.../bindings/media/i2c/ovti,ov2735.yaml | 115 ++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
new file mode 100644
index 000000000000..d9d01db88844
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov2735.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OV2735 Image Sensor
+
+maintainers:
+ - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
+
+description: |
+ The OmniVision OV2735 is a 2MP (1920x1080) color CMOS image sensor controlled
+ through an I2C-compatible SCCB bus. it outputs RAW10 format and uses a 1/2.7"
+ optical format.
+
+properties:
+ compatible:
+ const: ovti,ov2735
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: XVCLK clock
+
+ clock-names:
+ items:
+ - const: xvclk
+
+ avdd-supply:
+ description: Analog Domain Power Supply
+
+ dovdd-supply:
+ description: I/O Domain Power Supply
+
+ dvdd-supply:
+ description: Digital Domain Power Supply
+
+ reset-gpios:
+ maxItems: 1
+ description: Reset Pin GPIO Control (active low)
+
+ enable-gpios:
+ maxItems: 1
+ description: |
+ Active-low enable pin. Labeled as 'PWDN' in the datasheet, but acts as
+ an enable signal. During power rail ramp-up, the device remains powered
+ down. Once power rails are stable, pulling this pin low powers on the
+ device.
+
+ port:
+ description: MIPI CSI-2 transmitter port
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ additionalProperties: false
+
+ properties:
+ endpoint:
+ $ref: /schemas/media/video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ data-lanes:
+ items:
+ - const: 1
+ - const: 2
+
+ required:
+ - data-lanes
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - avdd-supply
+ - dovdd-supply
+ - dvdd-supply
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/rk3399-cru.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ camera-sensor@3c {
+ compatible = "ovti,ov2735";
+ reg = <0x3c>;
+ clocks = <&ov2735_clk>;
+
+ assigned-clocks = <&ov2735_clk>;
+ assigned-clock-parents = <&ov2735_clk_parent>;
+ assigned-clock-rates = <24000000>;
+
+ avdd-supply = <&ov2735_avdd>;
+ dovdd-supply = <&ov2735_dovdd>;
+ dvdd-supply = <&ov2735_dvdd>;
+
+ reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+ enable-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;
+
+ port {
+ cam_out: endpoint {
+ remote-endpoint = <&mipi_in_cam>;
+ data-lanes = <1 2>;
+ };
+ };
+ };
+ };
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
2025-07-10 13:10 [PATCH v3 0/2] media: i2c: Add ov2735 camera sensor driver Hardevsinh Palaniya
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
@ 2025-07-10 13:10 ` Hardevsinh Palaniya
2025-07-10 13:46 ` Andy Shevchenko
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Hardevsinh Palaniya @ 2025-07-10 13:10 UTC (permalink / raw)
To: sakari.ailus, krzk+dt, kieran.bingham
Cc: dave.stevenson, pratap.nirujogi, laurent.pinchart, tarang.raval,
Hardevsinh Palaniya, Himanshu Bhavani, Mauro Carvalho Chehab,
Rob Herring, Conor Dooley, Hans Verkuil, Ricardo Ribalda,
Bryan O'Donoghue, André Apitzsch, Arnd Bergmann,
Matthias Fend, Sylvain Petinot, Dongcheng Yan, Benjamin Mugnier,
Heimir Thor Sverrisson, Jingjing Xiong, Andy Shevchenko,
Hans de Goede, Hao Yao, linux-media, devicetree, linux-kernel
Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
active array size of 1920 x 1080.
The following features are supported:
- Manual exposure an gain control support
- vblank/hblank control support
- Test pattern support control
- Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
Co-developed-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
---
MAINTAINERS | 9 +
drivers/media/i2c/Kconfig | 10 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/ov2735.c | 962 +++++++++++++++++++++++++++++++++++++
4 files changed, 982 insertions(+)
create mode 100644 drivers/media/i2c/ov2735.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 92e9d8c7708f..058bbfd9523b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18471,6 +18471,15 @@ T: git git://linuxtv.org/media.git
F: Documentation/devicetree/bindings/media/i2c/ovti,ov2685.yaml
F: drivers/media/i2c/ov2685.c
+OMNIVISION OV2735 SENSOR DRIVER
+M: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
+M: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
+L: linux-media@vger.kernel.org
+S: Maintained
+T: git git://linuxtv.org/media.git
+F: Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
+F: drivers/media/i2c/ov2735.c
+
OMNIVISION OV2740 SENSOR DRIVER
M: Tianshu Qiu <tian.shu.qiu@intel.com>
R: Sakari Ailus <sakari.ailus@linux.intel.com>
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 4b4c199da6ea..9646eab1b177 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -446,6 +446,16 @@ config VIDEO_OV2685
To compile this driver as a module, choose M here: the
module will be called ov2685.
+config VIDEO_OV2735
+ tristate "OmniVision OV2735 sensor support"
+ select V4L2_CCI_I2C
+ help
+ This is a Video4Linux2 sensor driver for the Sony
+ OV2735 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov2735.
+
config VIDEO_OV2740
tristate "OmniVision OV2740 sensor support"
depends on ACPI || COMPILE_TEST
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5873d29433ee..1adb27743fa1 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
+obj-$(CONFIG_VIDEO_OV2735) += ov2735.o
obj-$(CONFIG_VIDEO_OV2740) += ov2740.o
obj-$(CONFIG_VIDEO_OV4689) += ov4689.o
obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
diff --git a/drivers/media/i2c/ov2735.c b/drivers/media/i2c/ov2735.c
new file mode 100644
index 000000000000..9cfa34aa0966
--- /dev/null
+++ b/drivers/media/i2c/ov2735.c
@@ -0,0 +1,962 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * V4L2 Support for the OV2735
+ *
+ * Copyright (C) 2025 Silicon Signals Pvt. Ltd.
+ *
+ * Based on Rockchip ov2735 Camera Driver
+ * Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd.
+ *
+ * Inspired from ov8858, imx219, imx283 camera drivers
+ */
+
+#include <linux/array_size.h>
+#include <linux/clk.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device/devres.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+#include <linux/types.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mediabus.h>
+
+#define OV2735_XCLK_FREQ (24 * HZ_PER_MHZ)
+
+/* Add page number in CCI private bits [31:28] of the register address */
+#define CCI_PAGE_REG8(x, p) ((p << CCI_REG_PRIVATE_SHIFT) | CCI_REG8(x))
+#define CCI_PAGE_REG16(x, p) ((p << CCI_REG_PRIVATE_SHIFT) | CCI_REG16(x))
+
+#define OV2735_REG_PAGE_SELECT CCI_REG8(0xfd)
+
+/* Page 0 */
+#define OV2735_REG_CHIPID CCI_PAGE_REG16(0x02, 0x00)
+#define OV2735_CHIPID 0x2735
+
+#define OV2735_REG_SOFT_REST CCI_PAGE_REG8(0x20, 0x00)
+
+/* Clock Settings */
+#define OV2735_REG_PLL_CTRL CCI_PAGE_REG8(0x2f, 0x00)
+#define OV2735_REG_PLL_OUTDIV CCI_PAGE_REG8(0x34, 0x00)
+#define OV2735_REG_CLK_MODE CCI_PAGE_REG8(0x30, 0x00)
+#define OV2735_REG_CLOCK_REG1 CCI_PAGE_REG8(0x33, 0x00)
+#define OV2735_REG_CLOCK_REG2 CCI_PAGE_REG8(0x35, 0x00)
+
+/* Page 1 */
+#define OV2735_REG_STREAM_CTRL CCI_PAGE_REG8(0xa0, 0x01)
+#define OV2735_STREAM_ON 0x01
+#define OV2735_STREAM_OFF 0x00
+
+#define OV2735_REG_UPDOWN_MIRROR CCI_PAGE_REG8(0x3f, 0x01)
+#define OV2735_REG_BINNING_DAC_CODE_MODE CCI_PAGE_REG8(0x30, 0x01)
+#define OV2735_REG_FRAME_LENGTH CCI_PAGE_REG16(0x0e, 0x01)
+#define OV2735_VTS_MAX 0x0fff
+#define OV2735_REG_FRAME_EXP_SEPERATE_EN CCI_PAGE_REG8(0x0d, 0x01)
+#define OV2735_FRAME_EXP_SEPERATE_EN 0x10
+#define OV2735_REG_FRAME_SYNC CCI_PAGE_REG8(0x01, 0x01)
+
+#define OV2735_REG_HBLANK CCI_PAGE_REG16(0x09, 0x01)
+
+#define OV2735_REG_HS_MIPI CCI_PAGE_REG8(0xb1, 0x01)
+#define OV2735_REG_MIPI_CTRL1 CCI_PAGE_REG8(0x92, 0x01)
+#define OV2735_REG_MIPI_CTRL2 CCI_PAGE_REG8(0x94, 0x01)
+#define OV2735_REG_MIPI_CTRL3 CCI_PAGE_REG8(0xa1, 0x01)
+#define OV2735_REG_MIPI_CTRL4 CCI_PAGE_REG8(0xb2, 0x01)
+#define OV2735_REG_MIPI_CTRL5 CCI_PAGE_REG8(0xb3, 0x01)
+#define OV2735_REG_MIPI_CTRL6 CCI_PAGE_REG8(0xb4, 0x01)
+#define OV2735_REG_MIPI_CTRL7 CCI_PAGE_REG8(0xb5, 0x01)
+#define OV2735_REG_PREPARE CCI_PAGE_REG8(0x95, 0x01)
+#define OV2735_REG_R_HS_ZERO CCI_PAGE_REG8(0x96, 0x01)
+#define OV2735_REG_TRAIL CCI_PAGE_REG8(0x98, 0x01)
+#define OV2735_REG_R_CLK_ZERO CCI_PAGE_REG8(0x9c, 0x01)
+#define OV2735_REG_MIPI_V_SIZE CCI_PAGE_REG16(0x8e, 0x01)
+#define OV2735_REG_MIPI_H_SIZE CCI_PAGE_REG16(0x90, 0x01)
+
+#define OV2735_REG_ANALOG_CTRL3 CCI_PAGE_REG8(0x25, 0x01)
+#define OV2735_REG_VNCP CCI_PAGE_REG8(0x20, 0x01)
+
+/* BLC registers */
+#define OV2735_REG_BLC_GAIN_BLUE CCI_PAGE_REG8(0x86, 0x01)
+#define OV2735_REG_BLC_GAIN_RED CCI_PAGE_REG8(0x87, 0x01)
+#define OV2735_REG_BLC_GAIN_GR CCI_PAGE_REG8(0x88, 0x01)
+#define OV2735_REG_BLC_GAIN_GB CCI_PAGE_REG8(0x89, 0x01)
+#define OV2735_REG_GB_SUBOFFSET CCI_PAGE_REG8(0xf0, 0x01)
+#define OV2735_REG_BLUE_SUBOFFSET CCI_PAGE_REG8(0xf1, 0x01)
+#define OV2735_REG_RED_SUBOFFSET CCI_PAGE_REG8(0xf2, 0x01)
+#define OV2735_REG_GR_SUBOFFSET CCI_PAGE_REG8(0xf3, 0x01)
+#define OV2735_REG_BLC_BPC_TH_P CCI_PAGE_REG8(0xfc, 0x01)
+#define OV2735_REG_BLC_BPC_TH_N CCI_PAGE_REG8(0xfe, 0x01)
+
+#define OV2735_REG_TEST_PATTERN CCI_PAGE_REG8(0xb2, 0x01)
+#define OV2735_TEST_PATTERN_ENABLE 0x01
+#define OV2735_TEST_PATTERN_DISABLE 0xfe
+
+#define OV2735_REG_LONG_EXPOSURE CCI_PAGE_REG16(0x03, 0x01)
+#define OV2735_EXPOSURE_MIN 4
+#define OV2735_EXPOSURE_STEP 1
+
+#define OV2735_REG_ANALOG_GAIN CCI_PAGE_REG8(0x24, 0x01)
+#define OV2735_ANALOG_GAIN_MIN 0x10
+#define OV2735_ANALOG_GAIN_MAX 0xff
+#define OV2735_ANALOG_GAIN_STEP 1
+#define OV2735_ANALOG_GAIN_DEFAULT 0x10
+
+/* Page 2 */
+#define OV2735_REG_V_START CCI_PAGE_REG16(0xa0, 0x02)
+#define OV2735_REG_V_SIZE CCI_PAGE_REG16(0xa2, 0x02)
+#define OV2735_REG_H_START CCI_PAGE_REG16(0xa4, 0x02)
+#define OV2735_REG_H_SIZE CCI_PAGE_REG16(0xa6, 0x02)
+
+#define OV2735_LINK_FREQ_420MHZ (420 * HZ_PER_MHZ)
+#define OV2735_PIXEL_RATE (168 * HZ_PER_MHZ)
+
+static const char * const ov2735_supply_name[] = {
+ "avdd", /* Analog power */
+ "dovdd", /* Digital I/O power */
+ "dvdd", /* Digital core power */
+};
+
+struct ov2735 {
+ struct device *dev;
+ struct regmap *cci;
+ struct v4l2_subdev sd;
+ struct media_pad pad;
+ struct i2c_client *client;
+ struct clk *xclk;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *enable_gpio;
+ struct regulator_bulk_data supplies[ARRAY_SIZE(ov2735_supply_name)];
+
+ /* V4L2 Controls */
+ 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 v4l2_ctrl *test_pattern;
+
+ u8 current_page;
+ struct mutex page_lock;
+};
+
+struct ov2735_reglist {
+ unsigned int num_regs;
+ const struct cci_reg_sequence *regvals;
+};
+
+struct ov2735_mode {
+ u32 width;
+ u32 height;
+ u32 hts_def;
+ u32 vts_def;
+ u32 exp_def;
+ struct ov2735_reglist reg_list;
+};
+
+static struct cci_reg_sequence ov2735_1920_1080_30fps[] = {
+ { OV2735_REG_PLL_CTRL, 0x10 },
+ { OV2735_REG_PLL_OUTDIV, 0x00 },
+ { OV2735_REG_CLK_MODE, 0x15 },
+ { OV2735_REG_CLOCK_REG1, 0x01 },
+ { OV2735_REG_CLOCK_REG2, 0x20 },
+ { OV2735_REG_BINNING_DAC_CODE_MODE, 0x00 },
+ { CCI_PAGE_REG8(0xfb, 0x01), 0x73 },
+ { OV2735_REG_FRAME_SYNC, 0x01 },
+
+ /* Timing ctrl */
+ { CCI_PAGE_REG8(0x1a, 0x01), 0x6b },
+ { CCI_PAGE_REG8(0x1c, 0x01), 0xea },
+ { CCI_PAGE_REG8(0x16, 0x01), 0x0c },
+ { CCI_PAGE_REG8(0x21, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x11, 0x01), 0x63 },
+ { CCI_PAGE_REG8(0x19, 0x01), 0xc3 },
+
+ /* Analog ctrl */
+ { CCI_PAGE_REG8(0x26, 0x01), 0x5a },
+ { CCI_PAGE_REG8(0x29, 0x01), 0x01 },
+ { CCI_PAGE_REG8(0x33, 0x01), 0x6f },
+ { CCI_PAGE_REG8(0x2a, 0x01), 0xd2 },
+ { CCI_PAGE_REG8(0x2c, 0x01), 0x40 },
+ { CCI_PAGE_REG8(0xd0, 0x01), 0x02 },
+ { CCI_PAGE_REG8(0xd1, 0x01), 0x01 },
+ { CCI_PAGE_REG8(0xd2, 0x01), 0x20 },
+ { CCI_PAGE_REG8(0xd3, 0x01), 0x04 },
+ { CCI_PAGE_REG8(0xd4, 0x01), 0x2a },
+ { CCI_PAGE_REG8(0x50, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x51, 0x01), 0x2c },
+ { CCI_PAGE_REG8(0x52, 0x01), 0x29 },
+ { CCI_PAGE_REG8(0x53, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x55, 0x01), 0x44 },
+ { CCI_PAGE_REG8(0x58, 0x01), 0x29 },
+ { CCI_PAGE_REG8(0x5a, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x5b, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x5d, 0x01), 0x00 },
+ { CCI_PAGE_REG8(0x64, 0x01), 0x2f },
+ { CCI_PAGE_REG8(0x66, 0x01), 0x62 },
+ { CCI_PAGE_REG8(0x68, 0x01), 0x5b },
+ { CCI_PAGE_REG8(0x75, 0x01), 0x46 },
+ { CCI_PAGE_REG8(0x76, 0x01), 0x36 },
+ { CCI_PAGE_REG8(0x77, 0x01), 0x4f },
+ { CCI_PAGE_REG8(0x78, 0x01), 0xef },
+ { CCI_PAGE_REG8(0x72, 0x01), 0xcf },
+ { CCI_PAGE_REG8(0x73, 0x01), 0x36 },
+ { CCI_PAGE_REG8(0x7d, 0x01), 0x0d },
+ { CCI_PAGE_REG8(0x7e, 0x01), 0x0d },
+ { CCI_PAGE_REG8(0x8a, 0x01), 0x77 },
+ { CCI_PAGE_REG8(0x8b, 0x01), 0x77 },
+
+ { OV2735_REG_HS_MIPI, 0x83 },
+ { OV2735_REG_MIPI_CTRL5, 0x0b },
+ { OV2735_REG_MIPI_CTRL6, 0x14 },
+ { CCI_PAGE_REG8(0x9d, 0x01), 0x40 },
+ { OV2735_REG_MIPI_CTRL3, 0x05 },
+ { OV2735_REG_MIPI_CTRL2, 0x44 },
+ { OV2735_REG_PREPARE, 0x33 },
+ { OV2735_REG_R_HS_ZERO, 0x1f },
+ { OV2735_REG_TRAIL, 0x45 },
+ { OV2735_REG_R_CLK_ZERO, 0x10 },
+ { OV2735_REG_MIPI_CTRL7, 0x70 },
+ { OV2735_REG_ANALOG_CTRL3, 0xe0 },
+ { OV2735_REG_VNCP, 0x7b },
+
+ /* BLC */
+ { OV2735_REG_BLC_GAIN_BLUE, 0x77 },
+ { OV2735_REG_BLC_GAIN_GB, 0x77 },
+ { OV2735_REG_BLC_GAIN_RED, 0x74 },
+ { OV2735_REG_BLC_GAIN_GR, 0x74 },
+ { OV2735_REG_BLC_BPC_TH_P, 0xe0 },
+ { OV2735_REG_BLC_BPC_TH_N, 0xe0 },
+ { OV2735_REG_GB_SUBOFFSET, 0x40 },
+ { OV2735_REG_BLUE_SUBOFFSET, 0x40 },
+ { OV2735_REG_RED_SUBOFFSET, 0x40 },
+ { OV2735_REG_GR_SUBOFFSET, 0x40 },
+
+ /* 1920x1080 */
+ { OV2735_REG_V_START, 0x0008 },
+ { OV2735_REG_V_SIZE, 0x0438 },
+ { OV2735_REG_H_START, 0x0008 },
+ { OV2735_REG_H_SIZE, 0x03c0 },
+ { OV2735_REG_MIPI_V_SIZE, 0x0780 },
+ { OV2735_REG_MIPI_H_SIZE, 0x0438 },
+};
+
+static const struct ov2735_mode supported_modes[] = {
+ {
+ .width = 1920,
+ .height = 1080,
+ .exp_def = 399,
+ .hts_def = 2200,
+ .vts_def = 2545,
+ .reg_list = {
+ .num_regs = ARRAY_SIZE(ov2735_1920_1080_30fps),
+ .regvals = ov2735_1920_1080_30fps,
+ },
+ },
+};
+
+static const s64 link_freq_menu_items[] = {
+ OV2735_LINK_FREQ_420MHZ,
+};
+
+static const char * const ov2735_test_pattern_menu[] = {
+ "Disabled",
+ "Vertical Color",
+};
+
+static int ov2735_cci_access(struct ov2735 *ov2735,
+ u32 reg, void *val, int *err, bool is_read)
+{
+ u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
+ u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
+ int ret = 0;
+
+ if (err && *err)
+ return *err;
+
+ mutex_lock(&ov2735->page_lock);
+
+ /* Perform page access before read/write */
+ if (ov2735->current_page != page) {
+ ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, &ret);
+ if (ret)
+ goto err_mutex_unlock;
+ ov2735->current_page = page;
+ }
+
+ if (is_read)
+ cci_read(ov2735->cci, addr, (u64 *)val, &ret);
+ else
+ cci_write(ov2735->cci, addr, *(u64 *)val, &ret);
+
+ if (!ret && err)
+ *err = ret;
+
+err_mutex_unlock:
+ mutex_unlock(&ov2735->page_lock);
+ return ret;
+}
+
+static int ov2735_cci_read(struct ov2735 *ov2735, u32 reg, u64 *val, int *err)
+{
+ return ov2735_cci_access(ov2735, reg, val, err, true);
+}
+
+static int ov2735_cci_write(struct ov2735 *ov2735, u32 reg, u64 val, int *err)
+{
+ return ov2735_cci_access(ov2735, reg, (void *)&val, err, false);
+}
+
+static int ov2735_cci_multi_reg_write(struct ov2735 *ov2735,
+ const struct cci_reg_sequence *regs,
+ unsigned int num_regs, int *err)
+{
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < num_regs; i++) {
+ ret = ov2735_cci_write(ov2735, regs[i].reg, regs[i].val, err);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static inline struct ov2735 *to_ov2735(struct v4l2_subdev *_sd)
+{
+ return container_of(_sd, struct ov2735, sd);
+}
+
+static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
+{
+ int ret = 0;
+ u64 val;
+
+ ov2735_cci_read(ov2735, OV2735_REG_TEST_PATTERN, &val, &ret);
+ if (ret)
+ return ret;
+
+ switch (pattern) {
+ case 0:
+ val &= ~OV2735_TEST_PATTERN_ENABLE;
+ break;
+ case 1:
+ val |= OV2735_TEST_PATTERN_ENABLE;
+ break;
+ }
+
+ return ov2735_cci_write(ov2735, OV2735_REG_TEST_PATTERN, val, NULL);
+}
+
+static int ov2735_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct ov2735 *ov2735 = container_of(ctrl->handler, struct ov2735,
+ handler);
+ const struct ov2735_mode *mode;
+ struct v4l2_mbus_framefmt *fmt;
+ struct v4l2_subdev_state *state;
+ u64 vts;
+ int ret = 0;
+
+ state = v4l2_subdev_get_locked_active_state(&ov2735->sd);
+ fmt = v4l2_subdev_state_get_format(state, 0);
+
+ mode = v4l2_find_nearest_size(supported_modes,
+ ARRAY_SIZE(supported_modes),
+ width, height,
+ fmt->width, fmt->height);
+
+ if (ctrl->id == V4L2_CID_VBLANK) {
+ /* Honour the VBLANK limits when setting exposure. */
+ s64 max = mode->height + ctrl->val - 4;
+
+ ret = __v4l2_ctrl_modify_range(ov2735->exposure,
+ ov2735->exposure->minimum, max,
+ ov2735->exposure->step,
+ ov2735->exposure->default_value);
+ if (ret)
+ return ret;
+ }
+
+ /*
+ * Applying V4L2 control value only happens
+ * when power is up for streaming.
+ */
+ if (pm_runtime_get_if_in_use(ov2735->dev) == 0)
+ return 0;
+
+ switch (ctrl->id) {
+ case V4L2_CID_EXPOSURE:
+ ov2735_cci_write(ov2735, OV2735_REG_LONG_EXPOSURE, ctrl->val, &ret);
+ break;
+ case V4L2_CID_ANALOGUE_GAIN:
+ ov2735_cci_write(ov2735, OV2735_REG_ANALOG_GAIN, ctrl->val, &ret);
+ break;
+ case V4L2_CID_HBLANK:
+ ov2735_cci_write(ov2735, OV2735_REG_HBLANK, ctrl->val, &ret);
+ break;
+ case V4L2_CID_VBLANK:
+ vts = ctrl->val + mode->height;
+ ov2735_cci_write(ov2735, OV2735_REG_FRAME_EXP_SEPERATE_EN,
+ OV2735_FRAME_EXP_SEPERATE_EN, &ret);
+ ov2735_cci_write(ov2735, OV2735_REG_FRAME_LENGTH, vts, &ret);
+ break;
+ case V4L2_CID_TEST_PATTERN:
+ ret = ov2735_enable_test_pattern(ov2735, ctrl->val);
+ break;
+ default:
+ dev_err(ov2735->dev, "ctrl(id:0x%x, val:0x%x) is not handled\n",
+ ctrl->id, ctrl->val);
+ break;
+ }
+ ov2735_cci_write(ov2735, OV2735_REG_FRAME_SYNC, 0x01, &ret);
+
+ pm_runtime_put(ov2735->dev);
+
+ return ret;
+}
+
+static const struct v4l2_ctrl_ops ov2735_ctrl_ops = {
+ .s_ctrl = ov2735_set_ctrl,
+};
+
+static int ov2735_init_controls(struct ov2735 *ov2735)
+{
+ struct v4l2_ctrl_handler *ctrl_hdlr;
+ struct v4l2_fwnode_device_properties props;
+ const struct ov2735_mode *mode = &supported_modes[0];
+ u64 hblank_def, vblank_def, exp_max;
+ int ret;
+
+ ctrl_hdlr = &ov2735->handler;
+ ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
+ if (ret)
+ return ret;
+
+ ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
+ V4L2_CID_PIXEL_RATE, 0, OV2735_PIXEL_RATE,
+ 1, OV2735_PIXEL_RATE);
+
+ ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ 0, 0, link_freq_menu_items);
+ if (ov2735->link_freq)
+ ov2735->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+ hblank_def = mode->hts_def - mode->width;
+ ov2735->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_HBLANK,
+ hblank_def, hblank_def, 1, hblank_def);
+
+ vblank_def = mode->vts_def - mode->height;
+ ov2735->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
+ V4L2_CID_VBLANK, vblank_def,
+ OV2735_VTS_MAX - mode->height,
+ 1, vblank_def);
+
+ exp_max = mode->vts_def - 4;
+ ov2735->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
+ V4L2_CID_EXPOSURE, OV2735_EXPOSURE_MIN,
+ exp_max, OV2735_EXPOSURE_STEP, mode->exp_def);
+
+ ov2735->gain = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
+ V4L2_CID_ANALOGUE_GAIN, OV2735_ANALOG_GAIN_MIN,
+ OV2735_ANALOG_GAIN_MAX, OV2735_ANALOG_GAIN_STEP,
+ OV2735_ANALOG_GAIN_DEFAULT);
+
+ ov2735->test_pattern = v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
+ &ov2735_ctrl_ops, V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(ov2735_test_pattern_menu) - 1,
+ 0, 0, ov2735_test_pattern_menu);
+ if (ctrl_hdlr->error) {
+ ret = ctrl_hdlr->error;
+ dev_err(ov2735->dev, "control init failed (%d)\n", ret);
+ goto err_handler_free;
+ }
+
+ ret = v4l2_fwnode_device_parse(ov2735->dev, &props);
+ if (ret)
+ goto err_handler_free;
+
+ ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2735_ctrl_ops,
+ &props);
+ if (ret)
+ goto err_handler_free;
+
+ ov2735->sd.ctrl_handler = ctrl_hdlr;
+
+ return 0;
+
+err_handler_free:
+ v4l2_ctrl_handler_free(ctrl_hdlr);
+
+ return ret;
+}
+
+static int ov2735_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct ov2735 *ov2735 = to_ov2735(sd);
+ const struct ov2735_mode *mode;
+ const struct ov2735_reglist *reg_list;
+ const struct v4l2_mbus_framefmt *fmt;
+ int ret;
+
+ fmt = v4l2_subdev_state_get_format(state, 0);
+ mode = v4l2_find_nearest_size(supported_modes,
+ ARRAY_SIZE(supported_modes),
+ width, height,
+ fmt->width, fmt->height);
+
+ ret = pm_runtime_resume_and_get(ov2735->dev);
+ if (ret < 0)
+ return ret;
+
+ reg_list = &mode->reg_list;
+ ov2735_cci_multi_reg_write(ov2735, reg_list->regvals, reg_list->num_regs, &ret);
+ if (ret) {
+ dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__);
+ goto err_rpm_put;
+ }
+
+ /* Apply customized values from user */
+ ret = __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler);
+ if (ret)
+ goto err_rpm_put;
+
+ /* set stream on register */
+ ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, &ret);
+ if (ret)
+ goto err_rpm_put;
+
+ return 0;
+
+err_rpm_put:
+ pm_runtime_put(ov2735->dev);
+ return ret;
+}
+
+static int ov2735_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct ov2735 *ov2735 = to_ov2735(sd);
+ int ret = 0;
+
+ /* set stream off register */
+ ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, &ret);
+ if (ret)
+ dev_err(ov2735->dev, "%s failed to set stream\n", __func__);
+
+ pm_runtime_put(ov2735->dev);
+
+ return ret;
+}
+
+static int ov2735_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->index >= 0)
+ return -EINVAL;
+
+ code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
+
+ return 0;
+}
+
+static int ov2735_enum_frame_size(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ u32 code;
+
+ if (fse->index >= ARRAY_SIZE(supported_modes))
+ return -EINVAL;
+
+ code = MEDIA_BUS_FMT_SGRBG10_1X10;
+ if (fse->code != code)
+ return -EINVAL;
+
+ fse->min_width = supported_modes[fse->index].width;
+ fse->max_width = fse->min_width;
+ fse->min_height = supported_modes[fse->index].height;
+ fse->max_height = fse->min_height;
+
+ return 0;
+}
+
+static void ov2735_set_framing_limits(struct ov2735 *ov2735,
+ const struct ov2735_mode *mode)
+{
+ u32 hblank, vblank_def;
+
+ hblank = mode->hts_def - mode->width;
+ __v4l2_ctrl_modify_range(ov2735->hblank, hblank, hblank, 1, hblank);
+
+ vblank_def = mode->vts_def - mode->height;
+ __v4l2_ctrl_modify_range(ov2735->vblank, vblank_def, OV2735_VTS_MAX - mode->height,
+ 1, vblank_def);
+}
+
+static int ov2735_set_pad_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *fmt)
+{
+ struct v4l2_mbus_framefmt *format;
+ const struct ov2735_mode *mode;
+ struct ov2735 *ov2735 = to_ov2735(sd);
+
+ format = v4l2_subdev_state_get_format(sd_state, 0);
+
+ mode = v4l2_find_nearest_size(supported_modes,
+ ARRAY_SIZE(supported_modes),
+ width, height,
+ fmt->format.width, fmt->format.height);
+
+ fmt->format.width = mode->width;
+ fmt->format.height = mode->height;
+ fmt->format.field = V4L2_FIELD_NONE;
+ fmt->format.colorspace = V4L2_COLORSPACE_RAW;
+ fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
+ fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
+
+ format = v4l2_subdev_state_get_format(sd_state, 0);
+
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ ov2735_set_framing_limits(ov2735, mode);
+
+ *format = fmt->format;
+
+ return 0;
+}
+
+static int ov2735_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state)
+{
+ struct v4l2_subdev_format fmt = {
+ .which = V4L2_SUBDEV_FORMAT_TRY,
+ .pad = 0,
+ .format = {
+ .code = MEDIA_BUS_FMT_SGRBG10_1X10,
+ .width = supported_modes[0].width,
+ .height = supported_modes[0].height,
+ },
+ };
+
+ ov2735_set_pad_format(sd, state, &fmt);
+
+ return 0;
+}
+
+static const struct v4l2_subdev_video_ops ov2735_video_ops = {
+ .s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops ov2735_pad_ops = {
+ .enum_mbus_code = ov2735_enum_mbus_code,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = ov2735_set_pad_format,
+ .enum_frame_size = ov2735_enum_frame_size,
+ .enable_streams = ov2735_enable_streams,
+ .disable_streams = ov2735_disable_streams,
+};
+
+static const struct v4l2_subdev_ops ov2735_subdev_ops = {
+ .video = &ov2735_video_ops,
+ .pad = &ov2735_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ov2735_internal_ops = {
+ .init_state = ov2735_init_state,
+};
+
+static int ov2735_power_on(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ov2735 *ov2735 = to_ov2735(sd);
+ unsigned long delay_us;
+ int ret;
+
+ gpiod_set_value_cansleep(ov2735->enable_gpio, 0);
+ fsleep(3000);
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
+ ov2735->supplies);
+ if (ret) {
+ dev_err(ov2735->dev, "failed to enable regulators\n");
+ return ret;
+ }
+
+ gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
+ fsleep(3000);
+
+ gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
+ fsleep(3000);
+
+ ret = clk_prepare_enable(ov2735->xclk);
+ if (ret) {
+ dev_err(ov2735->dev, "failed to enable clock\n");
+ goto err_regulator_off;
+ }
+
+ /* 8192 cycles prior to first SCCB transaction */
+ delay_us = DIV_ROUND_UP(8192, OV2735_XCLK_FREQ / 1000 / 1000);
+ fsleep(delay_us);
+
+ return 0;
+
+err_regulator_off:
+ regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
+ return ret;
+}
+
+static int ov2735_power_off(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct ov2735 *ov2735 = to_ov2735(sd);
+
+ gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
+ clk_disable_unprepare(ov2735->xclk);
+ gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
+ regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
+
+ return 0;
+}
+
+static int ov2735_get_regulators(struct ov2735 *ov2735)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(ov2735_supply_name); i++)
+ ov2735->supplies[i].supply = ov2735_supply_name[i];
+
+ return devm_regulator_bulk_get(ov2735->dev,
+ ARRAY_SIZE(ov2735_supply_name),
+ ov2735->supplies);
+}
+
+static int ov2735_identify_module(struct ov2735 *ov2735)
+{
+ u64 chip_id;
+ int ret = 0;
+
+ ov2735_cci_read(ov2735, OV2735_REG_CHIPID, &chip_id, &ret);
+ if (ret)
+ return dev_err_probe(ov2735->dev, ret, "failed to read chip id %x\n",
+ OV2735_CHIPID);
+
+ if (chip_id != OV2735_CHIPID)
+ return dev_err_probe(ov2735->dev, -EIO, "chip id mismatch: %x!=%llx\n",
+ OV2735_CHIPID, chip_id);
+
+ return 0;
+}
+
+static int ov2735_parse_endpoint(struct ov2735 *ov2735)
+{
+ struct fwnode_handle *fwnode;
+ struct v4l2_fwnode_endpoint bus_cfg = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY,
+ };
+ struct fwnode_handle *ep;
+ int ret;
+
+ fwnode = dev_fwnode(ov2735->dev);
+ ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+ if (!ep) {
+ dev_err(ov2735->dev, "Failed to get next endpoint\n");
+ return -ENXIO;
+ }
+
+ ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+ fwnode_handle_put(ep);
+ if (ret)
+ return ret;
+
+ if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
+ dev_err_probe(ov2735->dev, -EINVAL, "only 2 data lanes are supported\n");
+ goto error_out;
+ }
+
+ return 0;
+
+error_out:
+ v4l2_fwnode_endpoint_free(&bus_cfg);
+
+ return ret;
+};
+
+static int ov2735_probe(struct i2c_client *client)
+{
+ struct ov2735 *ov2735;
+ unsigned int xclk_freq;
+ int ret;
+
+ ov2735 = devm_kzalloc(&client->dev, sizeof(*ov2735), GFP_KERNEL);
+ if (!ov2735)
+ return -ENOMEM;
+
+ ov2735->client = client;
+ ov2735->dev = &client->dev;
+
+ v4l2_i2c_subdev_init(&ov2735->sd, client, &ov2735_subdev_ops);
+ ov2735->sd.internal_ops = &ov2735_internal_ops;
+
+ ov2735->cci = devm_cci_regmap_init_i2c(client, 8);
+ if (IS_ERR(ov2735->cci)) {
+ ret = PTR_ERR(ov2735->cci);
+ return dev_err_probe(ov2735->dev, ret, "failed to initialize CCI\n");
+ }
+
+ /* Set Current page to 0 */
+ ov2735->current_page = 0;
+ mutex_init(&ov2735->page_lock);
+
+ /* Get system clock (xvclk) */
+ ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
+ if (IS_ERR(ov2735->xclk)) {
+ return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->xclk),
+ "failed to get xclk\n");
+ }
+
+ xclk_freq = clk_get_rate(ov2735->xclk);
+ if (xclk_freq != OV2735_XCLK_FREQ)
+ return dev_err_probe(ov2735->dev, -EINVAL,
+ "xclk frequency not supported: %d Hz\n",
+ xclk_freq);
+
+ ret = ov2735_get_regulators(ov2735);
+ if (ret)
+ return dev_err_probe(ov2735->dev, ret, "failed to get regulators\n");
+
+ ret = ov2735_parse_endpoint(ov2735);
+ if (ret) {
+ dev_err(ov2735->dev, "failed to parse endpoint configuration\n");
+ return ret;
+ }
+
+ ov2735->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(ov2735->reset_gpio))
+ return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->reset_gpio),
+ "failed to get reset GPIO\n");
+
+ ov2735->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW);
+ if (IS_ERR(ov2735->enable_gpio))
+ return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->enable_gpio),
+ "failed to get enable GPIO\n");
+
+ ret = ov2735_power_on(ov2735->dev);
+ if (ret)
+ return ret;
+
+ ret = ov2735_identify_module(ov2735);
+ if (ret)
+ goto error_power_off;
+
+ /* This needs the pm runtime to be registered. */
+ ret = ov2735_init_controls(ov2735);
+ if (ret)
+ goto error_power_off;
+
+ /* Initialize subdev */
+ ov2735->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ ov2735->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+ ov2735->pad.flags = MEDIA_PAD_FL_SOURCE;
+
+ ret = media_entity_pads_init(&ov2735->sd.entity, 1, &ov2735->pad);
+ if (ret) {
+ dev_err_probe(ov2735->dev, ret, "failed to init entity pads\n");
+ goto error_handler_free;
+ }
+
+ ov2735->sd.state_lock = ov2735->handler.lock;
+ ret = v4l2_subdev_init_finalize(&ov2735->sd);
+ if (ret < 0) {
+ dev_err_probe(ov2735->dev, ret, "subdev init error\n");
+ goto error_media_entity;
+ }
+
+ pm_runtime_set_active(ov2735->dev);
+ pm_runtime_enable(ov2735->dev);
+
+ ret = v4l2_async_register_subdev_sensor(&ov2735->sd);
+ if (ret < 0) {
+ dev_err_probe(ov2735->dev, ret,
+ "failed to register ov2735 sub-device\n");
+ goto error_subdev_cleanup;
+ }
+
+ pm_runtime_idle(ov2735->dev);
+
+ return 0;
+
+error_subdev_cleanup:
+ v4l2_subdev_cleanup(&ov2735->sd);
+ pm_runtime_disable(ov2735->dev);
+ pm_runtime_set_suspended(ov2735->dev);
+
+error_media_entity:
+ media_entity_cleanup(&ov2735->sd.entity);
+
+error_handler_free:
+ v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
+
+error_power_off:
+ ov2735_power_off(ov2735->dev);
+
+ return ret;
+}
+
+static void ov2735_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct ov2735 *ov2735 = to_ov2735(sd);
+
+ v4l2_async_unregister_subdev(sd);
+ v4l2_subdev_cleanup(&ov2735->sd);
+ media_entity_cleanup(&sd->entity);
+ v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
+
+ pm_runtime_disable(ov2735->dev);
+ if (!pm_runtime_status_suspended(ov2735->dev))
+ ov2735_power_off(ov2735->dev);
+ pm_runtime_set_suspended(ov2735->dev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ov2735_pm_ops,
+ ov2735_power_off, ov2735_power_on, NULL);
+
+static const struct of_device_id ov2735_id[] = {
+ { .compatible = "ovti,ov2735" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov2735_id);
+
+static struct i2c_driver ov2735_driver = {
+ .driver = {
+ .name = "ov2735",
+ .pm = pm_ptr(&ov2735_pm_ops),
+ .of_match_table = ov2735_id,
+ },
+ .probe = ov2735_probe,
+ .remove = ov2735_remove,
+};
+module_i2c_driver(ov2735_driver);
+
+MODULE_DESCRIPTION("OV2735 Camera Sensor Driver");
+MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>");
+MODULE_AUTHOR("Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
@ 2025-07-10 13:33 ` Krzysztof Kozlowski
2025-07-10 13:35 ` Krzysztof Kozlowski
2025-07-10 18:33 ` Rob Herring
2 siblings, 0 replies; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-10 13:33 UTC (permalink / raw)
To: Hardevsinh Palaniya, sakari.ailus, krzk+dt, kieran.bingham
Cc: dave.stevenson, pratap.nirujogi, laurent.pinchart, tarang.raval,
Himanshu Bhavani, Mauro Carvalho Chehab, Rob Herring,
Conor Dooley, Hans Verkuil, Bryan O'Donoghue, Ricardo Ribalda,
André Apitzsch, Arnd Bergmann, Dongcheng Yan, Jingjing Xiong,
Matthias Fend, Benjamin Mugnier, Sylvain Petinot,
Heimir Thor Sverrisson, Andy Shevchenko, Hans de Goede,
linux-media, devicetree, linux-kernel
On 10/07/2025 15:10, Hardevsinh Palaniya wrote:
> +
> + clocks:
> + items:
> + - description: XVCLK clock
> +
> + clock-names:
> + items:
> + - const: xvclk
You don't use clock names here and example, so just drop the property.
With this fixed:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
<form letter>
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here. However, there's no need to repost
patches *only* to add the tags. The upstream maintainer will do that for
tags received on the version they apply.
Full context and explanation:
https://elixir.bootlin.com/linux/v6.12-rc3/source/Documentation/process/submitting-patches.rst#L577
</form letter>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
2025-07-10 13:33 ` Krzysztof Kozlowski
@ 2025-07-10 13:35 ` Krzysztof Kozlowski
2025-07-10 21:05 ` Laurent Pinchart
2025-07-10 18:33 ` Rob Herring
2 siblings, 1 reply; 11+ messages in thread
From: Krzysztof Kozlowski @ 2025-07-10 13:35 UTC (permalink / raw)
To: Hardevsinh Palaniya, sakari.ailus, krzk+dt, kieran.bingham
Cc: dave.stevenson, pratap.nirujogi, laurent.pinchart, tarang.raval,
Himanshu Bhavani, Mauro Carvalho Chehab, Rob Herring,
Conor Dooley, Hans Verkuil, Bryan O'Donoghue, Ricardo Ribalda,
André Apitzsch, Arnd Bergmann, Dongcheng Yan, Jingjing Xiong,
Matthias Fend, Benjamin Mugnier, Sylvain Petinot,
Heimir Thor Sverrisson, Andy Shevchenko, Hans de Goede,
linux-media, devicetree, linux-kernel
On 10/07/2025 15:10, Hardevsinh Palaniya wrote:
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - avdd-supply
> + - dovdd-supply
> + - dvdd-supply
> + - port
Now I looked at your driver and the code clearly says that GPIOs are
not optional.
You really need to sync the binding in the driver. They cannot define
completely different ABI.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
@ 2025-07-10 13:46 ` Andy Shevchenko
2025-07-10 21:21 ` Laurent Pinchart
2025-07-11 7:56 ` kernel test robot
2 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2025-07-10 13:46 UTC (permalink / raw)
To: Hardevsinh Palaniya
Cc: sakari.ailus, krzk+dt, kieran.bingham, dave.stevenson,
pratap.nirujogi, laurent.pinchart, tarang.raval, Himanshu Bhavani,
Mauro Carvalho Chehab, Rob Herring, Conor Dooley, Hans Verkuil,
Ricardo Ribalda, Bryan O'Donoghue, André Apitzsch,
Arnd Bergmann, Matthias Fend, Sylvain Petinot, Dongcheng Yan,
Benjamin Mugnier, Heimir Thor Sverrisson, Jingjing Xiong,
Hans de Goede, Hao Yao, linux-media, devicetree, linux-kernel
On Thu, Jul 10, 2025 at 06:40:59PM +0530, Hardevsinh Palaniya wrote:
> Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
>
> The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
> active array size of 1920 x 1080.
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Test pattern support control
> - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
...
> +static int ov2735_cci_access(struct ov2735 *ov2735,
> + u32 reg, void *val, int *err, bool is_read)
> +{
> + u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
> + u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
> + int ret = 0;
> +
> + if (err && *err)
> + return *err;
> +
> + mutex_lock(&ov2735->page_lock);
> +
> + /* Perform page access before read/write */
> + if (ov2735->current_page != page) {
> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, &ret);
> + if (ret)
> + goto err_mutex_unlock;
> + ov2735->current_page = page;
> + }
> +
> + if (is_read)
> + cci_read(ov2735->cci, addr, (u64 *)val, &ret);
> + else
> + cci_write(ov2735->cci, addr, *(u64 *)val, &ret);
> +
> + if (!ret && err)
ret here is never non-0. I believe the check is unneeded and this should be
inside the below label. Otherwise, what's the point?
> + *err = ret;
> +
> +err_mutex_unlock:
> + mutex_unlock(&ov2735->page_lock);
> + return ret;
> +}
Have you researched on how hard is to implement a page access framework as
being suggested in previous review?
...
> +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
> +{
> + int ret = 0;
> + u64 val;
> +
> + ov2735_cci_read(ov2735, OV2735_REG_TEST_PATTERN, &val, &ret);
In this case is better to have the NULL form with no assignment done.
Use a common sense.
> + if (ret)
> + return ret;
> +
> + switch (pattern) {
> + case 0:
> + val &= ~OV2735_TEST_PATTERN_ENABLE;
> + break;
> + case 1:
> + val |= OV2735_TEST_PATTERN_ENABLE;
> + break;
> + }
> +
> + return ov2735_cci_write(ov2735, OV2735_REG_TEST_PATTERN, val, NULL);
> +}
...
> +static int ov2735_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + const struct ov2735_mode *mode;
> + const struct ov2735_reglist *reg_list;
> + const struct v4l2_mbus_framefmt *fmt;
> + int ret;
> +
> + fmt = v4l2_subdev_state_get_format(state, 0);
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->width, fmt->height);
> +
> + ret = pm_runtime_resume_and_get(ov2735->dev);
> + if (ret < 0)
> + return ret;
> +
> + reg_list = &mode->reg_list;
> + ov2735_cci_multi_reg_write(ov2735, reg_list->regvals, reg_list->num_regs, &ret);
NULL-variant is better here. Revisit all these again, please.
> + if (ret) {
> + dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__);
> + goto err_rpm_put;
> + }
> +
> + /* Apply customized values from user */
> + ret = __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler);
> + if (ret)
> + goto err_rpm_put;
> +
> + /* set stream on register */
> + ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, &ret);
> + if (ret)
> + goto err_rpm_put;
> +
> + return 0;
> +
> +err_rpm_put:
> + pm_runtime_put(ov2735->dev);
> + return ret;
> +}
...
> +static int ov2735_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + unsigned long delay_us;
> + int ret;
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 0);
> + fsleep(3000);
Can you add a comment with a datasheet reference to explain the value used
here?
> + ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
> + ov2735->supplies);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable regulators\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
> + fsleep(3000);
Ditto.
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
> + fsleep(3000);
Ditto.
> + ret = clk_prepare_enable(ov2735->xclk);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable clock\n");
> + goto err_regulator_off;
> + }
> +
> + /* 8192 cycles prior to first SCCB transaction */
> + delay_us = DIV_ROUND_UP(8192, OV2735_XCLK_FREQ / 1000 / 1000);
Shouldn't these / 1000 / 1000 be simply / HZ_PER_MHZ?
Or if you want to keep it precise to the physics, use MEGA from units.h.
> + fsleep(delay_us);
> +
> + return 0;
> +
> +err_regulator_off:
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> + return ret;
> +}
...
> +static int ov2735_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
Power off enables the chip?! Perhaps you missed polarity in DT?
> + clk_disable_unprepare(ov2735->xclk);
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
In the similar way.
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> +
> + return 0;
> +}
...
> +static int ov2735_parse_endpoint(struct ov2735 *ov2735)
> +{
> + struct fwnode_handle *fwnode;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + fwnode = dev_fwnode(ov2735->dev);
> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!ep) {
> + dev_err(ov2735->dev, "Failed to get next endpoint\n");
> + return -ENXIO;
Please, be consistent, use dev_err_probe().
> + }
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> + dev_err_probe(ov2735->dev, -EINVAL, "only 2 data lanes are supported\n");
ret = dev_err_probe(...);
Otherwise it's 0.
> + goto error_out;
> + }
> +
> + return 0;
> +
> +error_out:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +};
...
> + ov2735->cci = devm_cci_regmap_init_i2c(client, 8);
> + if (IS_ERR(ov2735->cci)) {
> + ret = PTR_ERR(ov2735->cci);
> + return dev_err_probe(ov2735->dev, ret, "failed to initialize CCI\n");
Why not in a single expression?
> + }
...
> + /* Set Current page to 0 */
> + ov2735->current_page = 0;
> + mutex_init(&ov2735->page_lock);
ret = devm_mutex_init(...);
i.o.w. do not mix devm chain with non-devm allocations.
...
> + ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
> + if (IS_ERR(ov2735->xclk)) {
> + return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->xclk),
> + "failed to get xclk\n");
> + }
You can drop {}.
> + xclk_freq = clk_get_rate(ov2735->xclk);
> + if (xclk_freq != OV2735_XCLK_FREQ)
> + return dev_err_probe(ov2735->dev, -EINVAL,
> + "xclk frequency not supported: %d Hz\n",
> + xclk_freq);
You see, the code is full of inconsistencies, please take your time (no need to
hurry with a new version, you have at least two months for this series to be
reviewed and applied if okay).
...
> + ret = ov2735_get_regulators(ov2735);
> + if (ret)
> + return dev_err_probe(ov2735->dev, ret, "failed to get regulators\n");
> +
> + ret = ov2735_parse_endpoint(ov2735);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to parse endpoint configuration\n");
> + return ret;
return dev_error_probe(...);
> + }
Again, read above.
...
> + ret = v4l2_async_register_subdev_sensor(&ov2735->sd);
> + if (ret < 0) {
Why ' < 0'? What is the meaning of the positive return?
Same Q to all these inconsistencies (note, in some cases it's needed
but I doubt here and in many other places it's the case).
> + dev_err_probe(ov2735->dev, ret,
> + "failed to register ov2735 sub-device\n");
> + goto error_subdev_cleanup;
> + }
...
> +static void ov2735_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(&ov2735->sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
> + pm_runtime_disable(ov2735->dev);
Why not devm variant being used in probe?
> + if (!pm_runtime_status_suspended(ov2735->dev))
> + ov2735_power_off(ov2735->dev);
> + pm_runtime_set_suspended(ov2735->dev);
So, you probably want SMART_SUSPEND or something like that when enabling
runtime PM in probe. These lines looks like duplication of what PM runtime may
do for you.
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
2025-07-10 13:33 ` Krzysztof Kozlowski
2025-07-10 13:35 ` Krzysztof Kozlowski
@ 2025-07-10 18:33 ` Rob Herring
2025-07-10 21:08 ` Laurent Pinchart
2 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2025-07-10 18:33 UTC (permalink / raw)
To: Hardevsinh Palaniya
Cc: sakari.ailus, krzk+dt, kieran.bingham, dave.stevenson,
pratap.nirujogi, laurent.pinchart, tarang.raval, Himanshu Bhavani,
Mauro Carvalho Chehab, Conor Dooley, Hans Verkuil,
Bryan O'Donoghue, Ricardo Ribalda, André Apitzsch,
Arnd Bergmann, Dongcheng Yan, Jingjing Xiong, Matthias Fend,
Benjamin Mugnier, Sylvain Petinot, Heimir Thor Sverrisson,
Andy Shevchenko, Hans de Goede, linux-media, devicetree,
linux-kernel
On Thu, Jul 10, 2025 at 06:40:58PM +0530, Hardevsinh Palaniya wrote:
> From: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
>
> Add bindings for Omnivision OV2735 sensor.
>
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> ---
> .../bindings/media/i2c/ovti,ov2735.yaml | 115 ++++++++++++++++++
> 1 file changed, 115 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> new file mode 100644
> index 000000000000..d9d01db88844
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov2735.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OV2735 Image Sensor
> +
> +maintainers:
> + - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> +
> +description: |
Don't need '|' if no formatting to preserve.
> + The OmniVision OV2735 is a 2MP (1920x1080) color CMOS image sensor controlled
> + through an I2C-compatible SCCB bus. it outputs RAW10 format and uses a 1/2.7"
> + optical format.
> +
> +properties:
> + compatible:
> + const: ovti,ov2735
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: XVCLK clock
> +
> + clock-names:
> + items:
> + - const: xvclk
> +
> + avdd-supply:
> + description: Analog Domain Power Supply
> +
> + dovdd-supply:
> + description: I/O Domain Power Supply
> +
> + dvdd-supply:
> + description: Digital Domain Power Supply
> +
> + reset-gpios:
> + maxItems: 1
> + description: Reset Pin GPIO Control (active low)
> +
> + enable-gpios:
> + maxItems: 1
> + description: |
Same here.
> + Active-low enable pin. Labeled as 'PWDN' in the datasheet, but acts as
> + an enable signal. During power rail ramp-up, the device remains powered
> + down. Once power rails are stable, pulling this pin low powers on the
> + device.
> +
> + port:
> + description: MIPI CSI-2 transmitter port
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + additionalProperties: false
> +
> + properties:
> + endpoint:
> + $ref: /schemas/media/video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + data-lanes:
> + items:
> + - const: 1
> + - const: 2
> +
> + required:
> + - data-lanes
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - avdd-supply
> + - dovdd-supply
> + - dvdd-supply
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/rk3399-cru.h>
> + #include <dt-bindings/gpio/gpio.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + camera-sensor@3c {
> + compatible = "ovti,ov2735";
> + reg = <0x3c>;
> + clocks = <&ov2735_clk>;
> +
> + assigned-clocks = <&ov2735_clk>;
> + assigned-clock-parents = <&ov2735_clk_parent>;
> + assigned-clock-rates = <24000000>;
> +
> + avdd-supply = <&ov2735_avdd>;
> + dovdd-supply = <&ov2735_dovdd>;
> + dvdd-supply = <&ov2735_dvdd>;
> +
> + reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> + enable-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;
> +
> + port {
> + cam_out: endpoint {
> + remote-endpoint = <&mipi_in_cam>;
> + data-lanes = <1 2>;
> + };
> + };
> + };
> + };
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 13:35 ` Krzysztof Kozlowski
@ 2025-07-10 21:05 ` Laurent Pinchart
0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2025-07-10 21:05 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hardevsinh Palaniya, sakari.ailus, krzk+dt, kieran.bingham,
dave.stevenson, pratap.nirujogi, tarang.raval, Himanshu Bhavani,
Mauro Carvalho Chehab, Rob Herring, Conor Dooley, Hans Verkuil,
Bryan O'Donoghue, Ricardo Ribalda, André Apitzsch,
Arnd Bergmann, Dongcheng Yan, Jingjing Xiong, Matthias Fend,
Benjamin Mugnier, Sylvain Petinot, Heimir Thor Sverrisson,
Andy Shevchenko, Hans de Goede, linux-media, devicetree,
linux-kernel
On Thu, Jul 10, 2025 at 03:35:51PM +0200, Krzysztof Kozlowski wrote:
> On 10/07/2025 15:10, Hardevsinh Palaniya wrote:
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - avdd-supply
> > + - dovdd-supply
> > + - dvdd-supply
> > + - port
>
> Now I looked at your driver and the code clearly says that GPIOs are
> not optional.
>
> You really need to sync the binding in the driver. They cannot define
> completely different ABI.
I couldn't have said it in a clearer way.
For the GPIOs, I recommend making them optional in the driver, as GPIOs
are not always connected in all designs (unless of course we're dealing
with a GPIO whose control from the SoC is absolutely mandatory to make
the device work at all, but that doesn't seem to be the case here).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor
2025-07-10 18:33 ` Rob Herring
@ 2025-07-10 21:08 ` Laurent Pinchart
0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2025-07-10 21:08 UTC (permalink / raw)
To: Rob Herring
Cc: Hardevsinh Palaniya, sakari.ailus, krzk+dt, kieran.bingham,
dave.stevenson, pratap.nirujogi, tarang.raval, Himanshu Bhavani,
Mauro Carvalho Chehab, Conor Dooley, Hans Verkuil,
Bryan O'Donoghue, Ricardo Ribalda, André Apitzsch,
Arnd Bergmann, Dongcheng Yan, Jingjing Xiong, Matthias Fend,
Benjamin Mugnier, Sylvain Petinot, Heimir Thor Sverrisson,
Andy Shevchenko, Hans de Goede, linux-media, devicetree,
linux-kernel
On Thu, Jul 10, 2025 at 01:33:56PM -0500, Rob Herring wrote:
> On Thu, Jul 10, 2025 at 06:40:58PM +0530, Hardevsinh Palaniya wrote:
> > From: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> >
> > Add bindings for Omnivision OV2735 sensor.
> >
> > Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> > ---
> > .../bindings/media/i2c/ovti,ov2735.yaml | 115 ++++++++++++++++++
> > 1 file changed, 115 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> > new file mode 100644
> > index 000000000000..d9d01db88844
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov2735.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: OmniVision OV2735 Image Sensor
> > +
> > +maintainers:
> > + - Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> > +
> > +description: |
>
> Don't need '|' if no formatting to preserve.
>
> > + The OmniVision OV2735 is a 2MP (1920x1080) color CMOS image sensor controlled
> > + through an I2C-compatible SCCB bus. it outputs RAW10 format and uses a 1/2.7"
> > + optical format.
> > +
> > +properties:
> > + compatible:
> > + const: ovti,ov2735
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: XVCLK clock
> > +
> > + clock-names:
> > + items:
> > + - const: xvclk
> > +
> > + avdd-supply:
> > + description: Analog Domain Power Supply
> > +
> > + dovdd-supply:
> > + description: I/O Domain Power Supply
> > +
> > + dvdd-supply:
> > + description: Digital Domain Power Supply
> > +
> > + reset-gpios:
> > + maxItems: 1
> > + description: Reset Pin GPIO Control (active low)
> > +
> > + enable-gpios:
> > + maxItems: 1
> > + description: |
>
> Same here.
>
> > + Active-low enable pin. Labeled as 'PWDN' in the datasheet, but acts as
> > + an enable signal. During power rail ramp-up, the device remains powered
> > + down. Once power rails are stable, pulling this pin low powers on the
> > + device.
> > +
> > + port:
> > + description: MIPI CSI-2 transmitter port
> > + $ref: /schemas/graph.yaml#/$defs/port-base
> > + additionalProperties: false
> > +
> > + properties:
> > + endpoint:
> > + $ref: /schemas/media/video-interfaces.yaml#
> > + unevaluatedProperties: false
> > +
> > + properties:
> > + data-lanes:
> > + items:
> > + - const: 1
> > + - const: 2
> > +
> > + required:
> > + - data-lanes
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > + - avdd-supply
> > + - dovdd-supply
> > + - dvdd-supply
> > + - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/rk3399-cru.h>
Unused.
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + camera-sensor@3c {
> > + compatible = "ovti,ov2735";
> > + reg = <0x3c>;
> > + clocks = <&ov2735_clk>;
> > +
> > + assigned-clocks = <&ov2735_clk>;
> > + assigned-clock-parents = <&ov2735_clk_parent>;
> > + assigned-clock-rates = <24000000>;
I think you can drop those three properties from the example, they don't
add much value. Or you could switch to a clock generated by the SoC
(an RK3399 based on the #include above), in which case assigning the
rate makes sense.
> > +
> > + avdd-supply = <&ov2735_avdd>;
> > + dovdd-supply = <&ov2735_dovdd>;
> > + dvdd-supply = <&ov2735_dvdd>;
> > +
> > + reset-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
> > + enable-gpios = <&gpio2 11 GPIO_ACTIVE_LOW>;
> > +
> > + port {
> > + cam_out: endpoint {
> > + remote-endpoint = <&mipi_in_cam>;
> > + data-lanes = <1 2>;
> > + };
> > + };
> > + };
> > + };
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
2025-07-10 13:46 ` Andy Shevchenko
@ 2025-07-10 21:21 ` Laurent Pinchart
2025-07-11 7:56 ` kernel test robot
2 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2025-07-10 21:21 UTC (permalink / raw)
To: Hardevsinh Palaniya
Cc: sakari.ailus, krzk+dt, kieran.bingham, dave.stevenson,
pratap.nirujogi, tarang.raval, Himanshu Bhavani,
Mauro Carvalho Chehab, Rob Herring, Conor Dooley, Hans Verkuil,
Ricardo Ribalda, Bryan O'Donoghue, André Apitzsch,
Arnd Bergmann, Matthias Fend, Sylvain Petinot, Dongcheng Yan,
Benjamin Mugnier, Heimir Thor Sverrisson, Jingjing Xiong,
Andy Shevchenko, Hans de Goede, Hao Yao, linux-media, devicetree,
linux-kernel
On Thu, Jul 10, 2025 at 06:40:59PM +0530, Hardevsinh Palaniya wrote:
> Add a v4l2 subdevice driver for the Omnivision OV2735 sensor.
>
> The Omnivision OV2735 is a 1/2.7-Inch CMOS image sensor with an
> active array size of 1920 x 1080.
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - Test pattern support control
> - Supported resolution: 1920 x 1080 @ 30fps (SGRBG10)
>
> Co-developed-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> ---
> MAINTAINERS | 9 +
> drivers/media/i2c/Kconfig | 10 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov2735.c | 962 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 982 insertions(+)
> create mode 100644 drivers/media/i2c/ov2735.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 92e9d8c7708f..058bbfd9523b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18471,6 +18471,15 @@ T: git git://linuxtv.org/media.git
> F: Documentation/devicetree/bindings/media/i2c/ovti,ov2685.yaml
> F: drivers/media/i2c/ov2685.c
>
> +OMNIVISION OV2735 SENSOR DRIVER
> +M: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> +M: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +T: git git://linuxtv.org/media.git
> +F: Documentation/devicetree/bindings/media/i2c/ovti,ov2735.yaml
> +F: drivers/media/i2c/ov2735.c
> +
> OMNIVISION OV2740 SENSOR DRIVER
> M: Tianshu Qiu <tian.shu.qiu@intel.com>
> R: Sakari Ailus <sakari.ailus@linux.intel.com>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 4b4c199da6ea..9646eab1b177 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -446,6 +446,16 @@ config VIDEO_OV2685
> To compile this driver as a module, choose M here: the
> module will be called ov2685.
>
> +config VIDEO_OV2735
> + tristate "OmniVision OV2735 sensor support"
> + select V4L2_CCI_I2C
> + help
> + This is a Video4Linux2 sensor driver for the Sony
> + OV2735 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ov2735.
> +
> config VIDEO_OV2740
> tristate "OmniVision OV2740 sensor support"
> depends on ACPI || COMPILE_TEST
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5873d29433ee..1adb27743fa1 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_VIDEO_OV2640) += ov2640.o
> obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
> obj-$(CONFIG_VIDEO_OV2680) += ov2680.o
> obj-$(CONFIG_VIDEO_OV2685) += ov2685.o
> +obj-$(CONFIG_VIDEO_OV2735) += ov2735.o
> obj-$(CONFIG_VIDEO_OV2740) += ov2740.o
> obj-$(CONFIG_VIDEO_OV4689) += ov4689.o
> obj-$(CONFIG_VIDEO_OV5640) += ov5640.o
> diff --git a/drivers/media/i2c/ov2735.c b/drivers/media/i2c/ov2735.c
> new file mode 100644
> index 000000000000..9cfa34aa0966
> --- /dev/null
> +++ b/drivers/media/i2c/ov2735.c
> @@ -0,0 +1,962 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * V4L2 Support for the OV2735
> + *
> + * Copyright (C) 2025 Silicon Signals Pvt. Ltd.
> + *
> + * Based on Rockchip ov2735 Camera Driver
> + * Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd.
> + *
> + * Inspired from ov8858, imx219, imx283 camera drivers
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/clk.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device/devres.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
> +
> +#define OV2735_XCLK_FREQ (24 * HZ_PER_MHZ)
> +
> +/* Add page number in CCI private bits [31:28] of the register address */
> +#define CCI_PAGE_REG8(x, p) ((p << CCI_REG_PRIVATE_SHIFT) | CCI_REG8(x))
#define CCI_PAGE_REG8(x, p) (((p) << CCI_REG_PRIVATE_SHIFT) | CCI_REG8(x))
But name the macro with an OV2735_ prefix, it's not a generic CCI macro.
> +#define CCI_PAGE_REG16(x, p) ((p << CCI_REG_PRIVATE_SHIFT) | CCI_REG16(x))
Same here.
Could you swap the x and p arguments ? It's more readable to have the
page before the address.
> +
> +#define OV2735_REG_PAGE_SELECT CCI_REG8(0xfd)
> +
> +/* Page 0 */
> +#define OV2735_REG_CHIPID CCI_PAGE_REG16(0x02, 0x00)
> +#define OV2735_CHIPID 0x2735
> +
> +#define OV2735_REG_SOFT_REST CCI_PAGE_REG8(0x20, 0x00)
> +
> +/* Clock Settings */
> +#define OV2735_REG_PLL_CTRL CCI_PAGE_REG8(0x2f, 0x00)
> +#define OV2735_REG_PLL_OUTDIV CCI_PAGE_REG8(0x34, 0x00)
> +#define OV2735_REG_CLK_MODE CCI_PAGE_REG8(0x30, 0x00)
> +#define OV2735_REG_CLOCK_REG1 CCI_PAGE_REG8(0x33, 0x00)
> +#define OV2735_REG_CLOCK_REG2 CCI_PAGE_REG8(0x35, 0x00)
> +
> +/* Page 1 */
> +#define OV2735_REG_STREAM_CTRL CCI_PAGE_REG8(0xa0, 0x01)
> +#define OV2735_STREAM_ON 0x01
> +#define OV2735_STREAM_OFF 0x00
> +
> +#define OV2735_REG_UPDOWN_MIRROR CCI_PAGE_REG8(0x3f, 0x01)
> +#define OV2735_REG_BINNING_DAC_CODE_MODE CCI_PAGE_REG8(0x30, 0x01)
> +#define OV2735_REG_FRAME_LENGTH CCI_PAGE_REG16(0x0e, 0x01)
> +#define OV2735_VTS_MAX 0x0fff
> +#define OV2735_REG_FRAME_EXP_SEPERATE_EN CCI_PAGE_REG8(0x0d, 0x01)
> +#define OV2735_FRAME_EXP_SEPERATE_EN 0x10
> +#define OV2735_REG_FRAME_SYNC CCI_PAGE_REG8(0x01, 0x01)
> +
> +#define OV2735_REG_HBLANK CCI_PAGE_REG16(0x09, 0x01)
> +
> +#define OV2735_REG_HS_MIPI CCI_PAGE_REG8(0xb1, 0x01)
> +#define OV2735_REG_MIPI_CTRL1 CCI_PAGE_REG8(0x92, 0x01)
> +#define OV2735_REG_MIPI_CTRL2 CCI_PAGE_REG8(0x94, 0x01)
> +#define OV2735_REG_MIPI_CTRL3 CCI_PAGE_REG8(0xa1, 0x01)
> +#define OV2735_REG_MIPI_CTRL4 CCI_PAGE_REG8(0xb2, 0x01)
> +#define OV2735_REG_MIPI_CTRL5 CCI_PAGE_REG8(0xb3, 0x01)
> +#define OV2735_REG_MIPI_CTRL6 CCI_PAGE_REG8(0xb4, 0x01)
> +#define OV2735_REG_MIPI_CTRL7 CCI_PAGE_REG8(0xb5, 0x01)
> +#define OV2735_REG_PREPARE CCI_PAGE_REG8(0x95, 0x01)
> +#define OV2735_REG_R_HS_ZERO CCI_PAGE_REG8(0x96, 0x01)
> +#define OV2735_REG_TRAIL CCI_PAGE_REG8(0x98, 0x01)
> +#define OV2735_REG_R_CLK_ZERO CCI_PAGE_REG8(0x9c, 0x01)
> +#define OV2735_REG_MIPI_V_SIZE CCI_PAGE_REG16(0x8e, 0x01)
> +#define OV2735_REG_MIPI_H_SIZE CCI_PAGE_REG16(0x90, 0x01)
> +
> +#define OV2735_REG_ANALOG_CTRL3 CCI_PAGE_REG8(0x25, 0x01)
> +#define OV2735_REG_VNCP CCI_PAGE_REG8(0x20, 0x01)
> +
> +/* BLC registers */
> +#define OV2735_REG_BLC_GAIN_BLUE CCI_PAGE_REG8(0x86, 0x01)
> +#define OV2735_REG_BLC_GAIN_RED CCI_PAGE_REG8(0x87, 0x01)
> +#define OV2735_REG_BLC_GAIN_GR CCI_PAGE_REG8(0x88, 0x01)
> +#define OV2735_REG_BLC_GAIN_GB CCI_PAGE_REG8(0x89, 0x01)
> +#define OV2735_REG_GB_SUBOFFSET CCI_PAGE_REG8(0xf0, 0x01)
> +#define OV2735_REG_BLUE_SUBOFFSET CCI_PAGE_REG8(0xf1, 0x01)
> +#define OV2735_REG_RED_SUBOFFSET CCI_PAGE_REG8(0xf2, 0x01)
> +#define OV2735_REG_GR_SUBOFFSET CCI_PAGE_REG8(0xf3, 0x01)
> +#define OV2735_REG_BLC_BPC_TH_P CCI_PAGE_REG8(0xfc, 0x01)
> +#define OV2735_REG_BLC_BPC_TH_N CCI_PAGE_REG8(0xfe, 0x01)
> +
> +#define OV2735_REG_TEST_PATTERN CCI_PAGE_REG8(0xb2, 0x01)
> +#define OV2735_TEST_PATTERN_ENABLE 0x01
> +#define OV2735_TEST_PATTERN_DISABLE 0xfe
> +
> +#define OV2735_REG_LONG_EXPOSURE CCI_PAGE_REG16(0x03, 0x01)
> +#define OV2735_EXPOSURE_MIN 4
> +#define OV2735_EXPOSURE_STEP 1
> +
> +#define OV2735_REG_ANALOG_GAIN CCI_PAGE_REG8(0x24, 0x01)
> +#define OV2735_ANALOG_GAIN_MIN 0x10
> +#define OV2735_ANALOG_GAIN_MAX 0xff
> +#define OV2735_ANALOG_GAIN_STEP 1
> +#define OV2735_ANALOG_GAIN_DEFAULT 0x10
> +
> +/* Page 2 */
> +#define OV2735_REG_V_START CCI_PAGE_REG16(0xa0, 0x02)
> +#define OV2735_REG_V_SIZE CCI_PAGE_REG16(0xa2, 0x02)
> +#define OV2735_REG_H_START CCI_PAGE_REG16(0xa4, 0x02)
> +#define OV2735_REG_H_SIZE CCI_PAGE_REG16(0xa6, 0x02)
> +
> +#define OV2735_LINK_FREQ_420MHZ (420 * HZ_PER_MHZ)
If that's the only supported value, you need to validate that the link
frequencies specified in DT match. Better, you should calculate the PLL
parameters dynamically based on the input frequency and the link
frequency.
> +#define OV2735_PIXEL_RATE (168 * HZ_PER_MHZ)
> +
> +static const char * const ov2735_supply_name[] = {
> + "avdd", /* Analog power */
> + "dovdd", /* Digital I/O power */
> + "dvdd", /* Digital core power */
> +};
> +
> +struct ov2735 {
> + struct device *dev;
> + struct regmap *cci;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct i2c_client *client;
> + struct clk *xclk;
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *enable_gpio;
> + struct regulator_bulk_data supplies[ARRAY_SIZE(ov2735_supply_name)];
> +
> + /* V4L2 Controls */
> + 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 v4l2_ctrl *test_pattern;
> +
> + u8 current_page;
> + struct mutex page_lock;
> +};
> +
> +struct ov2735_reglist {
> + unsigned int num_regs;
> + const struct cci_reg_sequence *regvals;
> +};
> +
> +struct ov2735_mode {
> + u32 width;
> + u32 height;
> + u32 hts_def;
> + u32 vts_def;
> + u32 exp_def;
> + struct ov2735_reglist reg_list;
> +};
> +
> +static struct cci_reg_sequence ov2735_1920_1080_30fps[] = {
> + { OV2735_REG_PLL_CTRL, 0x10 },
> + { OV2735_REG_PLL_OUTDIV, 0x00 },
> + { OV2735_REG_CLK_MODE, 0x15 },
> + { OV2735_REG_CLOCK_REG1, 0x01 },
> + { OV2735_REG_CLOCK_REG2, 0x20 },
> + { OV2735_REG_BINNING_DAC_CODE_MODE, 0x00 },
> + { CCI_PAGE_REG8(0xfb, 0x01), 0x73 },
> + { OV2735_REG_FRAME_SYNC, 0x01 },
> +
> + /* Timing ctrl */
> + { CCI_PAGE_REG8(0x1a, 0x01), 0x6b },
> + { CCI_PAGE_REG8(0x1c, 0x01), 0xea },
> + { CCI_PAGE_REG8(0x16, 0x01), 0x0c },
> + { CCI_PAGE_REG8(0x21, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x11, 0x01), 0x63 },
> + { CCI_PAGE_REG8(0x19, 0x01), 0xc3 },
> +
> + /* Analog ctrl */
> + { CCI_PAGE_REG8(0x26, 0x01), 0x5a },
> + { CCI_PAGE_REG8(0x29, 0x01), 0x01 },
> + { CCI_PAGE_REG8(0x33, 0x01), 0x6f },
> + { CCI_PAGE_REG8(0x2a, 0x01), 0xd2 },
> + { CCI_PAGE_REG8(0x2c, 0x01), 0x40 },
> + { CCI_PAGE_REG8(0xd0, 0x01), 0x02 },
> + { CCI_PAGE_REG8(0xd1, 0x01), 0x01 },
> + { CCI_PAGE_REG8(0xd2, 0x01), 0x20 },
> + { CCI_PAGE_REG8(0xd3, 0x01), 0x04 },
> + { CCI_PAGE_REG8(0xd4, 0x01), 0x2a },
> + { CCI_PAGE_REG8(0x50, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x51, 0x01), 0x2c },
> + { CCI_PAGE_REG8(0x52, 0x01), 0x29 },
> + { CCI_PAGE_REG8(0x53, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x55, 0x01), 0x44 },
> + { CCI_PAGE_REG8(0x58, 0x01), 0x29 },
> + { CCI_PAGE_REG8(0x5a, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x5b, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x5d, 0x01), 0x00 },
> + { CCI_PAGE_REG8(0x64, 0x01), 0x2f },
> + { CCI_PAGE_REG8(0x66, 0x01), 0x62 },
> + { CCI_PAGE_REG8(0x68, 0x01), 0x5b },
> + { CCI_PAGE_REG8(0x75, 0x01), 0x46 },
> + { CCI_PAGE_REG8(0x76, 0x01), 0x36 },
> + { CCI_PAGE_REG8(0x77, 0x01), 0x4f },
> + { CCI_PAGE_REG8(0x78, 0x01), 0xef },
> + { CCI_PAGE_REG8(0x72, 0x01), 0xcf },
> + { CCI_PAGE_REG8(0x73, 0x01), 0x36 },
> + { CCI_PAGE_REG8(0x7d, 0x01), 0x0d },
> + { CCI_PAGE_REG8(0x7e, 0x01), 0x0d },
> + { CCI_PAGE_REG8(0x8a, 0x01), 0x77 },
> + { CCI_PAGE_REG8(0x8b, 0x01), 0x77 },
> +
> + { OV2735_REG_HS_MIPI, 0x83 },
> + { OV2735_REG_MIPI_CTRL5, 0x0b },
> + { OV2735_REG_MIPI_CTRL6, 0x14 },
> + { CCI_PAGE_REG8(0x9d, 0x01), 0x40 },
> + { OV2735_REG_MIPI_CTRL3, 0x05 },
> + { OV2735_REG_MIPI_CTRL2, 0x44 },
> + { OV2735_REG_PREPARE, 0x33 },
> + { OV2735_REG_R_HS_ZERO, 0x1f },
> + { OV2735_REG_TRAIL, 0x45 },
> + { OV2735_REG_R_CLK_ZERO, 0x10 },
> + { OV2735_REG_MIPI_CTRL7, 0x70 },
> + { OV2735_REG_ANALOG_CTRL3, 0xe0 },
> + { OV2735_REG_VNCP, 0x7b },
> +
> + /* BLC */
> + { OV2735_REG_BLC_GAIN_BLUE, 0x77 },
> + { OV2735_REG_BLC_GAIN_GB, 0x77 },
> + { OV2735_REG_BLC_GAIN_RED, 0x74 },
> + { OV2735_REG_BLC_GAIN_GR, 0x74 },
> + { OV2735_REG_BLC_BPC_TH_P, 0xe0 },
> + { OV2735_REG_BLC_BPC_TH_N, 0xe0 },
> + { OV2735_REG_GB_SUBOFFSET, 0x40 },
> + { OV2735_REG_BLUE_SUBOFFSET, 0x40 },
> + { OV2735_REG_RED_SUBOFFSET, 0x40 },
> + { OV2735_REG_GR_SUBOFFSET, 0x40 },
A lot of those registers are unrelated to the mode. Split the table in
two, with a list of common registers, and then per-mode registers (if
any).
> +
> + /* 1920x1080 */
> + { OV2735_REG_V_START, 0x0008 },
> + { OV2735_REG_V_SIZE, 0x0438 },
> + { OV2735_REG_H_START, 0x0008 },
> + { OV2735_REG_H_SIZE, 0x03c0 },
> + { OV2735_REG_MIPI_V_SIZE, 0x0780 },
> + { OV2735_REG_MIPI_H_SIZE, 0x0438 },
Don't hardcode those registers, program them based on the format (and
possibly selection rectangles.
> +};
> +
> +static const struct ov2735_mode supported_modes[] = {
> + {
> + .width = 1920,
> + .height = 1080,
> + .exp_def = 399,
> + .hts_def = 2200,
> + .vts_def = 2545,
> + .reg_list = {
> + .num_regs = ARRAY_SIZE(ov2735_1920_1080_30fps),
> + .regvals = ov2735_1920_1080_30fps,
> + },
> + },
> +};
> +
> +static const s64 link_freq_menu_items[] = {
> + OV2735_LINK_FREQ_420MHZ,
> +};
> +
> +static const char * const ov2735_test_pattern_menu[] = {
> + "Disabled",
> + "Vertical Color",
> +};
> +
> +static int ov2735_cci_access(struct ov2735 *ov2735,
> + u32 reg, void *val, int *err, bool is_read)
> +{
> + u8 page = (reg >> CCI_REG_PRIVATE_SHIFT) & 0xff;
> + u32 addr = reg & ~CCI_REG_PRIVATE_MASK;
> + int ret = 0;
> +
> + if (err && *err)
> + return *err;
> +
> + mutex_lock(&ov2735->page_lock);
> +
> + /* Perform page access before read/write */
> + if (ov2735->current_page != page) {
> + ret = cci_write(ov2735->cci, OV2735_REG_PAGE_SELECT, page, &ret);
> + if (ret)
> + goto err_mutex_unlock;
> + ov2735->current_page = page;
> + }
> +
> + if (is_read)
> + cci_read(ov2735->cci, addr, (u64 *)val, &ret);
> + else
> + cci_write(ov2735->cci, addr, *(u64 *)val, &ret);
> +
> + if (!ret && err)
> + *err = ret;
> +
> +err_mutex_unlock:
> + mutex_unlock(&ov2735->page_lock);
> + return ret;
> +}
> +
> +static int ov2735_cci_read(struct ov2735 *ov2735, u32 reg, u64 *val, int *err)
You can drop "cci_" in the function name. Same below.
> +{
> + return ov2735_cci_access(ov2735, reg, val, err, true);
> +}
> +
> +static int ov2735_cci_write(struct ov2735 *ov2735, u32 reg, u64 val, int *err)
> +{
> + return ov2735_cci_access(ov2735, reg, (void *)&val, err, false);
> +}
> +
> +static int ov2735_cci_multi_reg_write(struct ov2735 *ov2735,
> + const struct cci_reg_sequence *regs,
> + unsigned int num_regs, int *err)
> +{
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < num_regs; i++) {
> + ret = ov2735_cci_write(ov2735, regs[i].reg, regs[i].val, err);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static inline struct ov2735 *to_ov2735(struct v4l2_subdev *_sd)
> +{
> + return container_of(_sd, struct ov2735, sd);
> +}
> +
> +static int ov2735_enable_test_pattern(struct ov2735 *ov2735, u32 pattern)
> +{
> + int ret = 0;
> + u64 val;
> +
> + ov2735_cci_read(ov2735, OV2735_REG_TEST_PATTERN, &val, &ret);
> + if (ret)
> + return ret;
> +
> + switch (pattern) {
> + case 0:
> + val &= ~OV2735_TEST_PATTERN_ENABLE;
> + break;
> + case 1:
> + val |= OV2735_TEST_PATTERN_ENABLE;
> + break;
> + }
> +
> + return ov2735_cci_write(ov2735, OV2735_REG_TEST_PATTERN, val, NULL);
> +}
> +
> +static int ov2735_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct ov2735 *ov2735 = container_of(ctrl->handler, struct ov2735,
> + handler);
> + const struct ov2735_mode *mode;
> + struct v4l2_mbus_framefmt *fmt;
> + struct v4l2_subdev_state *state;
> + u64 vts;
> + int ret = 0;
> +
> + state = v4l2_subdev_get_locked_active_state(&ov2735->sd);
> + fmt = v4l2_subdev_state_get_format(state, 0);
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->width, fmt->height);
The format has to match one of the modes, so you can drop this and use
fmt->height below.
> +
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + /* Honour the VBLANK limits when setting exposure. */
> + s64 max = mode->height + ctrl->val - 4;
> +
> + ret = __v4l2_ctrl_modify_range(ov2735->exposure,
> + ov2735->exposure->minimum, max,
> + ov2735->exposure->step,
> + ov2735->exposure->default_value);
> + if (ret)
> + return ret;
> + }
> +
> + /*
> + * Applying V4L2 control value only happens
> + * when power is up for streaming.
> + */
> + if (pm_runtime_get_if_in_use(ov2735->dev) == 0)
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE:
> + ov2735_cci_write(ov2735, OV2735_REG_LONG_EXPOSURE, ctrl->val, &ret);
> + break;
> + case V4L2_CID_ANALOGUE_GAIN:
> + ov2735_cci_write(ov2735, OV2735_REG_ANALOG_GAIN, ctrl->val, &ret);
> + break;
> + case V4L2_CID_HBLANK:
> + ov2735_cci_write(ov2735, OV2735_REG_HBLANK, ctrl->val, &ret);
> + break;
> + case V4L2_CID_VBLANK:
> + vts = ctrl->val + mode->height;
> + ov2735_cci_write(ov2735, OV2735_REG_FRAME_EXP_SEPERATE_EN,
> + OV2735_FRAME_EXP_SEPERATE_EN, &ret);
> + ov2735_cci_write(ov2735, OV2735_REG_FRAME_LENGTH, vts, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + ret = ov2735_enable_test_pattern(ov2735, ctrl->val);
> + break;
> + default:
> + dev_err(ov2735->dev, "ctrl(id:0x%x, val:0x%x) is not handled\n",
> + ctrl->id, ctrl->val);
> + break;
> + }
> + ov2735_cci_write(ov2735, OV2735_REG_FRAME_SYNC, 0x01, &ret);
> +
> + pm_runtime_put(ov2735->dev);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops ov2735_ctrl_ops = {
> + .s_ctrl = ov2735_set_ctrl,
> +};
> +
> +static int ov2735_init_controls(struct ov2735 *ov2735)
> +{
> + struct v4l2_ctrl_handler *ctrl_hdlr;
> + struct v4l2_fwnode_device_properties props;
> + const struct ov2735_mode *mode = &supported_modes[0];
> + u64 hblank_def, vblank_def, exp_max;
> + int ret;
> +
> + ctrl_hdlr = &ov2735->handler;
> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 9);
> + if (ret)
> + return ret;
> +
> + ov2735->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> + V4L2_CID_PIXEL_RATE, 0, OV2735_PIXEL_RATE,
> + 1, OV2735_PIXEL_RATE);
> +
> + ov2735->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &ov2735_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + 0, 0, link_freq_menu_items);
> + if (ov2735->link_freq)
> + ov2735->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + hblank_def = mode->hts_def - mode->width;
> + ov2735->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops, V4L2_CID_HBLANK,
> + hblank_def, hblank_def, 1, hblank_def);
> +
> + vblank_def = mode->vts_def - mode->height;
> + ov2735->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> + V4L2_CID_VBLANK, vblank_def,
> + OV2735_VTS_MAX - mode->height,
> + 1, vblank_def);
> +
> + exp_max = mode->vts_def - 4;
> + ov2735->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> + V4L2_CID_EXPOSURE, OV2735_EXPOSURE_MIN,
> + exp_max, OV2735_EXPOSURE_STEP, mode->exp_def);
> +
> + ov2735->gain = v4l2_ctrl_new_std(ctrl_hdlr, &ov2735_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN, OV2735_ANALOG_GAIN_MIN,
> + OV2735_ANALOG_GAIN_MAX, OV2735_ANALOG_GAIN_STEP,
> + OV2735_ANALOG_GAIN_DEFAULT);
> +
> + ov2735->test_pattern = v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
> + &ov2735_ctrl_ops, V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov2735_test_pattern_menu) - 1,
> + 0, 0, ov2735_test_pattern_menu);
> + if (ctrl_hdlr->error) {
> + ret = ctrl_hdlr->error;
> + dev_err(ov2735->dev, "control init failed (%d)\n", ret);
> + goto err_handler_free;
> + }
> +
> + ret = v4l2_fwnode_device_parse(ov2735->dev, &props);
> + if (ret)
> + goto err_handler_free;
> +
> + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2735_ctrl_ops,
> + &props);
> + if (ret)
> + goto err_handler_free;
> +
> + ov2735->sd.ctrl_handler = ctrl_hdlr;
> +
> + return 0;
> +
> +err_handler_free:
> + v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> + return ret;
> +}
> +
> +static int ov2735_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + const struct ov2735_mode *mode;
> + const struct ov2735_reglist *reg_list;
> + const struct v4l2_mbus_framefmt *fmt;
> + int ret;
> +
> + fmt = v4l2_subdev_state_get_format(state, 0);
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->width, fmt->height);
> +
> + ret = pm_runtime_resume_and_get(ov2735->dev);
> + if (ret < 0)
> + return ret;
> +
> + reg_list = &mode->reg_list;
> + ov2735_cci_multi_reg_write(ov2735, reg_list->regvals, reg_list->num_regs, &ret);
> + if (ret) {
> + dev_err(ov2735->dev, "%s failed to send mfg header\n", __func__);
> + goto err_rpm_put;
> + }
> +
> + /* Apply customized values from user */
> + ret = __v4l2_ctrl_handler_setup(ov2735->sd.ctrl_handler);
> + if (ret)
> + goto err_rpm_put;
> +
> + /* set stream on register */
> + ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_ON, &ret);
> + if (ret)
> + goto err_rpm_put;
> +
> + return 0;
> +
> +err_rpm_put:
> + pm_runtime_put(ov2735->dev);
> + return ret;
> +}
> +
> +static int ov2735_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + int ret = 0;
> +
> + /* set stream off register */
> + ov2735_cci_write(ov2735, OV2735_REG_STREAM_CTRL, OV2735_STREAM_OFF, &ret);
> + if (ret)
> + dev_err(ov2735->dev, "%s failed to set stream\n", __func__);
> +
> + pm_runtime_put(ov2735->dev);
> +
> + return ret;
> +}
> +
> +static int ov2735_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index >= 0)
> + return -EINVAL;
> +
> + code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> + return 0;
> +}
> +
> +static int ov2735_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + u32 code;
> +
> + if (fse->index >= ARRAY_SIZE(supported_modes))
> + return -EINVAL;
> +
> + code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + if (fse->code != code)
> + return -EINVAL;
> +
> + fse->min_width = supported_modes[fse->index].width;
> + fse->max_width = fse->min_width;
> + fse->min_height = supported_modes[fse->index].height;
> + fse->max_height = fse->min_height;
> +
> + return 0;
> +}
> +
> +static void ov2735_set_framing_limits(struct ov2735 *ov2735,
> + const struct ov2735_mode *mode)
> +{
> + u32 hblank, vblank_def;
> +
> + hblank = mode->hts_def - mode->width;
> + __v4l2_ctrl_modify_range(ov2735->hblank, hblank, hblank, 1, hblank);
> +
> + vblank_def = mode->vts_def - mode->height;
> + __v4l2_ctrl_modify_range(ov2735->vblank, vblank_def, OV2735_VTS_MAX - mode->height,
> + 1, vblank_def);
> +}
> +
> +static int ov2735_set_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct v4l2_mbus_framefmt *format;
> + const struct ov2735_mode *mode;
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + format = v4l2_subdev_state_get_format(sd_state, 0);
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->format.width, fmt->format.height);
> +
> + fmt->format.width = mode->width;
> + fmt->format.height = mode->height;
> + fmt->format.field = V4L2_FIELD_NONE;
> + fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> +
> + format = v4l2_subdev_state_get_format(sd_state, 0);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> + ov2735_set_framing_limits(ov2735, mode);
> +
> + *format = fmt->format;
> +
> + return 0;
> +}
> +
> +static int ov2735_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .pad = 0,
> + .format = {
> + .code = MEDIA_BUS_FMT_SGRBG10_1X10,
> + .width = supported_modes[0].width,
> + .height = supported_modes[0].height,
> + },
> + };
> +
> + ov2735_set_pad_format(sd, state, &fmt);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops ov2735_video_ops = {
> + .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ov2735_pad_ops = {
> + .enum_mbus_code = ov2735_enum_mbus_code,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = ov2735_set_pad_format,
> + .enum_frame_size = ov2735_enum_frame_size,
> + .enable_streams = ov2735_enable_streams,
> + .disable_streams = ov2735_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops ov2735_subdev_ops = {
> + .video = &ov2735_video_ops,
> + .pad = &ov2735_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops ov2735_internal_ops = {
> + .init_state = ov2735_init_state,
> +};
> +
> +static int ov2735_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> + unsigned long delay_us;
> + int ret;
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 0);
> + fsleep(3000);
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(ov2735_supply_name),
> + ov2735->supplies);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable regulators\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
> + fsleep(3000);
> +
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
> + fsleep(3000);
> +
> + ret = clk_prepare_enable(ov2735->xclk);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to enable clock\n");
> + goto err_regulator_off;
> + }
> +
> + /* 8192 cycles prior to first SCCB transaction */
> + delay_us = DIV_ROUND_UP(8192, OV2735_XCLK_FREQ / 1000 / 1000);
> + fsleep(delay_us);
> +
> + return 0;
> +
> +err_regulator_off:
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> + return ret;
> +}
> +
> +static int ov2735_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + gpiod_set_value_cansleep(ov2735->enable_gpio, 1);
> + clk_disable_unprepare(ov2735->xclk);
> + gpiod_set_value_cansleep(ov2735->reset_gpio, 0);
> + regulator_bulk_disable(ARRAY_SIZE(ov2735_supply_name), ov2735->supplies);
> +
> + return 0;
> +}
> +
> +static int ov2735_get_regulators(struct ov2735 *ov2735)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ov2735_supply_name); i++)
> + ov2735->supplies[i].supply = ov2735_supply_name[i];
> +
> + return devm_regulator_bulk_get(ov2735->dev,
> + ARRAY_SIZE(ov2735_supply_name),
> + ov2735->supplies);
> +}
> +
> +static int ov2735_identify_module(struct ov2735 *ov2735)
> +{
> + u64 chip_id;
> + int ret = 0;
> +
> + ov2735_cci_read(ov2735, OV2735_REG_CHIPID, &chip_id, &ret);
> + if (ret)
> + return dev_err_probe(ov2735->dev, ret, "failed to read chip id %x\n",
> + OV2735_CHIPID);
> +
> + if (chip_id != OV2735_CHIPID)
> + return dev_err_probe(ov2735->dev, -EIO, "chip id mismatch: %x!=%llx\n",
> + OV2735_CHIPID, chip_id);
> +
> + return 0;
> +}
> +
> +static int ov2735_parse_endpoint(struct ov2735 *ov2735)
> +{
> + struct fwnode_handle *fwnode;
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + fwnode = dev_fwnode(ov2735->dev);
> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!ep) {
> + dev_err(ov2735->dev, "Failed to get next endpoint\n");
> + return -ENXIO;
> + }
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> + dev_err_probe(ov2735->dev, -EINVAL, "only 2 data lanes are supported\n");
> + goto error_out;
> + }
You leak the memory allocated by v4l2_fwnode_endpoint_alloc_parse()
here.
> +
> + return 0;
> +
> +error_out:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +};
> +
> +static int ov2735_probe(struct i2c_client *client)
> +{
> + struct ov2735 *ov2735;
> + unsigned int xclk_freq;
> + int ret;
> +
> + ov2735 = devm_kzalloc(&client->dev, sizeof(*ov2735), GFP_KERNEL);
> + if (!ov2735)
> + return -ENOMEM;
> +
> + ov2735->client = client;
> + ov2735->dev = &client->dev;
> +
> + v4l2_i2c_subdev_init(&ov2735->sd, client, &ov2735_subdev_ops);
> + ov2735->sd.internal_ops = &ov2735_internal_ops;
> +
> + ov2735->cci = devm_cci_regmap_init_i2c(client, 8);
> + if (IS_ERR(ov2735->cci)) {
> + ret = PTR_ERR(ov2735->cci);
> + return dev_err_probe(ov2735->dev, ret, "failed to initialize CCI\n");
> + }
> +
> + /* Set Current page to 0 */
> + ov2735->current_page = 0;
> + mutex_init(&ov2735->page_lock);
> +
> + /* Get system clock (xvclk) */
> + ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
> + if (IS_ERR(ov2735->xclk)) {
> + return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->xclk),
> + "failed to get xclk\n");
> + }
> +
> + xclk_freq = clk_get_rate(ov2735->xclk);
> + if (xclk_freq != OV2735_XCLK_FREQ)
> + return dev_err_probe(ov2735->dev, -EINVAL,
> + "xclk frequency not supported: %d Hz\n",
> + xclk_freq);
> +
> + ret = ov2735_get_regulators(ov2735);
> + if (ret)
> + return dev_err_probe(ov2735->dev, ret, "failed to get regulators\n");
> +
> + ret = ov2735_parse_endpoint(ov2735);
> + if (ret) {
> + dev_err(ov2735->dev, "failed to parse endpoint configuration\n");
> + return ret;
> + }
> +
> + ov2735->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(ov2735->reset_gpio))
> + return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->reset_gpio),
> + "failed to get reset GPIO\n");
> +
> + ov2735->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(ov2735->enable_gpio))
> + return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->enable_gpio),
> + "failed to get enable GPIO\n");
> +
> + ret = ov2735_power_on(ov2735->dev);
> + if (ret)
> + return ret;
> +
> + ret = ov2735_identify_module(ov2735);
> + if (ret)
> + goto error_power_off;
> +
> + /* This needs the pm runtime to be registered. */
> + ret = ov2735_init_controls(ov2735);
> + if (ret)
> + goto error_power_off;
> +
> + /* Initialize subdev */
> + ov2735->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov2735->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ov2735->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&ov2735->sd.entity, 1, &ov2735->pad);
> + if (ret) {
> + dev_err_probe(ov2735->dev, ret, "failed to init entity pads\n");
> + goto error_handler_free;
> + }
> +
> + ov2735->sd.state_lock = ov2735->handler.lock;
> + ret = v4l2_subdev_init_finalize(&ov2735->sd);
> + if (ret < 0) {
> + dev_err_probe(ov2735->dev, ret, "subdev init error\n");
> + goto error_media_entity;
> + }
> +
> + pm_runtime_set_active(ov2735->dev);
> + pm_runtime_enable(ov2735->dev);
> +
> + ret = v4l2_async_register_subdev_sensor(&ov2735->sd);
> + if (ret < 0) {
> + dev_err_probe(ov2735->dev, ret,
> + "failed to register ov2735 sub-device\n");
> + goto error_subdev_cleanup;
> + }
> +
> + pm_runtime_idle(ov2735->dev);
> +
> + return 0;
> +
> +error_subdev_cleanup:
> + v4l2_subdev_cleanup(&ov2735->sd);
> + pm_runtime_disable(ov2735->dev);
> + pm_runtime_set_suspended(ov2735->dev);
> +
> +error_media_entity:
> + media_entity_cleanup(&ov2735->sd.entity);
> +
> +error_handler_free:
> + v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
> +
> +error_power_off:
> + ov2735_power_off(ov2735->dev);
> +
> + return ret;
> +}
> +
> +static void ov2735_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov2735 *ov2735 = to_ov2735(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(&ov2735->sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
> +
> + pm_runtime_disable(ov2735->dev);
> + if (!pm_runtime_status_suspended(ov2735->dev))
> + ov2735_power_off(ov2735->dev);
> + pm_runtime_set_suspended(ov2735->dev);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(ov2735_pm_ops,
> + ov2735_power_off, ov2735_power_on, NULL);
> +
> +static const struct of_device_id ov2735_id[] = {
> + { .compatible = "ovti,ov2735" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov2735_id);
> +
> +static struct i2c_driver ov2735_driver = {
> + .driver = {
> + .name = "ov2735",
> + .pm = pm_ptr(&ov2735_pm_ops),
> + .of_match_table = ov2735_id,
> + },
> + .probe = ov2735_probe,
> + .remove = ov2735_remove,
> +};
> +module_i2c_driver(ov2735_driver);
> +
> +MODULE_DESCRIPTION("OV2735 Camera Sensor Driver");
> +MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>");
> +MODULE_AUTHOR("Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>");
> +MODULE_LICENSE("GPL");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
2025-07-10 13:46 ` Andy Shevchenko
2025-07-10 21:21 ` Laurent Pinchart
@ 2025-07-11 7:56 ` kernel test robot
2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2025-07-11 7:56 UTC (permalink / raw)
To: Hardevsinh Palaniya, sakari.ailus, krzk+dt, kieran.bingham
Cc: oe-kbuild-all, dave.stevenson, pratap.nirujogi, laurent.pinchart,
tarang.raval, Hardevsinh Palaniya, Himanshu Bhavani,
Mauro Carvalho Chehab, linux-media, Rob Herring, Conor Dooley,
Hans Verkuil, Ricardo Ribalda, Bryan O'Donoghue,
André Apitzsch, Arnd Bergmann, Matthias Fend,
Sylvain Petinot, Dongcheng Yan, Benjamin Mugnier,
Heimir Thor Sverrisson, Jingjing Xiong, Andy Shevchenko,
Hans de Goede, Hao Yao, devicetree, linux-kernel
Hi Hardevsinh,
kernel test robot noticed the following build errors:
[auto build test ERROR on sailus-media-tree/master]
[also build test ERROR on linus/master v6.16-rc5 next-20250710]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hardevsinh-Palaniya/dt-bindings-media-i2c-Add-ov2735-sensor/20250710-211325
base: git://linuxtv.org/sailus/media_tree.git master
patch link: https://lore.kernel.org/r/20250710131107.69017-3-hardevsinh.palaniya%40siliconsignals.io
patch subject: [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver
config: mips-randconfig-r072-20250711 (https://download.01.org/0day-ci/archive/20250711/202507111522.ERi7Nddk-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 01c97b4953e87ae455bd4c41e3de3f0f0f29c61c)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250711/202507111522.ERi7Nddk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507111522.ERi7Nddk-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/media/i2c/ov2735.c:829:17: error: call to undeclared function 'devm_v4l2_sensor_clk_get'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
829 | ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
| ^
>> drivers/media/i2c/ov2735.c:829:15: error: incompatible integer to pointer conversion assigning to 'struct clk *' from 'int' [-Wint-conversion]
829 | ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
vim +/devm_v4l2_sensor_clk_get +829 drivers/media/i2c/ov2735.c
801
802 static int ov2735_probe(struct i2c_client *client)
803 {
804 struct ov2735 *ov2735;
805 unsigned int xclk_freq;
806 int ret;
807
808 ov2735 = devm_kzalloc(&client->dev, sizeof(*ov2735), GFP_KERNEL);
809 if (!ov2735)
810 return -ENOMEM;
811
812 ov2735->client = client;
813 ov2735->dev = &client->dev;
814
815 v4l2_i2c_subdev_init(&ov2735->sd, client, &ov2735_subdev_ops);
816 ov2735->sd.internal_ops = &ov2735_internal_ops;
817
818 ov2735->cci = devm_cci_regmap_init_i2c(client, 8);
819 if (IS_ERR(ov2735->cci)) {
820 ret = PTR_ERR(ov2735->cci);
821 return dev_err_probe(ov2735->dev, ret, "failed to initialize CCI\n");
822 }
823
824 /* Set Current page to 0 */
825 ov2735->current_page = 0;
826 mutex_init(&ov2735->page_lock);
827
828 /* Get system clock (xvclk) */
> 829 ov2735->xclk = devm_v4l2_sensor_clk_get(ov2735->dev, NULL);
830 if (IS_ERR(ov2735->xclk)) {
831 return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->xclk),
832 "failed to get xclk\n");
833 }
834
835 xclk_freq = clk_get_rate(ov2735->xclk);
836 if (xclk_freq != OV2735_XCLK_FREQ)
837 return dev_err_probe(ov2735->dev, -EINVAL,
838 "xclk frequency not supported: %d Hz\n",
839 xclk_freq);
840
841 ret = ov2735_get_regulators(ov2735);
842 if (ret)
843 return dev_err_probe(ov2735->dev, ret, "failed to get regulators\n");
844
845 ret = ov2735_parse_endpoint(ov2735);
846 if (ret) {
847 dev_err(ov2735->dev, "failed to parse endpoint configuration\n");
848 return ret;
849 }
850
851 ov2735->reset_gpio = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
852 if (IS_ERR(ov2735->reset_gpio))
853 return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->reset_gpio),
854 "failed to get reset GPIO\n");
855
856 ov2735->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW);
857 if (IS_ERR(ov2735->enable_gpio))
858 return dev_err_probe(ov2735->dev, PTR_ERR(ov2735->enable_gpio),
859 "failed to get enable GPIO\n");
860
861 ret = ov2735_power_on(ov2735->dev);
862 if (ret)
863 return ret;
864
865 ret = ov2735_identify_module(ov2735);
866 if (ret)
867 goto error_power_off;
868
869 /* This needs the pm runtime to be registered. */
870 ret = ov2735_init_controls(ov2735);
871 if (ret)
872 goto error_power_off;
873
874 /* Initialize subdev */
875 ov2735->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
876 ov2735->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
877 ov2735->pad.flags = MEDIA_PAD_FL_SOURCE;
878
879 ret = media_entity_pads_init(&ov2735->sd.entity, 1, &ov2735->pad);
880 if (ret) {
881 dev_err_probe(ov2735->dev, ret, "failed to init entity pads\n");
882 goto error_handler_free;
883 }
884
885 ov2735->sd.state_lock = ov2735->handler.lock;
886 ret = v4l2_subdev_init_finalize(&ov2735->sd);
887 if (ret < 0) {
888 dev_err_probe(ov2735->dev, ret, "subdev init error\n");
889 goto error_media_entity;
890 }
891
892 pm_runtime_set_active(ov2735->dev);
893 pm_runtime_enable(ov2735->dev);
894
895 ret = v4l2_async_register_subdev_sensor(&ov2735->sd);
896 if (ret < 0) {
897 dev_err_probe(ov2735->dev, ret,
898 "failed to register ov2735 sub-device\n");
899 goto error_subdev_cleanup;
900 }
901
902 pm_runtime_idle(ov2735->dev);
903
904 return 0;
905
906 error_subdev_cleanup:
907 v4l2_subdev_cleanup(&ov2735->sd);
908 pm_runtime_disable(ov2735->dev);
909 pm_runtime_set_suspended(ov2735->dev);
910
911 error_media_entity:
912 media_entity_cleanup(&ov2735->sd.entity);
913
914 error_handler_free:
915 v4l2_ctrl_handler_free(ov2735->sd.ctrl_handler);
916
917 error_power_off:
918 ov2735_power_off(ov2735->dev);
919
920 return ret;
921 }
922
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-07-11 7:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-10 13:10 [PATCH v3 0/2] media: i2c: Add ov2735 camera sensor driver Hardevsinh Palaniya
2025-07-10 13:10 ` [PATCH v3 1/2] dt-bindings: media: i2c: Add ov2735 sensor Hardevsinh Palaniya
2025-07-10 13:33 ` Krzysztof Kozlowski
2025-07-10 13:35 ` Krzysztof Kozlowski
2025-07-10 21:05 ` Laurent Pinchart
2025-07-10 18:33 ` Rob Herring
2025-07-10 21:08 ` Laurent Pinchart
2025-07-10 13:10 ` [PATCH v3 2/2] media: i2c: add ov2735 image sensor driver Hardevsinh Palaniya
2025-07-10 13:46 ` Andy Shevchenko
2025-07-10 21:21 ` Laurent Pinchart
2025-07-11 7:56 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).