| Message ID | 20250315134009.157132-4-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ov02c10: OF extensions and fixups | expand |
On 15/03/2025 13:40, Bryan O'Donoghue wrote: > The power-on timing tables shows that t5 the time between XSHUTDOWN > deassert to system ready is a minimum 5 millseconds. The power-on timing table shows that .... minimum 5 milliseconds. -- bod
Hi Bryan, On 15-Mar-25 2:40 PM, Bryan O'Donoghue wrote: > Implement recommended power-on reset. > > ov02c10 documentation states that the hardware reset is active low and that > the reset pulse should be greater than 2 milliseconds. > > The power-on timing tables shows that t5 the time between XSHUTDOWN > deassert to system ready is a minimum 5 millseconds. > > Implement the recommended power-on reset minimums. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/media/i2c/ov02c10.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c > index 595998e60b22..d3dc614a3c01 100644 > --- a/drivers/media/i2c/ov02c10.c > +++ b/drivers/media/i2c/ov02c10.c > @@ -696,8 +696,10 @@ static int ov02c10_power_on(struct device *dev) > } > > if (ov02c10->reset) { > + gpiod_set_value_cansleep(ov02c10->reset, 1); We ask for the gpio to be high when requesting it and we also make it high in ov02c10_power_off() so it should always be high on entry of this function. > + usleep_range(2000, 2200); To make sure the GPIO is high for a long enough time after requesting it I've added the following to ov02c10_get_pm_resources(): ov02c10->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(ov02c10->reset)) return dev_err_probe(dev, PTR_ERR(ov02c10->reset), "failed to get reset gpio\n"); if (ov02c10->reset) fsleep(1000); Admittedly I got the amount of time sleeping here wrong. (Sakari, this also answers/explains one of your review questions). But my code above only takes care of driving reset high long enough between requesting the GPIO and power_on(). I guess we could get a power_on() power_off() in quick succession which might violate the 2ms rule. So I think for v10 I'll go with the above solution to sleep 2 ms in power_on() before de-asserting reset. Bryan, any comments on that ? > gpiod_set_value_cansleep(ov02c10->reset, 0); > - usleep_range(1500, 1800); > + usleep_range(5000, 5100); Ack for this change Sakari also pointed out I guessed the reset time too low. IIRC I took this from the ov08x40 driver, but I guess not all ov0xxxxx sensors have the same reset time. > } > > return 0; Regards, Hans p.s. I've gotten a report from Heimir that the new version does not work for him. He's hitting a problem after applying: "[PATCH v8 11/14] media: ov02c10: Switch to {enable,disable}_streams" https://lore.kernel.org/linux-media/20250313184314.91410-12-hdegoede@redhat.com/ so Bryan this may also not work for some of your testers.
On 15/03/2025 21:05, Hans de Goede wrote: > So I think for v10 I'll go with the above solution to sleep > 2 ms in power_on() before de-asserting reset. > > Bryan, any comments on that ? Don't we go through power_on/power_off cycles if/when we add runtime PM though ? So to me it makes sense to do it the same way we did it in ov08x40 for example. We might as well add the sequence now would be my feeling. --- bod
Hi, On 15-Mar-25 10:09 PM, Bryan O'Donoghue wrote: > On 15/03/2025 21:05, Hans de Goede wrote: >> So I think for v10 I'll go with the above solution to sleep >> 2 ms in power_on() before de-asserting reset. >> >> Bryan, any comments on that ? > > Don't we go through power_on/power_off cycles if/when we add runtime PM though ? Right, notice that I said "to sleep 2 ms in power_on() before de-asserting reset" IOW I'm suggesting to go with your solution. > So to me it makes sense to do it the same way we did it in ov08x40 for example. Ack, I agreee :) Except for explictly driving the GPIO high before the 2ms sleep, it should always be high when we enter the power_on() helper since power_on() / off() calls are guaranteed to be paired (and we request it to be high initially). Regards, Hans
On 15/03/2025 21:12, Hans de Goede wrote: > Except for explictly driving the GPIO high before > the 2ms sleep, it should always be high when we enter > the power_on() helper since power_on() / off() calls > are guaranteed to be paired (and we request it to be > high initially). Yeah I agree it should work. The important part is t5 => making sure we give enough time for the chip to "boot up" basically. --- bod
diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c index 595998e60b22..d3dc614a3c01 100644 --- a/drivers/media/i2c/ov02c10.c +++ b/drivers/media/i2c/ov02c10.c @@ -696,8 +696,10 @@ static int ov02c10_power_on(struct device *dev) } if (ov02c10->reset) { + gpiod_set_value_cansleep(ov02c10->reset, 1); + usleep_range(2000, 2200); gpiod_set_value_cansleep(ov02c10->reset, 0); - usleep_range(1500, 1800); + usleep_range(5000, 5100); } return 0;
Implement recommended power-on reset. ov02c10 documentation states that the hardware reset is active low and that the reset pulse should be greater than 2 milliseconds. The power-on timing tables shows that t5 the time between XSHUTDOWN deassert to system ready is a minimum 5 millseconds. Implement the recommended power-on reset minimums. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/media/i2c/ov02c10.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)