diff mbox series

[3/4] media: i2c: ov02c10: Implement power-on reset

Message ID 20250315134009.157132-4-bryan.odonoghue@linaro.org (mailing list archive)
State New
Headers show
Series ov02c10: OF extensions and fixups | expand

Commit Message

Bryan O'Donoghue March 15, 2025, 1:40 p.m. UTC
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(-)

Comments

Bryan O'Donoghue March 15, 2025, 1:50 p.m. UTC | #1
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
Hans de Goede March 15, 2025, 9:05 p.m. UTC | #2
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.
Bryan O'Donoghue March 15, 2025, 9:09 p.m. UTC | #3
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
Hans de Goede March 15, 2025, 9:12 p.m. UTC | #4
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
Bryan O'Donoghue March 15, 2025, 9:15 p.m. UTC | #5
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 mbox series

Patch

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;