[PATCH 0/8] drivers: media: i2c: Omnivision OV08D10 improvements

[PATCH 0/8] drivers: media: i2c: Omnivision OV08D10 improvements

[Matthias Fend]

This series provides basic device tree support and handling for power
supplies, an optional reset, and the option to use a 24MHz input clock.

In addition to a few minor fixes, a major problem with the configuration of
the sensor modes has also been resolved.

The changes have been tested on an i.MX8MP platform. Originally, the sensor
was apparently only used with ACPI - unfortunately, I do not have such a
hardware setup available.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
Matthias Fend (8):
      dt-bindings: media: i2c: document Omnivision OV08D10 CMOS image sensor
      media: i2c: ov08d10: add support for binding via device tree
      media: i2c: ov08d10: add support for reset and power management
      media: i2c: ov08d10: add support for 24 MHz input clock
      media: i2c: ov08d10: fix image vertical start setting
      media: i2c: ov08d10: remove duplicate register write
      media: i2c: ov08d10: fix some typos in comments
      media: i2c: ov08d10: add missing newline to prints

 .../bindings/media/i2c/ovti,ov08d10.yaml           | 101 +++++++++
 MAINTAINERS                                        |   1 +
 drivers/media/i2c/ov08d10.c                        | 246 ++++++++++++++++-----
 3 files changed, 297 insertions(+), 51 deletions(-)
---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260225-ov08d10-7b198f492bd4

Best regards,
-- 
Matthias Fend <matthias.fend@emfend.at>

[PATCH 1/8] dt-bindings: media: i2c: document Omnivision OV08D10 CMOS image sensor

[Matthias Fend]

Add YAML bindings for the Omnivision OV08D10 CMOS image sensor.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 .../bindings/media/i2c/ovti,ov08d10.yaml           | 101 +++++++++++++++++++++
 MAINTAINERS                                        |   1 +
 2 files changed, 102 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..96dbf61cf7c188544f4120216ae2b8e0155128b7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov08d10.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV08D10 1/4-Inch 8MP CMOS color image sensor
+
+maintainers:
+  - Jimmy Su <jimmy.su@intel.com>
+
+description:
+  The Omnivision OV08D10 is a 1/4-Inch 8MP CMOS color image sensor with an
+  active array size of 3280 x 2464. It is programmable through I2C
+  interface. Image data is transmitted via MIPI CSI-2 using 2 lanes.
+
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
+properties:
+  compatible:
+    const: ovti,ov08d10
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: MCLK input clock (6 - 27 MHz)
+    maxItems: 1
+
+  reset-gpios:
+    description: Active low XSHUTDN pin
+    maxItems: 1
+
+  dovdd-supply:
+    description: IO power supply (1.8V)
+
+  avdd-supply:
+    description: Analog power supply (2.8V)
+
+  dvdd-supply:
+    description: Core power supply (1.2V)
+
+  port:
+    $ref: /schemas/graph.yaml#/$defs/port-base
+    additionalProperties: false
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+        unevaluatedProperties: false
+
+        required:
+          - data-lanes
+          - link-frequencies
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/media/video-interfaces.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@36 {
+            compatible = "ovti,ov08d10";
+            reg = <0x36>;
+
+            clocks = <&ov08d10_clk>;
+
+            dovdd-supply = <&ov08d10_vdddo_1v8>;
+            avdd-supply = <&ov08d10_vdda_2v8>;
+            dvdd-supply = <&ov08d10_vddd_1v2>;
+
+            orientation = <2>;
+            rotation = <0>;
+
+            reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
+
+            port {
+                ov08d10_output: endpoint {
+                    data-lanes = <1 2>;
+                    link-frequencies = /bits/ 64 <360000000 720000000>;
+                    remote-endpoint = <&csi_input>;
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 55af015174a54e17cc7449e5a80b6cdc83aa6fde..2484d0bcc1f09582828cafbdb7d45dd12b55af60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19505,6 +19505,7 @@ M:	Jimmy Su <jimmy.su@intel.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media.git
+F:	Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
 F:	drivers/media/i2c/ov08d10.c
 
 OMNIVISION OV08X40 SENSOR DRIVER

-- 
2.34.1

[PATCH 2/8] media: i2c: ov08d10: add support for binding via device tree

[Matthias Fend]

The OV08D10 can be used also on embedded designs using device tree so allow
the sensor to bind to a device tree node.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index 43ec2a1f2fcffb7fa11a6268af3c2edc4df129f3..cfe18dcde174ddc1f198cb2aaa6b4a3b34045508 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -1466,10 +1466,17 @@ static const struct acpi_device_id ov08d10_acpi_ids[] = {
 MODULE_DEVICE_TABLE(acpi, ov08d10_acpi_ids);
 #endif
 
+static const struct of_device_id ov08d10_of_match[] = {
+	{ .compatible = "ovti,ov08d10" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov08d10_of_match);
+
 static struct i2c_driver ov08d10_i2c_driver = {
 	.driver = {
 		.name = "ov08d10",
 		.acpi_match_table = ACPI_PTR(ov08d10_acpi_ids),
+		.of_match_table = ov08d10_of_match,
 	},
 	.probe = ov08d10_probe,
 	.remove = ov08d10_remove,

-- 
2.34.1

[PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Matthias Fend]

Add support for the required power supplies as well as the control of an
optional sensor reset.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 104 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index cfe18dcde174ddc1f198cb2aaa6b4a3b34045508..4dba264488b3e1950016deb3fa34732871cc34fc 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -8,6 +8,7 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -514,9 +515,17 @@ static const char * const ov08d10_test_pattern_menu[] = {
 	"Standard Color Bar",
 };
 
+static const char *const ov08d10_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 struct ov08d10 {
 	struct device *dev;
 	struct clk *clk;
+	struct reset_control *reset;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov08d10_supply_names)];
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -1266,6 +1275,56 @@ static const struct v4l2_subdev_internal_ops ov08d10_internal_ops = {
 	.open = ov08d10_open,
 };
 
+static int ov08d10_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov08d10 *ov08d10 = to_ov08d10(sd);
+
+	reset_control_assert(ov08d10->reset);
+
+	regulator_bulk_disable(ARRAY_SIZE(ov08d10->supplies),
+			       ov08d10->supplies);
+
+	clk_disable_unprepare(ov08d10->clk);
+
+	return 0;
+}
+
+static int ov08d10_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov08d10 *ov08d10 = to_ov08d10(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov08d10->supplies),
+				    ov08d10->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ov08d10->clk);
+	if (ret < 0) {
+		regulator_bulk_disable(ARRAY_SIZE(ov08d10->supplies),
+				       ov08d10->supplies);
+
+		dev_err(dev, "failed to enable imaging clock: %d\n", ret);
+		return ret;
+	}
+
+	if (ov08d10->reset) {
+		/* Delay from DVDD stable to sensor XSHUTDN pull up: 5ms */
+		fsleep(5 * USEC_PER_MSEC);
+
+		reset_control_deassert(ov08d10->reset);
+
+		/* Delay from XSHUTDN pull up to SCCB start: 8ms */
+		fsleep(8 * USEC_PER_MSEC);
+	}
+
+	return 0;
+}
+
 static int ov08d10_identify_module(struct ov08d10 *ov08d10)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov08d10->sd);
@@ -1372,6 +1431,10 @@ static void ov08d10_remove(struct i2c_client *client)
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	pm_runtime_disable(ov08d10->dev);
+	if (!pm_runtime_status_suspended(ov08d10->dev)) {
+		ov08d10_power_off(ov08d10->dev);
+		pm_runtime_set_suspended(ov08d10->dev);
+	}
 	mutex_destroy(&ov08d10->mutex);
 }
 
@@ -1379,6 +1442,7 @@ static int ov08d10_probe(struct i2c_client *client)
 {
 	struct ov08d10 *ov08d10;
 	unsigned long freq;
+	unsigned int i;
 	int ret;
 
 	ov08d10 = devm_kzalloc(&client->dev, sizeof(*ov08d10), GFP_KERNEL);
@@ -1404,12 +1468,32 @@ static int ov08d10_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	ov08d10->reset = devm_reset_control_get_optional(ov08d10->dev, NULL);
+	if (IS_ERR(ov08d10->reset))
+		return dev_err_probe(ov08d10->dev, PTR_ERR(ov08d10->reset),
+				     "failed to get reset\n");
+	reset_control_assert(ov08d10->reset);
+
+	for (i = 0; i < ARRAY_SIZE(ov08d10_supply_names); i++)
+		ov08d10->supplies[i].supply = ov08d10_supply_names[i];
+
+	ret = devm_regulator_bulk_get(ov08d10->dev,
+				      ARRAY_SIZE(ov08d10->supplies),
+				      ov08d10->supplies);
+	if (ret)
+		return dev_err_probe(ov08d10->dev, ret,
+				     "failed to get regulators\n");
+
 	v4l2_i2c_subdev_init(&ov08d10->sd, client, &ov08d10_subdev_ops);
 
+	ret = ov08d10_power_on(ov08d10->dev);
+	if (ret)
+		return dev_err_probe(ov08d10->dev, ret, "failed to power on\n");
+
 	ret = ov08d10_identify_module(ov08d10);
 	if (ret) {
 		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto probe_error_power_off;
 	}
 
 	mutex_init(&ov08d10->mutex);
@@ -1430,6 +1514,9 @@ static int ov08d10_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
+	pm_runtime_set_active(ov08d10->dev);
+	pm_runtime_enable(ov08d10->dev);
+
 	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
 	if (ret < 0) {
 		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
@@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-	pm_runtime_set_active(ov08d10->dev);
-	pm_runtime_enable(ov08d10->dev);
 	pm_runtime_idle(ov08d10->dev);
 
 	return 0;
 
 probe_error_media_entity_cleanup:
+	pm_runtime_disable(ov08d10->dev);
+	pm_runtime_set_suspended(ov08d10->dev);
 	media_entity_cleanup(&ov08d10->sd.entity);
 
 probe_error_v4l2_ctrl_handler_free:
 	v4l2_ctrl_handler_free(ov08d10->sd.ctrl_handler);
 	mutex_destroy(&ov08d10->mutex);
 
+probe_error_power_off:
+	ov08d10_power_off(ov08d10->dev);
+
 	return ret;
 }
 
+static DEFINE_RUNTIME_DEV_PM_OPS(ov08d10_pm_ops,
+				 ov08d10_power_off, ov08d10_power_on, NULL);
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov08d10_acpi_ids[] = {
 	{ "OVTI08D1" },
@@ -1475,6 +1564,7 @@ MODULE_DEVICE_TABLE(of, ov08d10_of_match);
 static struct i2c_driver ov08d10_i2c_driver = {
 	.driver = {
 		.name = "ov08d10",
+		.pm = pm_ptr(&ov08d10_pm_ops),
 		.acpi_match_table = ACPI_PTR(ov08d10_acpi_ids),
 		.of_match_table = ov08d10_of_match,
 	},

-- 
2.34.1

[PATCH 4/8] media: i2c: ov08d10: add support for 24 MHz input clock

[Matthias Fend]

The sensor supports an input clock in the range of 6 to 27 MHz. Currently,
the driver only supports a 19.2 MHz clock. Extend the driver so that at
least 24 MHz, which is a typical frequency for this sensor, can also be
used.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 80 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index 4dba264488b3e1950016deb3fa34732871cc34fc..254ed22350e5ede021964d92e0fb350e7a397297 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -14,7 +14,6 @@
 #include <media/v4l2-fwnode.h>
 
 #define OV08D10_SCLK			144000000ULL
-#define OV08D10_XVCLK_19_2		19200000
 #define OV08D10_ROWCLK			36000
 #define OV08D10_DATA_LANES		2
 #define OV08D10_RGB_DEPTH		10
@@ -78,8 +77,13 @@ struct ov08d10_reg_list {
 	const struct ov08d10_reg *regs;
 };
 
+static const u32 ov08d10_xvclk_freqs[] = {
+	19200000,
+	24000000
+};
+
 struct ov08d10_link_freq_config {
-	const struct ov08d10_reg_list reg_list;
+	const struct ov08d10_reg_list reg_list[ARRAY_SIZE(ov08d10_xvclk_freqs)];
 };
 
 struct ov08d10_mode {
@@ -108,8 +112,8 @@ struct ov08d10_mode {
 	u8 data_lanes;
 };
 
-/* 3280x2460, 3264x2448 need 720Mbps/lane, 2 lanes */
-static const struct ov08d10_reg mipi_data_rate_720mbps[] = {
+/* 3280x2460, 3264x2448 need 720Mbps/lane, 2 lanes - 19.2 MHz */
+static const struct ov08d10_reg mipi_data_rate_720mbps_19_2[] = {
 	{0xfd, 0x00},
 	{0x11, 0x2a},
 	{0x14, 0x43},
@@ -119,8 +123,8 @@ static const struct ov08d10_reg mipi_data_rate_720mbps[] = {
 	{0xb7, 0x02}
 };
 
-/* 1632x1224 needs 360Mbps/lane, 2 lanes */
-static const struct ov08d10_reg mipi_data_rate_360mbps[] = {
+/* 1632x1224 needs 360Mbps/lane, 2 lanes - 19.2 MHz */
+static const struct ov08d10_reg mipi_data_rate_360mbps_19_2[] = {
 	{0xfd, 0x00},
 	{0x1a, 0x04},
 	{0x1b, 0xe1},
@@ -132,6 +136,30 @@ static const struct ov08d10_reg mipi_data_rate_360mbps[] = {
 	{0xb7, 0x02}
 };
 
+/* 3280x2460, 3264x2448 need 720Mbps/lane, 2 lanes - 24 MHz */
+static const struct ov08d10_reg mipi_data_rate_720mbps_24_0[] = {
+	{0xfd, 0x00},
+	{0x11, 0x2a},
+	{0x14, 0x43},
+	{0x1a, 0x04},
+	{0x1b, 0xb4},
+	{0x1e, 0x13},
+	{0xb7, 0x02}
+};
+
+/* 1632x1224 needs 360Mbps/lane, 2 lanes - 24 MHz */
+static const struct ov08d10_reg mipi_data_rate_360mbps_24_0[] = {
+	{0xfd, 0x00},
+	{0x1a, 0x04},
+	{0x1b, 0xb4},
+	{0x1d, 0x00},
+	{0x1c, 0x19},
+	{0x11, 0x2a},
+	{0x14, 0x54},
+	{0x1e, 0x13},
+	{0xb7, 0x02}
+};
+
 static const struct ov08d10_reg lane_2_mode_3280x2460[] = {
 	/* 3280x2460 resolution */
 	{0xfd, 0x01},
@@ -526,6 +554,7 @@ struct ov08d10 {
 	struct clk *clk;
 	struct reset_control *reset;
 	struct regulator_bulk_data supplies[ARRAY_SIZE(ov08d10_supply_names)];
+	u8 xvclk_index;
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -566,17 +595,29 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
 	},
 	{{
 		.reg_list = {
+		{
 			.num_of_regs =
-				ARRAY_SIZE(mipi_data_rate_720mbps),
-			.regs = mipi_data_rate_720mbps,
-		}
+				ARRAY_SIZE(mipi_data_rate_720mbps_19_2),
+			.regs = mipi_data_rate_720mbps_19_2,
+		},
+		{
+			.num_of_regs =
+				ARRAY_SIZE(mipi_data_rate_720mbps_24_0),
+			.regs = mipi_data_rate_720mbps_24_0,
+		}}
 	},
 	{
 		.reg_list = {
+		{
 			.num_of_regs =
-				ARRAY_SIZE(mipi_data_rate_360mbps),
-			.regs = mipi_data_rate_360mbps,
-		}
+				ARRAY_SIZE(mipi_data_rate_360mbps_19_2),
+			.regs = mipi_data_rate_360mbps_19_2,
+		},
+		{
+			.num_of_regs =
+				ARRAY_SIZE(mipi_data_rate_360mbps_24_0),
+			.regs = mipi_data_rate_360mbps_24_0,
+		}}
 	}},
 	{{
 		.width = 3280,
@@ -1029,7 +1070,8 @@ static int ov08d10_start_streaming(struct ov08d10 *ov08d10)
 
 	link_freq_index = ov08d10->cur_mode->link_freq_index;
 	reg_list =
-	    &ov08d10->priv_lane->link_freq_configs[link_freq_index].reg_list;
+		&ov08d10->priv_lane->link_freq_configs[link_freq_index]
+			 .reg_list[ov08d10->xvclk_index];
 
 	/* soft reset */
 	ret = i2c_smbus_write_byte_data(client, OV08D10_REG_PAGE, 0x00);
@@ -1457,9 +1499,15 @@ static int ov08d10_probe(struct i2c_client *client)
 				     "failed to get clock\n");
 
 	freq = clk_get_rate(ov08d10->clk);
-	if (freq != OV08D10_XVCLK_19_2)
-		dev_warn(ov08d10->dev,
-			 "external clock rate %lu is not supported\n", freq);
+	for (i = 0; i < ARRAY_SIZE(ov08d10_xvclk_freqs); i++) {
+		if (freq == ov08d10_xvclk_freqs[i])
+			break;
+	}
+	if (i >= ARRAY_SIZE(ov08d10_xvclk_freqs))
+		return dev_err_probe(ov08d10->dev, -EINVAL,
+				     "external clock rate %lu is not supported\n",
+				     freq);
+	ov08d10->xvclk_index = i;
 
 	ret = ov08d10_get_hwcfg(ov08d10);
 	if (ret) {

-- 
2.34.1

[PATCH 5/8] media: i2c: ov08d10: fix image vertical start setting

[Matthias Fend]

The current settings for the "image vertical start" register appear to be
incorrect. While this only results in an incorrect start line for native
modes, this faulty setting causes actual problems in binning mode. At least
on an i.MX8MP test system, only corrupted frames could be received.
To correct this, the recommended settings from the reference register sets
are used for all modes. Since this shifts the start by one line, the Bayer
pattern also changes, which has also been corrected.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index 254ed22350e5ede021964d92e0fb350e7a397297..ee193ba44f17d5659b3e22329b5e0ee421e3668f 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -246,7 +246,7 @@ static const struct ov08d10_reg lane_2_mode_3280x2460[] = {
 	{0x9a, 0x30},
 	{0xa8, 0x02},
 	{0xfd, 0x02},
-	{0xa1, 0x01},
+	{0xa1, 0x00},
 	{0xa2, 0x09},
 	{0xa3, 0x9c},
 	{0xa5, 0x00},
@@ -364,7 +364,7 @@ static const struct ov08d10_reg lane_2_mode_3264x2448[] = {
 	{0x9a, 0x30},
 	{0xa8, 0x02},
 	{0xfd, 0x02},
-	{0xa1, 0x09},
+	{0xa1, 0x08},
 	{0xa2, 0x09},
 	{0xa3, 0x90},
 	{0xa5, 0x08},
@@ -496,7 +496,7 @@ static const struct ov08d10_reg lane_2_mode_1632x1224[] = {
 	{0xaa, 0xd0},
 	{0xab, 0x06},
 	{0xac, 0x68},
-	{0xa1, 0x09},
+	{0xa1, 0x04},
 	{0xa2, 0x04},
 	{0xa3, 0xc8},
 	{0xa5, 0x04},
@@ -663,8 +663,8 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
 static u32 ov08d10_get_format_code(struct ov08d10 *ov08d10)
 {
 	static const u32 codes[2][2] = {
-		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10},
-		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10},
+		{ MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10 },
+		{ MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10 },
 	};
 
 	return codes[ov08d10->vflip->val][ov08d10->hflip->val];

-- 
2.34.1

[PATCH 6/8] media: i2c: ov08d10: remove duplicate register write

[Matthias Fend]

Register P1:0x33 is written twice with the same value in the
'lane_2_mode_1632x1224' sequence. Remove one unnecessary write access.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index ee193ba44f17d5659b3e22329b5e0ee421e3668f..fa8d29a62fbfa956dff6b57ccd6c9b7777967731 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -410,7 +410,6 @@ static const struct ov08d10_reg lane_2_mode_1632x1224[] = {
 	{0x07, 0x05},
 	{0x21, 0x02},
 	{0x24, 0x30},
-	{0x33, 0x03},
 	{0x31, 0x06},
 	{0x33, 0x03},
 	{0x01, 0x03},

-- 
2.34.1

[PATCH 7/8] media: i2c: ov08d10: fix some typos in comments

[Matthias Fend]

Fix some spelling errors in comments.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index fa8d29a62fbfa956dff6b57ccd6c9b7777967731..f5c187d763c4264056100b904ab9585b25909e95 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -93,13 +93,13 @@ struct ov08d10_mode {
 	/* Frame height in pixels */
 	u32 height;
 
-	/* Horizontal timining size */
+	/* Horizontal timing size */
 	u32 hts;
 
-	/* Default vertical timining size */
+	/* Default vertical timing size */
 	u32 vts_def;
 
-	/* Min vertical timining size */
+	/* Min vertical timing size */
 	u32 vts_min;
 
 	/* Link frequency needed for this resolution */
@@ -571,7 +571,7 @@ struct ov08d10 {
 	/* Current mode */
 	const struct ov08d10_mode *cur_mode;
 
-	/* To serialize asynchronus callbacks */
+	/* To serialize asynchronous callbacks */
 	struct mutex mutex;
 
 	/* lanes index */
@@ -913,7 +913,7 @@ static int ov08d10_set_ctrl(struct v4l2_ctrl *ctrl)
 					 exposure_max);
 	}
 
-	/* V4L2 controls values will be applied only when power is already up */
+	/* V4L2 control values will be applied only when power is already up */
 	if (!pm_runtime_get_if_in_use(ov08d10->dev))
 		return 0;
 

-- 
2.34.1

[PATCH 8/8] media: i2c: ov08d10: add missing newline to prints

[Matthias Fend]

Add trailing \n to dev_* prints where missing.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 drivers/media/i2c/ov08d10.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index f5c187d763c4264056100b904ab9585b25909e95..6eab9a899726aaacdddb12d2861e7b0f8d5e5fd7 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -714,7 +714,7 @@ static int ov08d10_write_reg_list(struct ov08d10 *ov08d10,
 						r_list->regs[i].val);
 		if (ret) {
 			dev_err_ratelimited(ov08d10->dev,
-					    "failed to write reg 0x%2.2x. error = %d",
+					    "failed to write reg 0x%2.2x. error = %d\n",
 					    r_list->regs[i].address, ret);
 			return ret;
 		}
@@ -1075,32 +1075,32 @@ static int ov08d10_start_streaming(struct ov08d10 *ov08d10)
 	/* soft reset */
 	ret = i2c_smbus_write_byte_data(client, OV08D10_REG_PAGE, 0x00);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to reset sensor");
+		dev_err(ov08d10->dev, "failed to reset sensor\n");
 		return ret;
 	}
 	ret = i2c_smbus_write_byte_data(client, 0x20, 0x0e);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to reset sensor");
+		dev_err(ov08d10->dev, "failed to reset sensor\n");
 		return ret;
 	}
 	usleep_range(3000, 4000);
 	ret = i2c_smbus_write_byte_data(client, 0x20, 0x0b);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to reset sensor");
+		dev_err(ov08d10->dev, "failed to reset sensor\n");
 		return ret;
 	}
 
 	/* update sensor setting */
 	ret = ov08d10_write_reg_list(ov08d10, reg_list);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to set plls");
+		dev_err(ov08d10->dev, "failed to set plls\n");
 		return ret;
 	}
 
 	reg_list = &ov08d10->cur_mode->reg_list;
 	ret = ov08d10_write_reg_list(ov08d10, reg_list);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to set mode");
+		dev_err(ov08d10->dev, "failed to set mode\n");
 		return ret;
 	}
 
@@ -1127,19 +1127,19 @@ static void ov08d10_stop_streaming(struct ov08d10 *ov08d10)
 
 	ret = i2c_smbus_write_byte_data(client, OV08D10_REG_PAGE, 0x00);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to stop streaming");
+		dev_err(ov08d10->dev, "failed to stop streaming\n");
 		return;
 	}
 	ret = i2c_smbus_write_byte_data(client, OV08D10_REG_MODE_SELECT,
 					OV08D10_MODE_STANDBY);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to stop streaming");
+		dev_err(ov08d10->dev, "failed to stop streaming\n");
 		return;
 	}
 
 	ret = i2c_smbus_write_byte_data(client, OV08D10_REG_PAGE, 0x01);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to stop streaming");
+		dev_err(ov08d10->dev, "failed to stop streaming\n");
 		return;
 	}
 }
@@ -1425,7 +1425,7 @@ static int ov08d10_get_hwcfg(struct ov08d10 *ov08d10)
 
 	/* Get number of data lanes */
 	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
-		dev_err(dev, "number of CSI2 data lanes %d is not supported",
+		dev_err(dev, "number of CSI2 data lanes %d is not supported\n",
 			bus_cfg.bus.mipi_csi2.num_data_lanes);
 		ret = -EINVAL;
 		goto check_hwcfg_error;
@@ -1437,7 +1437,7 @@ static int ov08d10_get_hwcfg(struct ov08d10 *ov08d10)
 	ov08d10->modes_size = ov08d10_modes_num(ov08d10);
 
 	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_err(dev, "no link frequencies defined");
+		dev_err(dev, "no link frequencies defined\n");
 		ret = -EINVAL;
 		goto check_hwcfg_error;
 	}
@@ -1450,7 +1450,7 @@ static int ov08d10_get_hwcfg(struct ov08d10 *ov08d10)
 		}
 
 		if (j == bus_cfg.nr_of_link_frequencies) {
-			dev_err(dev, "no link frequency %lld supported",
+			dev_err(dev, "no link frequency %lld supported\n",
 				ov08d10->priv_lane->link_freq_menu[i]);
 			ret = -EINVAL;
 			goto check_hwcfg_error;
@@ -1510,7 +1510,7 @@ static int ov08d10_probe(struct i2c_client *client)
 
 	ret = ov08d10_get_hwcfg(ov08d10);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to get HW configuration: %d",
+		dev_err(ov08d10->dev, "failed to get HW configuration: %d\n",
 			ret);
 		return ret;
 	}
@@ -1539,7 +1539,7 @@ static int ov08d10_probe(struct i2c_client *client)
 
 	ret = ov08d10_identify_module(ov08d10);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
+		dev_err(ov08d10->dev, "failed to find sensor: %d\n", ret);
 		goto probe_error_power_off;
 	}
 
@@ -1547,7 +1547,7 @@ static int ov08d10_probe(struct i2c_client *client)
 	ov08d10->cur_mode = &ov08d10->priv_lane->sp_modes[0];
 	ret = ov08d10_init_controls(ov08d10);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to init controls: %d", ret);
+		dev_err(ov08d10->dev, "failed to init controls: %d\n", ret);
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
@@ -1557,7 +1557,7 @@ static int ov08d10_probe(struct i2c_client *client)
 	ov08d10->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&ov08d10->sd.entity, 1, &ov08d10->pad);
 	if (ret) {
-		dev_err(ov08d10->dev, "failed to init entity pads: %d", ret);
+		dev_err(ov08d10->dev, "failed to init entity pads: %d\n", ret);
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
@@ -1566,7 +1566,7 @@ static int ov08d10_probe(struct i2c_client *client)
 
 	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
 	if (ret < 0) {
-		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
+		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d\n",
 			ret);
 		goto probe_error_media_entity_cleanup;
 	}

-- 
2.34.1

Re: [PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Philipp Zabel]

On Do, 2026-02-26 at 09:56 +0100, Matthias Fend wrote:
> Add support for the required power supplies as well as the control of an
> optional sensor reset.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  drivers/media/i2c/ov08d10.c | 104 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 97 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
> index cfe18dcde174ddc1f198cb2aaa6b4a3b34045508..4dba264488b3e1950016deb3fa34732871cc34fc 100644
> --- a/drivers/media/i2c/ov08d10.c
> +++ b/drivers/media/i2c/ov08d10.c
[...]
> @@ -1379,6 +1442,7 @@ static int ov08d10_probe(struct i2c_client *client)
>  {
>  	struct ov08d10 *ov08d10;
>  	unsigned long freq;
> +	unsigned int i;
>  	int ret;
>  
>  	ov08d10 = devm_kzalloc(&client->dev, sizeof(*ov08d10), GFP_KERNEL);
> @@ -1404,12 +1468,32 @@ static int ov08d10_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	ov08d10->reset = devm_reset_control_get_optional(ov08d10->dev, NULL);

Please use devm_reset_control_get_optional_exclusive() directly.

> +	if (IS_ERR(ov08d10->reset))
> +		return dev_err_probe(ov08d10->dev, PTR_ERR(ov08d10->reset),
> +				     "failed to get reset\n");
> +	reset_control_assert(ov08d10->reset);
> +
> +	for (i = 0; i < ARRAY_SIZE(ov08d10_supply_names); i++)
> +		ov08d10->supplies[i].supply = ov08d10_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(ov08d10->dev,
> +				      ARRAY_SIZE(ov08d10->supplies),
> +				      ov08d10->supplies);
> +	if (ret)
> +		return dev_err_probe(ov08d10->dev, ret,
> +				     "failed to get regulators\n");
> +
>  	v4l2_i2c_subdev_init(&ov08d10->sd, client, &ov08d10_subdev_ops);
>  
> +	ret = ov08d10_power_on(ov08d10->dev);
> +	if (ret)
> +		return dev_err_probe(ov08d10->dev, ret, "failed to power on\n");
> +
>  	ret = ov08d10_identify_module(ov08d10);
>  	if (ret) {
>  		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
> -		return ret;
> +		goto probe_error_power_off;
>  	}
>  
>  	mutex_init(&ov08d10->mutex);
> @@ -1430,6 +1514,9 @@ static int ov08d10_probe(struct i2c_client *client)
>  		goto probe_error_v4l2_ctrl_handler_free;
>  	}
>  
> +	pm_runtime_set_active(ov08d10->dev);
> +	pm_runtime_enable(ov08d10->dev);
> +
>  	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
>  	if (ret < 0) {
>  		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
> @@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
>  		goto probe_error_media_entity_cleanup;
>  	}
>  
> -	/*
> -	 * Device is already turned on by i2c-core with ACPI domain PM.
> -	 * Enable runtime PM and turn off the device.
> -	 */

The commit message does not explain why this comment is dropped.

> -	pm_runtime_set_active(ov08d10->dev);
> -	pm_runtime_enable(ov08d10->dev);
>  	pm_runtime_idle(ov08d10->dev);
>  
>  	return 0;
>  
>  probe_error_media_entity_cleanup:
> +	pm_runtime_disable(ov08d10->dev);
> +	pm_runtime_set_suspended(ov08d10->dev);

Does this do the correct thing if v4l2_async_register_subdev_sensor()
returns -EPROBE_DEFER (for example via privacy led) and then it probes
a second time? It looks like the assumption pm_runtime_set_active()
doesn't hold then.

>  	media_entity_cleanup(&ov08d10->sd.entity);
>  
>  probe_error_v4l2_ctrl_handler_free:
>  	v4l2_ctrl_handler_free(ov08d10->sd.ctrl_handler);
>  	mutex_destroy(&ov08d10->mutex);
>  
> +probe_error_power_off:
> +	ov08d10_power_off(ov08d10->dev);
> +
>  	return ret;
>  }

regards
Philipp

Re: [PATCH 7/8] media: i2c: ov08d10: fix some typos in comments

[Philipp Zabel]

On Do, 2026-02-26 at 09:56 +0100, Matthias Fend wrote:
> Fix some spelling errors in comments.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Matthias Fend]

Hi Philipp,

thanks for your feedback.

Am 26.02.2026 um 11:13 schrieb Philipp Zabel:
> On Do, 2026-02-26 at 09:56 +0100, Matthias Fend wrote:
>> Add support for the required power supplies as well as the control of an
>> optional sensor reset.
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   drivers/media/i2c/ov08d10.c | 104 +++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 97 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
>> index cfe18dcde174ddc1f198cb2aaa6b4a3b34045508..4dba264488b3e1950016deb3fa34732871cc34fc 100644
>> --- a/drivers/media/i2c/ov08d10.c
>> +++ b/drivers/media/i2c/ov08d10.c
> [...]
>> @@ -1379,6 +1442,7 @@ static int ov08d10_probe(struct i2c_client *client)
>>   {
>>   	struct ov08d10 *ov08d10;
>>   	unsigned long freq;
>> +	unsigned int i;
>>   	int ret;
>>   
>>   	ov08d10 = devm_kzalloc(&client->dev, sizeof(*ov08d10), GFP_KERNEL);
>> @@ -1404,12 +1468,32 @@ static int ov08d10_probe(struct i2c_client *client)
>>   		return ret;
>>   	}
>>   
>> +	ov08d10->reset = devm_reset_control_get_optional(ov08d10->dev, NULL);
> 
> Please use devm_reset_control_get_optional_exclusive() directly.

ACK

> 
>> +	if (IS_ERR(ov08d10->reset))
>> +		return dev_err_probe(ov08d10->dev, PTR_ERR(ov08d10->reset),
>> +				     "failed to get reset\n");
>> +	reset_control_assert(ov08d10->reset);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ov08d10_supply_names); i++)
>> +		ov08d10->supplies[i].supply = ov08d10_supply_names[i];
>> +
>> +	ret = devm_regulator_bulk_get(ov08d10->dev,
>> +				      ARRAY_SIZE(ov08d10->supplies),
>> +				      ov08d10->supplies);
>> +	if (ret)
>> +		return dev_err_probe(ov08d10->dev, ret,
>> +				     "failed to get regulators\n");
>> +
>>   	v4l2_i2c_subdev_init(&ov08d10->sd, client, &ov08d10_subdev_ops);
>>   
>> +	ret = ov08d10_power_on(ov08d10->dev);
>> +	if (ret)
>> +		return dev_err_probe(ov08d10->dev, ret, "failed to power on\n");
>> +
>>   	ret = ov08d10_identify_module(ov08d10);
>>   	if (ret) {
>>   		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
>> -		return ret;
>> +		goto probe_error_power_off;
>>   	}
>>   
>>   	mutex_init(&ov08d10->mutex);
>> @@ -1430,6 +1514,9 @@ static int ov08d10_probe(struct i2c_client *client)
>>   		goto probe_error_v4l2_ctrl_handler_free;
>>   	}
>>   
>> +	pm_runtime_set_active(ov08d10->dev);
>> +	pm_runtime_enable(ov08d10->dev);
>> +
>>   	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
>>   	if (ret < 0) {
>>   		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
>> @@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
>>   		goto probe_error_media_entity_cleanup;
>>   	}
>>   
>> -	/*
>> -	 * Device is already turned on by i2c-core with ACPI domain PM.
>> -	 * Enable runtime PM and turn off the device.
>> -	 */
> 
> The commit message does not explain why this comment is dropped.

I didn't find the comment particularly helpful and since other sensors 
manage without it and there's now more than just ACPI, and "turn off" 
happens later, I thought it was fine to just drop the comment.

If you think it should still be included, I'd be happy to change it.

> 
>> -	pm_runtime_set_active(ov08d10->dev);
>> -	pm_runtime_enable(ov08d10->dev);
>>   	pm_runtime_idle(ov08d10->dev);
>>   
>>   	return 0;
>>   
>>   probe_error_media_entity_cleanup:
>> +	pm_runtime_disable(ov08d10->dev);
>> +	pm_runtime_set_suspended(ov08d10->dev);
> 
> Does this do the correct thing if v4l2_async_register_subdev_sensor()
> returns -EPROBE_DEFER (for example via privacy led) and then it probes
> a second time? It looks like the assumption pm_runtime_set_active()
> doesn't hold then.

At least it works as expected for me. But as mentioned, I don't have an 
ACPI hardware setup available. Does your point maybe refer to ACPI, or 
what exactly do you mean?
To me, it now looks very similar to other Omnivision ACPI drivers – do 
you perhaps have a specific suggestion for what should be changed?

Thanks a lot
  ~Matthias

> 
>>   	media_entity_cleanup(&ov08d10->sd.entity);
>>   
>>   probe_error_v4l2_ctrl_handler_free:
>>   	v4l2_ctrl_handler_free(ov08d10->sd.ctrl_handler);
>>   	mutex_destroy(&ov08d10->mutex);
>>   
>> +probe_error_power_off:
>> +	ov08d10_power_off(ov08d10->dev);
>> +
>>   	return ret;
>>   }
> 
> regards
> Philipp

Re: [PATCH 1/8] dt-bindings: media: i2c: document Omnivision OV08D10 CMOS image sensor

[Krzysztof Kozlowski]

On Thu, Feb 26, 2026 at 09:56:01AM +0100, Matthias Fend wrote:
> Add YAML bindings for the Omnivision OV08D10 CMOS image sensor.

Drop YAML. You add DT bindings, not YAML. No YAML is bound here to
anything.

> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  .../bindings/media/i2c/ovti,ov08d10.yaml           | 101 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  2 files changed, 102 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..96dbf61cf7c188544f4120216ae2b8e0155128b7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov08d10.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV08D10 1/4-Inch 8MP CMOS color image sensor
> +
> +maintainers:
> +  - Jimmy Su <jimmy.su@intel.com>
> +
> +description:
> +  The Omnivision OV08D10 is a 1/4-Inch 8MP CMOS color image sensor with an
> +  active array size of 3280 x 2464. It is programmable through I2C
> +  interface. Image data is transmitted via MIPI CSI-2 using 2 lanes.
> +
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
> +properties:
> +  compatible:
> +    const: ovti,ov08d10
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description: MCLK input clock (6 - 27 MHz)
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: Active low XSHUTDN pin

So maybe rather powerdown-gpios, see gpio-consumer-common.yaml?

> +    maxItems: 1

Best regards,
Krzysztof

Re: [PATCH 1/8] dt-bindings: media: i2c: document Omnivision OV08D10 CMOS image sensor

[Matthias Fend]

Hi Krzysztof,

thanks for your comments.

Am 27.02.2026 um 11:45 schrieb Krzysztof Kozlowski:
> On Thu, Feb 26, 2026 at 09:56:01AM +0100, Matthias Fend wrote:
>> Add YAML bindings for the Omnivision OV08D10 CMOS image sensor.
> 
> Drop YAML. You add DT bindings, not YAML. No YAML is bound here to
> anything.

ACK

> 
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   .../bindings/media/i2c/ovti,ov08d10.yaml           | 101 +++++++++++++++++++++
>>   MAINTAINERS                                        |   1 +
>>   2 files changed, 102 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..96dbf61cf7c188544f4120216ae2b8e0155128b7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov08d10.yaml
>> @@ -0,0 +1,101 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov08d10.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Omnivision OV08D10 1/4-Inch 8MP CMOS color image sensor
>> +
>> +maintainers:
>> +  - Jimmy Su <jimmy.su@intel.com>
>> +
>> +description:
>> +  The Omnivision OV08D10 is a 1/4-Inch 8MP CMOS color image sensor with an
>> +  active array size of 3280 x 2464. It is programmable through I2C
>> +  interface. Image data is transmitted via MIPI CSI-2 using 2 lanes.
>> +
>> +allOf:
>> +  - $ref: /schemas/media/video-interface-devices.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: ovti,ov08d10
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    description: MCLK input clock (6 - 27 MHz)
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    description: Active low XSHUTDN pin
> 
> So maybe rather powerdown-gpios, see gpio-consumer-common.yaml?

As activating the XSHUTDN pin on this sensor works like a reset, I guess 
it's okay as it is.

 From what I've seen, it's also common practice for some other 
Omnivision sensors to designate the XSHUTDN pin as the reset pin.
This is also the case, for example, with the recently added OS05B10.

During a review for another sensor, I was asked to use the reset 
controller framework instead of the GPIO pin for the reset function, 
which I did in this case as well.
To my knowledge, the fallback from the reset controller to a simple 
reset pin only works with the 'reset-gpios' property.

Thanks
  ~Matthias

> 
>> +    maxItems: 1
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/8] dt-bindings: media: i2c: document Omnivision OV08D10 CMOS image sensor

[Sakari Ailus]

Hi Matthias, Krzysztof,

On Fri, Feb 27, 2026 at 04:10:50PM +0100, Matthias Fend wrote:
> > > +  reset-gpios:
> > > +    description: Active low XSHUTDN pin
> > 
> > So maybe rather powerdown-gpios, see gpio-consumer-common.yaml?
> 
> As activating the XSHUTDN pin on this sensor works like a reset, I guess
> it's okay as it is.
> 
> From what I've seen, it's also common practice for some other Omnivision
> sensors to designate the XSHUTDN pin as the reset pin.
> This is also the case, for example, with the recently added OS05B10.
> 
> During a review for another sensor, I was asked to use the reset controller
> framework instead of the GPIO pin for the reset function, which I did in
> this case as well.
> To my knowledge, the fallback from the reset controller to a simple reset
> pin only works with the 'reset-gpios' property.

I'd use "reset" for this, too: it's first and foremost reset. In most
camera sensors with "xshutdown" or similar pin the function of the pin is
documented to be hardware reset. Most such devices still use very little
power in hardware reset state but from software point of view that is still
secondary to the reset function.

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Sakari Ailus]

Hi Matthias,

On Thu, Feb 26, 2026 at 09:30:08PM +0100, Matthias Fend wrote:
> Hi Philipp,
> 
> thanks for your feedback.
> 
> Am 26.02.2026 um 11:13 schrieb Philipp Zabel:
> > On Do, 2026-02-26 at 09:56 +0100, Matthias Fend wrote:
> > > Add support for the required power supplies as well as the control of an
> > > optional sensor reset.
> > > 
> > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > ---
> > >   drivers/media/i2c/ov08d10.c | 104 +++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 97 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
> > > index cfe18dcde174ddc1f198cb2aaa6b4a3b34045508..4dba264488b3e1950016deb3fa34732871cc34fc 100644
> > > --- a/drivers/media/i2c/ov08d10.c
> > > +++ b/drivers/media/i2c/ov08d10.c
> > [...]
> > > @@ -1379,6 +1442,7 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   {
> > >   	struct ov08d10 *ov08d10;
> > >   	unsigned long freq;
> > > +	unsigned int i;
> > >   	int ret;
> > >   	ov08d10 = devm_kzalloc(&client->dev, sizeof(*ov08d10), GFP_KERNEL);
> > > @@ -1404,12 +1468,32 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		return ret;
> > >   	}
> > > +	ov08d10->reset = devm_reset_control_get_optional(ov08d10->dev, NULL);
> > 
> > Please use devm_reset_control_get_optional_exclusive() directly.
> 
> ACK
> 
> > 
> > > +	if (IS_ERR(ov08d10->reset))
> > > +		return dev_err_probe(ov08d10->dev, PTR_ERR(ov08d10->reset),
> > > +				     "failed to get reset\n");
> > > +	reset_control_assert(ov08d10->reset);
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(ov08d10_supply_names); i++)
> > > +		ov08d10->supplies[i].supply = ov08d10_supply_names[i];
> > > +
> > > +	ret = devm_regulator_bulk_get(ov08d10->dev,
> > > +				      ARRAY_SIZE(ov08d10->supplies),
> > > +				      ov08d10->supplies);
> > > +	if (ret)
> > > +		return dev_err_probe(ov08d10->dev, ret,
> > > +				     "failed to get regulators\n");
> > > +
> > >   	v4l2_i2c_subdev_init(&ov08d10->sd, client, &ov08d10_subdev_ops);
> > > +	ret = ov08d10_power_on(ov08d10->dev);
> > > +	if (ret)
> > > +		return dev_err_probe(ov08d10->dev, ret, "failed to power on\n");
> > > +
> > >   	ret = ov08d10_identify_module(ov08d10);
> > >   	if (ret) {
> > >   		dev_err(ov08d10->dev, "failed to find sensor: %d", ret);
> > > -		return ret;
> > > +		goto probe_error_power_off;
> > >   	}
> > >   	mutex_init(&ov08d10->mutex);
> > > @@ -1430,6 +1514,9 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		goto probe_error_v4l2_ctrl_handler_free;
> > >   	}
> > > +	pm_runtime_set_active(ov08d10->dev);
> > > +	pm_runtime_enable(ov08d10->dev);
> > > +
> > >   	ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
> > >   	if (ret < 0) {
> > >   		dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d",
> > > @@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		goto probe_error_media_entity_cleanup;
> > >   	}
> > > -	/*
> > > -	 * Device is already turned on by i2c-core with ACPI domain PM.
> > > -	 * Enable runtime PM and turn off the device.
> > > -	 */
> > 
> > The commit message does not explain why this comment is dropped.
> 
> I didn't find the comment particularly helpful and since other sensors
> manage without it and there's now more than just ACPI, and "turn off"
> happens later, I thought it was fine to just drop the comment.
> 
> If you think it should still be included, I'd be happy to change it.

I'd drop it and mention this in the commit message, there's nothing
specific to this driver here (nor even camera sensors in general).

> 
> > 
> > > -	pm_runtime_set_active(ov08d10->dev);
> > > -	pm_runtime_enable(ov08d10->dev);
> > >   	pm_runtime_idle(ov08d10->dev);
> > >   	return 0;
> > >   probe_error_media_entity_cleanup:
> > > +	pm_runtime_disable(ov08d10->dev);
> > > +	pm_runtime_set_suspended(ov08d10->dev);
> > 
> > Does this do the correct thing if v4l2_async_register_subdev_sensor()
> > returns -EPROBE_DEFER (for example via privacy led) and then it probes
> > a second time? It looks like the assumption pm_runtime_set_active()
> > doesn't hold then.
> 
> At least it works as expected for me. But as mentioned, I don't have an ACPI
> hardware setup available. Does your point maybe refer to ACPI, or what
> exactly do you mean?
> To me, it now looks very similar to other Omnivision ACPI drivers – do you
> perhaps have a specific suggestion for what should be changed?

Moving pm_runtime_set_active() and pm_runtime_enable() calls above
v4l2_async_register_subdev_sensor() (and related teardown changes) are
actually a bugfix (the driver's external APIs are exposed before runtime PM
is enabled and thus e.g. streaming on will fail). It'd be nice to post that
separately from non-ACPI power management changes.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/8] media: i2c: ov08d10: add support for reset and power management

[Philipp Zabel]

Hi Matthias,

On Do, 2026-02-26 at 21:30 +0100, Matthias Fend wrote:

[...]
> > 
> > > @@ -1437,26 +1524,28 @@ static int ov08d10_probe(struct i2c_client *client)
> > >   		goto probe_error_media_entity_cleanup;
> > >   	}
> > >   
> > > -	/*
> > > -	 * Device is already turned on by i2c-core with ACPI domain PM.
> > > -	 * Enable runtime PM and turn off the device.
> > > -	 */
> > 
> > The commit message does not explain why this comment is dropped.
> 
> I didn't find the comment particularly helpful and since other sensors 
> manage without it and there's now more than just ACPI, and "turn off" 
> happens later, I thought it was fine to just drop the comment.
> 
> If you think it should still be included, I'd be happy to change it.

That's ok, I'd just add this explanation to the commit message.
Or something along the lines of dropping a comment that's obvious.

I wouldn't have pointed this out if it didn't look to me like the
"device is already turned on" precondition had maybe changed with the
point discussed below.

> > > -	pm_runtime_set_active(ov08d10->dev);
> > > -	pm_runtime_enable(ov08d10->dev);
> > >   	pm_runtime_idle(ov08d10->dev);
> > >   
> > >   	return 0;
> > >   
> > >   probe_error_media_entity_cleanup:
> > > +	pm_runtime_disable(ov08d10->dev);
> > > +	pm_runtime_set_suspended(ov08d10->dev);
> > 
> > Does this do the correct thing if v4l2_async_register_subdev_sensor()
> > returns -EPROBE_DEFER (for example via privacy led) and then it probes
> > a second time? It looks like the assumption pm_runtime_set_active()
> > doesn't hold then.
> 
> At least it works as expected for me. But as mentioned, I don't have an 
> ACPI hardware setup available. Does your point maybe refer to ACPI, or 
> what exactly do you mean?

I confused myself, thinking that disabling the device was moved into
the error path. But pm_runtime_idle() is still only called directly
before return 0, so my point is moot. Sorry for the noise.

regards
Philipp