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

[Sakari Ailus]

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@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Babis Danias <b.danias@xxxxxxxxxxxx>
> ---
>  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