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

[Laurent Pinchart]

On Wed, Nov 04, 2026 at 11:42:49AM +0200, Babis Danias via B4 Relay wrote:
> From: Babis Danias <b.danias@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Babis Danias <b.danias@xxxxxxxxxxxx>
> ---
>  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