[PATCH 0/3] media: i2c: ov8856: Power management fixes

[PATCH 0/3] media: i2c: ov8856: Power management fixes

[Babis Danias via B4 Relay]

Since commit 609a71568664 ("media: i2c: ov8856 : remove ACPI node
bypass mechanism") in v7.4-rc1, ov8856 also controls the sensor's power
on ACPI systems. Bringing up the rear camera of a Dell Latitude 7320
Detachable (OV8856 and a DW9714 VCM, behind a TPS68470 PMIC) on top of
that showed three power management bugs, fixed here:

1. remove() powers the sensor off even when runtime PM already has,
   giving unbalanced regulator and clock disables. This also affects
   devicetree systems, since 2020.

2. probe() registers the sub-device before enabling runtime PM, so
   ipu-bridge can't runtime-resume the sensor to instantiate its VCM
   (-EACCES).

3. With the VCM driver bound, the sensor stays powered through system
   suspend: the VCM's device link stops the sensor from using
   direct-complete, acpi_subsys_suspend() runtime-resumes it, and ov8856
   has no system sleep callbacks to power it off again.

Patches 1 and 2 fix older bugs, so they carry Cc: stable. Patch 3 does
too, because 609a71568664 is tagged for stable: the bug it brings in
will reach stable kernels with it.

Testing

Tested on a Dell Latitude 7320 Detachable (BIOS 1.39.0) running the
Fedora rawhide kernel 7.4.0-0.rc1.13.fc46, with these patches built out
of tree. The board also needs TPS68470 board data that isn't upstream
yet: the front camera entry from Sahan Nissanka's series adding board
data for the Dell 7320 Detachable, plus the rear sensor's supplies,
reset GPIO and VCM supply, which I plan to post once I've agreed it with
him. With that:

- unbinding and rebinding the sensor gives no warnings (before: the
  warnings quoted in patch 1, on every unbind);
- the VCM is instantiated, and the dw9714 driver binds and moves the
  lens (before: the -EACCES error quoted in patch 2, on every bind);
- in 20 s of s2idle with the camera idle, regulator and clock traces
  show the sensor powered for about 12 ms at suspend entry, then off
  (before: on for the whole suspend);
- streaming with libcamera's cam works at 1640x1232 and 3264x2448.

The "before" results, and the logs quoted in the patches, come from
Fedora's 7.2.15 kernel with 609a71568664 backported, where these
patches also passed the same tests. On v7.4-rc1 I tested with the
patches applied only. checkpatch.pl --strict is clean, and each patch
builds without warnings with W=1. I haven't run sparse or smatch.

AI assistance

I'm not a kernel developer. I did this work with the help of an AI
assistant, Claude Opus 5.5, in one long session. I asked it to help me
get the rear camera working, one step at a time. It read the kernel
code, proposed each test, analysed the logs and traces I sent back,
found the commits the Fixes: tags point to, ran the build and checkpatch
checks, and wrote all three patches, their commit messages and this
cover letter. I ran every test on the hardware and reviewed each change
line by line before signing off.

---
Babis Danias (3):
      media: i2c: ov8856: Don't power off a suspended sensor in remove()
      media: i2c: ov8856: Enable runtime PM before registering the sub-device
      media: i2c: ov8856: Power off the sensor during system suspend

 drivers/media/i2c/ov8856.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261103-ov8856-pm-fixes-ae4af48c822d

Best regards,
-- 
Babis Danias <b.danias@tutamail.com>

[PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove()

[Babis Danias via B4 Relay]

From: Babis Danias <b.danias@tutamail.com>

ov8856_remove() calls ov8856_power_off() unconditionally. The sensor is
normally runtime-suspended by then, so its regulators and clock get
disabled a second time. On every unbind, the regulator core refuses the
extra disable with an "unbalanced disables" warning, and the clock
framework warns that the clock is already disabled and unprepared.

For the clock this is more than a warning: the clock framework keeps
one enable count per clock, shared by all its consumers. Where sensors
share a clock, such as the TPS68470 clock that int3472 hands to every
sensor behind it, the extra disable can stop the clock under another
sensor that is streaming.

This has been the case on devicetree systems since the power handling
was added. Since commit 609a71568664 ("media: i2c: ov8856 : remove ACPI
node bypass mechanism"), it happens on ACPI systems too. Seen on a Dell
Latitude 7320 Detachable (rear OV8856 behind a TPS68470) when unbinding
the driver:

  [  220.072107] unbalanced disables for CORE
  [  220.072109] WARNING: drivers/regulator/core.c:3262 at _regulator_disable+0xc3/0x150, CPU#2: tee/8667
  [  220.072825] tps68470-clk already disabled
  [  220.072828] WARNING: drivers/clk/clk.c:1188 at clk_core_disable+0x82/0x140, CPU#2: tee/8667
  [  220.073620] tps68470-clk already unprepared
  [  220.073623] WARNING: drivers/clk/clk.c:1047 at clk_core_unprepare+0x1cf/0x200, CPU#2: tee/8667

Power the sensor off only if it isn't runtime-suspended, then mark it
suspended, as other sensor drivers such as ov5675 do.

The problem was found, and this patch written, with the help of an AI
coding assistant (Claude Opus 5.5). See the cover letter for details.

Fixes: 0c2c7a1e0d69 ("media: ov8856: Add devicetree support")
Cc: stable@vger.kernel.org
Signed-off-by: Babis Danias <b.danias@tutamail.com>
---
 drivers/media/i2c/ov8856.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 42c4b9c4e..988ce0310 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -2367,7 +2367,9 @@ static void ov8856_remove(struct i2c_client *client)
 	pm_runtime_disable(ov8856->dev);
 	mutex_destroy(&ov8856->mutex);
 
-	ov8856_power_off(ov8856->dev);
+	if (!pm_runtime_status_suspended(ov8856->dev))
+		ov8856_power_off(ov8856->dev);
+	pm_runtime_set_suspended(ov8856->dev);
 }
 
 static int ov8856_probe(struct i2c_client *client)

-- 
2.56.0

[PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device

[Babis Danias via B4 Relay]

From: Babis Danias <b.danias@tutamail.com>

ov8856_probe() registers the async sub-device before it enables runtime
PM. But the sub-device can be used as soon as it is registered. On ACPI
systems where ipu-bridge instantiates a VCM for the sensor, the bridge's
.bound() callback queues a work item that runtime-resumes the sensor.
If the work runs before probe reaches pm_runtime_enable(),
pm_runtime_get_sync() fails with -EACCES and the VCM is never created:

  [  213.265894] ov8856 i2c-OVTI8856:00: Error -13 runtime-resuming sensor, cannot instantiate VCM

On a Dell Latitude 7320 Detachable this happened on every bind of the
rear sensor after the IPU6 ISYS driver had probed, so the dw9714 focus
motor never showed up. Documentation/driver-api/media/v4l2-subdev.rst
asks drivers to enable runtime PM before registering the sub-device.

Enable runtime PM first, and hold a usage reference until registration
is done, so that the sensor can't be suspended halfway through. Drop the
reference with pm_runtime_put(), which also lets the sensor suspend once
probe is done, as the old pm_runtime_idle() call did. If registration
fails, undo the runtime PM setup. The probe_error_media_entity_cleanup
label then has no users left, so remove it.

The problem was found, and this patch written, with the help of an AI
coding assistant (Claude Opus 5.5). See the cover letter for details.

Fixes: 879347f0c258 ("media: ov8856: Add support for OV8856 sensor")
Cc: stable@vger.kernel.org
Signed-off-by: Babis Danias <b.danias@tutamail.com>
---
 drivers/media/i2c/ov8856.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 988ce0310..f9bcdd412 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -2425,22 +2425,31 @@ static int ov8856_probe(struct i2c_client *client)
 		goto probe_error_v4l2_ctrl_handler_free;
 	}
 
+	/* Set the device's state to active if it's in D0 state. */
+	if (full_power)
+		pm_runtime_set_active(ov8856->dev);
+	/*
+	 * The sub-device can be used as soon as it is registered, so enable
+	 * runtime PM first, and hold a reference until registration is done.
+	 */
+	pm_runtime_get_noresume(ov8856->dev);
+	pm_runtime_enable(ov8856->dev);
+
 	ret = v4l2_async_register_subdev_sensor(&ov8856->sd);
 	if (ret < 0) {
 		dev_err(ov8856->dev, "failed to register V4L2 subdev: %d",
 			ret);
-		goto probe_error_media_entity_cleanup;
+		goto probe_error_pm_runtime_disable;
 	}
 
-	/* Set the device's state to active if it's in D0 state. */
-	if (full_power)
-		pm_runtime_set_active(ov8856->dev);
-	pm_runtime_enable(ov8856->dev);
-	pm_runtime_idle(ov8856->dev);
+	pm_runtime_put(ov8856->dev);
 
 	return 0;
 
-probe_error_media_entity_cleanup:
+probe_error_pm_runtime_disable:
+	pm_runtime_disable(ov8856->dev);
+	pm_runtime_set_suspended(ov8856->dev);
+	pm_runtime_put_noidle(ov8856->dev);
 	media_entity_cleanup(&ov8856->sd.entity);
 
 probe_error_v4l2_ctrl_handler_free:

-- 
2.56.0

[PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend

[Babis Danias via B4 Relay]

From: Babis Danias <b.danias@tutamail.com>

On a Dell Latitude 7320 Detachable, the rear OV8856 stays powered for
the whole of s2idle once the dw9714 VCM driver is bound, even with the
camera idle. Tracing a 20 second suspend shows the sensor's clock and
regulators enabled about 0.5 s after "PM: suspend entry", and disabled
only just before "PM: suspend exit". Without the VCM driver bound, they
stay off.

ipu-bridge links the VCM to the sensor with a runtime PM device link,
which makes the sensor a supplier of the VCM. The dw9714 driver has
system sleep callbacks, so the VCM can't use direct-complete, and
dpm_clear_superiors_direct_complete() then clears direct_complete for
the sensor as well. The sensor goes through the full system suspend
path, where acpi_subsys_suspend() runtime-resumes it before calling the
driver's suspend callback. But ov8856 has had no system sleep callbacks
since commit 19ae6ba57f88 ("media: i2c: ov8856: Drop system suspend and
resume handlers"), so nothing powers the sensor off again.

This matters since commit 609a71568664 ("media: i2c: ov8856 : remove
ACPI node bypass mechanism") made the runtime PM callbacks control the
sensor's power on ACPI systems.

Use DEFINE_RUNTIME_DEV_PM_OPS(), so that pm_runtime_force_suspend()
powers the sensor off during system suspend, and
pm_runtime_force_resume() powers it on again only if it was in use. With
this, the sensor is off for the whole suspend. It is still powered up
for a few milliseconds at suspend entry, by acpi_subsys_suspend().

The problem was found, and this patch written, with the help of an AI
coding assistant (Claude Opus 5.5). See the cover letter for details.

Fixes: 609a71568664 ("media: i2c: ov8856 : remove ACPI node bypass mechanism")
Cc: stable@vger.kernel.org
Signed-off-by: Babis Danias <b.danias@tutamail.com>
---
 drivers/media/i2c/ov8856.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index f9bcdd412..848e981ff 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -2462,9 +2462,8 @@ static int ov8856_probe(struct i2c_client *client)
 	return ret;
 }
 
-static const struct dev_pm_ops ov8856_pm_ops = {
-	SET_RUNTIME_PM_OPS(ov8856_power_off, ov8856_power_on, NULL)
-};
+static DEFINE_RUNTIME_DEV_PM_OPS(ov8856_pm_ops, ov8856_power_off,
+				 ov8856_power_on, NULL);
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov8856_acpi_ids[] = {
@@ -2484,7 +2483,7 @@ MODULE_DEVICE_TABLE(of, ov8856_of_match);
 static struct i2c_driver ov8856_i2c_driver = {
 	.driver = {
 		.name = "ov8856",
-		.pm = &ov8856_pm_ops,
+		.pm = pm_ptr(&ov8856_pm_ops),
 		.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
 		.of_match_table = ov8856_of_match,
 	},

-- 
2.56.0

Re: [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend

[Charles Drolet]

Hi,

I have tested this patch on a Dell Latitude 7320 Detachable with BIOS 1.48
on Ubuntu 26.04.1 with Linux 7.4-rc1 + patches for TPS68470 board-data and
TPS68470 privacy led (tied to VSIO).

Without this patch, the privacy led turns on when entering s2idle and turns
off on resume.

With this patch, the privacy led flashes once when entering s2idle and
stays off. It stays off on resume.

Tested-by: Charles Drolet <linux@cdrolet.dev>