[PATCH v2 0/2] Fix OV02C10 camera pipeline lock issues

[PATCH v2 0/2] Fix OV02C10 camera pipeline lock issues

[Saikiran]

This series fixes critical pipeline lock leaks that permanently lock the
camera on Snapdragon X Elite systems when applications close unexpectedly
or subdevices fail to stop cleanly.

Changes in v2:
- Patch 1: Added Fixes tag and Cc: stable as requested by Bryan O'Donoghue
          Fixed code style (moved opening brace)
          Simplified commit message
- Patch 2: Added Fixes tag and Cc: stable
          Removed unnecessary multi-line comment from code
          Removed invalid commit reference from commit message

Note: The brownout prevention patch (originally patch 3/3) is being reworked
based on maintainer feedback to improve power sequencing timings. It will be
submitted separately after testing the suggested approach.

Testing: All patches tested on Lenovo Yoga Slim 7x (Snapdragon X Elite,
ov02c10 camera) with libcamera/qcam, browser WebRTC, and PipeWire.

Saikiran (2):
  media: qcom: camss: Fix pipeline lock leak in stop_streaming
  media: i2c: ov02c10: Check for errors in disable_streams

 drivers/media/i2c/ov02c10.c                     |  6 +++++-
 drivers/media/platform/qcom/camss/camss-video.c | 10 ++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.51.0

[PATCH v2 1/2] media: qcom: camss: Fix pipeline lock leak in stop_streaming

[Saikiran]

When a browser or application closes the camera, if any subdevice fails
to stop streaming, video_stop_streaming() returns early without calling
video_device_pipeline_stop(). This leaves the pipeline permanently locked,
preventing any future camera access until reboot.

Fix this by logging errors but continuing to stop all remaining subdevices
and always releasing the pipeline lock, even when errors occur during the
stop sequence.

Fixes: 89013969e232 ("media: camss: sm8250: Pipeline starting and stopping for multiple virtual channels")
Cc: stable@vger.kernel.org
Tested-on: Lenovo Yoga Slim 7x (Snapdragon X Elite, ov02c10 camera)
Signed-off-by: Saikiran <bjsaikiran@gmail.com>
---
 drivers/media/platform/qcom/camss/camss-video.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-video.c b/drivers/media/platform/qcom/camss/camss-video.c
index 831486e14754..242c44f97801 100644
--- a/drivers/media/platform/qcom/camss/camss-video.c
+++ b/drivers/media/platform/qcom/camss/camss-video.c
@@ -312,9 +312,15 @@ static void video_stop_streaming(struct vb2_queue *q)
 
 		ret = v4l2_subdev_call(subdev, video, s_stream, 0);
 
+		/*
+		 * Don't return early on error - we must continue to stop
+		 * remaining subdevices and release the pipeline lock to
+		 * prevent the camera from being permanently locked.
+		 */
 		if (ret) {
-			dev_err(video->camss->dev, "Video pipeline stop failed: %d\n", ret);
-			return;
+			dev_err(video->camss->dev,
+				"Failed to stop subdev '%s': %d\n",
+				subdev->name, ret);
 		}
 	}
 
-- 
2.51.0

[PATCH v2 2/2] media: i2c: ov02c10: Check for errors in disable_streams

[Saikiran]

The ov02c10_disable_streams() function ignores the return value from
cci_write() when stopping the sensor. If the I2C write fails (e.g.,
due to CCI timeout, power management race, or device removal), the
error is silently lost.

While we still need to return 0 and call pm_runtime_put() regardless
of hardware state (to prevent PM reference leaks and pipeline lock
issues), we should at least log when the hardware stop fails.

This change:
1. Captures the cci_write() return value
2. Logs an error if the write fails
3. Still returns 0 to ensure proper cleanup

Returning an error from disable_streams would cause the camss driver's
video_stop_streaming() to exit early without releasing the pipeline
lock, permanently locking the camera.

Fixes: 0e98938b0157 ("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, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
index b86cae3d2b74..743d8544ac53 100644
--- a/drivers/media/i2c/ov02c10.c
+++ b/drivers/media/i2c/ov02c10.c
@@ -629,8 +629,12 @@ static int ov02c10_disable_streams(struct v4l2_subdev *sd,
 				   u32 pad, u64 streams_mask)
 {
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
+	int ret;
+
+	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);
+	if (ret)
+		dev_err(ov02c10->dev, "failed to stop streaming: %d\n", ret);
 
-	cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);
 	pm_runtime_put(ov02c10->dev);
 
 	return 0;
-- 
2.51.0

Re: [PATCH v2 2/2] media: i2c: ov02c10: Check for errors in disable_streams

[Konrad Dybcio]

On 1/25/26 6:17 PM, Saikiran wrote:
> The ov02c10_disable_streams() function ignores the return value from
> cci_write() when stopping the sensor. If the I2C write fails (e.g.,
> due to CCI timeout, power management race, or device removal), the
         ^ CCI -> I2C

CCI is a name for a small camera-subsystem-adjacent I2C host on
Qualcomm hardware specifically

Konrad

Re: [PATCH v2 2/2] media: i2c: ov02c10: Check for errors in disable_streams

[Hans de Goede]

Hi,

On 25-Jan-26 18:17, Saikiran wrote:
> The ov02c10_disable_streams() function ignores the return value from
> cci_write() when stopping the sensor. If the I2C write fails (e.g.,
> due to CCI timeout, power management race, or device removal), the
> error is silently lost.
> 
> While we still need to return 0 and call pm_runtime_put() regardless
> of hardware state (to prevent PM reference leaks and pipeline lock
> issues), we should at least log when the hardware stop fails.
> 
> This change:
> 1. Captures the cci_write() return value
> 2. Logs an error if the write fails
> 3. Still returns 0 to ensure proper cleanup
> 
> Returning an error from disable_streams would cause the camss driver's
> video_stop_streaming() to exit early without releasing the pipeline
> lock, permanently locking the camera.
> 
> Fixes: 0e98938b0157 ("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, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index b86cae3d2b74..743d8544ac53 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -629,8 +629,12 @@ static int ov02c10_disable_streams(struct v4l2_subdev *sd,
>  				   u32 pad, u64 streams_mask)
>  {
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
> +	int ret;
> +
> +	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);
> +	if (ret)
> +		dev_err(ov02c10->dev, "failed to stop streaming: %d\n", ret);
>  
> -	cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);

cci_write() already logs a message on errors itself, so this is
undesirable as it will lead to duplicate log messages.

NACK, please drop this patch.

Regards,

Hans

[PATCH v2 0/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran]

This patch is a follow-up to the series "[PATCH v2 0/2] Fix OV02C10 camera
pipeline lock issues" sent yesterday [1]. It addresses the remaining brownout
stability issue on Snapdragon X Elite platforms.

This patch completes the v2 series by replacing the "cool-down" patch
(Patch 3/3) from my original v1 submission [2].

(Note: The separate series "[PATCH 0/2] Fix OV02C10 camera color and
stability issues" regarding Bayer patterns and remove() race conditions
remains independent and is currently under review.)

History:
In v1 [2], I proposed a hard 3-second delay between power cycles to prevent
brownouts. As discussed with Bryan O'Donoghue, that approach was a workaround
with a negative user experience. This patch implements the proper fix we
discussed.

Problem:
The RPMh regulators on X1E80100 platforms lack active discharge, taking
2+ seconds to passively discharge. Rapid camera cycling (e.g., WebRTC)
causes brownouts because the sensor is powered on while still holding
residual charge, locking the internal microcontroller.

Solution:
Instead of a delay, we now keep regulators and clocks continuously enabled
after the first power-on. We control sensor state exclusively via:
1. Hardware GPIO reset
2. Software reset (OmniVision register 0x0103)

This reduces power cycle latency from ~2300ms to ~70ms (including a new
50ms stabilization delay) and eliminates brownouts entirely.

Testing:
- Validated on Lenovo Yoga Slim 7x (Snapdragon X Elite).
- Performed 100+ rapid open/close cycles with no black frames or lockups.

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

Saikiran (1):
  media: i2c: ov02c10: Keep power on and use reset for power management

 drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 50 deletions(-)

--
2.51.0

[PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran]

The OV02C10 sensor was experiencing brownout conditions during rapid
power cycles (e.g., browser WebRTC permission checks) on Qualcomm
platforms, causing the sensor to lock up and require a system reboot.

Root cause:
The Qualcomm RPMh regulator driver does not support active discharge,
requiring regulators to passively discharge via leakage current. This
takes 2+ seconds on X1E80100 platforms. Without complete voltage
discharge, the sensor's internal microcontroller does not fully reset,
leading to I2C timeouts and a locked state.

Solution:
Instead of power cycling the regulators, keep them continuously enabled
and use reset signals to control the sensor state:

- power_off(): Assert hardware reset GPIO (keep regulators/clock ON)
- power_on(): Release hardware reset + trigger software reset via
  register 0x0103 (standard OmniVision software reset)

This approach:
- Eliminates the 2+ second discharge delay
- Enables instant camera reopening (~17ms vs 2.3s)
- Properly resets the sensor state machine via reset signals
- Maintains correct power sequencing on first initialization
- Follows OmniVision sensor conventions (0x0103 software reset)

The first power-on still performs full regulator and clock
initialization. Subsequent power cycles only toggle reset signals,
avoiding the discharge delay entirely.

Tested on Lenovo Yoga Slim 7x (X1E80100) with rapid camera open/close
cycles - no brownouts or lockups observed.

Fixes: 44f89010dae0 ("media: i2c: add OmniVision OV02C10 sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Saikiran <bjsaikiran@gmail.com>
---
 drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
index 7e9454e8540c..08d268de60ec 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)
 
@@ -390,8 +392,8 @@ struct ov02c10 {
 	u32 link_freq_index;
 	u8 mipi_lanes;
 
-	/* Power cycling rate limit */
-	ktime_t last_power_off;
+	/* Power management: track if regulators are enabled */
+	bool powered;
 };
 
 static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
@@ -680,25 +682,16 @@ static int ov02c10_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 
-	/* 1. Assert Reset */
-	gpiod_set_value_cansleep(ov02c10->reset, 1);
-
-	/* 2. Disable Clock (Stop sensor state machine) */
-	clk_disable_unprepare(ov02c10->img_clk);
-	usleep_range(1000, 1500);
-
-	/* 3. Disable Power */
-	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
-			       ov02c10->supplies);
-
 	/*
-	 * 4. Discharge Wait
-	 * Wait for regulators to fully discharge before returning.
-	 * This delay ensures clean power cycling.
+	 * Keep regulators and clock ON to avoid discharge delay.
+	 * Just assert hardware reset to put sensor in reset state.
+	 * This allows instant power-on without waiting for regulator discharge.
 	 */
-	usleep_range(50000, 55000);
+	if (ov02c10->reset)
+		gpiod_set_value_cansleep(ov02c10->reset, 1);
 
-	ov02c10->last_power_off = ktime_get();
+	/* Keep clock running - sensor needs it for software reset */
+	/* Keep regulators enabled - avoids 2.3s discharge delay */
 
 	return 0;
 }
@@ -708,50 +701,63 @@ static int ov02c10_power_on(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov02c10 *ov02c10 = to_ov02c10(sd);
 	int ret;
-	s64 delta_us;
 
 	/*
-	 * Mandatory Cool-Down:
-	 * If the camera was powered off within the last 3 seconds, ensure at least
-	 * 2 seconds have elapsed to allow full regulator discharge and sensor reset.
-	 * This prevents brownouts during rapid open/close/open sequences.
+	 * On first power-on, do full initialization.
+	 * On subsequent power-ons, regulators/clock are already on,
+	 * so we just need to release reset and do software reset.
 	 */
-	delta_us = ktime_us_delta(ktime_get(), ov02c10->last_power_off);
-	if (delta_us < 3000000) {
-		dev_dbg(dev, "Enforcing %lld us cool-down period\n", 2000000 - delta_us);
-		fsleep(2000000 - delta_us);
+	if (!ov02c10->powered) {
+		/* First time: enable everything */
+		if (ov02c10->reset) {
+			gpiod_set_value_cansleep(ov02c10->reset, 1);
+			usleep_range(2000, 2200);
+		}
+
+		ret = clk_prepare_enable(ov02c10->img_clk);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable imaging clock: %d", ret);
+			return ret;
+		}
+
+		usleep_range(2000, 2200);
+
+		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;
+		}
+
+		ov02c10->powered = true;
 	}
 
-	/*
-	 * Standard Power-Up Sequence:
-	 * 1. Enable Regulators
-	 * 2. Enable Clock
-	 * 3. Release Reset (with ample boot time)
-	 */
-
-	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
-				    ov02c10->supplies);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable regulators: %d", ret);
-		return ret;
+	/* Release hardware reset */
+	if (ov02c10->reset) {
+		/* Ensure reset was asserted for at least 2ms */
+		usleep_range(2000, 2200);
+		gpiod_set_value_cansleep(ov02c10->reset, 0);
+		/*
+		 * Wait for sensor microcontroller to stabilize after reset release.
+		 * 50ms prevents black frames during rapid power cycling by ensuring
+		 * the sensor's internal state machine is fully initialized before
+		 * software reset and register configuration.
+		 */
+		msleep(50);
 	}
 
-	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);
+	/* 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 power/clock to stabilize */
+	/* Wait for software reset to complete */
 	usleep_range(5000, 5500);
 
-	if (ov02c10->reset) {
-		gpiod_set_value_cansleep(ov02c10->reset, 0);
-		usleep_range(80000, 85000);
-	}
-
 	return 0;
 }
 
@@ -924,6 +930,19 @@ static void ov02c10_remove(struct i2c_client *client)
 		ov02c10_power_off(ov02c10->dev);
 		pm_runtime_set_suspended(ov02c10->dev);
 	}
+
+	/* Clean up regulators/clock if still enabled */
+	if (ov02c10->powered) {
+		/* Assert reset before disabling power for clean shutdown */
+		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);
+		ov02c10->powered = false;
+	}
+
 	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sd->entity);
 	v4l2_ctrl_handler_free(sd->ctrl_handler);
-- 
2.51.0

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Hans de Goede]

Hi,

On 26-Jan-26 07:15, Saikiran wrote:
> The OV02C10 sensor was experiencing brownout conditions during rapid
> power cycles (e.g., browser WebRTC permission checks) on Qualcomm
> platforms, causing the sensor to lock up and require a system reboot.
> 
> Root cause:
> The Qualcomm RPMh regulator driver does not support active discharge,
> requiring regulators to passively discharge via leakage current. This
> takes 2+ seconds on X1E80100 platforms. Without complete voltage
> discharge, the sensor's internal microcontroller does not fully reset,
> leading to I2C timeouts and a locked state.
> 
> Solution:
> Instead of power cycling the regulators, keep them continuously enabled
> and use reset signals to control the sensor state:
> 
> - power_off(): Assert hardware reset GPIO (keep regulators/clock ON)
> - power_on(): Release hardware reset + trigger software reset via
>   register 0x0103 (standard OmniVision software reset)
> 
> This approach:
> - Eliminates the 2+ second discharge delay
> - Enables instant camera reopening (~17ms vs 2.3s)
> - Properly resets the sensor state machine via reset signals
> - Maintains correct power sequencing on first initialization
> - Follows OmniVision sensor conventions (0x0103 software reset)
> 
> The first power-on still performs full regulator and clock
> initialization. Subsequent power cycles only toggle reset signals,
> avoiding the discharge delay entirely.
> 
> Tested on Lenovo Yoga Slim 7x (X1E80100) with rapid camera open/close
> cycles - no brownouts or lockups observed.
> 
> Fixes: 44f89010dae0 ("media: i2c: add OmniVision OV02C10 sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saikiran <bjsaikiran@gmail.com>

Thank you for your work on this, both the root-cause analysis
and the code changes.

However I do not believe that this is the right approach.

This is adding platform (qcom regulator) knowledge to the sensor
driver where it does not belong.

If these qualcomm regulators need to be powered-off for at least
say 3 seconds before being powered on again then that should be
represented by setting struct regulator_desc.off_on_delay
to e.g. 3000000 (usec). On a quick check I'm not seeing a way
to set this in devicetree, so maybe this needs to be done
inside the qualcomm regulator driver.

Either way this does *not* belong inside the sensor driver.
We don't want this in the sensor driver from a design pov and
we also don't want it in the sensor driver because then it
needs to be repeated for all sensor drivers.

To avoid this 3 second delay everytime on rapid stream on/off
runtime-pm's autosuspend feature should be used with an
autosuspend delay of say 1 second. See drivers/media/i2c/ov2680.c
for an example.

This is a good idea regardless to also avoid unnecessary delays
on quick on/off related to the reset signal timing.

Regards,

Hans






> ---
>  drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index 7e9454e8540c..08d268de60ec 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)
>  
> @@ -390,8 +392,8 @@ struct ov02c10 {
>  	u32 link_freq_index;
>  	u8 mipi_lanes;
>  
> -	/* Power cycling rate limit */
> -	ktime_t last_power_off;
> +	/* Power management: track if regulators are enabled */
> +	bool powered;
>  };
>  
>  static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
> @@ -680,25 +682,16 @@ static int ov02c10_power_off(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>  
> -	/* 1. Assert Reset */
> -	gpiod_set_value_cansleep(ov02c10->reset, 1);
> -
> -	/* 2. Disable Clock (Stop sensor state machine) */
> -	clk_disable_unprepare(ov02c10->img_clk);
> -	usleep_range(1000, 1500);
> -
> -	/* 3. Disable Power */
> -	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
> -			       ov02c10->supplies);
> -
>  	/*
> -	 * 4. Discharge Wait
> -	 * Wait for regulators to fully discharge before returning.
> -	 * This delay ensures clean power cycling.
> +	 * Keep regulators and clock ON to avoid discharge delay.
> +	 * Just assert hardware reset to put sensor in reset state.
> +	 * This allows instant power-on without waiting for regulator discharge.
>  	 */
> -	usleep_range(50000, 55000);
> +	if (ov02c10->reset)
> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
>  
> -	ov02c10->last_power_off = ktime_get();
> +	/* Keep clock running - sensor needs it for software reset */
> +	/* Keep regulators enabled - avoids 2.3s discharge delay */
>  
>  	return 0;
>  }
> @@ -708,50 +701,63 @@ static int ov02c10_power_on(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>  	int ret;
> -	s64 delta_us;
>  
>  	/*
> -	 * Mandatory Cool-Down:
> -	 * If the camera was powered off within the last 3 seconds, ensure at least
> -	 * 2 seconds have elapsed to allow full regulator discharge and sensor reset.
> -	 * This prevents brownouts during rapid open/close/open sequences.
> +	 * On first power-on, do full initialization.
> +	 * On subsequent power-ons, regulators/clock are already on,
> +	 * so we just need to release reset and do software reset.
>  	 */
> -	delta_us = ktime_us_delta(ktime_get(), ov02c10->last_power_off);
> -	if (delta_us < 3000000) {
> -		dev_dbg(dev, "Enforcing %lld us cool-down period\n", 2000000 - delta_us);
> -		fsleep(2000000 - delta_us);
> +	if (!ov02c10->powered) {
> +		/* First time: enable everything */
> +		if (ov02c10->reset) {
> +			gpiod_set_value_cansleep(ov02c10->reset, 1);
> +			usleep_range(2000, 2200);
> +		}
> +
> +		ret = clk_prepare_enable(ov02c10->img_clk);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable imaging clock: %d", ret);
> +			return ret;
> +		}
> +
> +		usleep_range(2000, 2200);
> +
> +		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;
> +		}
> +
> +		ov02c10->powered = true;
>  	}
>  
> -	/*
> -	 * Standard Power-Up Sequence:
> -	 * 1. Enable Regulators
> -	 * 2. Enable Clock
> -	 * 3. Release Reset (with ample boot time)
> -	 */
> -
> -	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
> -				    ov02c10->supplies);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable regulators: %d", ret);
> -		return ret;
> +	/* Release hardware reset */
> +	if (ov02c10->reset) {
> +		/* Ensure reset was asserted for at least 2ms */
> +		usleep_range(2000, 2200);
> +		gpiod_set_value_cansleep(ov02c10->reset, 0);
> +		/*
> +		 * Wait for sensor microcontroller to stabilize after reset release.
> +		 * 50ms prevents black frames during rapid power cycling by ensuring
> +		 * the sensor's internal state machine is fully initialized before
> +		 * software reset and register configuration.
> +		 */
> +		msleep(50);
>  	}
>  
> -	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);
> +	/* 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 power/clock to stabilize */
> +	/* Wait for software reset to complete */
>  	usleep_range(5000, 5500);
>  
> -	if (ov02c10->reset) {
> -		gpiod_set_value_cansleep(ov02c10->reset, 0);
> -		usleep_range(80000, 85000);
> -	}
> -
>  	return 0;
>  }
>  
> @@ -924,6 +930,19 @@ static void ov02c10_remove(struct i2c_client *client)
>  		ov02c10_power_off(ov02c10->dev);
>  		pm_runtime_set_suspended(ov02c10->dev);
>  	}
> +
> +	/* Clean up regulators/clock if still enabled */
> +	if (ov02c10->powered) {
> +		/* Assert reset before disabling power for clean shutdown */
> +		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);
> +		ov02c10->powered = false;
> +	}
> +
>  	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&sd->entity);
>  	v4l2_ctrl_handler_free(sd->ctrl_handler);

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

Hi,

Thanks for your review, I understand. Let me try that. Will resubmit
if that approach works.

Thanks & regards,
Saikiran

On Mon, Jan 26, 2026 at 4:02 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 26-Jan-26 07:15, Saikiran wrote:
> > The OV02C10 sensor was experiencing brownout conditions during rapid
> > power cycles (e.g., browser WebRTC permission checks) on Qualcomm
> > platforms, causing the sensor to lock up and require a system reboot.
> >
> > Root cause:
> > The Qualcomm RPMh regulator driver does not support active discharge,
> > requiring regulators to passively discharge via leakage current. This
> > takes 2+ seconds on X1E80100 platforms. Without complete voltage
> > discharge, the sensor's internal microcontroller does not fully reset,
> > leading to I2C timeouts and a locked state.
> >
> > Solution:
> > Instead of power cycling the regulators, keep them continuously enabled
> > and use reset signals to control the sensor state:
> >
> > - power_off(): Assert hardware reset GPIO (keep regulators/clock ON)
> > - power_on(): Release hardware reset + trigger software reset via
> >   register 0x0103 (standard OmniVision software reset)
> >
> > This approach:
> > - Eliminates the 2+ second discharge delay
> > - Enables instant camera reopening (~17ms vs 2.3s)
> > - Properly resets the sensor state machine via reset signals
> > - Maintains correct power sequencing on first initialization
> > - Follows OmniVision sensor conventions (0x0103 software reset)
> >
> > The first power-on still performs full regulator and clock
> > initialization. Subsequent power cycles only toggle reset signals,
> > avoiding the discharge delay entirely.
> >
> > Tested on Lenovo Yoga Slim 7x (X1E80100) with rapid camera open/close
> > cycles - no brownouts or lockups observed.
> >
> > Fixes: 44f89010dae0 ("media: i2c: add OmniVision OV02C10 sensor driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Saikiran <bjsaikiran@gmail.com>
>
> Thank you for your work on this, both the root-cause analysis
> and the code changes.
>
> However I do not believe that this is the right approach.
>
> This is adding platform (qcom regulator) knowledge to the sensor
> driver where it does not belong.
>
> If these qualcomm regulators need to be powered-off for at least
> say 3 seconds before being powered on again then that should be
> represented by setting struct regulator_desc.off_on_delay
> to e.g. 3000000 (usec). On a quick check I'm not seeing a way
> to set this in devicetree, so maybe this needs to be done
> inside the qualcomm regulator driver.
>
> Either way this does *not* belong inside the sensor driver.
> We don't want this in the sensor driver from a design pov and
> we also don't want it in the sensor driver because then it
> needs to be repeated for all sensor drivers.
>
> To avoid this 3 second delay everytime on rapid stream on/off
> runtime-pm's autosuspend feature should be used with an
> autosuspend delay of say 1 second. See drivers/media/i2c/ov2680.c
> for an example.
>
> This is a good idea regardless to also avoid unnecessary delays
> on quick on/off related to the reset signal timing.
>
> Regards,
>
> Hans
>
>
>
>
>
>
> > ---
> >  drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
> >  1 file changed, 69 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> > index 7e9454e8540c..08d268de60ec 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)
> >
> > @@ -390,8 +392,8 @@ struct ov02c10 {
> >       u32 link_freq_index;
> >       u8 mipi_lanes;
> >
> > -     /* Power cycling rate limit */
> > -     ktime_t last_power_off;
> > +     /* Power management: track if regulators are enabled */
> > +     bool powered;
> >  };
> >
> >  static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
> > @@ -680,25 +682,16 @@ static int ov02c10_power_off(struct device *dev)
> >       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >       struct ov02c10 *ov02c10 = to_ov02c10(sd);
> >
> > -     /* 1. Assert Reset */
> > -     gpiod_set_value_cansleep(ov02c10->reset, 1);
> > -
> > -     /* 2. Disable Clock (Stop sensor state machine) */
> > -     clk_disable_unprepare(ov02c10->img_clk);
> > -     usleep_range(1000, 1500);
> > -
> > -     /* 3. Disable Power */
> > -     regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
> > -                            ov02c10->supplies);
> > -
> >       /*
> > -      * 4. Discharge Wait
> > -      * Wait for regulators to fully discharge before returning.
> > -      * This delay ensures clean power cycling.
> > +      * Keep regulators and clock ON to avoid discharge delay.
> > +      * Just assert hardware reset to put sensor in reset state.
> > +      * This allows instant power-on without waiting for regulator discharge.
> >        */
> > -     usleep_range(50000, 55000);
> > +     if (ov02c10->reset)
> > +             gpiod_set_value_cansleep(ov02c10->reset, 1);
> >
> > -     ov02c10->last_power_off = ktime_get();
> > +     /* Keep clock running - sensor needs it for software reset */
> > +     /* Keep regulators enabled - avoids 2.3s discharge delay */
> >
> >       return 0;
> >  }
> > @@ -708,50 +701,63 @@ static int ov02c10_power_on(struct device *dev)
> >       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >       struct ov02c10 *ov02c10 = to_ov02c10(sd);
> >       int ret;
> > -     s64 delta_us;
> >
> >       /*
> > -      * Mandatory Cool-Down:
> > -      * If the camera was powered off within the last 3 seconds, ensure at least
> > -      * 2 seconds have elapsed to allow full regulator discharge and sensor reset.
> > -      * This prevents brownouts during rapid open/close/open sequences.
> > +      * On first power-on, do full initialization.
> > +      * On subsequent power-ons, regulators/clock are already on,
> > +      * so we just need to release reset and do software reset.
> >        */
> > -     delta_us = ktime_us_delta(ktime_get(), ov02c10->last_power_off);
> > -     if (delta_us < 3000000) {
> > -             dev_dbg(dev, "Enforcing %lld us cool-down period\n", 2000000 - delta_us);
> > -             fsleep(2000000 - delta_us);
> > +     if (!ov02c10->powered) {
> > +             /* First time: enable everything */
> > +             if (ov02c10->reset) {
> > +                     gpiod_set_value_cansleep(ov02c10->reset, 1);
> > +                     usleep_range(2000, 2200);
> > +             }
> > +
> > +             ret = clk_prepare_enable(ov02c10->img_clk);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable imaging clock: %d", ret);
> > +                     return ret;
> > +             }
> > +
> > +             usleep_range(2000, 2200);
> > +
> > +             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;
> > +             }
> > +
> > +             ov02c10->powered = true;
> >       }
> >
> > -     /*
> > -      * Standard Power-Up Sequence:
> > -      * 1. Enable Regulators
> > -      * 2. Enable Clock
> > -      * 3. Release Reset (with ample boot time)
> > -      */
> > -
> > -     ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
> > -                                 ov02c10->supplies);
> > -     if (ret < 0) {
> > -             dev_err(dev, "failed to enable regulators: %d", ret);
> > -             return ret;
> > +     /* Release hardware reset */
> > +     if (ov02c10->reset) {
> > +             /* Ensure reset was asserted for at least 2ms */
> > +             usleep_range(2000, 2200);
> > +             gpiod_set_value_cansleep(ov02c10->reset, 0);
> > +             /*
> > +              * Wait for sensor microcontroller to stabilize after reset release.
> > +              * 50ms prevents black frames during rapid power cycling by ensuring
> > +              * the sensor's internal state machine is fully initialized before
> > +              * software reset and register configuration.
> > +              */
> > +             msleep(50);
> >       }
> >
> > -     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);
> > +     /* 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 power/clock to stabilize */
> > +     /* Wait for software reset to complete */
> >       usleep_range(5000, 5500);
> >
> > -     if (ov02c10->reset) {
> > -             gpiod_set_value_cansleep(ov02c10->reset, 0);
> > -             usleep_range(80000, 85000);
> > -     }
> > -
> >       return 0;
> >  }
> >
> > @@ -924,6 +930,19 @@ static void ov02c10_remove(struct i2c_client *client)
> >               ov02c10_power_off(ov02c10->dev);
> >               pm_runtime_set_suspended(ov02c10->dev);
> >       }
> > +
> > +     /* Clean up regulators/clock if still enabled */
> > +     if (ov02c10->powered) {
> > +             /* Assert reset before disabling power for clean shutdown */
> > +             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);
> > +             ov02c10->powered = false;
> > +     }
> > +
> >       v4l2_subdev_cleanup(sd);
> >       media_entity_cleanup(&sd->entity);
> >       v4l2_ctrl_handler_free(sd->ctrl_handler);
>

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 06:15, Saikiran wrote:
> The OV02C10 sensor was experiencing brownout conditions during rapid
> power cycles (e.g., browser WebRTC permission checks) on Qualcomm
> platforms, causing the sensor to lock up and require a system reboot.
> 
> Root cause:
> The Qualcomm RPMh regulator driver does not support active discharge,
> requiring regulators to passively discharge via leakage current. This
> takes 2+ seconds on X1E80100 platforms. Without complete voltage
> discharge, the sensor's internal microcontroller does not fully reset,
> leading to I2C timeouts and a locked state.

Where do you get this conclusion from ?

Are you inferring it from what you see on the platform or can you point 
to some known data-source for this ?

2 seconds to discharge ? These regulators are PM8010 anyway - so you're 
saying the PMIC takes two seconds to discharge ?

> Solution:
> Instead of power cycling the regulators, keep them continuously enabled
> and use reset signals to control the sensor state:

If this is really a problem with the regulators and I don't think we 
have established that - then it is a fix that needs to go into the 
regulators.

Did you try out the suggested fix I gave you yesterday ?

The options are:

1. Make power_on/power_off be more consistent with the data-sheet.
    This I'd guess 99% certain what is going wrong for you or

2. If we really can establish and show a two second discharge delay
    then bring the required delay into the RPMh code so that
    regulator_bulk_disable(); is atomic from the perspective of the
    caller.

I honestly can't imagine two seconds is a real thing here but, if it is, 
then the thing that needs to change is the regulator driver to account 
for that long delay not the users of the regulators.
> - power_off(): Assert hardware reset GPIO (keep regulators/clock ON)
> - power_on(): Release hardware reset + trigger software reset via
>    register 0x0103 (standard OmniVision software reset)
> 
> This approach:
> - Eliminates the 2+ second discharge delay
> - Enables instant camera reopening (~17ms vs 2.3s)
> - Properly resets the sensor state machine via reset signals
> - Maintains correct power sequencing on first initialization
> - Follows OmniVision sensor conventions (0x0103 software reset)
> 
> The first power-on still performs full regulator and clock
> initialization. Subsequent power cycles only toggle reset signals,
> avoiding the discharge delay entirely.
> 
> Tested on Lenovo Yoga Slim 7x (X1E80100) with rapid camera open/close
> cycles - no brownouts or lockups observed.
> 
> Fixes: 44f89010dae0 ("media: i2c: add OmniVision OV02C10 sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Saikiran <bjsaikiran@gmail.com>
> ---
>   drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
>   1 file changed, 69 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index 7e9454e8540c..08d268de60ec 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)
>   
> @@ -390,8 +392,8 @@ struct ov02c10 {
>   	u32 link_freq_index;
>   	u8 mipi_lanes;
>   
> -	/* Power cycling rate limit */
> -	ktime_t last_power_off;
> +	/* Power management: track if regulators are enabled */
> +	bool powered;
>   };
>   
>   static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
> @@ -680,25 +682,16 @@ static int ov02c10_power_off(struct device *dev)
>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>   	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>   
> -	/* 1. Assert Reset */
> -	gpiod_set_value_cansleep(ov02c10->reset, 1);
> -
> -	/* 2. Disable Clock (Stop sensor state machine) */
> -	clk_disable_unprepare(ov02c10->img_clk);
> -	usleep_range(1000, 1500);
> -
> -	/* 3. Disable Power */
> -	regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
> -			       ov02c10->supplies);
> -
>   	/*
> -	 * 4. Discharge Wait
> -	 * Wait for regulators to fully discharge before returning.
> -	 * This delay ensures clean power cycling.
> +	 * Keep regulators and clock ON to avoid discharge delay.
> +	 * Just assert hardware reset to put sensor in reset state.
> +	 * This allows instant power-on without waiting for regulator discharge.
>   	 */
> -	usleep_range(50000, 55000);
> +	if (ov02c10->reset)
> +		gpiod_set_value_cansleep(ov02c10->reset, 1);
>   
> -	ov02c10->last_power_off = ktime_get();
> +	/* Keep clock running - sensor needs it for software reset */
> +	/* Keep regulators enabled - avoids 2.3s discharge delay */
>   
>   	return 0;
>   }
> @@ -708,50 +701,63 @@ static int ov02c10_power_on(struct device *dev)
>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>   	struct ov02c10 *ov02c10 = to_ov02c10(sd);
>   	int ret;
> -	s64 delta_us;
>   
>   	/*
> -	 * Mandatory Cool-Down:
> -	 * If the camera was powered off within the last 3 seconds, ensure at least
> -	 * 2 seconds have elapsed to allow full regulator discharge and sensor reset.
> -	 * This prevents brownouts during rapid open/close/open sequences.
> +	 * On first power-on, do full initialization.
> +	 * On subsequent power-ons, regulators/clock are already on,
> +	 * so we just need to release reset and do software reset.
>   	 */
> -	delta_us = ktime_us_delta(ktime_get(), ov02c10->last_power_off);
> -	if (delta_us < 3000000) {
> -		dev_dbg(dev, "Enforcing %lld us cool-down period\n", 2000000 - delta_us);
> -		fsleep(2000000 - delta_us);
> +	if (!ov02c10->powered) {
> +		/* First time: enable everything */
> +		if (ov02c10->reset) {
> +			gpiod_set_value_cansleep(ov02c10->reset, 1);
> +			usleep_range(2000, 2200);
> +		}
> +
> +		ret = clk_prepare_enable(ov02c10->img_clk);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable imaging clock: %d", ret);
> +			return ret;
> +		}
> +
> +		usleep_range(2000, 2200);
> +
> +		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;
> +		}
> +
> +		ov02c10->powered = true;
>   	}
>   
> -	/*
> -	 * Standard Power-Up Sequence:
> -	 * 1. Enable Regulators
> -	 * 2. Enable Clock
> -	 * 3. Release Reset (with ample boot time)
> -	 */
> -
> -	ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
> -				    ov02c10->supplies);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable regulators: %d", ret);
> -		return ret;
> +	/* Release hardware reset */
> +	if (ov02c10->reset) {
> +		/* Ensure reset was asserted for at least 2ms */
> +		usleep_range(2000, 2200);
> +		gpiod_set_value_cansleep(ov02c10->reset, 0);
> +		/*
> +		 * Wait for sensor microcontroller to stabilize after reset release.
> +		 * 50ms prevents black frames during rapid power cycling by ensuring
> +		 * the sensor's internal state machine is fully initialized before
> +		 * software reset and register configuration.
> +		 */
> +		msleep(50);
>   	}
>   
> -	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);
> +	/* 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 power/clock to stabilize */
> +	/* Wait for software reset to complete */
>   	usleep_range(5000, 5500);
>   
> -	if (ov02c10->reset) {
> -		gpiod_set_value_cansleep(ov02c10->reset, 0);
> -		usleep_range(80000, 85000);
> -	}
> -
>   	return 0;
>   }
>   
> @@ -924,6 +930,19 @@ static void ov02c10_remove(struct i2c_client *client)
>   		ov02c10_power_off(ov02c10->dev);
>   		pm_runtime_set_suspended(ov02c10->dev);
>   	}
> +
> +	/* Clean up regulators/clock if still enabled */
> +	if (ov02c10->powered) {
> +		/* Assert reset before disabling power for clean shutdown */
> +		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);
> +		ov02c10->powered = false;
> +	}
> +
>   	v4l2_subdev_cleanup(sd);
>   	media_entity_cleanup(&sd->entity);
>   	v4l2_ctrl_handler_free(sd->ctrl_handler);

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

"Where do you get this conclusion from ? Are you inferring it from
what you see on the platform or can you point to some known
data-source for this ?"

This is determined on the Lenovo Yoga Slim 7x (X1E80100). I tested
extensively and found that if I attempt to power-on the sensor less
than ~2.3 seconds after power-off, it fails to identify or times out
on I2C (brownout behavior). If we wait >2.3s, it works reliably 100%
of the time.

"2 seconds to discharge ? These regulators are PM8010 anyway - so
you're saying the PMIC takes two seconds to discharge ?"

Yes. I checked the regulator driver
(drivers/regulator/qcom-rpmh-regulator.c) and found that unlike other
Qualcomm regulator drivers (e.g., spmi/glink), it currently lacks
active_discharge / pull-down support. Without active discharge, the
voltage rails float and decay very slowly via leakage current when the
load (sensor) is in reset/high-Z.

"Did you try out the suggested fix I gave you yesterday ? Make
power_on/power_off be more consistent with the data-sheet."

Yes, I implemented your suggested power sequence (asserting reset
before power-on, respecting T1 and T2 timings). While this is correct
and good practice, it did not solve the brownout issue during rapid
cycling. The sensor internal state machine seems to require the rails
to drop near 0V to reset completely, and the passive discharge is
simply too slow on this platform.

"If we really can establish and show a two second discharge delay then
the thing that needs to change is the regulator driver"

I agree with you and Hans that solving the discharge time belongs in
the regulator driver. However, implementing off-on-delay or active
discharge in the shared RPMh driver is a larger task that affects the
whole platform.

As Hans suggested, I will switch to using Runtime PM Autosuspend with
a ~2s delay and check.

Thanks & Regards,
Saikiran

On Mon, Jan 26, 2026 at 4:39 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 26/01/2026 06:15, Saikiran wrote:
> > The OV02C10 sensor was experiencing brownout conditions during rapid
> > power cycles (e.g., browser WebRTC permission checks) on Qualcomm
> > platforms, causing the sensor to lock up and require a system reboot.
> >
> > Root cause:
> > The Qualcomm RPMh regulator driver does not support active discharge,
> > requiring regulators to passively discharge via leakage current. This
> > takes 2+ seconds on X1E80100 platforms. Without complete voltage
> > discharge, the sensor's internal microcontroller does not fully reset,
> > leading to I2C timeouts and a locked state.
>
> Where do you get this conclusion from ?
>
> Are you inferring it from what you see on the platform or can you point
> to some known data-source for this ?
>
> 2 seconds to discharge ? These regulators are PM8010 anyway - so you're
> saying the PMIC takes two seconds to discharge ?
>
> > Solution:
> > Instead of power cycling the regulators, keep them continuously enabled
> > and use reset signals to control the sensor state:
>
> If this is really a problem with the regulators and I don't think we
> have established that - then it is a fix that needs to go into the
> regulators.
>
> Did you try out the suggested fix I gave you yesterday ?
>
> The options are:
>
> 1. Make power_on/power_off be more consistent with the data-sheet.
>     This I'd guess 99% certain what is going wrong for you or
>
> 2. If we really can establish and show a two second discharge delay
>     then bring the required delay into the RPMh code so that
>     regulator_bulk_disable(); is atomic from the perspective of the
>     caller.
>
> I honestly can't imagine two seconds is a real thing here but, if it is,
> then the thing that needs to change is the regulator driver to account
> for that long delay not the users of the regulators.
> > - power_off(): Assert hardware reset GPIO (keep regulators/clock ON)
> > - power_on(): Release hardware reset + trigger software reset via
> >    register 0x0103 (standard OmniVision software reset)
> >
> > This approach:
> > - Eliminates the 2+ second discharge delay
> > - Enables instant camera reopening (~17ms vs 2.3s)
> > - Properly resets the sensor state machine via reset signals
> > - Maintains correct power sequencing on first initialization
> > - Follows OmniVision sensor conventions (0x0103 software reset)
> >
> > The first power-on still performs full regulator and clock
> > initialization. Subsequent power cycles only toggle reset signals,
> > avoiding the discharge delay entirely.
> >
> > Tested on Lenovo Yoga Slim 7x (X1E80100) with rapid camera open/close
> > cycles - no brownouts or lockups observed.
> >
> > Fixes: 44f89010dae0 ("media: i2c: add OmniVision OV02C10 sensor driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Saikiran <bjsaikiran@gmail.com>
> > ---
> >   drivers/media/i2c/ov02c10.c | 119 +++++++++++++++++++++---------------
> >   1 file changed, 69 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> > index 7e9454e8540c..08d268de60ec 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)
> >
> > @@ -390,8 +392,8 @@ struct ov02c10 {
> >       u32 link_freq_index;
> >       u8 mipi_lanes;
> >
> > -     /* Power cycling rate limit */
> > -     ktime_t last_power_off;
> > +     /* Power management: track if regulators are enabled */
> > +     bool powered;
> >   };
> >
> >   static inline struct ov02c10 *to_ov02c10(struct v4l2_subdev *subdev)
> > @@ -680,25 +682,16 @@ static int ov02c10_power_off(struct device *dev)
> >       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >       struct ov02c10 *ov02c10 = to_ov02c10(sd);
> >
> > -     /* 1. Assert Reset */
> > -     gpiod_set_value_cansleep(ov02c10->reset, 1);
> > -
> > -     /* 2. Disable Clock (Stop sensor state machine) */
> > -     clk_disable_unprepare(ov02c10->img_clk);
> > -     usleep_range(1000, 1500);
> > -
> > -     /* 3. Disable Power */
> > -     regulator_bulk_disable(ARRAY_SIZE(ov02c10_supply_names),
> > -                            ov02c10->supplies);
> > -
> >       /*
> > -      * 4. Discharge Wait
> > -      * Wait for regulators to fully discharge before returning.
> > -      * This delay ensures clean power cycling.
> > +      * Keep regulators and clock ON to avoid discharge delay.
> > +      * Just assert hardware reset to put sensor in reset state.
> > +      * This allows instant power-on without waiting for regulator discharge.
> >        */
> > -     usleep_range(50000, 55000);
> > +     if (ov02c10->reset)
> > +             gpiod_set_value_cansleep(ov02c10->reset, 1);
> >
> > -     ov02c10->last_power_off = ktime_get();
> > +     /* Keep clock running - sensor needs it for software reset */
> > +     /* Keep regulators enabled - avoids 2.3s discharge delay */
> >
> >       return 0;
> >   }
> > @@ -708,50 +701,63 @@ static int ov02c10_power_on(struct device *dev)
> >       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >       struct ov02c10 *ov02c10 = to_ov02c10(sd);
> >       int ret;
> > -     s64 delta_us;
> >
> >       /*
> > -      * Mandatory Cool-Down:
> > -      * If the camera was powered off within the last 3 seconds, ensure at least
> > -      * 2 seconds have elapsed to allow full regulator discharge and sensor reset.
> > -      * This prevents brownouts during rapid open/close/open sequences.
> > +      * On first power-on, do full initialization.
> > +      * On subsequent power-ons, regulators/clock are already on,
> > +      * so we just need to release reset and do software reset.
> >        */
> > -     delta_us = ktime_us_delta(ktime_get(), ov02c10->last_power_off);
> > -     if (delta_us < 3000000) {
> > -             dev_dbg(dev, "Enforcing %lld us cool-down period\n", 2000000 - delta_us);
> > -             fsleep(2000000 - delta_us);
> > +     if (!ov02c10->powered) {
> > +             /* First time: enable everything */
> > +             if (ov02c10->reset) {
> > +                     gpiod_set_value_cansleep(ov02c10->reset, 1);
> > +                     usleep_range(2000, 2200);
> > +             }
> > +
> > +             ret = clk_prepare_enable(ov02c10->img_clk);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to enable imaging clock: %d", ret);
> > +                     return ret;
> > +             }
> > +
> > +             usleep_range(2000, 2200);
> > +
> > +             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;
> > +             }
> > +
> > +             ov02c10->powered = true;
> >       }
> >
> > -     /*
> > -      * Standard Power-Up Sequence:
> > -      * 1. Enable Regulators
> > -      * 2. Enable Clock
> > -      * 3. Release Reset (with ample boot time)
> > -      */
> > -
> > -     ret = regulator_bulk_enable(ARRAY_SIZE(ov02c10_supply_names),
> > -                                 ov02c10->supplies);
> > -     if (ret < 0) {
> > -             dev_err(dev, "failed to enable regulators: %d", ret);
> > -             return ret;
> > +     /* Release hardware reset */
> > +     if (ov02c10->reset) {
> > +             /* Ensure reset was asserted for at least 2ms */
> > +             usleep_range(2000, 2200);
> > +             gpiod_set_value_cansleep(ov02c10->reset, 0);
> > +             /*
> > +              * Wait for sensor microcontroller to stabilize after reset release.
> > +              * 50ms prevents black frames during rapid power cycling by ensuring
> > +              * the sensor's internal state machine is fully initialized before
> > +              * software reset and register configuration.
> > +              */
> > +             msleep(50);
> >       }
> >
> > -     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);
> > +     /* 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 power/clock to stabilize */
> > +     /* Wait for software reset to complete */
> >       usleep_range(5000, 5500);
> >
> > -     if (ov02c10->reset) {
> > -             gpiod_set_value_cansleep(ov02c10->reset, 0);
> > -             usleep_range(80000, 85000);
> > -     }
> > -
> >       return 0;
> >   }
> >
> > @@ -924,6 +930,19 @@ static void ov02c10_remove(struct i2c_client *client)
> >               ov02c10_power_off(ov02c10->dev);
> >               pm_runtime_set_suspended(ov02c10->dev);
> >       }
> > +
> > +     /* Clean up regulators/clock if still enabled */
> > +     if (ov02c10->powered) {
> > +             /* Assert reset before disabling power for clean shutdown */
> > +             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);
> > +             ov02c10->powered = false;
> > +     }
> > +
> >       v4l2_subdev_cleanup(sd);
> >       media_entity_cleanup(&sd->entity);
> >       v4l2_ctrl_handler_free(sd->ctrl_handler);
>

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 11:23, Saikiran B wrote:
> "Where do you get this conclusion from ? Are you inferring it from
> what you see on the platform or can you point to some known
> data-source for this ?"
> 
> This is determined on the Lenovo Yoga Slim 7x (X1E80100). I tested
> extensively and found that if I attempt to power-on the sensor less
> than ~2.3 seconds after power-off, it fails to identify or times out
> on I2C (brownout behavior). If we wait >2.3s, it works reliably 100%
> of the time.

I don't think we've established the regulator is at fault. That's the 
feedback I'm giving you here.

I think it is far, far, far more likely the power-on sequence of the 
sensor needs tweaking.

> 
> "2 seconds to discharge ? These regulators are PM8010 anyway - so
> you're saying the PMIC takes two seconds to discharge ?"
> 
> Yes. I checked the regulator driver
> (drivers/regulator/qcom-rpmh-regulator.c) and found that unlike other
> Qualcomm regulator drivers (e.g., spmi/glink), it currently lacks
> active_discharge / pull-down support. Without active discharge, the
> voltage rails float and decay very slowly via leakage current when the
> load (sensor) is in reset/high-Z.


Right so looking at the power for this part we have:

&cci1_i2c1 {
	camera@36 {
		compatible = "ovti,ov02c10";
		reg = <0x36>;

		reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
		pinctrl-names = "default";
		pinctrl-0 = <&cam_rgb_default>;

		clocks = <&camcc CAM_CC_MCLK4_CLK>;
		assigned-clocks = <&camcc CAM_CC_MCLK4_CLK>;
		assigned-clock-rates = <19200000>;

		orientation = <0>; /* front facing */

		avdd-supply = <&vreg_l7b_2p8>;
		dvdd-supply = <&vreg_l7b_2p8>;
		dovdd-supply = <&vreg_cam_1p8>;

		port {
			ov02e10_ep: endpoint {
				data-lanes = <1 2>;
				link-frequencies = /bits/ 64 <400000000>;
				remote-endpoint = <&csiphy4_ep>;
			};
		};
	};
};

// qcom standard RPMh -> PMIC LDO regulators
// these are not the droids you are looking for
vreg_l7b_2p8: ldo7 {
	regulator-name = "vreg_l7b_2p8";
	regulator-min-microvolt = <2800000>;
	regulator-max-microvolt = <2800000>;
	regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
};

// this OTOH
vreg_cam_1p8: regulator-cam-1p8 {
	compatible = "regulator-fixed";

	regulator-name = "VREG_CAM_1P8";
	regulator-min-microvolt = <1800000>;
	regulator-max-microvolt = <1800000>;

	gpio = <&tlmm 91 GPIO_ACTIVE_HIGH>;
	enable-active-high;

	pinctrl-0 = <&cam_ldo_en>;
	pinctrl-names = "default";
};

Dell has used - likely reused - part of the x86 design in the qcom 
implementation - and toggles 1v8 via a GPIO directly.

If your theory about brown-out is correct then

vreg_cam_1p8: regulator-cam-1p8 {
	// add this
	off-on-delay-us = <20000>;
};

Then please let us know how she goes.

---
bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

"I don't think we've established the regulator is at fault. That's the
feedback I'm giving you here. ... vreg_cam_1p8: regulator-cam-1p8 {
compatible = "regulator-fixed";"

Just to clarify on the regulators: on the Slim 7x, the camera supplies
(avdd, dvdd, dovdd) are all RPMh-controlled LDOs (pm8010 and pm8550),
not generic fixed regulators.

As I've confirmed that the qcom-rpmh-regulator driver doesn't natively
support active discharge or parsing off-on-delay-us (generic
property), which explains why the physical discharge constraint wasn't
being respected.

I'm taking your advice to fix this properly at the platform level in
my local tree (patching the RPMh driver and DTS to enforce the
discharge delay).

For the media driver patch (v3): I will follow Hans's suggestion to
use Runtime PM Autosuspend. This handles the rapid open/close UX
problem gracefully by keeping the regulators on during quick toggles,
avoiding the need for the driver to "know" about the platform
regulator constraints.

I will proceed with my testing and will let you know of the results.

Thanks for the feedback.

Thanks,
Saikiran


On Mon, Jan 26, 2026 at 5:11 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 26/01/2026 11:23, Saikiran B wrote:
> > "Where do you get this conclusion from ? Are you inferring it from
> > what you see on the platform or can you point to some known
> > data-source for this ?"
> >
> > This is determined on the Lenovo Yoga Slim 7x (X1E80100). I tested
> > extensively and found that if I attempt to power-on the sensor less
> > than ~2.3 seconds after power-off, it fails to identify or times out
> > on I2C (brownout behavior). If we wait >2.3s, it works reliably 100%
> > of the time.
>
> I don't think we've established the regulator is at fault. That's the
> feedback I'm giving you here.
>
> I think it is far, far, far more likely the power-on sequence of the
> sensor needs tweaking.
>
> >
> > "2 seconds to discharge ? These regulators are PM8010 anyway - so
> > you're saying the PMIC takes two seconds to discharge ?"
> >
> > Yes. I checked the regulator driver
> > (drivers/regulator/qcom-rpmh-regulator.c) and found that unlike other
> > Qualcomm regulator drivers (e.g., spmi/glink), it currently lacks
> > active_discharge / pull-down support. Without active discharge, the
> > voltage rails float and decay very slowly via leakage current when the
> > load (sensor) is in reset/high-Z.
>
>
> Right so looking at the power for this part we have:
>
> &cci1_i2c1 {
>         camera@36 {
>                 compatible = "ovti,ov02c10";
>                 reg = <0x36>;
>
>                 reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&cam_rgb_default>;
>
>                 clocks = <&camcc CAM_CC_MCLK4_CLK>;
>                 assigned-clocks = <&camcc CAM_CC_MCLK4_CLK>;
>                 assigned-clock-rates = <19200000>;
>
>                 orientation = <0>; /* front facing */
>
>                 avdd-supply = <&vreg_l7b_2p8>;
>                 dvdd-supply = <&vreg_l7b_2p8>;
>                 dovdd-supply = <&vreg_cam_1p8>;
>
>                 port {
>                         ov02e10_ep: endpoint {
>                                 data-lanes = <1 2>;
>                                 link-frequencies = /bits/ 64 <400000000>;
>                                 remote-endpoint = <&csiphy4_ep>;
>                         };
>                 };
>         };
> };
>
> // qcom standard RPMh -> PMIC LDO regulators
> // these are not the droids you are looking for
> vreg_l7b_2p8: ldo7 {
>         regulator-name = "vreg_l7b_2p8";
>         regulator-min-microvolt = <2800000>;
>         regulator-max-microvolt = <2800000>;
>         regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> };
>
> // this OTOH
> vreg_cam_1p8: regulator-cam-1p8 {
>         compatible = "regulator-fixed";
>
>         regulator-name = "VREG_CAM_1P8";
>         regulator-min-microvolt = <1800000>;
>         regulator-max-microvolt = <1800000>;
>
>         gpio = <&tlmm 91 GPIO_ACTIVE_HIGH>;
>         enable-active-high;
>
>         pinctrl-0 = <&cam_ldo_en>;
>         pinctrl-names = "default";
> };
>
> Dell has used - likely reused - part of the x86 design in the qcom
> implementation - and toggles 1v8 via a GPIO directly.
>
> If your theory about brown-out is correct then
>
> vreg_cam_1p8: regulator-cam-1p8 {
>         // add this
>         off-on-delay-us = <20000>;
> };
>
> Then please let us know how she goes.
>
> ---
> bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 11:58, Saikiran B wrote:
> "I don't think we've established the regulator is at fault. That's the
> feedback I'm giving you here. ... vreg_cam_1p8: regulator-cam-1p8 {
> compatible = "regulator-fixed";"
> 
> Just to clarify on the regulators: on the Slim 7x, the camera supplies
> (avdd, dvdd, dovdd) are all RPMh-controlled LDOs (pm8010 and pm8550),
> not generic fixed regulators.

Slim7x - not the Dell right ;)

> As I've confirmed that the qcom-rpmh-regulator driver doesn't natively
> support active discharge or parsing off-on-delay-us (generic
> property), which explains why the physical discharge constraint wasn't
> being respected.

No, the RPMh firmware should know how to do that. Not the Linux side, 
this is the part of your brown-out story that doesn't make sense.

BTW, did you try my given sequence - particularly the XSHUTDOWN in 
power_on(); ?

If the XSHUTDOWN pin is for example floating or not in the correct 
logical state when you power-on, the chip may not initialise correctly.

Which could lead you to conclude - you are having a regulator problem, 
when in fact you are having a sensor state-machine init problem.

?

---
bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

Yes, I implemented your suggested sequence in power_on():

Assert XSHUTDOWN (Reset GPIO = 1)
Enable Regulators
Enable Clock
Wait 2ms+
Release XSHUTDOWN (Reset GPIO = 0)

Even with this sequence, the brownout prevents detection if the
off-time was ~2.3s (I got this 2.3s number by conducting extensive
stress tests on the platform starting from 50ms to 3s. At 2.3s the
success rate was 100%. Anything below 2.3s, the sensor entered a
brownout state atleast once.)

Thanks & Regards,
Saikiran

On Mon, Jan 26, 2026 at 5:36 PM Bryan O'Donoghue <bod@kernel.org> wrote:
>
> On 26/01/2026 11:58, Saikiran B wrote:
> > "I don't think we've established the regulator is at fault. That's the
> > feedback I'm giving you here. ... vreg_cam_1p8: regulator-cam-1p8 {
> > compatible = "regulator-fixed";"
> >
> > Just to clarify on the regulators: on the Slim 7x, the camera supplies
> > (avdd, dvdd, dovdd) are all RPMh-controlled LDOs (pm8010 and pm8550),
> > not generic fixed regulators.
>
> Slim7x - not the Dell right ;)
>
> > As I've confirmed that the qcom-rpmh-regulator driver doesn't natively
> > support active discharge or parsing off-on-delay-us (generic
> > property), which explains why the physical discharge constraint wasn't
> > being respected.
>
> No, the RPMh firmware should know how to do that. Not the Linux side,
> this is the part of your brown-out story that doesn't make sense.
>
> BTW, did you try my given sequence - particularly the XSHUTDOWN in
> power_on(); ?
>
> If the XSHUTDOWN pin is for example floating or not in the correct
> logical state when you power-on, the chip may not initialise correctly.
>
> Which could lead you to conclude - you are having a regulator problem,
> when in fact you are having a sensor state-machine init problem.
>
> ?
>
> ---
> bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 12:24, Saikiran B wrote:
> Yes, I implemented your suggested sequence in power_on():
> 
> Assert XSHUTDOWN (Reset GPIO = 1)

+5 milliseconds

> Enable Regulators
> Enable Clock
> Wait 2ms+
> Release XSHUTDOWN (Reset GPIO = 0)
> 
> Even with this sequence, the brownout prevents detection if the
> off-time was ~2.3s (I got this 2.3s number by conducting extensive
> stress tests on the platform starting from 50ms to 3s. At 2.3s the
> success rate was 100%. Anything below 2.3s, the sensor entered a
> brownout state atleast once.)
> 
> Thanks & Regards,
> Saikiran

?

---
bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

I used a 2ms delay for the initial reset assertion.

Thanks & Regards,
Saikiran

On Mon, Jan 26, 2026 at 6:11 PM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 26/01/2026 12:24, Saikiran B wrote:
> > Yes, I implemented your suggested sequence in power_on():
> >
> > Assert XSHUTDOWN (Reset GPIO = 1)
>
> +5 milliseconds
>
> > Enable Regulators
> > Enable Clock
> > Wait 2ms+
> > Release XSHUTDOWN (Reset GPIO = 0)
> >
> > Even with this sequence, the brownout prevents detection if the
> > off-time was ~2.3s (I got this 2.3s number by conducting extensive
> > stress tests on the platform starting from 50ms to 3s. At 2.3s the
> > success rate was 100%. Anything below 2.3s, the sensor entered a
> > brownout state atleast once.)
> >
> > Thanks & Regards,
> > Saikiran
>
> ?
>
> ---
> bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Hans de Goede]

Hi,

On 26-Jan-26 13:48, Saikiran B wrote:
> I used a 2ms delay for the initial reset assertion.

I think that what Bryan means is you need a 5ms delay between
asserting reset/xshutdown and enabling the regulators to make
sure that the sensor sees the reset signal high before
the regulators are enabled.

Regards,

Hans




> On Mon, Jan 26, 2026 at 6:11 PM Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> On 26/01/2026 12:24, Saikiran B wrote:
>>> Yes, I implemented your suggested sequence in power_on():
>>>
>>> Assert XSHUTDOWN (Reset GPIO = 1)
>>
>> +5 milliseconds
>>
>>> Enable Regulators
>>> Enable Clock
>>> Wait 2ms+
>>> Release XSHUTDOWN (Reset GPIO = 0)
>>>
>>> Even with this sequence, the brownout prevents detection if the
>>> off-time was ~2.3s (I got this 2.3s number by conducting extensive
>>> stress tests on the platform starting from 50ms to 3s. At 2.3s the
>>> success rate was 100%. Anything below 2.3s, the sensor entered a
>>> brownout state atleast once.)
>>>
>>> Thanks & Regards,
>>> Saikiran
>>
>> ?
>>
>> ---
>> bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 14:08, Saikiran B wrote:
> The exact issue is:
> 1. Open Camera -> Close -> Wait 3s -> Open: WORKS.
> 2. Open Camera -> Close -> Wait 1.5s -> Open: FAILS (I2C Timeout / 
> Device Busy).
> 
> If the VDD rail is floating in the brownout region (~1.0V) during that 
> 1.5s window, does the sensor's internal Reset Logic Gate even have 
> enough bias voltage to function?

I think the VDD rail floating is unlikely, this would require the 
description of the LDO configured by XBL to be incorrect - possible but, 
then you'd expect to see an update for Windows to fix it.

Have you gotten the latest firmware for the board from Lenovo ? A 
misconfigured LDO - without active discharge set, should receive a 
firmware update to address.

Another possibility is CCI is powering the chip in sleep.

Lets have a look at the CCI pins.

         cam_rgb_default: cam-rgb-default-state {
                 mclk-pins {
                         pins = "gpio100";
                         function = "cam_aon";
                         drive-strength = <16>;
                         bias-disable;
                 };

                 reset-n-pins {
                         pins = "gpio237";
                         function = "gpio";
                         drive-strength = <2>;
                         bias-disable;
                 };
         };

add
	cam_rgb_sleep: cam-rgb-sleep-state {
                 mclk-pins {
                         pins = "gpio100";
                         function = "cam_aon";
                         drive-strength = <2>;
                         bias-pull-down; // Force to Ground
                 };

                 reset-n-pins {
                         pins = "gpio237";
                         function = "gpio";
                         drive-strength = <2>;
                         bias-pull-down; // Force to Ground
                 };
         };


&cci1_i2c1 {
         camera@36 {
                 compatible = "ovti,ov02c10";
                 reg = <0x36>;

                 reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
                 pinctrl-names = "default", "sleep";
                 pinctrl-0 = <&cam_rgb_default>;
                 pinctrl-1 = <&cam_rgb_sleep>;

Failing that we should try a more liberal power_on()

power_on():

     Assert Reset (GPIO Low).
     Wait 10ms.
     Enable all regulators (RPMh votes).
     Wait 20ms (Allow PM8010 to ramp and stabilize).
     Start the Clock (MCLK).
     Wait 10ms.
     De-assert Reset (GPIO High).
     Wait 5ms.

If that doesn't work, we will have to go and look at the LDO 
configuration via SPMI directly.

During the 2.3 second window can you run

Getting the kernel's view:
cat /sys/kernel/debug/regulator/regulator_summary

We are looking for use_count > 0 and open_count

We could also look at the SPMI LDO config register

Getting the firmware's view:
cat /sys/kernel/debug/regmap/spmi0-0x08/registers

It should be possible to interrogate the configruation of all of the 
relevant LDOs and ascertain if active-discharge is set, which TBH it 
should be.

> ​My testing suggests the sensor is physically incapable of processing 
> the Reset signal until the rail fully discharges (~2.3s), which is why 
> the 5ms delay has no effect.

Yes accepted but, a 2.3 second delay is avoidable if we root-cause.
P.S.
Please bottom post !

---
bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Saikiran B]

"Failing that we should try a more liberal power_on() Assert Reset ...
Wait 10ms ... Enable ... Wait 20ms ... Clock ..."

I have implemented a strict power sequencing in v3 as you and Hans requested:

- Assert Reset (5ms)
- Enable Regulators
- Enable Clock
- Wait 2ms
- De-assert Reset
- Wait 20ms (T2/Boot)

Regarding the root cause (LDO active discharge / pin states): I
suspect you are right that active_discharge should be enabled by
firmware but isn't, or the sleep state pinctrls are missing (causing
back-feeding). I will investigate the SPMI registers and sleep
pinctrls separately as a follow-up, as that affects the platform
stability beyond just this driver.

For this patch series (v3): I have implemented Runtime PM Autosuspend
(1000ms). This effectively masks the issue for the user (rapid
open/close works instantly because regulators stay on), while using
standard kernel infrastructure instead of custom workarounds.

This approach:
- Fixes the immediate "camera fails on reload" user bug.
- Uses the rigorous power sequence you defined.
- Aligns with other drivers (e.g. ov2680) using autosuspend for
performance/stability.

I'm sending the v3 series in a bit with all these changes. I'll
continue debugging the LDO configuration on the side.

Thanks for all the feedback. Much appreciated.

Thanks,
Saikiran

On Mon, Jan 26, 2026 at 8:34 PM Bryan O'Donoghue <bod@kernel.org> wrote:
>
> On 26/01/2026 14:08, Saikiran B wrote:
> > The exact issue is:
> > 1. Open Camera -> Close -> Wait 3s -> Open: WORKS.
> > 2. Open Camera -> Close -> Wait 1.5s -> Open: FAILS (I2C Timeout /
> > Device Busy).
> >
> > If the VDD rail is floating in the brownout region (~1.0V) during that
> > 1.5s window, does the sensor's internal Reset Logic Gate even have
> > enough bias voltage to function?
>
> I think the VDD rail floating is unlikely, this would require the
> description of the LDO configured by XBL to be incorrect - possible but,
> then you'd expect to see an update for Windows to fix it.
>
> Have you gotten the latest firmware for the board from Lenovo ? A
> misconfigured LDO - without active discharge set, should receive a
> firmware update to address.
>
> Another possibility is CCI is powering the chip in sleep.
>
> Lets have a look at the CCI pins.
>
>          cam_rgb_default: cam-rgb-default-state {
>                  mclk-pins {
>                          pins = "gpio100";
>                          function = "cam_aon";
>                          drive-strength = <16>;
>                          bias-disable;
>                  };
>
>                  reset-n-pins {
>                          pins = "gpio237";
>                          function = "gpio";
>                          drive-strength = <2>;
>                          bias-disable;
>                  };
>          };
>
> add
>         cam_rgb_sleep: cam-rgb-sleep-state {
>                  mclk-pins {
>                          pins = "gpio100";
>                          function = "cam_aon";
>                          drive-strength = <2>;
>                          bias-pull-down; // Force to Ground
>                  };
>
>                  reset-n-pins {
>                          pins = "gpio237";
>                          function = "gpio";
>                          drive-strength = <2>;
>                          bias-pull-down; // Force to Ground
>                  };
>          };
>
>
> &cci1_i2c1 {
>          camera@36 {
>                  compatible = "ovti,ov02c10";
>                  reg = <0x36>;
>
>                  reset-gpios = <&tlmm 237 GPIO_ACTIVE_LOW>;
>                  pinctrl-names = "default", "sleep";
>                  pinctrl-0 = <&cam_rgb_default>;
>                  pinctrl-1 = <&cam_rgb_sleep>;
>
> Failing that we should try a more liberal power_on()
>
> power_on():
>
>      Assert Reset (GPIO Low).
>      Wait 10ms.
>      Enable all regulators (RPMh votes).
>      Wait 20ms (Allow PM8010 to ramp and stabilize).
>      Start the Clock (MCLK).
>      Wait 10ms.
>      De-assert Reset (GPIO High).
>      Wait 5ms.
>
> If that doesn't work, we will have to go and look at the LDO
> configuration via SPMI directly.
>
> During the 2.3 second window can you run
>
> Getting the kernel's view:
> cat /sys/kernel/debug/regulator/regulator_summary
>
> We are looking for use_count > 0 and open_count
>
> We could also look at the SPMI LDO config register
>
> Getting the firmware's view:
> cat /sys/kernel/debug/regmap/spmi0-0x08/registers
>
> It should be possible to interrogate the configruation of all of the
> relevant LDOs and ascertain if active-discharge is set, which TBH it
> should be.
>
> > My testing suggests the sensor is physically incapable of processing
> > the Reset signal until the rail fully discharges (~2.3s), which is why
> > the 5ms delay has no effect.
>
> Yes accepted but, a 2.3 second delay is avoidable if we root-cause.
> P.S.
> Please bottom post !
>
> ---
> bod

Re: [PATCH v2 1/1] media: i2c: ov02c10: Keep power on and use reset for power management

[Bryan O'Donoghue]

On 26/01/2026 15:40, Saikiran B wrote:
> "Failing that we should try a more liberal power_on() Assert Reset ...
> Wait 10ms ... Enable ... Wait 20ms ... Clock ..."
> 
> I have implemented a strict power sequencing in v3 as you and Hans requested:
> 
> - Assert Reset (5ms)
> - Enable Regulators
> - Enable Clock
> - Wait 2ms
> - De-assert Reset
> - Wait 20ms (T2/Boot)

Yes understood, thank you. The ask is to extend the grace times a bit so 
we can be very sure.

power_on():

     Assert Reset (GPIO Low).
     Wait 10ms.
     Enable all regulators (RPMh votes).
     Wait 20ms (Allow PM8010 to ramp and stabilize).
     Start the Clock (MCLK).
     Wait 10ms.
     De-assert Reset (GPIO High).
     Wait 5ms.


> Regarding the root cause (LDO active discharge / pin states): I
> suspect you are right that active_discharge should be enabled by
> firmware but isn't, or the sleep state pinctrls are missing (causing
> back-feeding).

I would be surprised to find that active discharge hasn't been set on 
the relevant LDOs. It is possible but I would also expect then that 
Lenovo followed up because, this would an issue that would also affect 
Windows users then i.e. its up to the firmware to configure the LDOs.

  I will investigate the SPMI registers and sleep
> pinctrls separately as a follow-up, as that affects the platform
> stability beyond just this driver.

Yeah if its not chip power-on sequencing then we are probably feeding 
voltage from elsewhere unbeknownst.

> For this patch series (v3): I have implemented Runtime PM Autosuspend
> (1000ms). This effectively masks the issue for the user (rapid
> open/close works instantly because regulators stay on), while using
> standard kernel infrastructure instead of custom workarounds.
> 
> This approach:
> - Fixes the immediate "camera fails on reload" user bug.
> - Uses the rigorous power sequence you defined.
> - Aligns with other drivers (e.g. ov2680) using autosuspend for
> performance/stability.
> 
> I'm sending the v3 series in a bit with all these changes. I'll
> continue debugging the LDO configuration on the side.

Send away however, instead of working around an issue we don't 
understand, it would be preferable to root-cause it.

Please read:

Documentation/process/submitting-patches.rst

Documentation/process/submitting-patches.rst:Top-posting is strongly 
discouraged in Linux kernel development
Documentation/process/submitting-patches.rst:  A: 
http://en.wikipedia.org/wiki/Top_post
Documentation/process/submitting-patches.rst:  Q: Where do I find info 
about this thing called top-posting?
Documentation/process/submitting-patches.rst:  Q: Why is top-posting 
such a bad thing?
Documentation/process/submitting-patches.rst:  A: Top-posting.

---
bod