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

[Hans de Goede <hansg@kernel.org>]

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