[PATCH 0/3] media: ov9734, ipu-bridge: Enable the camera on the Intel NUC M15

[PATCH 0/3] media: ov9734, ipu-bridge: Enable the camera on the Intel NUC M15

[Tin Posavec via B4 Relay]

Hi,

The camera on my Intel NUC M15 laptop (LAPRC710, Alder Lake-P, IPU6)
does not work with mainline. It is an OmniVision OV9734 (OVTI9734) whose
avdd supply and clock come from an INT3472 discrete device; its ACPI
device has neither _PS0 nor _PR0.

Patch 1 makes ov9734 defer its probe until ipu-bridge has created the
endpoint, patch 2 adds power on/off sequencing with runtime PM, and
patch 3 adds the sensor to ipu-bridge. The camera needs all three.

Tested on the NUC M15 with Ubuntu's 7.0.0-38 kernel (7.0.14) and the
series backported; the tested ov9734.c differs from this series only
by commit 7eef49c16461 ("media: v4l2-subdev: Add struct
v4l2_subdev_client_info pointer to pad ops"). The sensor streams
1296x734 SGRBG10 at 30 fps (libcamera, simple pipeline, software ISP);
it is powered off when idle, and s2idle works with the camera idle.
Not tested: S3, other boards, and kernels with lock debugging enabled.
v4l2-compliance: 45/46, the SUBSCRIBE_EVENT failure is pre-existing
(no V4L2_SUBDEV_FL_HAS_EVENTS).

I used an AI coding assistant (Claude Code) to track down the cause,
compare with how other sensor drivers handle this, and draft the
patches, hence the Assisted-by tags. I have gone through every change
myself, tested the series on this laptop, and I am responsible for the
result.

---
Tin Posavec (3):
      media: i2c: ov9734: Return -EPROBE_DEFER if no endpoint is found
      media: i2c: ov9734: Add power on/off sequencing support
      media: ipu-bridge: Add OV9734 sensor config

 drivers/media/i2c/ov9734.c           | 139 +++++++++++++++++++++++++++++++----
 drivers/media/pci/intel/ipu-bridge.c |   2 +
 2 files changed, 128 insertions(+), 13 deletions(-)
---
base-commit: d7e7bca7169873b46e857c025c10f3336c1258d8
change-id: 20261017-ov9734-ipu6-4dc9d1792c28

Best regards,
--  
Tin Posavec <tin.posavec04@gmail.com>

[PATCH 1/3] media: i2c: ov9734: Return -EPROBE_DEFER if no endpoint is found

[Tin Posavec via B4 Relay]

From: Tin Posavec <tin.posavec04@gmail.com>

With ipu-bridge, the fwnode graph endpoint of the sensor is only created
when ipu-bridge is initialized, which may happen after the sensor driver
has first probed. ov9734_check_hwcfg() then returns -ENXIO, so the probe
fails and is not retried, and the sensor stays unbound.

Return -EPROBE_DEFER instead, as several other sensor drivers used with
ipu-bridge do (e.g. ov2740 and hi556), and use dev_err_probe() to record
the reason for the deferral.

Also log an error when parsing the endpoint fails, as the link
frequency checks already do, and drop the generic error message from
probe, which would otherwise be logged on every deferred probe attempt.

Assisted-by: LLM
Signed-off-by: Tin Posavec <tin.posavec04@gmail.com>
---
 drivers/media/i2c/ov9734.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov9734.c b/drivers/media/i2c/ov9734.c
index 8f39609ad..f9280ef0e 100644
--- a/drivers/media/i2c/ov9734.c
+++ b/drivers/media/i2c/ov9734.c
@@ -837,14 +837,19 @@ static int ov9734_check_hwcfg(struct device *dev)
 	if (!fwnode)
 		return -ENXIO;
 
+	/*
+	 * Sometimes the fwnode graph is initialized by the bridge driver,
+	 * wait for this.
+	 */
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
-		return -ENXIO;
+		return dev_err_probe(dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
 
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	fwnode_handle_put(ep);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "failed to parse endpoint\n");
 
 	if (!bus_cfg.nr_of_link_frequencies) {
 		dev_err(dev, "no link frequencies defined");
@@ -893,11 +898,8 @@ static int ov9734_probe(struct i2c_client *client)
 	int ret;
 
 	ret = ov9734_check_hwcfg(&client->dev);
-	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
-			ret);
+	if (ret)
 		return ret;
-	}
 
 	ov9734 = devm_kzalloc(&client->dev, sizeof(*ov9734), GFP_KERNEL);
 	if (!ov9734)

-- 
2.53.0

[PATCH 2/3] media: i2c: ov9734: Add power on/off sequencing support

[Tin Posavec via B4 Relay]

From: Tin Posavec <tin.posavec04@gmail.com>

So far the driver has relied on ACPI to power the sensor on and off. On
the Intel NUC M15 (LAPRC710), however, the sensor's ACPI device has
neither _PS0 nor _PR0. Its avdd supply and clock are provided by an
INT3472 discrete device instead, and as the driver does not enable
them, reading the chip ID in probe fails with -EREMOTEIO.

Get the dovdd, avdd and dvdd supplies and the optional reset and
powerdown GPIOs, and power the sensor on and off in the runtime PM
callbacks. Also power it on in probe before identifying it, and off on
probe errors and in remove() if it is still powered on. The 2 ms reset
assertion time and the 20 ms start-up delay are taken from ov01a10.

Move pm_runtime_idle() after the sub-device registration, so that the
sensor is not powered off twice if the registration fails, and drop the
comment saying that the I2C core has already powered the device on.

Assisted-by: LLM
Signed-off-by: Tin Posavec <tin.posavec04@gmail.com>
---
 drivers/media/i2c/ov9734.c | 125 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 118 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov9734.c b/drivers/media/i2c/ov9734.c
index f9280ef0e..feb15de2c 100644
--- a/drivers/media/i2c/ov9734.c
+++ b/drivers/media/i2c/ov9734.c
@@ -4,9 +4,12 @@
 #include <linux/acpi.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/time.h>
 #include <linux/unaligned.h>
 
 #include <media/v4l2-ctrls.h>
@@ -322,9 +325,18 @@ static const struct ov9734_mode supported_modes[] = {
 	},
 };
 
+static const char * const ov9734_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 struct ov9734 {
 	struct device *dev;
 	struct clk *clk;
+	struct gpio_desc *reset;
+	struct gpio_desc *powerdown;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(ov9734_supply_names)];
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -878,6 +890,88 @@ static int ov9734_check_hwcfg(struct device *dev)
 	return ret;
 }
 
+static int ov9734_get_pm_resources(struct ov9734 *ov9734)
+{
+	int ret;
+
+	ov9734->reset = devm_gpiod_get_optional(ov9734->dev, "reset",
+						GPIOD_OUT_HIGH);
+	if (IS_ERR(ov9734->reset))
+		return dev_err_probe(ov9734->dev, PTR_ERR(ov9734->reset),
+				     "failed to get reset gpio\n");
+
+	ov9734->powerdown = devm_gpiod_get_optional(ov9734->dev, "powerdown",
+						    GPIOD_OUT_HIGH);
+	if (IS_ERR(ov9734->powerdown))
+		return dev_err_probe(ov9734->dev, PTR_ERR(ov9734->powerdown),
+				     "failed to get powerdown gpio\n");
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(ov9734_supply_names); i++)
+		ov9734->supplies[i].supply = ov9734_supply_names[i];
+
+	ret = devm_regulator_bulk_get(ov9734->dev,
+				      ARRAY_SIZE(ov9734_supply_names),
+				      ov9734->supplies);
+	if (ret)
+		return dev_err_probe(ov9734->dev, ret,
+				     "failed to get regulators\n");
+
+	return 0;
+}
+
+static int ov9734_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov9734 *ov9734 = to_ov9734(sd);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov9734_supply_names),
+				    ov9734->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ov9734->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock: %d\n", ret);
+		regulator_bulk_disable(ARRAY_SIZE(ov9734_supply_names),
+				       ov9734->supplies);
+		return ret;
+	}
+
+	if (ov9734->reset || ov9734->powerdown) {
+		/*
+		 * Keep reset/powerdown asserted for at least 2 ms on a
+		 * back-to-back off-on cycle.
+		 */
+		fsleep(2 * USEC_PER_MSEC);
+		gpiod_set_value_cansleep(ov9734->powerdown, 0);
+		gpiod_set_value_cansleep(ov9734->reset, 0);
+	}
+
+	/* Give the sensor time to start up before accessing it */
+	fsleep(20 * USEC_PER_MSEC);
+
+	return 0;
+}
+
+static int ov9734_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct ov9734 *ov9734 = to_ov9734(sd);
+
+	gpiod_set_value_cansleep(ov9734->reset, 1);
+	gpiod_set_value_cansleep(ov9734->powerdown, 1);
+
+	clk_disable_unprepare(ov9734->clk);
+
+	regulator_bulk_disable(ARRAY_SIZE(ov9734_supply_names),
+			       ov9734->supplies);
+
+	return 0;
+}
+
 static void ov9734_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
@@ -887,7 +981,10 @@ static void ov9734_remove(struct i2c_client *client)
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	pm_runtime_disable(ov9734->dev);
-	pm_runtime_set_suspended(ov9734->dev);
+	if (!pm_runtime_status_suspended(ov9734->dev)) {
+		ov9734_power_off(ov9734->dev);
+		pm_runtime_set_suspended(ov9734->dev);
+	}
 	mutex_destroy(&ov9734->mutex);
 }
 
@@ -918,11 +1015,20 @@ static int ov9734_probe(struct i2c_client *client)
 				     "external clock %lu is not supported",
 				     freq);
 
+	ret = ov9734_get_pm_resources(ov9734);
+	if (ret)
+		return ret;
+
 	v4l2_i2c_subdev_init(&ov9734->sd, client, &ov9734_subdev_ops);
+
+	ret = ov9734_power_on(ov9734->dev);
+	if (ret)
+		return ret;
+
 	ret = ov9734_identify_module(ov9734);
 	if (ret) {
 		dev_err(ov9734->dev, "failed to find sensor: %d", ret);
-		return ret;
+		goto probe_error_power_off;
 	}
 
 	mutex_init(&ov9734->mutex);
@@ -944,13 +1050,8 @@ static int ov9734_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
 	pm_runtime_set_active(ov9734->dev);
 	pm_runtime_enable(ov9734->dev);
-	pm_runtime_idle(ov9734->dev);
 
 	ret = v4l2_async_register_subdev_sensor(&ov9734->sd);
 	if (ret < 0) {
@@ -959,6 +1060,8 @@ static int ov9734_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup_pm;
 	}
 
+	pm_runtime_idle(ov9734->dev);
+
 	return 0;
 
 probe_error_media_entity_cleanup_pm:
@@ -970,9 +1073,16 @@ static int ov9734_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(ov9734->sd.ctrl_handler);
 	mutex_destroy(&ov9734->mutex);
 
+probe_error_power_off:
+	ov9734_power_off(ov9734->dev);
+
 	return ret;
 }
 
+static const struct dev_pm_ops ov9734_pm_ops = {
+	RUNTIME_PM_OPS(ov9734_power_off, ov9734_power_on, NULL)
+};
+
 static const struct acpi_device_id ov9734_acpi_ids[] = {
 	{ "OVTI9734", },
 	{}
@@ -983,6 +1093,7 @@ MODULE_DEVICE_TABLE(acpi, ov9734_acpi_ids);
 static struct i2c_driver ov9734_i2c_driver = {
 	.driver = {
 		.name = "ov9734",
+		.pm = pm_ptr(&ov9734_pm_ops),
 		.acpi_match_table = ov9734_acpi_ids,
 	},
 	.probe = ov9734_probe,

-- 
2.53.0

[PATCH 3/3] media: ipu-bridge: Add OV9734 sensor config

[Tin Posavec via B4 Relay]

From: Tin Posavec <tin.posavec04@gmail.com>

Add the Omnivision OV9734 (ACPI HID OVTI9734) to the
ipu_supported_sensors[] table, with a single link frequency of 180 MHz,
the only one supported by the ov9734 driver.

The sensor is the RGB camera of the Intel NUC M15 laptop (LAPRC710).
Without this entry ipu-bridge does not create a fwnode graph endpoint
for the sensor, and the camera is not enumerated.

Tested on the NUC M15: the sensor binds and streams 1296x734 SGRBG10
at 30 fps.

Assisted-by: LLM
Signed-off-by: Tin Posavec <tin.posavec04@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 3739c4a2a..4982b77de 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -120,6 +120,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
+	/* Omnivision OV9734 */
+	IPU_SENSOR_CONFIG("OVTI9734", 1, 180000000),
 	/* Sony IMX471 */
 	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
 	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */

-- 
2.53.0

Re: [PATCH 0/3] media: ov9734, ipu-bridge: Enable the camera on the Intel NUC M15

[Afonso Figuinha]

Hi Tin,

I tested this series on a Microsoft Surface Laptop 1, which has the
same OV9734 behind an IPU3 CIO2. There an INT3472 discrete device
provides the clock enable and the reset GPIO, and the _PR0 power
resource of the sensor does nothing.

I used 6.19.8-surface-3 with patches 1 and 2 applied (they apply with
one-line offsets). Patch 3 does not apply there because the sensor list
differs, so I added the same entry by hand.

- the sensor probes and the chip ID reads back;
- libcamera master with the OV9734 fixes from series 6330 [1] (IPU3
  pipeline) streams 1280x704 NV12 at 30 fps;
- the sensor is runtime suspended when idle;
- unloading and reloading the module gives no kernel warnings;
- v4l2-compliance 1.32.0 on the subdev: 45/46, only the known
  SUBSCRIBE_EVENT failure.

Not tested: probe deferral at boot with these exact patches, and
system suspend.

Tested-by: Afonso Figuinha <afonso.figuinha@gmail.com> # Surface Laptop 1 (IPU3)

I posted a duplicate series by mistake before I saw yours, and I have
withdrawn it. Its patch 4 sets V4L2_SUBDEV_FL_HAS_EVENTS and uses the
control event helpers, which fixes the SUBSCRIBE_EVENT failure. I can
send it on top of your series, or you can fold it in, as you prefer.

[1] https://patchwork.libcamera.org/project/libcamera/list/?series=6330

Thanks,
Afonso