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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/
so Bryan this may also not work for some of your testers.
