Hi Marek
On Tue, Jul 25, 2023 at 12:22:10AM +0200, Marek Vasut wrote:
> Set OV5640_REG_IO_MIPI_CTRL00 bit 2 to 1 instead of 0, since 1 means
> MIPI CSI2 interface, while 0 means CPI parallel interface.
>
> In the ov5640_set_power_mipi() the interface should obviously be set
> to MIPI CSI2 since this functions is used to power up the sensor when
> operated in MIPI CSI2 mode. The sensor should not be in CPI mode in
> that case.
Does this actually help fixing your 'first frame corrupted issue' ?
I think the logic here was to power up the interface here in
ov5640_set_power_mipi() with the CSI-2 interface disabled to enter
LP-11 mode (something some receivers like the imx6 one are
particularly sensible to).
Then at stream time the CSI-2 interface is actually enabled in
ov5640_set_stream_mipi() just before streaming is started.
Also the register documentation is very confusing and as reported in
ov5640_set_stream_mipi() it is also probably wrong (at least in the
datasheet version I have).
I would be particularly cautious in touching this sequence as it has
been validated to work with multiple receivers. Of course if it
actually fixes an issue for you it should be taken in, but ideally, as
this sensor is still used in a large number of evaluation boards, it
should be validated by other consumers of this driver (st, imx,
microchip and rensas to name a few).
Thanks
j
>
> Fixes: aa4bb8b8838f ("media: ov5640: Re-work MIPI startup sequence")
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Cc: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Cc: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx
> ---
> drivers/media/i2c/ov5640.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 8f21839b08c78..8b7ff2f3bdda7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2539,9 +2539,9 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> * "ov5640_set_stream_mipi()")
> * [4] = 0 : Power up MIPI HS Tx
> * [3] = 0 : Power up MIPI LS Rx
> - * [2] = 0 : MIPI interface disabled
> + * [2] = 1 : MIPI interface enabled
> */
> - ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> + ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x44);
> if (ret)
> return ret;
>
> --
> 2.40.1
>
