[PATCH 0/4] media: ov9734: Support the Microsoft Surface Laptop 1 front camera

[PATCH 0/4] media: ov9734: Support the Microsoft Surface Laptop 1 front camera

[Afonso Figuinha]

The front camera of the Microsoft Surface Laptop 1 is an OmniVision
OV9734 connected to the IPU3 CIO2. It does not work today for two
reasons. The ipu-bridge does not know the OVTI9734 HID, so no fwnode
graph is built for the sensor. The ov9734 driver relies on ACPI to
power the sensor, while on this laptop an INT3472 discrete device
(SKC1 in the DSDT) controls the clock and the reset line.

Patch 1 adds the sensor to ipu-bridge. Patch 2 makes the driver wait
for the endpoint that the bridge creates. Patch 3 adds the reset GPIO,
the supplies and the PM callbacks. Patch 4 adds control event support,
so v4l2-compliance passes.

The series is based on media-committers next (v7.4-rc1).
ov9734.o and ipu-bridge.o build there with W=1 and C=1 (sparse from
git) with no warnings.

Hardware tests ran on a Surface Laptop 1 with 6.19.8-surface-3, the
linux-surface kernel. Patches 2 to 4 apply to 6.19.8 with offsets
only, apart from the v4l2_subdev_client_info API change, which they do
not touch. That kernel also carries the linux-surface camera patches,
and I did not check which of them this result depends on. Patch 1 does
not apply there because the sensor list differs, so the same entry was
added by hand.
Results:
- the sensor probes and the chip ID reads back as 0x9734;
- libcamera (IPU3 pipeline) streams 1280x704 at 30 fps;
- the sensor is runtime suspended when idle and active while
  streaming;
- unloading and reloading the module works with no warnings;
- v4l2-compliance 1.32.0 on the sensor subdev: 46 tests, 46 passed,
  0 warnings. Without patch 4, the event subscription test fails.

Not tested:
- a boot of the posted base;
- probe deferral at boot with this exact revision of patch 2. The
  sensor binds at boot, but a deferred probe is no longer logged, so I
  could not confirm the sensor driver probed before ipu3-cio2 on that
  boot. An earlier revision with the same logic logged the deferral
  and then bound;
- platforms that power the sensor through ACPI, which the driver
  supported before this series. Where no reset GPIO is described, the
  callbacks only enable and disable the clock, and the supplies fall
  back to dummy regulators;
- system suspend while streaming.

libcamera also needs changes for this sensor. I am sending them to
libcamera-devel at the same time.

Tool disclosure: this series was developed with an AI coding
assistant (Claude Code, model Claude Opus 5.5) in an interactive
session on the laptop. I asked it to get the front camera working. It
read the DSDT, the driver sources and the kernel logs, wrote the
patches, drafted the commit messages and this cover letter, and ran
the builds and tests listed above on the laptop. I reviewed every
change and can answer review comments on it.

Afonso Figuinha (4):
  media: ipu-bridge: Add OV9734 to the supported sensors list
  media: i2c: ov9734: Defer probe until the fwnode graph endpoint exists
  media: i2c: ov9734: Add reset GPIO, supplies and PM callbacks
  media: i2c: ov9734: Support control events

 drivers/media/i2c/ov9734.c           | 115 ++++++++++++++++++++++++---
 drivers/media/pci/intel/ipu-bridge.c |   2 +
 2 files changed, 107 insertions(+), 10 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.53.0

[PATCH 1/4] media: ipu-bridge: Add OV9734 to the supported sensors list

[Afonso Figuinha]

The Microsoft Surface Laptop 1 uses an OmniVision OV9734 as its front
camera. It is connected to the IPU3 CIO2 and described in ACPI with HID
OVTI9734. Add it to the list of supported sensors, so the bridge builds
the fwnode graph the ov9734 driver needs.

The ov9734 driver supports only a 180 MHz link frequency, so list that
one.

The change and this description were drafted with an AI coding
assistant (Claude Code, model Claude Opus 5.5). The author reviewed
them and tested them on a Microsoft Surface Laptop 1.

Signed-off-by: Afonso Figuinha <afonso.figuinha@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

[PATCH 2/4] media: i2c: ov9734: Defer probe until the fwnode graph endpoint exists

[Afonso Figuinha]

On ACPI platforms with an Intel IPU the endpoint is created by the
ipu-bridge code when the CIO2 driver probes, which can happen after the
sensor driver probes. ov9734_check_hwcfg() then fails with -ENXIO and
the sensor is never bound.

Return -EPROBE_DEFER through dev_err_probe() when the endpoint is
missing, as other sensor drivers used on these platforms do. Return the
error from probe() as it is, so the deferral reason is kept and a
deferred probe does not log an error.

The change and this description were drafted with an AI coding
assistant (Claude Code, model Claude Opus 5.5). The author reviewed
them and tested them on a Microsoft Surface Laptop 1.

Signed-off-by: Afonso Figuinha <afonso.figuinha@gmail.com>
---
 drivers/media/i2c/ov9734.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov9734.c b/drivers/media/i2c/ov9734.c
index 8f39609ad..5f3e277b9 100644
--- a/drivers/media/i2c/ov9734.c
+++ b/drivers/media/i2c/ov9734.c
@@ -837,9 +837,14 @@ 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);
@@ -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 3/4] media: i2c: ov9734: Add reset GPIO, supplies and PM callbacks

[Afonso Figuinha]

The driver relies on ACPI to power the sensor. On the Microsoft Surface
Laptop 1 an INT3472 discrete device controls the sensor clock and reset
line instead, so the sensor stays in reset and probe fails when reading
the chip ID.

Request the optional reset GPIO and the dovdd, avdd and dvdd supplies,
enable the clock, and add runtime PM callbacks that power the sensor on
and off. Power the sensor on in probe before identifying it. Call
pm_runtime_idle() only once the subdev is registered, so the probe
error path does not power the sensor off twice.

The INT3472 on the Surface Laptop 1 provides no supplies, so they fall
back to dummy regulators there. On platforms where ACPI powers the
sensor and no reset GPIO is described, the callbacks only enable and
disable the clock.

The public OV9734 documentation does not give power-up timings. The
2 ms wait before releasing reset and the 6 ms wait after it were chosen
empirically and work on the Surface Laptop 1.

The change and this description were drafted with an AI coding
assistant (Claude Code, model Claude Opus 5.5). The author reviewed
them and tested them on a Microsoft Surface Laptop 1.

Signed-off-by: Afonso Figuinha <afonso.figuinha@gmail.com>
---
 drivers/media/i2c/ov9734.c | 93 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 89 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov9734.c b/drivers/media/i2c/ov9734.c
index 5f3e277b9..bf19ea56f 100644
--- a/drivers/media/i2c/ov9734.c
+++ b/drivers/media/i2c/ov9734.c
@@ -4,9 +4,11 @@
 #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/unaligned.h>
 
 #include <media/v4l2-ctrls.h>
@@ -322,9 +324,17 @@ 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 regulator_bulk_data supplies[ARRAY_SIZE(ov9734_supply_names)];
 
 	struct v4l2_subdev sd;
 	struct media_pad pad;
@@ -824,6 +834,51 @@ static int ov9734_identify_module(struct ov9734 *ov9734)
 	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 = clk_prepare_enable(ov9734->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov9734_supply_names),
+				    ov9734->supplies);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators: %d\n", ret);
+		clk_disable_unprepare(ov9734->clk);
+		return ret;
+	}
+
+	if (ov9734->reset) {
+		fsleep(2000);
+		gpiod_set_value_cansleep(ov9734->reset, 0);
+		fsleep(6000);
+	}
+
+	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);
+	regulator_bulk_disable(ARRAY_SIZE(ov9734_supply_names),
+			       ov9734->supplies);
+	clk_disable_unprepare(ov9734->clk);
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ov9734_pm_ops, ov9734_power_off,
+				 ov9734_power_on, NULL);
+
 static int ov9734_check_hwcfg(struct device *dev)
 {
 	struct fwnode_handle *ep;
@@ -887,6 +942,8 @@ 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);
+	if (!pm_runtime_status_suspended(ov9734->dev))
+		ov9734_power_off(ov9734->dev);
 	pm_runtime_set_suspended(ov9734->dev);
 	mutex_destroy(&ov9734->mutex);
 }
@@ -895,6 +952,7 @@ static int ov9734_probe(struct i2c_client *client)
 {
 	struct ov9734 *ov9734;
 	unsigned long freq;
+	unsigned int i;
 	int ret;
 
 	ret = ov9734_check_hwcfg(&client->dev);
@@ -918,11 +976,32 @@ static int ov9734_probe(struct i2c_client *client)
 				     "external clock %lu is not supported",
 				     freq);
 
+	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");
+
+	for (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");
+
 	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);
@@ -945,12 +1024,12 @@ static int ov9734_probe(struct i2c_client *client)
 	}
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * The device is powered on at this point, by ov9734_power_on() and,
+	 * on some platforms, by i2c-core with ACPI domain PM. Enable runtime
+	 * PM and turn off the device once the subdev is registered.
 	 */
 	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 +1038,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,6 +1051,9 @@ 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;
 }
 
@@ -984,6 +1068,7 @@ static struct i2c_driver ov9734_i2c_driver = {
 	.driver = {
 		.name = "ov9734",
 		.acpi_match_table = ov9734_acpi_ids,
+		.pm = pm_ptr(&ov9734_pm_ops),
 	},
 	.probe = ov9734_probe,
 	.remove = ov9734_remove,
-- 
2.53.0

[PATCH 4/4] media: i2c: ov9734: Support control events

[Afonso Figuinha]

The driver exposes V4L2 controls on its subdev node but does not
implement event subscription, so v4l2-compliance reports a failure for
VIDIOC_SUBSCRIBE_EVENT.

Set V4L2_SUBDEV_FL_HAS_EVENTS and use the control event helpers for
subscribe_event and unsubscribe_event.

The change and this description were drafted with an AI coding
assistant (Claude Code, model Claude Opus 5.5). The author reviewed
them and tested them on a Microsoft Surface Laptop 1.

Signed-off-by: Afonso Figuinha <afonso.figuinha@gmail.com>
---
 drivers/media/i2c/ov9734.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov9734.c b/drivers/media/i2c/ov9734.c
index bf19ea56f..095dae964 100644
--- a/drivers/media/i2c/ov9734.c
+++ b/drivers/media/i2c/ov9734.c
@@ -13,6 +13,7 @@
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 
 #define OV9734_LINK_FREQ_180MHZ		180000000ULL
@@ -803,7 +804,13 @@ static const struct v4l2_subdev_pad_ops ov9734_pad_ops = {
 	.enum_frame_size = ov9734_enum_frame_size,
 };
 
+static const struct v4l2_subdev_core_ops ov9734_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_ops ov9734_subdev_ops = {
+	.core = &ov9734_core_ops,
 	.video = &ov9734_video_ops,
 	.pad = &ov9734_pad_ops,
 };
@@ -1013,7 +1020,8 @@ static int ov9734_probe(struct i2c_client *client)
 	}
 
 	ov9734->sd.internal_ops = &ov9734_internal_ops;
-	ov9734->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov9734->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+			     V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov9734->sd.entity.ops = &ov9734_subdev_entity_ops;
 	ov9734->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	ov9734->pad.flags = MEDIA_PAD_FL_SOURCE;
-- 
2.53.0

Re: [PATCH 0/4] media: ov9734: Support the Microsoft Surface Laptop 1 front camera

[Afonso Figuinha]

Hi all,

Please disregard this series. I missed that Tin Posavec posted the same
changes on 17 October:
https://lore.kernel.org/linux-media/20261017-ov9734-ipu6-v1-0-86eab22ae258@gmail.com/

Patches 1 to 3 of my series duplicate that series. I will test it on
the Surface Laptop 1 (IPU3) and reply there.

Patch 4 (control event support) is not in Tin's series. I can send it
on its own, on top of Tin's series, later.

Sorry for the noise.

Afonso