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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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