Re: [PATCH] media: ov2740: Ensure proper reset sequence on probe()

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

Hi Hans

On Thu, May 09, 2024 at 03:42:25PM +0200, Hans de Goede wrote:
> Hi Stanislaw,
> 
> On 5/9/24 1:42 PM, Stanislaw Gruszka wrote:
> > On Mon, May 06, 2024 at 03:24:38PM +0200, Hans de Goede wrote:
> >> Before this commit on probe() the driver would do:
> >>
> >> reset=1                // from probe() calling gpiod_get(GPIOD_OUT_HIGH)
> >> reset=0                // from resume()
> >> msleep(20)             // from resume()
> >>
> >> So if reset was 0 before getting the GPIO the reset line would only be
> >> driven high for a very short time and sometimes there would be errors
> >> reading the id register afterwards.
> >>
> >> Add a msleep(20) after getting the reset line to ensure the sensor is
> >> properly reset:
> >>
> >> reset=1                // from probe() calling gpiod_get(GPIOD_OUT_HIGH)
> >> msleep(20)             // from probe()
> >> reset=0                // from resume()
> >> msleep(20)             // from resume()
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > 
> > This fixes this issue:
> > 
> > [    7.742633] ov2740 i2c-INT3474:01: chip id mismatch: 2740 != 0
> > [    7.742638] ov2740 i2c-INT3474:01: error -ENXIO: failed to find sensor
> > 
> > for me as well.
> 
> Thank you for testing.
> 
> However there is still the issue of the sensor not always starting to
> stream reliably being discussed here:
> 
> https://github.com/intel/ipu6-drivers/issues/187
> 
> I have been thinking about this and I think that something like this
> should fix this, can you give this a try (with the patch to disable
> runtime-pm reverted) :
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index c48dbcde9877..58818bcfe086 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -1294,7 +1294,14 @@ static int ov2740_suspend(struct device *dev)
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
> +	/*
> +	 * Ensure reset is asserted for at least 20 ms before disabling the clk.
> +	 * This also assures reset is properly asserted for 20 ms when a runtime
> +	 * suspend is immediately followed by a runtime resume.
> +	 */
> +	msleep(20);
>  	clk_disable_unprepare(ov2740->clk);
> +
>  	return 0;
>  }
>  
> @@ -1308,6 +1315,9 @@ static int ov2740_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Give clk time to stabilize */
> +	msleep(20);
> +
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>  	msleep(20);
>  
> 
> Hopefully this will fix the start / stop stream issues when runtime
> pm is enabled.

With the both patches I have quite good reproducible "chip id mismatch"
problem. I test patch 1 and patch 2 separately and both of them together.
Unfortunately non combination fixes the issues: mismatch and steam
start and some other problems with ov2740 on my lenovo x1 laptop

Looks previously I get good result with original patch just by sheer luck.
Those are random problems and reuire more testing before confirmation
of fix.

So sadly I have to scratch result that original patch fixes chip mishmash id
issue :-(

Problems need to be properly debugged and require deeper investigation.

Regards
Stanislaw