* [PATCH 0/3] media: i2c: ov8856: Power management fixes
@ 2026-11-04 9:42 Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove() Babis Danias via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Babis Danias via B4 Relay @ 2026-11-04 9:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable, Babis Danias
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove()
2026-11-04 9:42 [PATCH 0/3] media: i2c: ov8856: Power management fixes Babis Danias via B4 Relay
@ 2026-11-04 9:42 ` Babis Danias via B4 Relay
2026-11-09 13:01 ` Sakari Ailus
2026-11-04 9:42 ` [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend Babis Danias via B4 Relay
2 siblings, 1 reply; 10+ messages in thread
From: Babis Danias via B4 Relay @ 2026-11-04 9:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable, Babis Danias
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device
2026-11-04 9:42 [PATCH 0/3] media: i2c: ov8856: Power management fixes Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove() Babis Danias via B4 Relay
@ 2026-11-04 9:42 ` Babis Danias via B4 Relay
2026-11-08 23:45 ` Charles Drolet
2026-11-09 13:02 ` Sakari Ailus
2026-11-04 9:42 ` [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend Babis Danias via B4 Relay
2 siblings, 2 replies; 10+ messages in thread
From: Babis Danias via B4 Relay @ 2026-11-04 9:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable, Babis Danias
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend
2026-11-04 9:42 [PATCH 0/3] media: i2c: ov8856: Power management fixes Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove() Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device Babis Danias via B4 Relay
@ 2026-11-04 9:42 ` Babis Danias via B4 Relay
2026-11-06 1:53 ` Charles Drolet
2026-11-09 0:24 ` Laurent Pinchart
2 siblings, 2 replies; 10+ messages in thread
From: Babis Danias via B4 Relay @ 2026-11-04 9:42 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable, Babis Danias
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend
2026-11-04 9:42 ` [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend Babis Danias via B4 Relay
@ 2026-11-06 1:53 ` Charles Drolet
2026-11-09 0:24 ` Laurent Pinchart
1 sibling, 0 replies; 10+ messages in thread
From: Charles Drolet @ 2026-11-06 1:53 UTC (permalink / raw)
To: devnull+b.danias.tutamail.com
Cc: b.danias, bod, hansg, kieran.bingham, laurent.pinchart,
linux-kernel, linux-media, mchehab, rfoss, sakari.ailus,
serin.yeh, stable, 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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device
2026-11-04 9:42 ` [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device Babis Danias via B4 Relay
@ 2026-11-08 23:45 ` Charles Drolet
2026-11-09 13:04 ` Sakari Ailus
2026-11-09 13:02 ` Sakari Ailus
1 sibling, 1 reply; 10+ messages in thread
From: Charles Drolet @ 2026-11-08 23:45 UTC (permalink / raw)
To: devnull+b.danias.tutamail.com
Cc: b.danias, bod, hansg, kieran.bingham, laurent.pinchart,
linux-kernel, linux-media, mchehab, rfoss, sakari.ailus,
serin.yeh, stable, Charles Drolet
This issue occurred on my Dell Latitude 7320 Detachable about one in four
times I rebooted. After applying this patch, it didn't recur during 10
reboots. I also ran a very similar patch for a couple of weeks without
seeing the issue.
Tested on Ubuntu 26.04.1 with Linux 7.4-rc1 and a patch for TPS68470
board data. BIOS: 1.48
Tested-by: Charles Drolet <linux@cdrolet.dev>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend
2026-11-04 9:42 ` [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend Babis Danias via B4 Relay
2026-11-06 1:53 ` Charles Drolet
@ 2026-11-09 0:24 ` Laurent Pinchart
1 sibling, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2026-11-09 0:24 UTC (permalink / raw)
To: b.danias
Cc: Sakari Ailus, Mauro Carvalho Chehab, Serin Yeh, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable
On Wed, Nov 04, 2026 at 11:42:49AM +0200, Babis Danias via B4 Relay wrote:
> 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.
No. I'm really tired of the combination of bad PM APIs and cargo-cult in
drivers/media/i2c/. I don't want to see dozens of LLM slop series for
sensor drivers.
> 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,
> },
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove()
2026-11-04 9:42 ` [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove() Babis Danias via B4 Relay
@ 2026-11-09 13:01 ` Sakari Ailus
0 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2026-11-09 13:01 UTC (permalink / raw)
To: b.danias
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable
Hi Babis,
Thanks for the set.
On Wed, Nov 04, 2026 at 11:42:47AM +0200, Babis Danias via B4 Relay wrote:
> 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);
Also setting the device suspended should be conditional to its status, not
just powering it off.
> }
>
> static int ov8856_probe(struct i2c_client *client)
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device
2026-11-04 9:42 ` [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device Babis Danias via B4 Relay
2026-11-08 23:45 ` Charles Drolet
@ 2026-11-09 13:02 ` Sakari Ailus
1 sibling, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2026-11-09 13:02 UTC (permalink / raw)
To: b.danias
Cc: Mauro Carvalho Chehab, Serin Yeh, Laurent Pinchart, Robert Foss,
Hans de Goede, Bryan O'Donoghue, Kieran Bingham, linux-media,
linux-kernel, stable
Hi Babis,
On Wed, Nov 04, 2026 at 11:42:48AM +0200, Babis Danias via B4 Relay wrote:
> 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);
Don't do pm_runtime_get_noresume() here, instead call pm_runtime_idle()
after enabling Runtime PM, perhaps still after registering the async
sub-device.
> + 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:
>
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device
2026-11-08 23:45 ` Charles Drolet
@ 2026-11-09 13:04 ` Sakari Ailus
0 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2026-11-09 13:04 UTC (permalink / raw)
To: Charles Drolet
Cc: devnull+b.danias.tutamail.com, b.danias, bod, hansg,
kieran.bingham, laurent.pinchart, linux-kernel, linux-media,
mchehab, rfoss, serin.yeh, stable
Hi Charles,
On Sun, Nov 08, 2026 at 06:45:57PM -0500, Charles Drolet wrote:
> This issue occurred on my Dell Latitude 7320 Detachable about one in four
> times I rebooted. After applying this patch, it didn't recur during 10
> reboots. I also ran a very similar patch for a couple of weeks without
> seeing the issue.
I've thought this was only a theoretical issue, while we've been fixing
these it's the first time I've actually heard someone has hit this. Thanks
for testing!
>
> Tested on Ubuntu 26.04.1 with Linux 7.4-rc1 and a patch for TPS68470
> board data. BIOS: 1.48
>
> Tested-by: Charles Drolet <linux@cdrolet.dev>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-11-09 13:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-04 9:42 [PATCH 0/3] media: i2c: ov8856: Power management fixes Babis Danias via B4 Relay
2026-11-04 9:42 ` [PATCH 1/3] media: i2c: ov8856: Don't power off a suspended sensor in remove() Babis Danias via B4 Relay
2026-11-09 13:01 ` Sakari Ailus
2026-11-04 9:42 ` [PATCH 2/3] media: i2c: ov8856: Enable runtime PM before registering the sub-device Babis Danias via B4 Relay
2026-11-08 23:45 ` Charles Drolet
2026-11-09 13:04 ` Sakari Ailus
2026-11-09 13:02 ` Sakari Ailus
2026-11-04 9:42 ` [PATCH 3/3] media: i2c: ov8856: Power off the sensor during system suspend Babis Danias via B4 Relay
2026-11-06 1:53 ` Charles Drolet
2026-11-09 0:24 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®