On 11/07/2024 12:17, Dave Stevenson wrote:
Hi Quentin and Bryan
On Thu, 11 Jul 2024 at 11:40, Quentin Schulz <quentin.schulz@xxxxxxxxx> wrote:
Hi Bryan,
On 7/11/24 12:20 PM, Bryan O'Donoghue wrote:
The ov5675 specification says that the gap between XSHUTDN deassert and the
first I2C transaction should be a minimum of 8192 XVCLK cycles.
Right now we use a usleep_rage() that gives a sleep time of between about
430 and 860 microseconds.
On the Lenovo X13s we have observed that in about 1/20 cases the current
timing is too tight and we start transacting before the ov5675's reset
cycle completes, leading to I2C bus transaction failures.
The reset racing is sometimes triggered at initial chip probe but, more
usually on a subsequent power-off/power-on cycle e.g.
[ 71.451662] ov5675 24-0010: failed to write reg 0x0103. error = -5
[ 71.451686] ov5675 24-0010: failed to set plls
The current quiescence period we have is too tight, doubling the minimum
appears to fix the issue observed on X13s.
Fixes: 49d9ad719e89 ("media: ov5675: add device-tree support and support runtime PM")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
---
drivers/media/i2c/ov5675.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
index 92bd35133a5d..0498f8f3064d 100644
--- a/drivers/media/i2c/ov5675.c
+++ b/drivers/media/i2c/ov5675.c
@@ -1018,8 +1018,13 @@ static int ov5675_power_on(struct device *dev)
gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
- /* 8192 xvclk cycles prior to the first SCCB transation */
- usleep_range(delay_us, delay_us * 2);
+ /* The spec calls for a minimum delay of 8192 XVCLK cycles prior to
+ * transacting on the I2C bus, which translates to about 430
+ * microseconds at 19.2 MHz.
+ * Testing shows the range 8192 - 16384 cycles to be unreliable.
+ * Grant a more liberal 2x -3x clock cycle grace time.
+ */
+ usleep_range(delay_us * 2, delay_us * 3);
Would it make sense to have power_off have the same logic? We do a
usleep_range of those same values currently, so keeping them in sync
seems to make sense to me.
Also, I'm wondering if it isn't an issue with the gpio not being high
right after gpoiod_set_value_cansleep() returns, i.e. the time it
actually takes for the HW to reach the IO level that means "high" for
the camera. And that this increased sleep is just a way to mitigate that?
With this patch we essentially postpone the power_on by another 430ms
making it almost a full second before we can start using the camera.
That's quite a lot I think? We don't have a usecase right now that
requires this to be blazing fast (and we anyway would need at the very
least 430ms), so take this remark as what it is, a remark.
I think you've misread 430 usec as 430 msec.
I was looking at the series and trying to decide whether it's worth
going to the effort of computing the time at all when even on the
slowest 6MHz XVCLK we're sub 1.5ms for the required delay.
At the max XVLCK of 24MHz you could save 1ms. I know of very few use
cases that would suffer for a 1ms delay.
I know we all like to be precise, but it sounds like the precision
actually causes grief in this situation.
Yeah the first draft of the patch just had a post-reset delay of I
forget - I think I just used usleep_range(2000, 2200); again but I kind
respected the attempt to hit the specification and wanted to fix the
original logic, which is close but no cigar ATM.
---
bod
