[PATCH v3 0/3] media: i2c: ov02c10: Fix brownouts and power sequence

[PATCH v3 0/3] media: i2c: ov02c10: Fix brownouts and power sequence

[Saikiran]

This series addresses stability issues with the OV02C10 sensor on Qualcomm
Snapdragon X Elite (X1E80100) platforms, specifically the Lenovo Yoga Slim 7x.

Note: This series supersedes all previous patch sets submitted by me regarding
OV02C10 / media stability and cleanup on X1E80100 (including the 'pipeline
lock' and 'brownout' series). Please disregard prior versions.

Problem 1: Brownouts during rapid cycling
On this platform, the RPMh-controlled regulators lack active discharge, taking
~2.3s to passively discharge. Rapid close/open cycles (e.g., WebRTC checks)
re-enable regulators while the rails are floating, causing the sensor to hang.
Previous attempts to manage this via open-loop delays in power_on were deemed
incorrect.

Problem 2: Incorrect Power Sequence
The driver was not strictly following the datasheet power-up timing (T1/T2),
potentially leading to race conditions between the reset pin and power rails.

Problem 3: Race condition on removal
The remove() function freed resources before powering off the device, causing
use-after-free errors if userspace (PipeWire) accessed controls during removal.

Solution in v3:
1. Implement Runtime PM Autosuspend (1000ms). This prevents the driver from
   cutting power during rapid user interactions, sidestepping the slow
   regulator discharge window entirely. (Patch 3)
2. Enforce strict datasheet power-on sequencing with ample delays to satisfy
   maintainer requirements for clean boot. (Patch 2)
3. Fix the remove() race condition by reordering cleanup. (Patch 1)

Changes in v3:
- Dropped the "always-on" regulator patch from v2.
- Added Runtime PM Autosuspend support (Patch 3).
- Added strict power-on sequencing with 10ms/20ms delays (Patch 2).
- Added fix for use-after-free in remove() (Patch 1).

Link: https://lore.kernel.org/linux-media/20260125171745.484806-1-bjsaikiran@gmail.com/T/#t [1]

Saikiran (3):
  media: i2c: ov02c10: Fix use-after-free in remove function
  media: i2c: ov02c10: Correct power-on sequence and timing
  media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

 drivers/media/i2c/ov02c10.c | 70 ++++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 12 deletions(-)

[PATCH v3 1/3] media: i2c: ov02c10: Fix use-after-free in remove function

[Saikiran]

The ov02c10_remove() function has a race condition where v4l2_ctrl_handler
and media_entity resources are freed before the device is powered off.
If userspace (e.g., PipeWire/WirePlumber) accesses the device during
removal, this causes a use-after-free leading to kernel oops with
"Execute from non-executable memory" errors.

The issue occurs because:
1. v4l2_ctrl_handler_free() is called first
2. Userspace may still have the device open
3. Control access triggers use-after-free
4. Device is powered off afterwards (too late)

Fix by reordering cleanup to disable runtime PM and power off the device
BEFORE freeing v4l2_ctrl_handler and media_entity resources. This ensures
the device is in a safe state before any resources are freed.

Call sequence after fix:
1. v4l2_async_unregister_subdev() - unregister from V4L2
2. pm_runtime_disable() - disable runtime PM
3. ov02c10_power_off() - power off device if needed
4. v4l2_subdev_cleanup() - clean up subdev
5. media_entity_cleanup() - clean up media entity
6. v4l2_ctrl_handler_free() - free control handler (safe now)

Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Saikiran <bjsaikiran@gmail.com>
---
 drivers/media/i2c/ov02c10.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
index cf93d36032e1..fa7cc48b769a 100644
--- a/drivers/media/i2c/ov02c10.c
+++ b/drivers/media/i2c/ov02c10.c
@@ -864,14 +864,14 @@ static void ov02c10_remove(struct i2c_client *client)
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 
 	v4l2_async_unregister_subdev(sd);
-	v4l2_subdev_cleanup(sd);
-	media_entity_cleanup(&sd->entity);
-	v4l2_ctrl_handler_free(sd->ctrl_handler);
 	pm_runtime_disable(ov02c10->dev);
 	if (!pm_runtime_status_suspended(ov02c10->dev)) {
 		ov02c10_power_off(ov02c10->dev);
 		pm_runtime_set_suspended(ov02c10->dev);
 	}
+	v4l2_subdev_cleanup(sd);
+	media_entity_cleanup(&sd->entity);
+	v4l2_ctrl_handler_free(sd->ctrl_handler);
 }
 
 static int ov02c10_probe(struct i2c_client *client)
-- 
2.51.0

[PATCH v3 2/3] media: i2c: ov02c10: Correct power-on sequence and timing

[Saikiran]

1. Assert XSHUTDOWN (reset) for 10ms (T1 >= 5ms) before enabling power.
2. Enable regulators and wait 20ms for ramp-up stabilization.
3. Enable clock and wait 10ms for stabilization.
4. De-assert XSHUTDOWN.
5. Wait 20ms (T2 >= 20ms) for sensor boot before I2C access.
6. Perform software reset (0x0103) to ensure clean state.

This eliminates potential race conditions and stability issues during cold boot initialization.

Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Saikiran <bjsaikiran@gmail.com>
---
 drivers/media/i2c/ov02c10.c | 57 ++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
index fa7cc48b769a..ba8bbb4f433a 100644
--- a/drivers/media/i2c/ov02c10.c
+++ b/drivers/media/i2c/ov02c10.c
@@ -22,6 +22,8 @@
 #define OV02C10_CHIP_ID			0x5602
 
 #define OV02C10_REG_STREAM_CONTROL	CCI_REG8(0x0100)
+#define OV02C10_REG_SOFTWARE_RESET	CCI_REG8(0x0103)
+#define OV02C10_SOFTWARE_RESET_TRIGGER	0x01
 
 #define OV02C10_REG_HTS			CCI_REG16(0x380c)
 
@@ -616,6 +618,13 @@ static int ov02c10_enable_streams(struct v4l2_subdev *sd,
 	if (ret)
 		goto out;
 
+	/*
+	 * Delay before streaming:
+	 * Give the sensor time to process all the register writes and internal
+	 * calibration before we assert the STREAM_ON bit.
+	 */
+	usleep_range(2000, 2500);
+
 	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 1, NULL);
 out:
 	if (ret)
@@ -660,13 +669,13 @@ static int ov02c10_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 
-	gpiod_set_value_cansleep(ov02c10->reset, 1);
+	if (ov02c10->reset)
+		gpiod_set_value_cansleep(ov02c10->reset, 1);
 
+	clk_disable_unprepare(ov02c10->img_clk);
 	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
 			       ov02c10->supplies);
 
-	clk_disable_unprepare(ov02c10->img_clk);
-
 	return 0;
 }
 
@@ -676,27 +685,53 @@ static int ov02c10_power_on(struct device *dev)
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 	int ret;
 
-	ret = clk_prepare_enable(ov02c10->img_clk);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable imaging clock: %d", ret);
-		return ret;
+	if (ov02c10->reset) {
+		gpiod_set_value_cansleep(ov02c10->reset, 1);
+		usleep_range(10000, 11000);
 	}
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
 				    ov02c10->supplies);
 	if (ret < 0) {
 		dev_err(dev, "failed to enable regulators: %d", ret);
-		clk_disable_unprepare(ov02c10->img_clk);
 		return ret;
 	}
 
+	/* Allow PMIC to ramp and stabilize */
+	usleep_range(20000, 22000);
+
+	ret = clk_prepare_enable(ov02c10->img_clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable imaging clock: %d", ret);
+		regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
+				       ov02c10->supplies);
+		return ret;
+	}
+
+	/* Let the clock stabilise */
+	usleep_range(10000, 11000);
+
+	/* Release hardware reset */
 	if (ov02c10->reset) {
-		/* Assert reset for at least 2ms on back to back off-on */
-		usleep_range(2000, 2200);
 		gpiod_set_value_cansleep(ov02c10->reset, 0);
-		usleep_range(5000, 5100);
+		/*
+		 * Wait for sensor microcontroller to stabilize after reset release.
+		 * 20ms prevents black frames during rapid power cycling.
+		 */
+		usleep_range(20000, 22000);
+	}
+
+	/* Perform software reset to ensure clean state */
+	ret = cci_write(ov02c10->regmap, OV02C10_REG_SOFTWARE_RESET,
+			OV02C10_SOFTWARE_RESET_TRIGGER, NULL);
+	if (ret) {
+		dev_err(dev, "failed to send software reset: %d", ret);
+		return ret;
 	}
 
+	/* Wait for software reset to complete */
+	usleep_range(5000, 5500);
+
 	return 0;
 }
 
-- 
2.51.0

[PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Saikiran]

On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences brownouts
if power-cycled too quickly (< 2.3s) due to slow passive discharge of
regulator rails.

Implement Runtime PM Autosuspend with a delay of 1000ms. This keeps the
regulators enabled for a short duration after the device is closed,
preventing costly power-off/power-on cycles during rapid user
interactions (e.g., browser permission checks).

This aligns the driver with standard power management practices used in
other sensor drivers (e.g., ov2680) to handle platform latency
constraints.

Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Saikiran <bjsaikiran@gmail.com>
---
 drivers/media/i2c/ov02c10.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
index ba8bbb4f433a..9f6c4e8b06cc 100644
--- a/drivers/media/i2c/ov02c10.c
+++ b/drivers/media/i2c/ov02c10.c
@@ -628,7 +628,7 @@ static int ov02c10_enable_streams(struct v4l2_subdev *sd,
 	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 1, NULL);
 out:
 	if (ret)
-		pm_runtime_put(ov02c10->dev);
+		pm_runtime_put_autosuspend(ov02c10->dev);
 
 	return ret;
 }
@@ -640,7 +640,7 @@ static int ov02c10_disable_streams(struct v4l2_subdev *sd,
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 
 	cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);
-	pm_runtime_put(ov02c10->dev);
+	pm_runtime_put_autosuspend(ov02c10->dev);
 
 	return 0;
 }
@@ -900,6 +900,7 @@ static void ov02c10_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(sd);
 	pm_runtime_disable(ov02c10->dev);
+	pm_runtime_dont_use_autosuspend(ov02c10->dev);
 	if (!pm_runtime_status_suspended(ov02c10->dev)) {
 		ov02c10_power_off(ov02c10->dev);
 		pm_runtime_set_suspended(ov02c10->dev);
@@ -983,6 +984,8 @@ static int ov02c10_probe(struct i2c_client *client)
 		goto probe_error_media_entity_cleanup;
 	}
 
+	pm_runtime_set_autosuspend_delay(ov02c10->dev, 1000);
+	pm_runtime_use_autosuspend(ov02c10->dev);
 	pm_runtime_set_active(ov02c10->dev);
 	pm_runtime_enable(ov02c10->dev);
 
-- 
2.51.0

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Bryan O'Donoghue]

On 26/01/2026 17:34, Saikiran wrote:
> On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences brownouts
> if power-cycled too quickly (< 2.3s) due to slow passive discharge of
> regulator rails.
> 
> Implement Runtime PM Autosuspend with a delay of 1000ms. This keeps the
> regulators enabled for a short duration after the device is closed,
> preventing costly power-off/power-on cycles during rapid user
> interactions (e.g., browser permission checks).

But if you try to power the sensor 1.1 seconds later what happens ?

With this commit log this submission is a NAK, for example why do I want 
this change on an x86 machine ?

We need to root-cause the failure not paper over it.

---
bod

Re: [PATCH v3 1/3] media: i2c: ov02c10: Fix use-after-free in remove function

[Hans de Goede]

Hi,

On 26-Jan-26 18:34, Saikiran wrote:
> The ov02c10_remove() function has a race condition where v4l2_ctrl_handler
> and media_entity resources are freed before the device is powered off.
> If userspace (e.g., PipeWire/WirePlumber) accesses the device during
> removal, this causes a use-after-free leading to kernel oops with
> "Execute from non-executable memory" errors.
> 
> The issue occurs because:
> 1. v4l2_ctrl_handler_free() is called first
> 2. Userspace may still have the device open
> 3. Control access triggers use-after-free
> 4. Device is powered off afterwards (too late)
> 
> Fix by reordering cleanup to disable runtime PM and power off the device
> BEFORE freeing v4l2_ctrl_handler and media_entity resources. This ensures
> the device is in a safe state before any resources are freed.
> 
> Call sequence after fix:
> 1. v4l2_async_unregister_subdev() - unregister from V4L2
> 2. pm_runtime_disable() - disable runtime PM
> 3. ov02c10_power_off() - power off device if needed
> 4. v4l2_subdev_cleanup() - clean up subdev
> 5. media_entity_cleanup() - clean up media entity
> 6. v4l2_ctrl_handler_free() - free control handler (safe now)
> 
> Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
> Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saikiran <bjsaikiran@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/i2c/ov02c10.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index cf93d36032e1..fa7cc48b769a 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -864,14 +864,14 @@ static void ov02c10_remove(struct i2c_client *client)
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>  
>  	v4l2_async_unregister_subdev(sd);
> -	v4l2_subdev_cleanup(sd);
> -	media_entity_cleanup(&sd->entity);
> -	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  	pm_runtime_disable(ov02c10->dev);
>  	if (!pm_runtime_status_suspended(ov02c10->dev)) {
>  		ov02c10_power_off(ov02c10->dev);
>  		pm_runtime_set_suspended(ov02c10->dev);
>  	}
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
>  }
>  
>  static int ov02c10_probe(struct i2c_client *client)

Re: [PATCH v3 2/3] media: i2c: ov02c10: Correct power-on sequence and timing

[Hans de Goede]

Hi,

On 26-Jan-26 18:34, Saikiran wrote:
> 1. Assert XSHUTDOWN (reset) for 10ms (T1 >= 5ms) before enabling power.
> 2. Enable regulators and wait 20ms for ramp-up stabilization.
> 3. Enable clock and wait 10ms for stabilization.
> 4. De-assert XSHUTDOWN.
> 5. Wait 20ms (T2 >= 20ms) for sensor boot before I2C access.
> 6. Perform software reset (0x0103) to ensure clean state.
> 
> This eliminates potential race conditions and stability issues during cold boot initialization.
> 
> Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
> Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saikiran <bjsaikiran@gmail.com>
> ---
>  drivers/media/i2c/ov02c10.c | 57 ++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index fa7cc48b769a..ba8bbb4f433a 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -22,6 +22,8 @@
>  #define OV02C10_CHIP_ID			0x5602
>  
>  #define OV02C10_REG_STREAM_CONTROL	CCI_REG8(0x0100)
> +#define OV02C10_REG_SOFTWARE_RESET	CCI_REG8(0x0103)
> +#define OV02C10_SOFTWARE_RESET_TRIGGER	0x01
>  
>  #define OV02C10_REG_HTS			CCI_REG16(0x380c)
>  
> @@ -616,6 +618,13 @@ static int ov02c10_enable_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		goto out;
>  
> +	/*
> +	 * Delay before streaming:
> +	 * Give the sensor time to process all the register writes and internal
> +	 * calibration before we assert the STREAM_ON bit.
> +	 */
> +	usleep_range(2000, 2500);
> +

Why? I've never seen any sensor driver do this and AFAICT this
is also not mentioned as a requirement in the datasheet.

>  	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 1, NULL);
>  out:
>  	if (ret)
> @@ -660,13 +669,13 @@ static int ov02c10_power_off(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>  
> -	gpiod_set_value_cansleep(ov02c10->reset, 1);
> +	if (ov02c10->reset)
> +		gpiod_set_value_cansleep(ov02c10->reset, 1);

No need to add this if (), gpiod_set_value() will happily
take a NULL gpio_desc * and ignore it.

> +	clk_disable_unprepare(ov02c10->img_clk);
>  	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
>  			       ov02c10->supplies);
>  
> -	clk_disable_unprepare(ov02c10->img_clk);
> -

Why? All datasheets say that the clock may be enabled either
before or after the regulators there is no need for this change.


>  	return 0;
>  }
>  
> @@ -676,27 +685,53 @@ static int ov02c10_power_on(struct device *dev)
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>  	int ret;
>  
> -	ret = clk_prepare_enable(ov02c10->img_clk);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable imaging clock: %d", ret);
> -		return ret;
> +	if (ov02c10->reset) {
> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
> +		usleep_range(10000, 11000);
>  	}

Ack for asserting the reset for 10 ms here, that is the only part
of this patch which seems to actually be useful.

>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
>  				    ov02c10->supplies);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to enable regulators: %d", ret);
> -		clk_disable_unprepare(ov02c10->img_clk);
>  		return ret;
>  	}
>  
> +	/* Allow PMIC to ramp and stabilize */
> +	usleep_range(20000, 22000);


If the regulators need a delay before stabilizing that should
be done by the regulator driver, not here.

> +
> +	ret = clk_prepare_enable(ov02c10->img_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
> +		regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
> +				       ov02c10->supplies);
> +		return ret;
> +	}

Again no need to change the clk vs regulator enable order.

> +
> +	/* Let the clock stabilise */
> +	usleep_range(10000, 11000);

Same as with regulators if this is necessary it should be
handled by the clk driver.

> +
> +	/* Release hardware reset */
>  	if (ov02c10->reset) {
> -		/* Assert reset for at least 2ms on back to back off-on */
> -		usleep_range(2000, 2200);

Ack for dropping this usleep() since this is now done before
enabling the regulators.

>  		gpiod_set_value_cansleep(ov02c10->reset, 0);
> -		usleep_range(5000, 5100);
> +		/*
> +		 * Wait for sensor microcontroller to stabilize after reset release.
> +		 * 20ms prevents black frames during rapid power cycling.
> +		 */
> +		usleep_range(20000, 22000);
> +	}

Why? this is not what the datasheet says.

> +
> +	/* Perform software reset to ensure clean state */
> +	ret = cci_write(ov02c10->regmap, OV02C10_REG_SOFTWARE_RESET,
> +			OV02C10_SOFTWARE_RESET_TRIGGER, NULL);
> +	if (ret) {
> +		dev_err(dev, "failed to send software reset: %d", ret);
> +		return ret;
>  	}
>  
> +	/* Wait for software reset to complete */
> +	usleep_range(5000, 5500);
> +

Please drop this whole sw-reset thing. We've just hw-reset the sensor
so there is no need. Also this should be done in a separate commit
if it were to be done at all.

Regards,

Hans

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Hans de Goede]

Hi,

On 27-Jan-26 10:46, Bryan O'Donoghue wrote:
> On 26/01/2026 17:34, Saikiran wrote:
>> On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences brownouts
>> if power-cycled too quickly (< 2.3s) due to slow passive discharge of
>> regulator rails.
>>
>> Implement Runtime PM Autosuspend with a delay of 1000ms. This keeps the
>> regulators enabled for a short duration after the device is closed,
>> preventing costly power-off/power-on cycles during rapid user
>> interactions (e.g., browser permission checks).
> 
> But if you try to power the sensor 1.1 seconds later what happens ?
> 
> With this commit log this submission is a NAK, for example why do I want this change on an x86 machine ?
> 
> We need to root-cause the failure not paper over it.

I agree the commit message needs work.

This is overall a useful change to have though. Even on x86_64
it is better to keep the sensor enabled when userspace
does a stop + start stream in quick succession rather then
needlessly powercycle it.

But the commit message needs to explain that this is generally
a good thing to have to avoid unnecessary delays related to
power-sequencing when userspace does a stop + start stream in quick
succession.

Regards,

Hans

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Hans de Goede]

On 27-Jan-26 10:46, Bryan O'Donoghue wrote:
> On 26/01/2026 17:34, Saikiran wrote:
>> On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences brownouts
>> if power-cycled too quickly (< 2.3s) due to slow passive discharge of
>> regulator rails.
>>
>> Implement Runtime PM Autosuspend with a delay of 1000ms. This keeps the
>> regulators enabled for a short duration after the device is closed,
>> preventing costly power-off/power-on cycles during rapid user
>> interactions (e.g., browser permission checks).
> 
> But if you try to power the sensor 1.1 seconds later what happens ?
> 
> With this commit log this submission is a NAK, for example why do I want this change on an x86 machine ?
> 
> We need to root-cause the failure not paper over it.

p.s.

I also agree that the actual problem with stop + start stream in
calls quickly after each other still needs to be properly figured
out and fixed.

Regards,

Hans

Re: [PATCH v3 2/3] media: i2c: ov02c10: Correct power-on sequence and timing

[Bryan O'Donoghue]

On 27/01/2026 10:40, Hans de Goede wrote:
> Hi,
> 
> On 26-Jan-26 18:34, Saikiran wrote:
>> 1. Assert XSHUTDOWN (reset) for 10ms (T1 >= 5ms) before enabling power.
>> 2. Enable regulators and wait 20ms for ramp-up stabilization.
>> 3. Enable clock and wait 10ms for stabilization.
>> 4. De-assert XSHUTDOWN.
>> 5. Wait 20ms (T2 >= 20ms) for sensor boot before I2C access.
>> 6. Perform software reset (0x0103) to ensure clean state.
>>
>> This eliminates potential race conditions and stability issues during cold boot initialization.
>>
>> Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite)
>> Fixes: 44f8901 ("media: i2c: add OmniVision OV02C10 sensor driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Saikiran <bjsaikiran@gmail.com>
>> ---
>>   drivers/media/i2c/ov02c10.c | 57 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
>> index fa7cc48b769a..ba8bbb4f433a 100644
>> --- a/drivers/media/i2c/ov02c10.c
>> +++ b/drivers/media/i2c/ov02c10.c
>> @@ -22,6 +22,8 @@
>>   #define OV02C10_CHIP_ID			0x5602
>>   
>>   #define OV02C10_REG_STREAM_CONTROL	CCI_REG8(0x0100)
>> +#define OV02C10_REG_SOFTWARE_RESET	CCI_REG8(0x0103)
>> +#define OV02C10_SOFTWARE_RESET_TRIGGER	0x01
>>   
>>   #define OV02C10_REG_HTS			CCI_REG16(0x380c)
>>   
>> @@ -616,6 +618,13 @@ static int ov02c10_enable_streams(struct v4l2_subdev *sd,
>>   	if (ret)
>>   		goto out;
>>   
>> +	/*
>> +	 * Delay before streaming:
>> +	 * Give the sensor time to process all the register writes and internal
>> +	 * calibration before we assert the STREAM_ON bit.
>> +	 */
>> +	usleep_range(2000, 2500);
>> +
> 
> Why? I've never seen any sensor driver do this and AFAICT this
> is also not mentioned as a requirement in the datasheet.
> 
>>   	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 1, NULL);
>>   out:
>>   	if (ret)
>> @@ -660,13 +669,13 @@ static int ov02c10_power_off(struct device *dev)
>>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>   	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>>   
>> -	gpiod_set_value_cansleep(ov02c10->reset, 1);
>> +	if (ov02c10->reset)
>> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
> 
> No need to add this if (), gpiod_set_value() will happily
> take a NULL gpio_desc * and ignore it.
> 
>> +	clk_disable_unprepare(ov02c10->img_clk);
>>   	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
>>   			       ov02c10->supplies);
>>   
>> -	clk_disable_unprepare(ov02c10->img_clk);
>> -
> 
> Why? All datasheets say that the clock may be enabled either
> before or after the regulators there is no need for this change.
> 
> 
>>   	return 0;
>>   }
>>   
>> @@ -676,27 +685,53 @@ static int ov02c10_power_on(struct device *dev)
>>   	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>>   	int ret;
>>   
>> -	ret = clk_prepare_enable(ov02c10->img_clk);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> -		return ret;
>> +	if (ov02c10->reset) {
>> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
>> +		usleep_range(10000, 11000);
>>   	}
> 
> Ack for asserting the reset for 10 ms here, that is the only part
> of this patch which seems to actually be useful.
> 
>>   
>>   	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
>>   				    ov02c10->supplies);
>>   	if (ret < 0) {
>>   		dev_err(dev, "failed to enable regulators: %d", ret);
>> -		clk_disable_unprepare(ov02c10->img_clk);
>>   		return ret;
>>   	}
>>   
>> +	/* Allow PMIC to ramp and stabilize */
>> +	usleep_range(20000, 22000);
> 
> 
> If the regulators need a delay before stabilizing that should
> be done by the regulator driver, not here.
> 
>> +
>> +	ret = clk_prepare_enable(ov02c10->img_clk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> +		regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
>> +				       ov02c10->supplies);
>> +		return ret;
>> +	}
> 
> Again no need to change the clk vs regulator enable order.
> 
>> +
>> +	/* Let the clock stabilise */
>> +	usleep_range(10000, 11000);
> 
> Same as with regulators if this is necessary it should be
> handled by the clk driver.
> 
>> +
>> +	/* Release hardware reset */
>>   	if (ov02c10->reset) {
>> -		/* Assert reset for at least 2ms on back to back off-on */
>> -		usleep_range(2000, 2200);
> 
> Ack for dropping this usleep() since this is now done before
> enabling the regulators.
> 
>>   		gpiod_set_value_cansleep(ov02c10->reset, 0);
>> -		usleep_range(5000, 5100);
>> +		/*
>> +		 * Wait for sensor microcontroller to stabilize after reset release.
>> +		 * 20ms prevents black frames during rapid power cycling.
>> +		 */
>> +		usleep_range(20000, 22000);
>> +	}
> 
> Why? this is not what the datasheet says.
> 
>> +
>> +	/* Perform software reset to ensure clean state */
>> +	ret = cci_write(ov02c10->regmap, OV02C10_REG_SOFTWARE_RESET,
>> +			OV02C10_SOFTWARE_RESET_TRIGGER, NULL);
>> +	if (ret) {
>> +		dev_err(dev, "failed to send software reset: %d", ret);
>> +		return ret;
>>   	}
>>   
>> +	/* Wait for software reset to complete */
>> +	usleep_range(5000, 5500);
>> +
> 
> Please drop this whole sw-reset thing. We've just hw-reset the sensor
> so there is no need. Also this should be done in a separate commit
> if it were to be done at all.
> 
> Regards,
> 
> Hans
> 
> 

To be clear, I was asking for an _experiment_ not a patch ...

---
bod

Re: [PATCH v3 2/3] media: i2c: ov02c10: Correct power-on sequence and timing

[Hans de Goede]

Hi,

On 27-Jan-26 11:40, Hans de Goede wrote:

...

>> @@ -676,27 +685,53 @@ static int ov02c10_power_on(struct device *dev)
>>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>>  	int ret;
>>  
>> -	ret = clk_prepare_enable(ov02c10->img_clk);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to enable imaging clock: %d", ret);
>> -		return ret;
>> +	if (ov02c10->reset) {
>> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
>> +		usleep_range(10000, 11000);
>>  	}
> 
> Ack for asserting the reset for 10 ms here, that is the only part
> of this patch which seems to actually be useful.

Quick note on this, your commit msg says the datasheet requires 5 ms,
so 10 seems a bit much for the next version please
use a usleep_range() with 5 - 7 ms. Or just use fsleep(5000) which
turns this into a usleep_range() for you.

Regards,

Hans

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Bryan O'Donoghue]

On 27/01/2026 10:40, Saikiran B wrote:
> Hi Bryan,
> 
> Regarding the 1.1s race condition:
> 
> I have implemented support for the generic regulator-off-on-delay-us 
> property
> in the qcom-rpmh-regulator driver and set the constraint to 2.3s in the
> device tree for the Yoga Slim 7x.

Yes but please listen to me. That is an extraordinary delay being 
introduced.

It is indicative of a serious problem we have not root caused. These 
LDOs are used in mobile phones which are aggressively designed to save 
power all the time.

In fact the whole idea of voting for clocks and bandwidth is it mitigate 
the default assumption in these class of devices - switch off the power 
first.

What is that 2.3 seconds, why is it needed. "Brownout" but why ? I don't 
think we have really established.

> I tested the 1.1s scenario you mentioned, and it is working fine. The 
> regulator
> core now correctly blocks the enable call until the physical discharge delay
> has passed, preventing the brownout without needing logic in the camera 
> driver.

The physical discharge delay we have _not_ established IMO. Have you 
checked the CCI pins ?

I think we should stop pushing patches until a root-cause has been 
identified.

For example - we can interrogate the LDO settings via SPMI registers to 
see if the LDO is really switched off.

Similarly we can interrogate the LDOs to see if they are set for active 
discharge.

A fix might be to make a platform driver to set those bits for the 
relevant LDOs absent a firmware fix for the same.

I'm not comfortable pushing changes predicated on papering over an issue 
that hasn't been root-caused.

> 
> I am going to drop the Autosuspend patch entirely and verify the clean
> driver one last time.
> 
> Plan for v4:
> 1. Submit the Regulator/DT fixes separately to linux-arm-msm.
> 2. Submit v4 of this series containing only the cleanup and power- 
> sequence fixes.
> 
> Thanks for pushing for the correct fix, the regulator approach is indeed 
> much cleaner.
> 
> Thanks & Regards,
> Saikiran
> 
> 
> On Tue, 27 Jan 2026, 15:16 Bryan O'Donoghue, <bryan.odonoghue@linaro.org 
> <mailto:bryan.odonoghue@linaro.org>> wrote:
> 
>     On 26/01/2026 17:34, Saikiran wrote:
>      > On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences
>     brownouts
>      > if power-cycled too quickly (< 2.3s) due to slow passive discharge of
>      > regulator rails.
>      >
>      > Implement Runtime PM Autosuspend with a delay of 1000ms. This
>     keeps the
>      > regulators enabled for a short duration after the device is closed,
>      > preventing costly power-off/power-on cycles during rapid user
>      > interactions (e.g., browser permission checks).
> 
>     But if you try to power the sensor 1.1 seconds later what happens ?
> 
>     With this commit log this submission is a NAK, for example why do I
>     want
>     this change on an x86 machine ?
> 
>     We need to root-cause the failure not paper over it.
> 
>     ---
>     bod
> 

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Bryan O'Donoghue]

On 27/01/2026 10:56, Saikiran B wrote:
> Hi Bryan,
> 
> I understand your suspicion regarding the LDO behavior, but I lack the
> hardware documentation (PMIC register maps) and tooling to interrogate
> the SPMI registers to the depth you are requesting.
> 

So, SPMI is not exported in /sys/kernel/debug/regmap - however

drivers/regulator/qcom-rpmh-regulator.c

Lets add this to probe

unsigned int val, i;
     u16 bases[] = {0x4000, 0x4300, 0x4600}; // LDO1, LDO4, LDO7
     const char *names[] = {"LDO1(1.2V)", "LDO4(1.8V)", "LDO7(2.8V)"};
     struct regmap *p_regmap = dev_get_regmap(dev->parent, NULL);

     if (p_regmap) {
         pr_info("--- OV02C10 PMIC RAIL DUMP START ---\n");
         for (i = 0; i < 3; i++) {
             // Check Config (Active Discharge)
             regmap_read(p_regmap, bases[i] + 0x41, &val);
             pr_info("!!! %s SEC_CTRL (0x%04x) = 0x%02x (Bit7: Active 
Discharge)\n",
                     names[i], bases[i] + 0x41, val);

             // Check Status (Is it actually on?)
             regmap_read(p_regmap, bases[i] + 0x08, &val);
             pr_info("!!! %s STATUS   (0x%04x) = 0x%02x (Bit7: VREG_OK, 
Bit0: VREG_ON)\n",
                     names[i], bases[i] + 0x08, val);

             // Check Pull-down config (Secondary check)
             regmap_read(p_regmap, bases[i] + 0x42, &val);
             pr_info("!!! %s PD_CTRL   (0x%04x) = 0x%02x\n",
                     names[i], bases[i] + 0x42, val);
         }
         pr_info("--- OV02C10 PMIC RAIL DUMP END ---\n");
     }

>  From my end, the empirical reality on my machine is that the sensor 
> fails if power-cycled
> faster than 2.3s, and enforcing that delay (via software or regulator core)
> fixes the issue reliably.
> 
> Since we cannot agree on the root cause and I cannot perform the hardware
> debugging required, I will stop submitting patches for this issue.
> I'll maintain the regulator workaround in my local tree.
> 
> I kindly thank you and Hans for your time reviewing the previous versions.
> 
> Thanks & Regards,
> Saikiran
> 
> On Tue, 27 Jan 2026, 16:21 Bryan O'Donoghue, <bryan.odonoghue@linaro.org 
> <mailto:bryan.odonoghue@linaro.org>> wrote:
> 
>     On 27/01/2026 10:40, Saikiran B wrote:
>      > Hi Bryan,
>      >
>      > Regarding the 1.1s race condition:
>      >
>      > I have implemented support for the generic regulator-off-on-delay-us
>      > property
>      > in the qcom-rpmh-regulator driver and set the constraint to 2.3s
>     in the
>      > device tree for the Yoga Slim 7x.
> 
>     Yes but please listen to me. That is an extraordinary delay being
>     introduced.
> 
>     It is indicative of a serious problem we have not root caused. These
>     LDOs are used in mobile phones which are aggressively designed to save
>     power all the time.
> 
>     In fact the whole idea of voting for clocks and bandwidth is it
>     mitigate
>     the default assumption in these class of devices - switch off the power
>     first.
> 
>     What is that 2.3 seconds, why is it needed. "Brownout" but why ? I
>     don't
>     think we have really established.
> 
>      > I tested the 1.1s scenario you mentioned, and it is working fine.
>     The
>      > regulator
>      > core now correctly blocks the enable call until the physical
>     discharge delay
>      > has passed, preventing the brownout without needing logic in the
>     camera
>      > driver.
> 
>     The physical discharge delay we have _not_ established IMO. Have you
>     checked the CCI pins ?
> 
>     I think we should stop pushing patches until a root-cause has been
>     identified.
> 
>     For example - we can interrogate the LDO settings via SPMI registers to
>     see if the LDO is really switched off.
> 
>     Similarly we can interrogate the LDOs to see if they are set for active
>     discharge.
> 
>     A fix might be to make a platform driver to set those bits for the
>     relevant LDOs absent a firmware fix for the same.
> 
>     I'm not comfortable pushing changes predicated on papering over an
>     issue
>     that hasn't been root-caused.
> 
>      >
>      > I am going to drop the Autosuspend patch entirely and verify the
>     clean
>      > driver one last time.
>      >
>      > Plan for v4:
>      > 1. Submit the Regulator/DT fixes separately to linux-arm-msm.
>      > 2. Submit v4 of this series containing only the cleanup and power-
>      > sequence fixes.
>      >
>      > Thanks for pushing for the correct fix, the regulator approach is
>     indeed
>      > much cleaner.
>      >
>      > Thanks & Regards,
>      > Saikiran
>      >
>      >
>      > On Tue, 27 Jan 2026, 15:16 Bryan O'Donoghue,
>     <bryan.odonoghue@linaro.org <mailto:bryan.odonoghue@linaro.org>
>      > <mailto:bryan.odonoghue@linaro.org
>     <mailto:bryan.odonoghue@linaro.org>>> wrote:
>      >
>      >     On 26/01/2026 17:34, Saikiran wrote:
>      >      > On Qualcomm X1E80100 platforms, the OV02C10 sensor experiences
>      >     brownouts
>      >      > if power-cycled too quickly (< 2.3s) due to slow passive
>     discharge of
>      >      > regulator rails.
>      >      >
>      >      > Implement Runtime PM Autosuspend with a delay of 1000ms. This
>      >     keeps the
>      >      > regulators enabled for a short duration after the device
>     is closed,
>      >      > preventing costly power-off/power-on cycles during rapid user
>      >      > interactions (e.g., browser permission checks).
>      >
>      >     But if you try to power the sensor 1.1 seconds later what
>     happens ?
>      >
>      >     With this commit log this submission is a NAK, for example
>     why do I
>      >     want
>      >     this change on an x86 machine ?
>      >
>      >     We need to root-cause the failure not paper over it.
>      >
>      >     ---
>      >     bod
>      >
> 

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Bryan O'Donoghue]

On 27/01/2026 11:06, Bryan O'Donoghue wrote:
> So, SPMI is not exported in /sys/kernel/debug/regmap - however
> 
> drivers/regulator/qcom-rpmh-regulator.c
> 
> Lets add this to probe
> 
> unsigned int val, i;
>       u16 bases[] = {0x4000, 0x4300, 0x4600}; // LDO1, LDO4, LDO7
>       const char *names[] = {"LDO1(1.2V)", "LDO4(1.8V)", "LDO7(2.8V)"};
>       struct regmap *p_regmap = dev_get_regmap(dev->parent, NULL);
> 
>       if (p_regmap) {
>           pr_info("--- OV02C10 PMIC RAIL DUMP START ---\n");
>           for (i = 0; i < 3; i++) {
>               // Check Config (Active Discharge)
>               regmap_read(p_regmap, bases[i] + 0x41, &val);
>               pr_info("!!! %s SEC_CTRL (0x%04x) = 0x%02x (Bit7: Active
> Discharge)\n",
>                       names[i], bases[i] + 0x41, val);
> 
>               // Check Status (Is it actually on?)
>               regmap_read(p_regmap, bases[i] + 0x08, &val);
>               pr_info("!!! %s STATUS   (0x%04x) = 0x%02x (Bit7: VREG_OK,
> Bit0: VREG_ON)\n",
>                       names[i], bases[i] + 0x08, val);
> 
>               // Check Pull-down config (Secondary check)
>               regmap_read(p_regmap, bases[i] + 0x42, &val);
>               pr_info("!!! %s PD_CTRL   (0x%04x) = 0x%02x\n",
>                       names[i], bases[i] + 0x42, val);
>           }
>           pr_info("--- OV02C10 PMIC RAIL DUMP END ---\n");
>       }

Obviously only do this for PM8010 for the other RPMh which may not have 
this offset.

---
bod

Re: [PATCH v3 3/3] media: i2c: ov02c10: Use runtime PM autosuspend to avoid brownouts

[Saikiran B]

Hi Bryan and Hans,

I implemented your suggested debug probe in qcom-rpmh-regulator.c to inspect
the registers directly. Unfortunately, on this platform (X1E80100), it returns:

  rpmh_regulator_probe: --- OV02C10 PMIC DUMP: Failed to get parent regmap ---

This confirms that the AP does not have direct access to the PMIC configuration
registers (likely blocked by the RPMh firewall/access control).

However, I have confirmed definitively that the root cause is the 2.3s passive
discharge requiring a mandatory off-time.

When I apply the `regulator-off-on-delay-us = <2300000>;` property to the
camera regulators in the device tree (and patch the regulator driver to support
it), the camera operates flawlessly without needing any workarounds (like
autosuspend / Software reset / additional delays) in the sensor driver.
The regulator core correctly blocks the
re-enable until the discharge constraint is met.

The regulator delay and figuring the 2.3s out is another issue which I
will keep digging at from now.

Plan for v4:

1. For this ov02c10 series (v4), I will drop the "Autosuspend" patch as it is
   no longer needed with the correct platform fix.

3. I will keep the "Race Fix" (Patch 1) and a cleaned-up "Power Sequence"
   (Patch 2) which addresses Hans's feedback (5ms reset assertion, no
software reset) to ensure
   the driver is compliant with the datasheet.

Please let me know if you have any questions.

Regards,
Saikiran

On Tue, Jan 27, 2026 at 4:41 PM Bryan O'Donoghue <bod@kernel.org> wrote:
>
> On 27/01/2026 11:06, Bryan O'Donoghue wrote:
> > So, SPMI is not exported in /sys/kernel/debug/regmap - however
> >
> > drivers/regulator/qcom-rpmh-regulator.c
> >
> > Lets add this to probe
> >
> > unsigned int val, i;
> >       u16 bases[] = {0x4000, 0x4300, 0x4600}; // LDO1, LDO4, LDO7
> >       const char *names[] = {"LDO1(1.2V)", "LDO4(1.8V)", "LDO7(2.8V)"};
> >       struct regmap *p_regmap = dev_get_regmap(dev->parent, NULL);
> >
> >       if (p_regmap) {
> >           pr_info("--- OV02C10 PMIC RAIL DUMP START ---\n");
> >           for (i = 0; i < 3; i++) {
> >               // Check Config (Active Discharge)
> >               regmap_read(p_regmap, bases[i] + 0x41, &val);
> >               pr_info("!!! %s SEC_CTRL (0x%04x) = 0x%02x (Bit7: Active
> > Discharge)\n",
> >                       names[i], bases[i] + 0x41, val);
> >
> >               // Check Status (Is it actually on?)
> >               regmap_read(p_regmap, bases[i] + 0x08, &val);
> >               pr_info("!!! %s STATUS   (0x%04x) = 0x%02x (Bit7: VREG_OK,
> > Bit0: VREG_ON)\n",
> >                       names[i], bases[i] + 0x08, val);
> >
> >               // Check Pull-down config (Secondary check)
> >               regmap_read(p_regmap, bases[i] + 0x42, &val);
> >               pr_info("!!! %s PD_CTRL   (0x%04x) = 0x%02x\n",
> >                       names[i], bases[i] + 0x42, val);
> >           }
> >           pr_info("--- OV02C10 PMIC RAIL DUMP END ---\n");
> >       }
>
> Obviously only do this for PM8010 for the other RPMh which may not have
> this offset.
>
> ---
> bod