[PATCH v4 0/6] media: i2c: ov5640 feature enhancement and fixes

[PATCH v4 0/6] media: i2c: ov5640 feature enhancement and fixes

[Lad Prabhakar]

Hi All,

This patch series fixes DVP support and enables BT656 mode in
the driver.

Cheers,
Prabhakar

Changes for v4:
* Split patch 1/3 from v3 patch series [1] more granular as
  requested by Sakari
* Included [Tested/Reviewed]-by tag from Jacopo

[1] https://patchwork.kernel.org/cover/11712735/

Changes for v3 (https://patchwork.kernel.org/cover/11712735/):
* Dropped DT binding patch
* Fail probe if unsupported bus_type is passed
* Fixed review comments pointed by Jacopo

Changes for v2 (https://lkml.org/lkml/2020/8/3/830):
* Added support to fallback in parallel mode
* Documented bus-type property
* Added descriptive commit message for patch 2/4 as pointed
  by Sakari
* Fixed review comments pointed by Laurent to have separate functions
  for mipi and dvp setup
* Made sure the sensor is in power down mode during startup too for
  DVP mode

Lad Prabhakar (6):
  media: i2c: ov5640: Remain in power down for DVP mode unless streaming
  media: i2c: ov5640: Separate out mipi configuration from s_power
  media: i2c: ov5640: Enable data pins on poweron for DVP mode
  media: i2c: ov5640: Configure HVP lines in s_power callback
  media: i2c: ov5640: Add support for BT656 mode
  media: i2c: ov5640: Fail probe on unsupported bus_type

 drivers/media/i2c/ov5640.c | 336 +++++++++++++++++++++----------------
 1 file changed, 191 insertions(+), 145 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Lad Prabhakar]

Keep the sensor in software power down mode and wake up only in
ov5640_set_stream_dvp() callback.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7ac0592..880fde73a030 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,6 +34,8 @@
 #define OV5640_REG_SYS_RESET02		0x3002
 #define OV5640_REG_SYS_CLOCK_ENABLE02	0x3006
 #define OV5640_REG_SYS_CTRL0		0x3008
+#define OV5640_REG_SYS_CTRL0_SW_PWDN	0x42
+#define OV5640_REG_SYS_CTRL0_SW_PWUP	0x02
 #define OV5640_REG_CHIP_ID		0x300a
 #define OV5640_REG_IO_MIPI_CTRL00	0x300e
 #define OV5640_REG_PAD_OUTPUT_ENABLE01	0x3017
@@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 		val = regs->val;
 		mask = regs->mask;
 
+		/* remain in power down mode for DVP */
+		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
+		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
+		    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
+			continue;
+
 		if (mask)
 			ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
 		else
@@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	 * PAD OUTPUT ENABLE 02
 	 * - [7:2]:	D[5:0] output enable
 	 */
-	return ov5640_write_reg(sensor,
-				OV5640_REG_PAD_OUTPUT_ENABLE02,
-				on ? 0xfc : 0);
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
+			       on ? 0xfc : 0);
+	if (ret)
+		return ret;
+
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+				OV5640_REG_SYS_CTRL0_SW_PWUP :
+				OV5640_REG_SYS_CTRL0_SW_PWDN);
 }
 
 static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
-- 
2.17.1

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Hugues FRUCHET]

Hi Prabhakar,

As discussed separately I would prefer to better understand issue before 
going to this patch.
Nevertheless I have some remarks in code in case we'll need it at the end.

On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> Keep the sensor in software power down mode and wake up only in
> ov5640_set_stream_dvp() callback.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7ac0592..880fde73a030 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -34,6 +34,8 @@
>   #define OV5640_REG_SYS_RESET02		0x3002
>   #define OV5640_REG_SYS_CLOCK_ENABLE02	0x3006
>   #define OV5640_REG_SYS_CTRL0		0x3008
> +#define OV5640_REG_SYS_CTRL0_SW_PWDN	0x42
> +#define OV5640_REG_SYS_CTRL0_SW_PWUP	0x02

For the time being this section was only referring to registers 
addresses and bit details was explained into a comment right before 
affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.

>   #define OV5640_REG_CHIP_ID		0x300a
>   #define OV5640_REG_IO_MIPI_CTRL00	0x300e
>   #define OV5640_REG_PAD_OUTPUT_ENABLE01	0x3017
> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>   		val = regs->val;
>   		mask = regs->mask;
>   
> +		/* remain in power down mode for DVP */
> +		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> +		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> +		    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> +			continue;
> +

I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008) 
has been partially removed from big init sequence: for power-up part, 
but power-dwn remains at very beginning of sequence.
We should completely remove 0x3008 from init sequence, including 
power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that 
should be called at the right place instead.


>   		if (mask)
>   			ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>   		else
> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>   	 * PAD OUTPUT ENABLE 02
>   	 * - [7:2]:	D[5:0] output enable
>   	 */
> -	return ov5640_write_reg(sensor,
> -				OV5640_REG_PAD_OUTPUT_ENABLE02,
> -				on ? 0xfc : 0);
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> +			       on ? 0xfc : 0);
> +	if (ret)
> +		return ret;
> +
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> +				OV5640_REG_SYS_CTRL0_SW_PWUP :
> +				OV5640_REG_SYS_CTRL0_SW_PWDN);
>   }
>   
>   static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> 


BR,
Hugues.

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Eugen.Hristev]

On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> Hi Prabhakar,
> 
> As discussed separately I would prefer to better understand issue before
> going to this patch.
> Nevertheless I have some remarks in code in case we'll need it at the end.
> 
> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>> Keep the sensor in software power down mode and wake up only in
>> ov5640_set_stream_dvp() callback.
>>
>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>    drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 2fe4a7ac0592..880fde73a030 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -34,6 +34,8 @@
>>    #define OV5640_REG_SYS_RESET02              0x3002
>>    #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
>>    #define OV5640_REG_SYS_CTRL0                0x3008
>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> 
> For the time being this section was only referring to registers
> addresses and bit details was explained into a comment right before
> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> 
>>    #define OV5640_REG_CHIP_ID          0x300a
>>    #define OV5640_REG_IO_MIPI_CTRL00   0x300e
>>    #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>                val = regs->val;
>>                mask = regs->mask;
>>
>> +             /* remain in power down mode for DVP */
>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>> +                     continue;
>> +
> 
> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> has been partially removed from big init sequence: for power-up part,
> but power-dwn remains at very beginning of sequence.
> We should completely remove 0x3008 from init sequence, including
> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> should be called at the right place instead.
> 
> 
>>                if (mask)
>>                        ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>                else
>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>         * PAD OUTPUT ENABLE 02
>>         * - [7:2]:     D[5:0] output enable
>>         */
>> -     return ov5640_write_reg(sensor,
>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
>> -                             on ? 0xfc : 0);
>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>> +                            on ? 0xfc : 0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
>>    }
>>
>>    static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>
> 
> 
> BR,
> Hugues.
> 


Hello everyone,

When I updated driver in my tree with this patch, I noticed that my 
setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped 
working.

It looks like the culprit is this patch.
I am not sure which is the best way to fix it.
It looks like initially things work fine when probing the driver, but 
when we want to start streaming at a later point, the register SYS_CTRL0 
cannot be written anymore.
Here is an output of the log:

  --- Opening /dev/video0...
     Trying source module v4l2...
     /dev/video0 opened.
     No input was specified, using the first.
     ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
     atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
     Error starting stream.
     VIDIOC_STREAMON: Remote I/O error
     Unable to use mmap. Using read instead.
     Unable to use read.

I am using a simple application to read from /dev/video0 (which is 
registered by the atmel-isc once the sensor probed)

I have a feeling that something is wrong related to power, but I cannot 
tell for sure.

Reverting this patch makes it work fine again.

Help is appreciated,
Thanks,
Eugen

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Eugen.Hristev]

On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>> Hi Prabhakar,
>>
>> As discussed separately I would prefer to better understand issue before
>> going to this patch.
>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>
>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>> Keep the sensor in software power down mode and wake up only in
>>> ov5640_set_stream_dvp() callback.
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>     drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>>     1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>> index 2fe4a7ac0592..880fde73a030 100644
>>> --- a/drivers/media/i2c/ov5640.c
>>> +++ b/drivers/media/i2c/ov5640.c
>>> @@ -34,6 +34,8 @@
>>>     #define OV5640_REG_SYS_RESET02              0x3002
>>>     #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
>>>     #define OV5640_REG_SYS_CTRL0                0x3008
>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>
>> For the time being this section was only referring to registers
>> addresses and bit details was explained into a comment right before
>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>
>>>     #define OV5640_REG_CHIP_ID          0x300a
>>>     #define OV5640_REG_IO_MIPI_CTRL00   0x300e
>>>     #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>>                 val = regs->val;
>>>                 mask = regs->mask;
>>>
>>> +             /* remain in power down mode for DVP */
>>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>> +                     continue;
>>> +
>>
>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>> has been partially removed from big init sequence: for power-up part,
>> but power-dwn remains at very beginning of sequence.
>> We should completely remove 0x3008 from init sequence, including
>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>> should be called at the right place instead.
>>
>>
>>>                 if (mask)
>>>                         ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>>                 else
>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>          * PAD OUTPUT ENABLE 02
>>>          * - [7:2]:     D[5:0] output enable
>>>          */
>>> -     return ov5640_write_reg(sensor,
>>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
>>> -                             on ? 0xfc : 0);
>>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>> +                            on ? 0xfc : 0);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
>>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>     }
>>>
>>>     static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>
>>
>>
>> BR,
>> Hugues.
>>
> 
> 
> Hello everyone,
> 
> When I updated driver in my tree with this patch, I noticed that my
> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> working.
> 
> It looks like the culprit is this patch.
> I am not sure which is the best way to fix it.
> It looks like initially things work fine when probing the driver, but
> when we want to start streaming at a later point, the register SYS_CTRL0
> cannot be written anymore.
> Here is an output of the log:
> 
>    --- Opening /dev/video0...
>       Trying source module v4l2...
>       /dev/video0 opened.
>       No input was specified, using the first.
>       ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
>       atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
>       Error starting stream.
>       VIDIOC_STREAMON: Remote I/O error
>       Unable to use mmap. Using read instead.
>       Unable to use read.
> 
> I am using a simple application to read from /dev/video0 (which is
> registered by the atmel-isc once the sensor probed)
> 
> I have a feeling that something is wrong related to power, but I cannot
> tell for sure.
> 
> Reverting this patch makes it work fine again.
> 
> Help is appreciated,
> Thanks,
> Eugen
> 


Gentle ping.

I would like to send a revert patch if nobody cares about this 
driver/patch except me.

Thanks,
Eugen

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Sakari Ailus]

Hi Hugues, Eugen,

On Tue, Dec 21, 2021 at 07:37:47AM +0000, Eugen.Hristev@microchip.com wrote:
> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> > On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >> Hi Prabhakar,
> >>
> >> As discussed separately I would prefer to better understand issue before
> >> going to this patch.
> >> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>
> >> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>> Keep the sensor in software power down mode and wake up only in
> >>> ov5640_set_stream_dvp() callback.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>     drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>     1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>> index 2fe4a7ac0592..880fde73a030 100644
> >>> --- a/drivers/media/i2c/ov5640.c
> >>> +++ b/drivers/media/i2c/ov5640.c
> >>> @@ -34,6 +34,8 @@
> >>>     #define OV5640_REG_SYS_RESET02              0x3002
> >>>     #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
> >>>     #define OV5640_REG_SYS_CTRL0                0x3008
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>
> >> For the time being this section was only referring to registers
> >> addresses and bit details was explained into a comment right before
> >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>
> >>>     #define OV5640_REG_CHIP_ID          0x300a
> >>>     #define OV5640_REG_IO_MIPI_CTRL00   0x300e
> >>>     #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
> >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>                 val = regs->val;
> >>>                 mask = regs->mask;
> >>>
> >>> +             /* remain in power down mode for DVP */
> >>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>> +                     continue;
> >>> +
> >>
> >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >> has been partially removed from big init sequence: for power-up part,
> >> but power-dwn remains at very beginning of sequence.
> >> We should completely remove 0x3008 from init sequence, including
> >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >> should be called at the right place instead.
> >>
> >>
> >>>                 if (mask)
> >>>                         ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>                 else
> >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>          * PAD OUTPUT ENABLE 02
> >>>          * - [7:2]:     D[5:0] output enable
> >>>          */
> >>> -     return ov5640_write_reg(sensor,
> >>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> -                             on ? 0xfc : 0);
> >>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> +                            on ? 0xfc : 0);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>     }
> >>>
> >>>     static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>
> >>
> >>
> >> BR,
> >> Hugues.
> >>
> > 
> > 
> > Hello everyone,
> > 
> > When I updated driver in my tree with this patch, I noticed that my
> > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> > working.
> > 
> > It looks like the culprit is this patch.
> > I am not sure which is the best way to fix it.
> > It looks like initially things work fine when probing the driver, but
> > when we want to start streaming at a later point, the register SYS_CTRL0
> > cannot be written anymore.
> > Here is an output of the log:
> > 
> >    --- Opening /dev/video0...
> >       Trying source module v4l2...
> >       /dev/video0 opened.
> >       No input was specified, using the first.
> >       ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >       atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >       Error starting stream.
> >       VIDIOC_STREAMON: Remote I/O error
> >       Unable to use mmap. Using read instead.
> >       Unable to use read.
> > 
> > I am using a simple application to read from /dev/video0 (which is
> > registered by the atmel-isc once the sensor probed)
> > 
> > I have a feeling that something is wrong related to power, but I cannot
> > tell for sure.
> > 
> > Reverting this patch makes it work fine again.
> > 
> > Help is appreciated,
> > Thanks,
> > Eugen
> > 
> 
> 
> Gentle ping.
> 
> I would like to send a revert patch if nobody cares about this 
> driver/patch except me.

Hugues: any thoughts on this?

Without knowing much about the sensor, the stated intent of the patch seems
reasonable. But if it breaks things, then reverting it sounds like a good
idea until a proper fix is available.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Lad, Prabhakar]

Hi,

Sorry for the delay.

On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote:
>
> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> > On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >> Hi Prabhakar,
> >>
> >> As discussed separately I would prefer to better understand issue before
> >> going to this patch.
> >> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>
> >> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>> Keep the sensor in software power down mode and wake up only in
> >>> ov5640_set_stream_dvp() callback.
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>     drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>     1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>> index 2fe4a7ac0592..880fde73a030 100644
> >>> --- a/drivers/media/i2c/ov5640.c
> >>> +++ b/drivers/media/i2c/ov5640.c
> >>> @@ -34,6 +34,8 @@
> >>>     #define OV5640_REG_SYS_RESET02              0x3002
> >>>     #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
> >>>     #define OV5640_REG_SYS_CTRL0                0x3008
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>
> >> For the time being this section was only referring to registers
> >> addresses and bit details was explained into a comment right before
> >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>
> >>>     #define OV5640_REG_CHIP_ID          0x300a
> >>>     #define OV5640_REG_IO_MIPI_CTRL00   0x300e
> >>>     #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
> >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>                 val = regs->val;
> >>>                 mask = regs->mask;
> >>>
> >>> +             /* remain in power down mode for DVP */
> >>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>> +                     continue;
> >>> +
> >>
> >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >> has been partially removed from big init sequence: for power-up part,
> >> but power-dwn remains at very beginning of sequence.
> >> We should completely remove 0x3008 from init sequence, including
> >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >> should be called at the right place instead.
> >>
> >>
> >>>                 if (mask)
> >>>                         ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>                 else
> >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>          * PAD OUTPUT ENABLE 02
> >>>          * - [7:2]:     D[5:0] output enable
> >>>          */
> >>> -     return ov5640_write_reg(sensor,
> >>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> -                             on ? 0xfc : 0);
> >>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> +                            on ? 0xfc : 0);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>     }
> >>>
> >>>     static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>
> >>
> >>
> >> BR,
> >> Hugues.
> >>
> >
> >
> > Hello everyone,
> >
> > When I updated driver in my tree with this patch, I noticed that my
> > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> > working.
> >
> > It looks like the culprit is this patch.
> > I am not sure which is the best way to fix it.
> > It looks like initially things work fine when probing the driver, but
> > when we want to start streaming at a later point, the register SYS_CTRL0
> > cannot be written anymore.
> > Here is an output of the log:
> >
> >    --- Opening /dev/video0...
> >       Trying source module v4l2...
> >       /dev/video0 opened.
> >       No input was specified, using the first.
> >       ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >       atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >       Error starting stream.
> >       VIDIOC_STREAMON: Remote I/O error
> >       Unable to use mmap. Using read instead.
> >       Unable to use read.
> >
> > I am using a simple application to read from /dev/video0 (which is
> > registered by the atmel-isc once the sensor probed)
> >
> > I have a feeling that something is wrong related to power, but I cannot
> > tell for sure.
> >
> > Reverting this patch makes it work fine again.
> >
> > Help is appreciated,
> > Thanks,
> > Eugen
> >
>
>
> Gentle ping.
>
> I would like to send a revert patch if nobody cares about this
> driver/patch except me.
>
I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
was able to capture the video data [0] using the yavta application.
I'm fine with reverting the patch too as this works fine as well.

Just some suggestions:
- What is the application you are trying to use for capturing?
- Have you tried reducing the i2c clock speed?
- Can you try and do a dummy write for some other register in
ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
- Can you add ov5640_read_reg() in ov5640_write_reg() when
i2c_transfer() fails to check if read too is failing.

[0] https://paste.debian.net/1224317/

Cheers,
Prabhakar

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Eugen.Hristev]

On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> Hi,
> 
> Sorry for the delay.
> 
> On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote:
>>
>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>>>> Hi Prabhakar,
>>>>
>>>> As discussed separately I would prefer to better understand issue before
>>>> going to this patch.
>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>>>
>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>>>> Keep the sensor in software power down mode and wake up only in
>>>>> ov5640_set_stream_dvp() callback.
>>>>>
>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>      drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>>>>      1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>> index 2fe4a7ac0592..880fde73a030 100644
>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>> @@ -34,6 +34,8 @@
>>>>>      #define OV5640_REG_SYS_RESET02              0x3002
>>>>>      #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
>>>>>      #define OV5640_REG_SYS_CTRL0                0x3008
>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>>>
>>>> For the time being this section was only referring to registers
>>>> addresses and bit details was explained into a comment right before
>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>>>
>>>>>      #define OV5640_REG_CHIP_ID          0x300a
>>>>>      #define OV5640_REG_IO_MIPI_CTRL00   0x300e
>>>>>      #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>>>>                  val = regs->val;
>>>>>                  mask = regs->mask;
>>>>>
>>>>> +             /* remain in power down mode for DVP */
>>>>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>>>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>>>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>>>> +                     continue;
>>>>> +
>>>>
>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>>>> has been partially removed from big init sequence: for power-up part,
>>>> but power-dwn remains at very beginning of sequence.
>>>> We should completely remove 0x3008 from init sequence, including
>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>>>> should be called at the right place instead.
>>>>
>>>>
>>>>>                  if (mask)
>>>>>                          ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>>>>                  else
>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>>>           * PAD OUTPUT ENABLE 02
>>>>>           * - [7:2]:     D[5:0] output enable
>>>>>           */
>>>>> -     return ov5640_write_reg(sensor,
>>>>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>> -                             on ? 0xfc : 0);
>>>>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>> +                            on ? 0xfc : 0);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>>>      }
>>>>>
>>>>>      static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>>>
>>>>
>>>>
>>>> BR,
>>>> Hugues.
>>>>
>>>
>>>
>>> Hello everyone,
>>>
>>> When I updated driver in my tree with this patch, I noticed that my
>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
>>> working.
>>>
>>> It looks like the culprit is this patch.
>>> I am not sure which is the best way to fix it.
>>> It looks like initially things work fine when probing the driver, but
>>> when we want to start streaming at a later point, the register SYS_CTRL0
>>> cannot be written anymore.
>>> Here is an output of the log:
>>>
>>>     --- Opening /dev/video0...
>>>        Trying source module v4l2...
>>>        /dev/video0 opened.
>>>        No input was specified, using the first.
>>>        ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
>>>        atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
>>>        Error starting stream.
>>>        VIDIOC_STREAMON: Remote I/O error
>>>        Unable to use mmap. Using read instead.
>>>        Unable to use read.
>>>
>>> I am using a simple application to read from /dev/video0 (which is
>>> registered by the atmel-isc once the sensor probed)
>>>
>>> I have a feeling that something is wrong related to power, but I cannot
>>> tell for sure.
>>>
>>> Reverting this patch makes it work fine again.
>>>
>>> Help is appreciated,
>>> Thanks,
>>> Eugen
>>>
>>
>>
>> Gentle ping.
>>
>> I would like to send a revert patch if nobody cares about this
>> driver/patch except me.
>>
> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> was able to capture the video data [0] using the yavta application.
> I'm fine with reverting the patch too as this works fine as well.

Hi Prabhakar,

Thanks for trying this out.
ov5640 works in both parallel or CSI2 mode. Looking at the board, it 
looks a parallel connection but I am not 100%, you tested using parallel 
interface ?

> 
> Just some suggestions:
> - What is the application you are trying to use for capturing?

I was using a simple app that reads from /dev/video0, it's called 
fswebcam. I can try more apps if it's needed.

> - Have you tried reducing the i2c clock speed?

I did not, but without this patch, there is no problem whatsoever, and I 
have been using this sensor since around 4.9 kernel version.

> - Can you try and do a dummy write for some other register in
> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?

I can try

> - Can you add ov5640_read_reg() in ov5640_write_reg() when
> i2c_transfer() fails to check if read too is failing.
> 
> [0] https://paste.debian.net/1224317/

You seem to be able to capture successfully, I have a feeling that in my 
case the sensor is somehow not powered up when the capture is being 
requested.
Could you share a snippet of your device tree node for the sensor so I 
can have a look ?

Thanks,
Eugen

> 
> Cheers,
> Prabhakar
> 

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Lad, Prabhakar]

Hi Eugen,

On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote:
>
> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> > Hi,
> >
> > Sorry for the delay.
> >
> > On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote:
> >>
> >> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> >>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >>>> Hi Prabhakar,
> >>>>
> >>>> As discussed separately I would prefer to better understand issue before
> >>>> going to this patch.
> >>>> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>>>
> >>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>>>> Keep the sensor in software power down mode and wake up only in
> >>>>> ov5640_set_stream_dvp() callback.
> >>>>>
> >>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>      drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>>>      1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>> index 2fe4a7ac0592..880fde73a030 100644
> >>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>> @@ -34,6 +34,8 @@
> >>>>>      #define OV5640_REG_SYS_RESET02              0x3002
> >>>>>      #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
> >>>>>      #define OV5640_REG_SYS_CTRL0                0x3008
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>>>
> >>>> For the time being this section was only referring to registers
> >>>> addresses and bit details was explained into a comment right before
> >>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>>>
> >>>>>      #define OV5640_REG_CHIP_ID          0x300a
> >>>>>      #define OV5640_REG_IO_MIPI_CTRL00   0x300e
> >>>>>      #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
> >>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>>>                  val = regs->val;
> >>>>>                  mask = regs->mask;
> >>>>>
> >>>>> +             /* remain in power down mode for DVP */
> >>>>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>>>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>>>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>>>> +                     continue;
> >>>>> +
> >>>>
> >>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >>>> has been partially removed from big init sequence: for power-up part,
> >>>> but power-dwn remains at very beginning of sequence.
> >>>> We should completely remove 0x3008 from init sequence, including
> >>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >>>> should be called at the right place instead.
> >>>>
> >>>>
> >>>>>                  if (mask)
> >>>>>                          ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>>>                  else
> >>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>>>           * PAD OUTPUT ENABLE 02
> >>>>>           * - [7:2]:     D[5:0] output enable
> >>>>>           */
> >>>>> -     return ov5640_write_reg(sensor,
> >>>>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> -                             on ? 0xfc : 0);
> >>>>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> +                            on ? 0xfc : 0);
> >>>>> +     if (ret)
> >>>>> +             return ret;
> >>>>> +
> >>>>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>>>      }
> >>>>>
> >>>>>      static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>>>
> >>>>
> >>>>
> >>>> BR,
> >>>> Hugues.
> >>>>
> >>>
> >>>
> >>> Hello everyone,
> >>>
> >>> When I updated driver in my tree with this patch, I noticed that my
> >>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> >>> working.
> >>>
> >>> It looks like the culprit is this patch.
> >>> I am not sure which is the best way to fix it.
> >>> It looks like initially things work fine when probing the driver, but
> >>> when we want to start streaming at a later point, the register SYS_CTRL0
> >>> cannot be written anymore.
> >>> Here is an output of the log:
> >>>
> >>>     --- Opening /dev/video0...
> >>>        Trying source module v4l2...
> >>>        /dev/video0 opened.
> >>>        No input was specified, using the first.
> >>>        ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >>>        atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >>>        Error starting stream.
> >>>        VIDIOC_STREAMON: Remote I/O error
> >>>        Unable to use mmap. Using read instead.
> >>>        Unable to use read.
> >>>
> >>> I am using a simple application to read from /dev/video0 (which is
> >>> registered by the atmel-isc once the sensor probed)
> >>>
> >>> I have a feeling that something is wrong related to power, but I cannot
> >>> tell for sure.
> >>>
> >>> Reverting this patch makes it work fine again.
> >>>
> >>> Help is appreciated,
> >>> Thanks,
> >>> Eugen
> >>>
> >>
> >>
> >> Gentle ping.
> >>
> >> I would like to send a revert patch if nobody cares about this
> >> driver/patch except me.
> >>
> > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> > was able to capture the video data [0] using the yavta application.
> > I'm fine with reverting the patch too as this works fine as well.
>
> Hi Prabhakar,
>
> Thanks for trying this out.
> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
> looks a parallel connection but I am not 100%, you tested using parallel
> interface ?
>
I have tested it in parallel interface mode as RZ/G1H supports parallel capture

> >
> > Just some suggestions:
> > - What is the application you are trying to use for capturing?
>
> I was using a simple app that reads from /dev/video0, it's called
> fswebcam. I can try more apps if it's needed.
>
could you give it a shot with yavta please.

> > - Have you tried reducing the i2c clock speed?
>
> I did not, but without this patch, there is no problem whatsoever, and I
> have been using this sensor since around 4.9 kernel version.
>
Agreed, I'm thinking aloud just to narrow things.

> > - Can you try and do a dummy write for some other register in
> > ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
>
> I can try
>
Thank you.

> > - Can you add ov5640_read_reg() in ov5640_write_reg() when
> > i2c_transfer() fails to check if read too is failing.
> >
> > [0] https://paste.debian.net/1224317/
>
> You seem to be able to capture successfully, I have a feeling that in my
> case the sensor is somehow not powered up when the capture is being
> requested.
> Could you share a snippet of your device tree node for the sensor so I
> can have a look ?
>
[0] contains the bridge node and [1] contains the sensor node.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6

Cheers,
Prabhakar

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Eugen.Hristev]

On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> Hi Eugen,
> 
> On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote:
>>
>> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
>>> Hi,
>>>
>>> Sorry for the delay.
>>>
>>> On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote:
>>>>
>>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
>>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>>>>>> Hi Prabhakar,
>>>>>>
>>>>>> As discussed separately I would prefer to better understand issue before
>>>>>> going to this patch.
>>>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>>>>>
>>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>>>>>> Keep the sensor in software power down mode and wake up only in
>>>>>>> ov5640_set_stream_dvp() callback.
>>>>>>>
>>>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>>> ---
>>>>>>>       drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>>>>>>       1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>>>> index 2fe4a7ac0592..880fde73a030 100644
>>>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>>>> @@ -34,6 +34,8 @@
>>>>>>>       #define OV5640_REG_SYS_RESET02              0x3002
>>>>>>>       #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
>>>>>>>       #define OV5640_REG_SYS_CTRL0                0x3008
>>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>>>>>
>>>>>> For the time being this section was only referring to registers
>>>>>> addresses and bit details was explained into a comment right before
>>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>>>>>
>>>>>>>       #define OV5640_REG_CHIP_ID          0x300a
>>>>>>>       #define OV5640_REG_IO_MIPI_CTRL00   0x300e
>>>>>>>       #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
>>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>>>>>>                   val = regs->val;
>>>>>>>                   mask = regs->mask;
>>>>>>>
>>>>>>> +             /* remain in power down mode for DVP */
>>>>>>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>>>>>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>>>>>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>>>>>> +                     continue;
>>>>>>> +
>>>>>>
>>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>>>>>> has been partially removed from big init sequence: for power-up part,
>>>>>> but power-dwn remains at very beginning of sequence.
>>>>>> We should completely remove 0x3008 from init sequence, including
>>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>>>>>> should be called at the right place instead.
>>>>>>
>>>>>>
>>>>>>>                   if (mask)
>>>>>>>                           ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>>>>>>                   else
>>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>>>>>            * PAD OUTPUT ENABLE 02
>>>>>>>            * - [7:2]:     D[5:0] output enable
>>>>>>>            */
>>>>>>> -     return ov5640_write_reg(sensor,
>>>>>>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>>>> -                             on ? 0xfc : 0);
>>>>>>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>>>> +                            on ? 0xfc : 0);
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>> +
>>>>>>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
>>>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>>>>>       }
>>>>>>>
>>>>>>>       static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>>>>>
>>>>>>
>>>>>>
>>>>>> BR,
>>>>>> Hugues.
>>>>>>
>>>>>
>>>>>
>>>>> Hello everyone,
>>>>>
>>>>> When I updated driver in my tree with this patch, I noticed that my
>>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
>>>>> working.
>>>>>
>>>>> It looks like the culprit is this patch.
>>>>> I am not sure which is the best way to fix it.
>>>>> It looks like initially things work fine when probing the driver, but
>>>>> when we want to start streaming at a later point, the register SYS_CTRL0
>>>>> cannot be written anymore.
>>>>> Here is an output of the log:
>>>>>
>>>>>      --- Opening /dev/video0...
>>>>>         Trying source module v4l2...
>>>>>         /dev/video0 opened.
>>>>>         No input was specified, using the first.
>>>>>         ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
>>>>>         atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
>>>>>         Error starting stream.
>>>>>         VIDIOC_STREAMON: Remote I/O error
>>>>>         Unable to use mmap. Using read instead.
>>>>>         Unable to use read.
>>>>>
>>>>> I am using a simple application to read from /dev/video0 (which is
>>>>> registered by the atmel-isc once the sensor probed)
>>>>>
>>>>> I have a feeling that something is wrong related to power, but I cannot
>>>>> tell for sure.
>>>>>
>>>>> Reverting this patch makes it work fine again.
>>>>>
>>>>> Help is appreciated,
>>>>> Thanks,
>>>>> Eugen
>>>>>
>>>>
>>>>
>>>> Gentle ping.
>>>>
>>>> I would like to send a revert patch if nobody cares about this
>>>> driver/patch except me.
>>>>
>>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
>>> was able to capture the video data [0] using the yavta application.
>>> I'm fine with reverting the patch too as this works fine as well.
>>
>> Hi Prabhakar,
>>
>> Thanks for trying this out.
>> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
>> looks a parallel connection but I am not 100%, you tested using parallel
>> interface ?
>>
> I have tested it in parallel interface mode as RZ/G1H supports parallel capture
> 
>>>
>>> Just some suggestions:
>>> - What is the application you are trying to use for capturing?
>>
>> I was using a simple app that reads from /dev/video0, it's called
>> fswebcam. I can try more apps if it's needed.
>>
> could you give it a shot with yavta please.

Hello Lad,

I debugged this further, and I have some news:

It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather 
the sensor refuses to accept a power up.

I tried to read the register before the write, and it reads 0x42.
Then, I tried to write 0x42 back, and it works fine.
So, I do not think there is a problem with i2c communication.
The only problem is that the sensor refuses to power up (accept the 0x2 
into the SYS_CTRL_0 ), due to an unknown (to me) reason.

If the power up is performed at the initialization phase, it works.

I also tried to capture with v4l2-ctl, and the result is the same.

Which of the init configuration set of registers your test is using?
It may be that it does not work in a specific config .

The datasheet which I have does not claim that the 'power up' might fail 
in some circumstances.

Thanks for helping,

Eugen


> 
>>> - Have you tried reducing the i2c clock speed?
>>
>> I did not, but without this patch, there is no problem whatsoever, and I
>> have been using this sensor since around 4.9 kernel version.
>>
> Agreed, I'm thinking aloud just to narrow things.
> 
>>> - Can you try and do a dummy write for some other register in
>>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
>>
>> I can try
>>
> Thank you.
> 
>>> - Can you add ov5640_read_reg() in ov5640_write_reg() when
>>> i2c_transfer() fails to check if read too is failing.
>>>
>>> [0] https://paste.debian.net/1224317/
>>
>> You seem to be able to capture successfully, I have a feeling that in my
>> case the sensor is somehow not powered up when the capture is being
>> requested.
>> Could you share a snippet of your device tree node for the sensor so I
>> can have a look ?
>>
> [0] contains the bridge node and [1] contains the sensor node.
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6
> 
> Cheers,
> Prabhakar
> 

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Lad, Prabhakar]

Hi Eugen,

On Mon, Jan 3, 2022 at 11:29 AM <Eugen.Hristev@microchip.com> wrote:
>
> On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> > Hi Eugen,
> >
> > On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@microchip.com> wrote:
> >>
> >> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> >>> Hi,
> >>>
> >>> Sorry for the delay.
> >>>
> >>> On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@microchip.com> wrote:
> >>>>
> >>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> >>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >>>>>> Hi Prabhakar,
> >>>>>>
> >>>>>> As discussed separately I would prefer to better understand issue before
> >>>>>> going to this patch.
> >>>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>>>>>
> >>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>>>>>> Keep the sensor in software power down mode and wake up only in
> >>>>>>> ov5640_set_stream_dvp() callback.
> >>>>>>>
> >>>>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>>>> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >>>>>>> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>> ---
> >>>>>>>       drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>>>>>       1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>>>> index 2fe4a7ac0592..880fde73a030 100644
> >>>>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>>>> @@ -34,6 +34,8 @@
> >>>>>>>       #define OV5640_REG_SYS_RESET02              0x3002
> >>>>>>>       #define OV5640_REG_SYS_CLOCK_ENABLE02       0x3006
> >>>>>>>       #define OV5640_REG_SYS_CTRL0                0x3008
> >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>>>>>
> >>>>>> For the time being this section was only referring to registers
> >>>>>> addresses and bit details was explained into a comment right before
> >>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>>>>>
> >>>>>>>       #define OV5640_REG_CHIP_ID          0x300a
> >>>>>>>       #define OV5640_REG_IO_MIPI_CTRL00   0x300e
> >>>>>>>       #define OV5640_REG_PAD_OUTPUT_ENABLE01      0x3017
> >>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>>>>>                   val = regs->val;
> >>>>>>>                   mask = regs->mask;
> >>>>>>>
> >>>>>>> +             /* remain in power down mode for DVP */
> >>>>>>> +             if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>>>>>> +                 val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>>>>>> +                 sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>>>>>> +                     continue;
> >>>>>>> +
> >>>>>>
> >>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >>>>>> has been partially removed from big init sequence: for power-up part,
> >>>>>> but power-dwn remains at very beginning of sequence.
> >>>>>> We should completely remove 0x3008 from init sequence, including
> >>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >>>>>> should be called at the right place instead.
> >>>>>>
> >>>>>>
> >>>>>>>                   if (mask)
> >>>>>>>                           ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>>>>>                   else
> >>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>>>>>            * PAD OUTPUT ENABLE 02
> >>>>>>>            * - [7:2]:     D[5:0] output enable
> >>>>>>>            */
> >>>>>>> -     return ov5640_write_reg(sensor,
> >>>>>>> -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>>>> -                             on ? 0xfc : 0);
> >>>>>>> +     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>>>> +                            on ? 0xfc : 0);
> >>>>>>> +     if (ret)
> >>>>>>> +             return ret;
> >>>>>>> +
> >>>>>>> +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>>>>>> +                             OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>>>>>       }
> >>>>>>>
> >>>>>>>       static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> BR,
> >>>>>> Hugues.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> Hello everyone,
> >>>>>
> >>>>> When I updated driver in my tree with this patch, I noticed that my
> >>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> >>>>> working.
> >>>>>
> >>>>> It looks like the culprit is this patch.
> >>>>> I am not sure which is the best way to fix it.
> >>>>> It looks like initially things work fine when probing the driver, but
> >>>>> when we want to start streaming at a later point, the register SYS_CTRL0
> >>>>> cannot be written anymore.
> >>>>> Here is an output of the log:
> >>>>>
> >>>>>      --- Opening /dev/video0...
> >>>>>         Trying source module v4l2...
> >>>>>         /dev/video0 opened.
> >>>>>         No input was specified, using the first.
> >>>>>         ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >>>>>         atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >>>>>         Error starting stream.
> >>>>>         VIDIOC_STREAMON: Remote I/O error
> >>>>>         Unable to use mmap. Using read instead.
> >>>>>         Unable to use read.
> >>>>>
> >>>>> I am using a simple application to read from /dev/video0 (which is
> >>>>> registered by the atmel-isc once the sensor probed)
> >>>>>
> >>>>> I have a feeling that something is wrong related to power, but I cannot
> >>>>> tell for sure.
> >>>>>
> >>>>> Reverting this patch makes it work fine again.
> >>>>>
> >>>>> Help is appreciated,
> >>>>> Thanks,
> >>>>> Eugen
> >>>>>
> >>>>
> >>>>
> >>>> Gentle ping.
> >>>>
> >>>> I would like to send a revert patch if nobody cares about this
> >>>> driver/patch except me.
> >>>>
> >>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> >>> was able to capture the video data [0] using the yavta application.
> >>> I'm fine with reverting the patch too as this works fine as well.
> >>
> >> Hi Prabhakar,
> >>
> >> Thanks for trying this out.
> >> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
> >> looks a parallel connection but I am not 100%, you tested using parallel
> >> interface ?
> >>
> > I have tested it in parallel interface mode as RZ/G1H supports parallel capture
> >
> >>>
> >>> Just some suggestions:
> >>> - What is the application you are trying to use for capturing?
> >>
> >> I was using a simple app that reads from /dev/video0, it's called
> >> fswebcam. I can try more apps if it's needed.
> >>
> > could you give it a shot with yavta please.
>
> Hello Lad,
>
> I debugged this further, and I have some news:
>
> It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather
> the sensor refuses to accept a power up.
>
> I tried to read the register before the write, and it reads 0x42.
> Then, I tried to write 0x42 back, and it works fine.
> So, I do not think there is a problem with i2c communication.
> The only problem is that the sensor refuses to power up (accept the 0x2
> into the SYS_CTRL_0 ), due to an unknown (to me) reason.
>
That's strange.

> If the power up is performed at the initialization phase, it works.
>
> I also tried to capture with v4l2-ctl, and the result is the same.
>
you mean yavta ?

> Which of the init configuration set of registers your test is using?
I have been testing 320x240 and 640x480. Could you give that a try please?

> It may be that it does not work in a specific config .
>
> The datasheet which I have does not claim that the 'power up' might fail
> in some circumstances.
>
Let me check if I can ping OmniVision FAE.

> Thanks for helping,
>
> Eugen
>
>
> >
> >>> - Have you tried reducing the i2c clock speed?
> >>
> >> I did not, but without this patch, there is no problem whatsoever, and I
> >> have been using this sensor since around 4.9 kernel version.
> >>
> > Agreed, I'm thinking aloud just to narrow things.
> >
> >>> - Can you try and do a dummy write for some other register in
> >>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
> >>
> >> I can try
> >>
> > Thank you.
> >
> >>> - Can you add ov5640_read_reg() in ov5640_write_reg() when
> >>> i2c_transfer() fails to check if read too is failing.
> >>>
> >>> [0] https://paste.debian.net/1224317/
> >>
> >> You seem to be able to capture successfully, I have a feeling that in my
> >> case the sensor is somehow not powered up when the capture is being
> >> requested.
> >> Could you share a snippet of your device tree node for the sensor so I
> >> can have a look ?
> >>
> > [0] contains the bridge node and [1] contains the sensor node.
> >
> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6
> >
Cheers,
Prabhakar

Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

[Lad, Prabhakar]

On Tue, Jan 4, 2022 at 9:57 AM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Eugen,
>
> On Mon, Jan 3, 2022 at 11:29 AM <Eugen.Hristev@microchip.com> wrote:
> >
> > On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> > > Hi Eugen,
> > >
<snip>
> > > could you give it a shot with yavta please.
> >
> > Hello Lad,
> >
> > I debugged this further, and I have some news:
> >
> > It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather
> > the sensor refuses to accept a power up.
> >
> > I tried to read the register before the write, and it reads 0x42.
> > Then, I tried to write 0x42 back, and it works fine.
> > So, I do not think there is a problem with i2c communication.
> > The only problem is that the sensor refuses to power up (accept the 0x2
> > into the SYS_CTRL_0 ), due to an unknown (to me) reason.
> >
> That's strange.
>
> > If the power up is performed at the initialization phase, it works.
> >
> > I also tried to capture with v4l2-ctl, and the result is the same.
> >
> you mean yavta ?
>
> > Which of the init configuration set of registers your test is using?
> I have been testing 320x240 and 640x480. Could you give that a try please?
>
> > It may be that it does not work in a specific config .
> >
> > The datasheet which I have does not claim that the 'power up' might fail
> > in some circumstances.
> >
> Let me check if I can ping OmniVision FAE.
>
Fyi.. I got the below feedback from OmniVision FAE.

SW standby bit is working as expected from my side.


As far as the sensor initialization is concerned  we use HW power up
sequence defined in the datasheet followed by SW initialization.

SW initialization consist of the following :-

78 3103  11    ;  I2C timing ( do not modify)
78 3008  82  ;   SW reset
78 3008  42  ;   Stop streaming
78 …….            Sensor settings for required mode

78 3008  02  ;   Start  streaming

Note:-  0x3008[7]  SW reset bit is volatile, as soon as a reset is
applied the  all the bits  are  cleared  and  0x3008[7:0] set to
default value hence  should read  0x2

Cheers,
Prabhakar

[PATCH v4 2/6] media: i2c: ov5640: Separate out mipi configuration from s_power

[Lad Prabhakar]

In preparation for adding DVP configuration in s_power callback
move mipi configuration into separate function

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 116 +++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 56 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 880fde73a030..8af11d532699 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2014,6 +2014,61 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
 	clk_disable_unprepare(sensor->xclk);
 }
 
+static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	if (!on) {
+		/* Reset MIPI bus settings to their default values. */
+		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
+		ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x04);
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x00);
+		return 0;
+	}
+
+	/*
+	 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+	 *
+	 * 0x300e = 0x40
+	 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
+	 *		  "ov5640_set_stream_mipi()")
+	 * [4] = 0	: Power up MIPI HS Tx
+	 * [3] = 0	: Power up MIPI LS Rx
+	 * [2] = 0	: MIPI interface disabled
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
+	if (ret)
+		return ret;
+
+	/*
+	 * Gate clock and set LP11 in 'no packets mode' (idle)
+	 *
+	 * 0x4800 = 0x24
+	 * [5] = 1	: Gate clock when 'no packets'
+	 * [2] = 1	: MIPI bus in LP11 when 'no packets'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
+	if (ret)
+		return ret;
+
+	/*
+	 * Set data lanes and clock in LP11 when 'sleeping'
+	 *
+	 * 0x3019 = 0x70
+	 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
+	 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
+	 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
+	if (ret)
+		return ret;
+
+	/* Give lanes some time to coax into LP11 state. */
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
 static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 {
 	int ret = 0;
@@ -2026,67 +2081,16 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		ret = ov5640_restore_mode(sensor);
 		if (ret)
 			goto power_off;
+	}
 
-		/* We're done here for DVP bus, while CSI-2 needs setup. */
-		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
-			return 0;
-
-		/*
-		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
-		 *
-		 * 0x300e = 0x40
-		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
-		 *		  "ov5640_set_stream_mipi()")
-		 * [4] = 0	: Power up MIPI HS Tx
-		 * [3] = 0	: Power up MIPI LS Rx
-		 * [2] = 0	: MIPI interface disabled
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
-		if (ret)
-			goto power_off;
-
-		/*
-		 * Gate clock and set LP11 in 'no packets mode' (idle)
-		 *
-		 * 0x4800 = 0x24
-		 * [5] = 1	: Gate clock when 'no packets'
-		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_MIPI_CTRL00, 0x24);
-		if (ret)
-			goto power_off;
-
-		/*
-		 * Set data lanes and clock in LP11 when 'sleeping'
-		 *
-		 * 0x3019 = 0x70
-		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
-		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
-		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_PAD_OUTPUT00, 0x70);
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
+		ret = ov5640_set_power_mipi(sensor, on);
 		if (ret)
 			goto power_off;
+	}
 
-		/* Give lanes some time to coax into LP11 state. */
-		usleep_range(500, 1000);
-
-	} else {
-		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
-			/* Reset MIPI bus settings to their default values. */
-			ov5640_write_reg(sensor,
-					 OV5640_REG_IO_MIPI_CTRL00, 0x58);
-			ov5640_write_reg(sensor,
-					 OV5640_REG_MIPI_CTRL00, 0x04);
-			ov5640_write_reg(sensor,
-					 OV5640_REG_PAD_OUTPUT00, 0x00);
-		}
-
+	if (!on)
 		ov5640_set_power_off(sensor);
-	}
 
 	return 0;
 
-- 
2.17.1

[PATCH v4 3/6] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad Prabhakar]

During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
mode with rcar-vin bridge noticed the capture worked fine for the first run
(with yavta), but for subsequent runs the bridge driver waited for the
frame to be captured. Debugging further noticed the data lines were
enabled/disabled in stream on/off callback and dumping the register
contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
values, but yet frame capturing failed.

To get around this issue data lines are enabled in s_power callback.
(Also the sensor remains in power down mode if not streaming so power
consumption shouldn't be affected)

Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 73 +++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8af11d532699..8288728d8704 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -276,8 +276,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 /* YUV422 UYVY VGA@30fps */
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
-	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
-	{0x3630, 0x36, 0, 0},
+	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
 	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -1283,33 +1282,6 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	if (ret)
 		return ret;
 
-	/*
-	 * enable VSYNC/HREF/PCLK DVP control lines
-	 * & D[9:6] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 01
-	 * - 6:		VSYNC output enable
-	 * - 5:		HREF output enable
-	 * - 4:		PCLK output enable
-	 * - [3:0]:	D[9:6] output enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_PAD_OUTPUT_ENABLE01,
-			       on ? 0x7f : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * enable D[5:0] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 02
-	 * - [7:2]:	D[5:0] output enable
-	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
-			       on ? 0xfc : 0);
-	if (ret)
-		return ret;
-
 	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
 				OV5640_REG_SYS_CTRL0_SW_PWUP :
 				OV5640_REG_SYS_CTRL0_SW_PWDN);
@@ -2069,6 +2041,40 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 	return 0;
 }
 
+static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	if (!on) {
+		/* Reset settings to their default values. */
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
+		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
+		return 0;
+	}
+
+	/*
+	 * enable VSYNC/HREF/PCLK DVP control lines
+	 * & D[9:6] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 01
+	 * - 6:		VSYNC output enable
+	 * - 5:		HREF output enable
+	 * - 4:		PCLK output enable
+	 * - [3:0]:	D[9:6] output enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+	if (ret)
+		return ret;
+
+	/*
+	 * enable D[5:0] DVP data lines
+	 *
+	 * PAD OUTPUT ENABLE 02
+	 * - [7:2]:	D[5:0] output enable
+	 */
+	return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
+}
+
 static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 {
 	int ret = 0;
@@ -2083,11 +2089,12 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 			goto power_off;
 	}
 
-	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
 		ret = ov5640_set_power_mipi(sensor, on);
-		if (ret)
-			goto power_off;
-	}
+	else
+		ret = ov5640_set_power_dvp(sensor, on);
+	if (ret)
+		goto power_off;
 
 	if (!on)
 		ov5640_set_power_off(sensor);
-- 
2.17.1

Re: [PATCH v4 3/6] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Hugues FRUCHET]

Hi Prabhakar,

Thanks for your patches, good to see one more OV5640 stakeholder 
upstreaming some fixes/features.

I'm also using a parallel setup with OV5640 connected on STM32 DCMI 
camera interface.
First basic tests have not shown any regressions on my side but I would 
like to better understand the problem you encountered and the way you 
solve it, see below my comments.


On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> mode with rcar-vin bridge noticed the capture worked fine for the first run
> (with yavta), but for subsequent runs the bridge driver waited for the
> frame to be captured. Debugging further noticed the data lines were
> enabled/disabled in stream on/off callback and dumping the register
> contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> values, but yet frame capturing failed.

Could you show the sequence of V4L2 calls which lead to freeze ?

Reading the patch you proposed, my guess is that issue is coming when 
multiple S_STREAM(on)/S_STREAM(off) are made while power remains, is 
that true ?
I have added some traces in code and tried to reproduce with yavta, 
v4l2-ctl and GStreamer but I'm not able to generate such sequence, here 
is what I got everytime:

[  809.113790] ov5640 0-003c: ov5640_s_power>
[  809.116431] ov5640 0-003c: ov5640_set_power>
[  809.120788] ov5640 0-003c: ov5640_set_power_on>
[  809.622047] ov5640 0-003c: ov5640_set_power_dvp>
[  809.862734] ov5640 0-003c: ov5640_s_stream>
[  809.865462] ov5640 0-003c: ov5640_set_stream_dvp on>
<capturing here>
[  828.549531] ov5640 0-003c: ov5640_s_stream>
[  828.552265] ov5640 0-003c: ov5640_set_stream_dvp off>
[  828.580970] ov5640 0-003c: ov5640_s_power>
[  828.583613] ov5640 0-003c: ov5640_set_power>
[  828.587921] ov5640 0-003c: ov5640_set_power_dvp>
[  828.620346] ov5640 0-003c: ov5640_set_power_off>

Which application/command line are you using to reproduce your problem ?


> 
> To get around this issue data lines are enabled in s_power callback.
> (Also the sensor remains in power down mode if not streaming so power
> consumption shouldn't be affected)

For the time being, I really don't understand why this patch is fixing 
capture freeze.

> 
> Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Tested-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   drivers/media/i2c/ov5640.c | 73 +++++++++++++++++++++-----------------
>   1 file changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 8af11d532699..8288728d8704 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -276,8 +276,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>   /* YUV422 UYVY VGA@30fps */
>   static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>   	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> -	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> -	{0x3630, 0x36, 0, 0},
> +	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
>   	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>   	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
>   	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
> @@ -1283,33 +1282,6 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>   	if (ret)
>   		return ret;
>   
> -	/*
> -	 * enable VSYNC/HREF/PCLK DVP control lines
> -	 * & D[9:6] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 01
> -	 * - 6:		VSYNC output enable
> -	 * - 5:		HREF output enable
> -	 * - 4:		PCLK output enable
> -	 * - [3:0]:	D[9:6] output enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_PAD_OUTPUT_ENABLE01,
> -			       on ? 0x7f : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable D[5:0] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 02
> -	 * - [7:2]:	D[5:0] output enable
> -	 */
> -	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> -			       on ? 0xfc : 0);
> -	if (ret)
> -		return ret;
> -
>   	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>   				OV5640_REG_SYS_CTRL0_SW_PWUP :
>   				OV5640_REG_SYS_CTRL0_SW_PWDN);
> @@ -2069,6 +2041,40 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
>   	return 0;
>   }
>   
> +static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
> +{
> +	int ret;
> +
> +	if (!on) {
> +		/* Reset settings to their default values. */
> +		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> +		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
> +		return 0;
> +	}
> +
> +	/*
> +	 * enable VSYNC/HREF/PCLK DVP control lines
> +	 * & D[9:6] DVP data lines
> +	 *
> +	 * PAD OUTPUT ENABLE 01
> +	 * - 6:		VSYNC output enable
> +	 * - 5:		HREF output enable
> +	 * - 4:		PCLK output enable
> +	 * - [3:0]:	D[9:6] output enable
> +	 */
> +	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * enable D[5:0] DVP data lines
> +	 *
> +	 * PAD OUTPUT ENABLE 02
> +	 * - [7:2]:	D[5:0] output enable
> +	 */
> +	return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> +}
> +
>   static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>   {
>   	int ret = 0;
> @@ -2083,11 +2089,12 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>   			goto power_off;
>   	}
>   
> -	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
>   		ret = ov5640_set_power_mipi(sensor, on);
> -		if (ret)
> -			goto power_off;
> -	}
> +	else
> +		ret = ov5640_set_power_dvp(sensor, on);
> +	if (ret)
> +		goto power_off;
>   
>   	if (!on)
>   		ov5640_set_power_off(sensor);
> 

Best regards,
Hugues.

Re: [PATCH v4 3/6] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Lad, Prabhakar]

Hi Hugues,

Thank you for the review.

On Mon, Sep 7, 2020 at 10:44 AM Hugues FRUCHET <hugues.fruchet@st.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patches, good to see one more OV5640 stakeholder
> upstreaming some fixes/features.
>
> I'm also using a parallel setup with OV5640 connected on STM32 DCMI
> camera interface.
> First basic tests have not shown any regressions on my side but I would
> like to better understand the problem you encountered and the way you
> solve it, see below my comments.
>
>
Thank you for testing the patches.

> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> > During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
> > mode with rcar-vin bridge noticed the capture worked fine for the first run
> > (with yavta), but for subsequent runs the bridge driver waited for the
> > frame to be captured. Debugging further noticed the data lines were
> > enabled/disabled in stream on/off callback and dumping the register
> > contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
> > values, but yet frame capturing failed.
>
> Could you show the sequence of V4L2 calls which lead to freeze ?
>
> Reading the patch you proposed, my guess is that issue is coming when
> multiple S_STREAM(on)/S_STREAM(off) are made while power remains, is
> that true ?
> I have added some traces in code and tried to reproduce with yavta,
> v4l2-ctl and GStreamer but I'm not able to generate such sequence, here
> is what I got everytime:
>
> [  809.113790] ov5640 0-003c: ov5640_s_power>
> [  809.116431] ov5640 0-003c: ov5640_set_power>
> [  809.120788] ov5640 0-003c: ov5640_set_power_on>
> [  809.622047] ov5640 0-003c: ov5640_set_power_dvp>
> [  809.862734] ov5640 0-003c: ov5640_s_stream>
> [  809.865462] ov5640 0-003c: ov5640_set_stream_dvp on>
> <capturing here>
> [  828.549531] ov5640 0-003c: ov5640_s_stream>
> [  828.552265] ov5640 0-003c: ov5640_set_stream_dvp off>
> [  828.580970] ov5640 0-003c: ov5640_s_power>
> [  828.583613] ov5640 0-003c: ov5640_set_power>
> [  828.587921] ov5640 0-003c: ov5640_set_power_dvp>
> [  828.620346] ov5640 0-003c: ov5640_set_power_off>
>
> Which application/command line are you using to reproduce your problem ?
>
yavta.
>
> >
> > To get around this issue data lines are enabled in s_power callback.
> > (Also the sensor remains in power down mode if not streaming so power
> > consumption shouldn't be affected)
>
> For the time being, I really don't understand why this patch is fixing
> capture freeze.
>

Below is the log with this series applied in DVP mode:

root@iwg21m:~#
root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
[   36.191661] ov5640_s_power>
[   36.194452] ov5640_set_power>
[   36.197413] ov5640_set_power_on>
[   36.200714] ov5640_reset>
[   36.203328] ov5640_restore_mode>
[   36.206549] ov5640_load_regs>
[   36.550255] ov5640_set_timings>
[   36.554572] ov5640_set_mode>
[   36.557963] ov5640_calc_pixel_rate>
[   36.561458] ov5640_set_dvp_pclk>
[   36.564679] ov5640_calc_pclk>
[   36.567639] ov5640_calc_sys_clk>
[   36.572190] ov5640_set_mode_direct>
[   36.575671] ov5640_load_regs>
[   36.583205] ov5640_set_timings>
[   36.591494] ov5640_set_framefmt>
[   36.595717] ov5640_set_power_dvp>
[   36.599486] ov5640_s_ctrl>
[   36.602200] ov5640_set_ctrl_white_balance>
[   36.606550] ov5640_s_ctrl>
[   36.609250] ov5640_set_ctrl_exposure>
[   36.613179] ov5640_s_ctrl>
[   36.615878] ov5640_set_ctrl_gain>
[   36.619446] ov5640_s_ctrl>
[   36.622160] ov5640_set_ctrl_saturation>
[   36.626476] ov5640_s_ctrl>
[   36.629177] ov5640_set_ctrl_hue>
[   36.632670] ov5640_s_ctrl>
[   36.635370] ov5640_set_ctrl_contrast>
[   36.639282] ov5640_s_ctrl>
[   36.642112] ov5640_s_ctrl>
[   36.644813] ov5640_set_ctrl_hflip>
[   36.648465] ov5640_s_ctrl>
[   36.651179] ov5640_set_ctrl_vflip>
[   36.654833] ov5640_s_ctrl>
[   36.657533] ov5640_set_ctrl_light_freq>
Device /dev/video0 opened.
Device `R_Car_VIN' on `platform:e6ef3[   36.662120] ov5640_set_fmt>
000.video' (driver 'rcar_vin') supports video, capture, without [
36.670491] ov5640_try_fmt_internal>
mplanes.
[   36.679593] ov5640_find_mode>
[   36.683428] ov5640_calc_pixel_rate>
Video format set: UYVY (59565955) 640x480 (stride 1280) field none
buffer size 614400
Video format: UYVY (59565955) 640x480 (stride 1280) field none buffer
siz[   36.696456] ov5640_s_stream>
e 614400
4 buffers requested.
length: 614400 offset: 0 timesta[   36.703716] ov5640_set_stream_dvp>
mp type/source: mono/EoF
Buffer 0/0 mapped at address 0xb6b4c000.
length: 614400 offset: 614400 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xb6ab6000.
length: 614400 offset: 1228800 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xb6a20000.
length: 614400 offset: 1843200 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xb698a000.
0 (0) [-] none 0 614400 B 36.776928 36.776946 15.545 fps ts mono/EoF
[   36.900255] ov5640_s_stream>
[   36.903130] ov5640_set_stream_dvp>
Captured 1 frames in 0.064348 seconds (15.540378 fps, 9548008.11[
36.907351] ov5640_s_power>
2077 B/s).
4 buffers released.
[   36.915167] ov5640_set_power>
[   36.920979] ov5640_set_power_dvp>
[   36.924765] ov5640_set_power_off>
root@iwg21m:~#
root@iwg21m:~#
root@iwg21m:~# dmesg | grep ov5640
[    2.412247] ov5640_probe>
[    2.414913] ov5640_get_regulators>
[    2.418320] ov5640 1-003c: supply DOVDD not found, using dummy regulator
[    2.425089] ov5640 1-003c: supply AVDD not found, using dummy regulator
[    2.431758] ov5640 1-003c: supply DVDD not found, using dummy regulator
[    2.438406] ov5640_check_chip_id>
[    2.441724] ov5640_set_power_on>
[    2.444996] ov5640_reset>
[    2.447664] ov5640 1-003c: ov5640_read_reg: error: reg=300a
[    2.453241] ov5640 1-003c: ov5640_check_chip_id: failed to read
chip identifier
[    2.460548] ov5640_set_power_off>
[    2.464096] ov5640_probe>
[    2.466755] ov5640_get_regulators>
[    2.470159] ov5640 3-003c: supply DOVDD not found, using dummy regulator
[    2.476917] ov5640 3-003c: supply AVDD not found, using dummy regulator
[    2.483588] ov5640 3-003c: supply DVDD not found, using dummy regulator
[    2.490240] ov5640_check_chip_id>
[    2.493548] ov5640_set_power_on>
[    2.496805] ov5640_reset>
[    2.499705] ov5640_set_power_off>
[    2.503033] ov5640_init_controls>
[    2.506342] ov5640_calc_pixel_rate>
[    2.511902] ov5640_enum_mbus_code>
[    2.515297] ov5640_enum_mbus_code>
[    2.518826] ov5640_get_fmt>
[    4.381930] ov5640_s_power>
[    4.384725] ov5640_set_power>
[    4.387687] ov5640_set_power_on>
[    4.391301] ov5640_reset>
[    4.393920] ov5640_restore_mode>
[    4.397142] ov5640_load_regs>
[    4.750263] ov5640_set_timings>
[    4.754620] ov5640_set_mode>
[    4.758008] ov5640_calc_pixel_rate>
[    4.761513] ov5640_set_dvp_pclk>
[    4.764734] ov5640_calc_pclk>
[    4.767701] ov5640_calc_sys_clk>
[    4.772239] ov5640_set_mode_direct>
[    4.775720] ov5640_load_regs>
[    4.783195] ov5640_set_timings>
[    4.791443] ov5640_set_framefmt>
[    4.795659] ov5640_set_power_dvp>
[    4.799426] ov5640_s_ctrl>
[    4.802158] ov5640_set_ctrl_white_balance>
[    4.806510] ov5640_s_ctrl>
[    4.809210] ov5640_set_ctrl_exposure>
[    4.813140] ov5640_s_ctrl>
[    4.815840] ov5640_set_ctrl_gain>
[    4.819415] ov5640_s_ctrl>
[    4.822129] ov5640_set_ctrl_saturation>
[    4.826449] ov5640_s_ctrl>
[    4.829149] ov5640_set_ctrl_hue>
[    4.832646] ov5640_s_ctrl>
[    4.835352] ov5640_set_ctrl_contrast>
[    4.839269] ov5640_s_ctrl>
[    4.842099] ov5640_s_ctrl>
[    4.844800] ov5640_set_ctrl_hflip>
[    4.848455] ov5640_s_ctrl>
[    4.851169] ov5640_set_ctrl_vflip>
[    4.854831] ov5640_s_ctrl>
[    4.857531] ov5640_set_ctrl_light_freq>
[    4.862077] ov5640_s_power>
[    4.864864] ov5640_set_power>
[    4.867824] ov5640_set_power_dvp>
[    4.871625] ov5640_set_power_off>
[   36.191661] ov5640_s_power>
[   36.194452] ov5640_set_power>
[   36.197413] ov5640_set_power_on>
[   36.200714] ov5640_reset>
[   36.203328] ov5640_restore_mode>
[   36.206549] ov5640_load_regs>
[   36.550255] ov5640_set_timings>
[   36.554572] ov5640_set_mode>
[   36.557963] ov5640_calc_pixel_rate>
[   36.561458] ov5640_set_dvp_pclk>
[   36.564679] ov5640_calc_pclk>
[   36.567639] ov5640_calc_sys_clk>
[   36.572190] ov5640_set_mode_direct>
[   36.575671] ov5640_load_regs>
[   36.583205] ov5640_set_timings>
[   36.591494] ov5640_set_framefmt>
[   36.595717] ov5640_set_power_dvp>
[   36.599486] ov5640_s_ctrl>
[   36.602200] ov5640_set_ctrl_white_balance>
[   36.606550] ov5640_s_ctrl>
[   36.609250] ov5640_set_ctrl_exposure>
[   36.613179] ov5640_s_ctrl>
[   36.615878] ov5640_set_ctrl_gain>
[   36.619446] ov5640_s_ctrl>
[   36.622160] ov5640_set_ctrl_saturation>
[   36.626476] ov5640_s_ctrl>
[   36.629177] ov5640_set_ctrl_hue>
[   36.632670] ov5640_s_ctrl>
[   36.635370] ov5640_set_ctrl_contrast>
[   36.639282] ov5640_s_ctrl>
[   36.642112] ov5640_s_ctrl>
[   36.644813] ov5640_set_ctrl_hflip>
[   36.648465] ov5640_s_ctrl>
[   36.651179] ov5640_set_ctrl_vflip>
[   36.654833] ov5640_s_ctrl>
[   36.657533] ov5640_set_ctrl_light_freq>
[   36.662120] ov5640_set_fmt>
[   36.670491] ov5640_try_fmt_internal>
[   36.679593] ov5640_find_mode>
[   36.683428] ov5640_calc_pixel_rate>
[   36.696456] ov5640_s_stream>
[   36.703716] ov5640_set_stream_dvp>
[   36.900255] ov5640_s_stream>
[   36.903130] ov5640_set_stream_dvp>
[   36.907351] ov5640_s_power>
[   36.915167] ov5640_set_power>
[   36.920979] ov5640_set_power_dvp>
[   36.924765] ov5640_set_power_off>
root@iwg21m:~#
root@iwg21m:~#
root@iwg21m:~# dmesg --clear
root@iwg21m:~#
root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
[   72.934594] ov5640_s_power>
[   72.937385] ov5640_set_power>
[   72.940385] ov5640_set_power_on>
[   72.943658] ov5640_reset>
[   72.946273] ov5640_restore_mode>
[   72.949493] ov5640_load_regs>
[   73.290250] ov5640_set_timings>
[   73.294578] ov5640_set_mode>
[   73.297974] ov5640_calc_pixel_rate>
[   73.301470] ov5640_set_dvp_pclk>
[   73.304689] ov5640_calc_pclk>
[   73.307649] ov5640_calc_sys_clk>
[   73.312190] ov5640_set_mode_direct>
[   73.315671] ov5640_load_regs>
[   73.323202] ov5640_set_timings>
[   73.331533] ov5640_set_framefmt>
[   73.335765] ov5640_set_power_dvp>
[   73.339539] ov5640_s_ctrl>
[   73.342254] ov5640_set_ctrl_white_balance>
[   73.346605] ov5640_s_ctrl>
[   73.349305] ov5640_set_ctrl_exposure>
[   73.353234] ov5640_s_ctrl>
[   73.355934] ov5640_set_ctrl_gain>
[   73.359503] ov5640_s_ctrl>
[   73.362217] ov5640_set_ctrl_saturation>
[   73.366537] ov5640_s_ctrl>
[   73.369237] ov5640_set_ctrl_hue>
[   73.372733] ov5640_s_ctrl>
[   73.375434] ov5640_set_ctrl_contrast>
[   73.379349] ov5640_s_ctrl>
[   73.382180] ov5640_s_ctrl>
[   73.384888] ov5640_set_ctrl_hflip>
[   73.388543] ov5640_s_ctrl>
[   73.391257] ov5640_set_ctrl_vflip>
[   73.394914] ov5640_s_ctrl>
[   73.397615] ov5640_set_ctrl_light_freq>
Device /dev/video0 opened.
Device `R_Car_VIN' on `platform:e6ef3[   73.402191] ov5640_set_fmt>
000.video' (driver 'rcar_vin') supports video, capture, without [
73.410568] ov5640_try_fmt_internal>
mplanes.
[   73.419661] ov5640_find_mode>
[   73.423489] ov5640_calc_pixel_rate>
Video format set: UYVY (59565955) 640x480 (stride 1280) field none
buffer size 614400
Video for[   73.430741] ov5640_s_stream>
mat: UYVY (59565955) 640x480 (stride 1280) field none buffer siz[
73.438227] ov5640_set_stream_dvp>
e 614400
4 buffers requested.
length: 614400 offset: 0 timestamp type/source: mono/EoF
Buffer 0/0 mapped at address 0xb6b7e000.
length: 614400 offset: 614400 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xb6ae8000.
length: 614400 offset: 1228800 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xb6a52000.
length: 614400 offset: 1843200 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xb69bc000.
0 (0) [-] none 0 614400 B 73.544608 73.544626 10.257 fps ts mono/EoF
[   73.670256] ov5640_s_stream>
[   73.673132] ov5640_set_stream_dvp>
Captured 1 frames in 0.097510 seconds (10.255285 fps, 6300846.98[
73.677350] ov5640_s_power>
3972 B/s).
4 buffers released.
[   73.685162] ov5640_set_power>
[   73.690979] ov5640_set_power_dvp>
[   73.694757] ov5640_set_power_off>
root@iwg21m:~#
root@iwg21m:~#
root@iwg21m:~#

Below is the log without the series applied in DVP mode:

root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
[   45.262397] ov5640_s_power>
[   45.265189] ov5640_set_power>
[   45.268150] ov5640_set_power_on>
[   45.271455] ov5640_reset>
[   45.274071] ov5640_restore_mode>
[   45.621705] ov5640_set_mode>
[   45.625104] ov5640_calc_pixel_rate>
[   45.629916] ov5640_set_mode_direct>
[   45.640983] ov5640_get_sysclk>
[   45.646295] ov5640_set_framefmt>
[   45.650537] ov5640_s_ctrl>
[   45.653237] ov5640_set_ctrl_white_balance>
[   45.657588] ov5640_s_ctrl>
[   45.660289] ov5640_set_ctrl_exposure>
[   45.664217] ov5640_s_ctrl>
[   45.666918] ov5640_set_ctrl_gain>
[   45.670483] ov5640_s_ctrl>
[   45.673201] ov5640_set_ctrl_saturation>
[   45.677523] ov5640_s_ctrl>
[   45.680223] ov5640_set_ctrl_hue>
[   45.683721] ov5640_s_ctrl>
[   45.686684] ov5640_s_ctrl>
[   45.689501] ov5640_s_ctrl>
[   45.692213] ov5640_set_ctrl_hflip>
[   45.695870] ov5640_s_ctrl>
[   45.698570] ov5640_set_ctrl_vflip>
[   45.702240] ov5640_s_ctrl>
[   45.704940] ov5640_set_ctrl_light_freq>
Device /dev/video0 opened.
Device `R_Car_VIN' on `platform:e6ef1[   45.709474] ov5640_set_fmt>
000.video' (driver 'rcar_vin') supports video, capture, without [
45.717852] ov5640_try_fmt_internal>
mplanes.
[   45.726964] ov5640_find_mode>
[   45.730785] ov5640_calc_pixel_rate>
Video format set: UYVY (59565955) 640x480 (stride 1280) field none
buffer size 614400
Video format: UYVY (59565955) 640x480 (stride 1280) field none buffer
siz[   45.743733] ov5640_s_stream>
e 614400
4 buffers requested.
length: 614400 offset: 0 timesta[   45.751076] ov5640_set_stream_dvp>
mp type/source: mono/EoF
Buffer 0/0 mapped at address 0xb6b61000.
length: 614400 offset: 614400 timestamp type/source: mono/EoF
Buffer 1/0 mapped at address 0xb6acb000.
length: 614400 offset: 1228800 timestamp type/source: mono/EoF
Buffer 2/0 mapped at address 0xb6a35000.
length: 614400 offset: 1843200 timestamp type/source: mono/EoF
Buffer 3/0 mapped at address 0xb699f000.
^C[   57.048902] ov5640_s_stream>
[   57.051800] ov5640_set_stream_dvp>
[   57.056180] ov5640_s_power>
[   57.058967] ov5640_set_power>
[   57.061943] ov5640_set_power_off>

root@iwg21m:~#




root@iwg21m:~# dmesg | grep ov564
[    2.412449] ov5640 1-003c: supply DOVDD not found, using dummy regulator
[    2.419202] ov5640 1-003c: supply AVDD not found, using dummy regulator
[    2.425872] ov5640 1-003c: supply DVDD not found, using dummy regulator
[    2.432527] ov5640_set_power_on>
[    2.435801] ov5640_reset>
[    2.438704] ov5640_set_power_off>
[    2.442037] ov5640_init_controls>
[    2.445346] ov5640_calc_pixel_rate>
[    2.450955] ov5640_enum_mbus_code>
[    2.454350] ov5640_enum_mbus_code>
[    2.457882] ov5640_get_fmt>
[    4.222662] ov5640_s_power>
[    4.225458] ov5640_set_power>
[    4.228420] ov5640_set_power_on>
[    4.231737] ov5640_reset>
[    4.234356] ov5640_restore_mode>
[    4.591733] ov5640_set_mode>
[    4.595133] ov5640_calc_pixel_rate>
[    4.599930] ov5640_set_mode_direct>
[    4.610913] ov5640_get_sysclk>
[    4.616195] ov5640_set_framefmt>
[    4.620424] ov5640_s_ctrl>
[    4.623136] ov5640_set_ctrl_white_balance>
[    4.627485] ov5640_s_ctrl>
[    4.630185] ov5640_set_ctrl_exposure>
[    4.634115] ov5640_s_ctrl>
[    4.636815] ov5640_set_ctrl_gain>
[    4.640382] ov5640_s_ctrl>
[    4.643095] ov5640_set_ctrl_saturation>
[    4.647412] ov5640_s_ctrl>
[    4.650112] ov5640_set_ctrl_hue>
[    4.653612] ov5640_s_ctrl>
[    4.656573] ov5640_s_ctrl>
[    4.659389] ov5640_s_ctrl>
[    4.662105] ov5640_set_ctrl_hflip>
[    4.665760] ov5640_s_ctrl>
[    4.668460] ov5640_set_ctrl_vflip>
[    4.672126] ov5640_s_ctrl>
[    4.674826] ov5640_set_ctrl_light_freq>
[    4.679405] ov5640_s_power>
[    4.682211] ov5640_set_power>
[    4.685171] ov5640_set_power_off>
[   45.262397] ov5640_s_power>
[   45.265189] ov5640_set_power>
[   45.268150] ov5640_set_power_on>
[   45.271455] ov5640_reset>
[   45.274071] ov5640_restore_mode>
[   45.621705] ov5640_set_mode>
[   45.625104] ov5640_calc_pixel_rate>
[   45.629916] ov5640_set_mode_direct>
[   45.640983] ov5640_get_sysclk>
[   45.646295] ov5640_set_framefmt>
[   45.650537] ov5640_s_ctrl>
[   45.653237] ov5640_set_ctrl_white_balance>
[   45.657588] ov5640_s_ctrl>
[   45.660289] ov5640_set_ctrl_exposure>
[   45.664217] ov5640_s_ctrl>
[   45.666918] ov5640_set_ctrl_gain>
[   45.670483] ov5640_s_ctrl>
[   45.673201] ov5640_set_ctrl_saturation>
[   45.677523] ov5640_s_ctrl>
[   45.680223] ov5640_set_ctrl_hue>
[   45.683721] ov5640_s_ctrl>
[   45.686684] ov5640_s_ctrl>
[   45.689501] ov5640_s_ctrl>
[   45.692213] ov5640_set_ctrl_hflip>
[   45.695870] ov5640_s_ctrl>
[   45.698570] ov5640_set_ctrl_vflip>
[   45.702240] ov5640_s_ctrl>
[   45.704940] ov5640_set_ctrl_light_freq>
[   45.709474] ov5640_set_fmt>
[   45.717852] ov5640_try_fmt_internal>
[   45.726964] ov5640_find_mode>
[   45.730785] ov5640_calc_pixel_rate>
[   45.743733] ov5640_s_stream>
[   45.751076] ov5640_set_stream_dvp>
[   57.048902] ov5640_s_stream>
[   57.051800] ov5640_set_stream_dvp>
[   57.056180] ov5640_s_power>
[   57.058967] ov5640_set_power>
[   57.061943] ov5640_set_power_off>

Cheers,
Prabhakar

Re: [PATCH v4 3/6] media: i2c: ov5640: Enable data pins on poweron for DVP mode

[Hugues FRUCHET]

Hi Prabhakar,

I have checked quickly both OK and KO traces an some differences are 
ther that we have to understand, see bottom of this mail.

On 9/7/20 4:35 PM, Lad, Prabhakar wrote:
> Hi Hugues,
> 
> Thank you for the review.
> 
> On Mon, Sep 7, 2020 at 10:44 AM Hugues FRUCHET <hugues.fruchet@st.com> wrote:
>>
>> Hi Prabhakar,
>>
>> Thanks for your patches, good to see one more OV5640 stakeholder
>> upstreaming some fixes/features.
>>
>> I'm also using a parallel setup with OV5640 connected on STM32 DCMI
>> camera interface.
>> First basic tests have not shown any regressions on my side but I would
>> like to better understand the problem you encountered and the way you
>> solve it, see below my comments.
>>
>>
> Thank you for testing the patches.
> 
>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>> During testing this sensor on iW-RainboW-G21D-Qseven platform in 8-bit DVP
>>> mode with rcar-vin bridge noticed the capture worked fine for the first run
>>> (with yavta), but for subsequent runs the bridge driver waited for the
>>> frame to be captured. Debugging further noticed the data lines were
>>> enabled/disabled in stream on/off callback and dumping the register
>>> contents 0x3017/0x3018 in ov5640_set_stream_dvp() reported the correct
>>> values, but yet frame capturing failed.
>>
>> Could you show the sequence of V4L2 calls which lead to freeze ?
>>
>> Reading the patch you proposed, my guess is that issue is coming when
>> multiple S_STREAM(on)/S_STREAM(off) are made while power remains, is
>> that true ?

So reading your traces, we are not in that case...

>> I have added some traces in code and tried to reproduce with yavta,
>> v4l2-ctl and GStreamer but I'm not able to generate such sequence, here
>> is what I got everytime:
>>
>> [  809.113790] ov5640 0-003c: ov5640_s_power>
>> [  809.116431] ov5640 0-003c: ov5640_set_power>
>> [  809.120788] ov5640 0-003c: ov5640_set_power_on>
>> [  809.622047] ov5640 0-003c: ov5640_set_power_dvp>
>> [  809.862734] ov5640 0-003c: ov5640_s_stream>
>> [  809.865462] ov5640 0-003c: ov5640_set_stream_dvp on>
>> <capturing here>
>> [  828.549531] ov5640 0-003c: ov5640_s_stream>
>> [  828.552265] ov5640 0-003c: ov5640_set_stream_dvp off>
>> [  828.580970] ov5640 0-003c: ov5640_s_power>
>> [  828.583613] ov5640 0-003c: ov5640_set_power>
>> [  828.587921] ov5640 0-003c: ov5640_set_power_dvp>
>> [  828.620346] ov5640 0-003c: ov5640_set_power_off>
>>
>> Which application/command line are you using to reproduce your problem ?
>>
> yavta.
>>
>>>
>>> To get around this issue data lines are enabled in s_power callback.
>>> (Also the sensor remains in power down mode if not streaming so power
>>> consumption shouldn't be affected)
>>
>> For the time being, I really don't understand why this patch is fixing
>> capture freeze.
>>
> 
> Below is the log with this series applied in DVP mode:
> 
> root@iwg21m:~#
> root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
> [   36.191661] ov5640_s_power>
> [   36.194452] ov5640_set_power>
> [   36.197413] ov5640_set_power_on>
> [   36.200714] ov5640_reset>
> [   36.203328] ov5640_restore_mode>
> [   36.206549] ov5640_load_regs>
> [   36.550255] ov5640_set_timings>
> [   36.554572] ov5640_set_mode>
> [   36.557963] ov5640_calc_pixel_rate>
> [   36.561458] ov5640_set_dvp_pclk>
> [   36.564679] ov5640_calc_pclk>
> [   36.567639] ov5640_calc_sys_clk>
> [   36.572190] ov5640_set_mode_direct>
> [   36.575671] ov5640_load_regs>
> [   36.583205] ov5640_set_timings>
> [   36.591494] ov5640_set_framefmt>
> [   36.595717] ov5640_set_power_dvp>
> [   36.599486] ov5640_s_ctrl>
> [   36.602200] ov5640_set_ctrl_white_balance>
> [   36.606550] ov5640_s_ctrl>
> [   36.609250] ov5640_set_ctrl_exposure>
> [   36.613179] ov5640_s_ctrl>
> [   36.615878] ov5640_set_ctrl_gain>
> [   36.619446] ov5640_s_ctrl>
> [   36.622160] ov5640_set_ctrl_saturation>
> [   36.626476] ov5640_s_ctrl>
> [   36.629177] ov5640_set_ctrl_hue>
> [   36.632670] ov5640_s_ctrl>
> [   36.635370] ov5640_set_ctrl_contrast>
> [   36.639282] ov5640_s_ctrl>
> [   36.642112] ov5640_s_ctrl>
> [   36.644813] ov5640_set_ctrl_hflip>
> [   36.648465] ov5640_s_ctrl>
> [   36.651179] ov5640_set_ctrl_vflip>
> [   36.654833] ov5640_s_ctrl>
> [   36.657533] ov5640_set_ctrl_light_freq>
> Device /dev/video0 opened.
> Device `R_Car_VIN' on `platform:e6ef3[   36.662120] ov5640_set_fmt>
> 000.video' (driver 'rcar_vin') supports video, capture, without [
> 36.670491] ov5640_try_fmt_internal>
> mplanes.
> [   36.679593] ov5640_find_mode>
> [   36.683428] ov5640_calc_pixel_rate>
> Video format set: UYVY (59565955) 640x480 (stride 1280) field none
> buffer size 614400
> Video format: UYVY (59565955) 640x480 (stride 1280) field none buffer
> siz[   36.696456] ov5640_s_stream>
> e 614400
> 4 buffers requested.
> length: 614400 offset: 0 timesta[   36.703716] ov5640_set_stream_dvp>
> mp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb6b4c000.
> length: 614400 offset: 614400 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xb6ab6000.
> length: 614400 offset: 1228800 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xb6a20000.
> length: 614400 offset: 1843200 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xb698a000.
> 0 (0) [-] none 0 614400 B 36.776928 36.776946 15.545 fps ts mono/EoF
> [   36.900255] ov5640_s_stream>
> [   36.903130] ov5640_set_stream_dvp>
> Captured 1 frames in 0.064348 seconds (15.540378 fps, 9548008.11[
> 36.907351] ov5640_s_power>
> 2077 B/s).
> 4 buffers released.
> [   36.915167] ov5640_set_power>
> [   36.920979] ov5640_set_power_dvp>
> [   36.924765] ov5640_set_power_off>
> root@iwg21m:~#
> root@iwg21m:~#
> root@iwg21m:~# dmesg | grep ov5640
> [    2.412247] ov5640_probe>
> [    2.414913] ov5640_get_regulators>
> [    2.418320] ov5640 1-003c: supply DOVDD not found, using dummy regulator
> [    2.425089] ov5640 1-003c: supply AVDD not found, using dummy regulator
> [    2.431758] ov5640 1-003c: supply DVDD not found, using dummy regulator
> [    2.438406] ov5640_check_chip_id>
> [    2.441724] ov5640_set_power_on>
> [    2.444996] ov5640_reset>
> [    2.447664] ov5640 1-003c: ov5640_read_reg: error: reg=300a
> [    2.453241] ov5640 1-003c: ov5640_check_chip_id: failed to read
> chip identifier
> [    2.460548] ov5640_set_power_off>
> [    2.464096] ov5640_probe>
> [    2.466755] ov5640_get_regulators>
> [    2.470159] ov5640 3-003c: supply DOVDD not found, using dummy regulator
> [    2.476917] ov5640 3-003c: supply AVDD not found, using dummy regulator
> [    2.483588] ov5640 3-003c: supply DVDD not found, using dummy regulator
> [    2.490240] ov5640_check_chip_id>
> [    2.493548] ov5640_set_power_on>
> [    2.496805] ov5640_reset>
> [    2.499705] ov5640_set_power_off>
> [    2.503033] ov5640_init_controls>
> [    2.506342] ov5640_calc_pixel_rate>
> [    2.511902] ov5640_enum_mbus_code>
> [    2.515297] ov5640_enum_mbus_code>
> [    2.518826] ov5640_get_fmt>
> [    4.381930] ov5640_s_power>
> [    4.384725] ov5640_set_power>
> [    4.387687] ov5640_set_power_on>
> [    4.391301] ov5640_reset>
> [    4.393920] ov5640_restore_mode>
> [    4.397142] ov5640_load_regs>
> [    4.750263] ov5640_set_timings>
> [    4.754620] ov5640_set_mode>
> [    4.758008] ov5640_calc_pixel_rate>
> [    4.761513] ov5640_set_dvp_pclk>
> [    4.764734] ov5640_calc_pclk>
> [    4.767701] ov5640_calc_sys_clk>
> [    4.772239] ov5640_set_mode_direct>
> [    4.775720] ov5640_load_regs>
> [    4.783195] ov5640_set_timings>
> [    4.791443] ov5640_set_framefmt>
> [    4.795659] ov5640_set_power_dvp>
> [    4.799426] ov5640_s_ctrl>
> [    4.802158] ov5640_set_ctrl_white_balance>
> [    4.806510] ov5640_s_ctrl>
> [    4.809210] ov5640_set_ctrl_exposure>
> [    4.813140] ov5640_s_ctrl>
> [    4.815840] ov5640_set_ctrl_gain>
> [    4.819415] ov5640_s_ctrl>
> [    4.822129] ov5640_set_ctrl_saturation>
> [    4.826449] ov5640_s_ctrl>
> [    4.829149] ov5640_set_ctrl_hue>
> [    4.832646] ov5640_s_ctrl>
> [    4.835352] ov5640_set_ctrl_contrast>
> [    4.839269] ov5640_s_ctrl>
> [    4.842099] ov5640_s_ctrl>
> [    4.844800] ov5640_set_ctrl_hflip>
> [    4.848455] ov5640_s_ctrl>
> [    4.851169] ov5640_set_ctrl_vflip>
> [    4.854831] ov5640_s_ctrl>
> [    4.857531] ov5640_set_ctrl_light_freq>
> [    4.862077] ov5640_s_power>
> [    4.864864] ov5640_set_power>
> [    4.867824] ov5640_set_power_dvp>
> [    4.871625] ov5640_set_power_off>
> [   36.191661] ov5640_s_power>
> [   36.194452] ov5640_set_power>
> [   36.197413] ov5640_set_power_on>
> [   36.200714] ov5640_reset>
> [   36.203328] ov5640_restore_mode>
> [   36.206549] ov5640_load_regs>
> [   36.550255] ov5640_set_timings>
> [   36.554572] ov5640_set_mode>
> [   36.557963] ov5640_calc_pixel_rate>
> [   36.561458] ov5640_set_dvp_pclk>
> [   36.564679] ov5640_calc_pclk>
> [   36.567639] ov5640_calc_sys_clk>
> [   36.572190] ov5640_set_mode_direct>
> [   36.575671] ov5640_load_regs>
> [   36.583205] ov5640_set_timings>
> [   36.591494] ov5640_set_framefmt>
> [   36.595717] ov5640_set_power_dvp>
> [   36.599486] ov5640_s_ctrl>
> [   36.602200] ov5640_set_ctrl_white_balance>
> [   36.606550] ov5640_s_ctrl>
> [   36.609250] ov5640_set_ctrl_exposure>
> [   36.613179] ov5640_s_ctrl>
> [   36.615878] ov5640_set_ctrl_gain>
> [   36.619446] ov5640_s_ctrl>
> [   36.622160] ov5640_set_ctrl_saturation>
> [   36.626476] ov5640_s_ctrl>
> [   36.629177] ov5640_set_ctrl_hue>
> [   36.632670] ov5640_s_ctrl>
> [   36.635370] ov5640_set_ctrl_contrast>
> [   36.639282] ov5640_s_ctrl>
> [   36.642112] ov5640_s_ctrl>
> [   36.644813] ov5640_set_ctrl_hflip>
> [   36.648465] ov5640_s_ctrl>
> [   36.651179] ov5640_set_ctrl_vflip>
> [   36.654833] ov5640_s_ctrl>
> [   36.657533] ov5640_set_ctrl_light_freq>
> [   36.662120] ov5640_set_fmt>
> [   36.670491] ov5640_try_fmt_internal>
> [   36.679593] ov5640_find_mode>
> [   36.683428] ov5640_calc_pixel_rate>
> [   36.696456] ov5640_s_stream>
> [   36.703716] ov5640_set_stream_dvp>
> [   36.900255] ov5640_s_stream>
> [   36.903130] ov5640_set_stream_dvp>
> [   36.907351] ov5640_s_power>
> [   36.915167] ov5640_set_power>
> [   36.920979] ov5640_set_power_dvp>
> [   36.924765] ov5640_set_power_off>
> root@iwg21m:~#
> root@iwg21m:~#
> root@iwg21m:~# dmesg --clear
> root@iwg21m:~#
> root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
> [   72.934594] ov5640_s_power>
> [   72.937385] ov5640_set_power>
> [   72.940385] ov5640_set_power_on>
> [   72.943658] ov5640_reset>
> [   72.946273] ov5640_restore_mode>
> [   72.949493] ov5640_load_regs>
> [   73.290250] ov5640_set_timings>
> [   73.294578] ov5640_set_mode>
> [   73.297974] ov5640_calc_pixel_rate>
> [   73.301470] ov5640_set_dvp_pclk>
> [   73.304689] ov5640_calc_pclk>
> [   73.307649] ov5640_calc_sys_clk>
> [   73.312190] ov5640_set_mode_direct>
> [   73.315671] ov5640_load_regs>
> [   73.323202] ov5640_set_timings>
> [   73.331533] ov5640_set_framefmt>
> [   73.335765] ov5640_set_power_dvp>
> [   73.339539] ov5640_s_ctrl>
> [   73.342254] ov5640_set_ctrl_white_balance>
> [   73.346605] ov5640_s_ctrl>
> [   73.349305] ov5640_set_ctrl_exposure>
> [   73.353234] ov5640_s_ctrl>
> [   73.355934] ov5640_set_ctrl_gain>
> [   73.359503] ov5640_s_ctrl>
> [   73.362217] ov5640_set_ctrl_saturation>
> [   73.366537] ov5640_s_ctrl>
> [   73.369237] ov5640_set_ctrl_hue>
> [   73.372733] ov5640_s_ctrl>
> [   73.375434] ov5640_set_ctrl_contrast>
> [   73.379349] ov5640_s_ctrl>
> [   73.382180] ov5640_s_ctrl>
> [   73.384888] ov5640_set_ctrl_hflip>
> [   73.388543] ov5640_s_ctrl>
> [   73.391257] ov5640_set_ctrl_vflip>
> [   73.394914] ov5640_s_ctrl>
> [   73.397615] ov5640_set_ctrl_light_freq>
> Device /dev/video0 opened.
> Device `R_Car_VIN' on `platform:e6ef3[   73.402191] ov5640_set_fmt>
> 000.video' (driver 'rcar_vin') supports video, capture, without [
> 73.410568] ov5640_try_fmt_internal>
> mplanes.
> [   73.419661] ov5640_find_mode>
> [   73.423489] ov5640_calc_pixel_rate>
> Video format set: UYVY (59565955) 640x480 (stride 1280) field none
> buffer size 614400
> Video for[   73.430741] ov5640_s_stream>
> mat: UYVY (59565955) 640x480 (stride 1280) field none buffer siz[
> 73.438227] ov5640_set_stream_dvp>
> e 614400
> 4 buffers requested.
> length: 614400 offset: 0 timestamp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb6b7e000.
> length: 614400 offset: 614400 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xb6ae8000.
> length: 614400 offset: 1228800 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xb6a52000.
> length: 614400 offset: 1843200 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xb69bc000.
> 0 (0) [-] none 0 614400 B 73.544608 73.544626 10.257 fps ts mono/EoF
> [   73.670256] ov5640_s_stream>
> [   73.673132] ov5640_set_stream_dvp>
> Captured 1 frames in 0.097510 seconds (10.255285 fps, 6300846.98[
> 73.677350] ov5640_s_power>
> 3972 B/s).
> 4 buffers released.
> [   73.685162] ov5640_set_power>
> [   73.690979] ov5640_set_power_dvp>
> [   73.694757] ov5640_set_power_off>
> root@iwg21m:~#
> root@iwg21m:~#
> root@iwg21m:~#
> 
> Below is the log without the series applied in DVP mode:
> 
> root@iwg21m:~# ./yavta /dev/video0 -c1 -n3 -s640x480 -fUYVY -Fov.raw
> [   45.262397] ov5640_s_power>
> [   45.265189] ov5640_set_power>
> [   45.268150] ov5640_set_power_on>
> [   45.271455] ov5640_reset>
> [   45.274071] ov5640_restore_mode>
> [   45.621705] ov5640_set_mode>
> [   45.625104] ov5640_calc_pixel_rate>
> [   45.629916] ov5640_set_mode_direct>
> [   45.640983] ov5640_get_sysclk>
> [   45.646295] ov5640_set_framefmt>
> [   45.650537] ov5640_s_ctrl>
> [   45.653237] ov5640_set_ctrl_white_balance>
> [   45.657588] ov5640_s_ctrl>
> [   45.660289] ov5640_set_ctrl_exposure>
> [   45.664217] ov5640_s_ctrl>
> [   45.666918] ov5640_set_ctrl_gain>
> [   45.670483] ov5640_s_ctrl>
> [   45.673201] ov5640_set_ctrl_saturation>
> [   45.677523] ov5640_s_ctrl>
> [   45.680223] ov5640_set_ctrl_hue>
> [   45.683721] ov5640_s_ctrl>
> [   45.686684] ov5640_s_ctrl>
> [   45.689501] ov5640_s_ctrl>
> [   45.692213] ov5640_set_ctrl_hflip>
> [   45.695870] ov5640_s_ctrl>
> [   45.698570] ov5640_set_ctrl_vflip>
> [   45.702240] ov5640_s_ctrl>
> [   45.704940] ov5640_set_ctrl_light_freq>
> Device /dev/video0 opened.
> Device `R_Car_VIN' on `platform:e6ef1[   45.709474] ov5640_set_fmt>
> 000.video' (driver 'rcar_vin') supports video, capture, without [
> 45.717852] ov5640_try_fmt_internal>
> mplanes.
> [   45.726964] ov5640_find_mode>
> [   45.730785] ov5640_calc_pixel_rate>
> Video format set: UYVY (59565955) 640x480 (stride 1280) field none
> buffer size 614400
> Video format: UYVY (59565955) 640x480 (stride 1280) field none buffer
> siz[   45.743733] ov5640_s_stream>
> e 614400
> 4 buffers requested.
> length: 614400 offset: 0 timesta[   45.751076] ov5640_set_stream_dvp>
> mp type/source: mono/EoF
> Buffer 0/0 mapped at address 0xb6b61000.
> length: 614400 offset: 614400 timestamp type/source: mono/EoF
> Buffer 1/0 mapped at address 0xb6acb000.
> length: 614400 offset: 1228800 timestamp type/source: mono/EoF
> Buffer 2/0 mapped at address 0xb6a35000.
> length: 614400 offset: 1843200 timestamp type/source: mono/EoF
> Buffer 3/0 mapped at address 0xb699f000.
> ^C[   57.048902] ov5640_s_stream>
> [   57.051800] ov5640_set_stream_dvp>
> [   57.056180] ov5640_s_power>
> [   57.058967] ov5640_set_power>
> [   57.061943] ov5640_set_power_off>
> 
> root@iwg21m:~#
> 
> 
> 
> 
> root@iwg21m:~# dmesg | grep ov564
> [    2.412449] ov5640 1-003c: supply DOVDD not found, using dummy regulator
> [    2.419202] ov5640 1-003c: supply AVDD not found, using dummy regulator
> [    2.425872] ov5640 1-003c: supply DVDD not found, using dummy regulator
> [    2.432527] ov5640_set_power_on>
> [    2.435801] ov5640_reset>
> [    2.438704] ov5640_set_power_off>
> [    2.442037] ov5640_init_controls>
> [    2.445346] ov5640_calc_pixel_rate>
> [    2.450955] ov5640_enum_mbus_code>
> [    2.454350] ov5640_enum_mbus_code>
> [    2.457882] ov5640_get_fmt>
> [    4.222662] ov5640_s_power>
> [    4.225458] ov5640_set_power>
> [    4.228420] ov5640_set_power_on>
> [    4.231737] ov5640_reset>
> [    4.234356] ov5640_restore_mode>
> [    4.591733] ov5640_set_mode>
> [    4.595133] ov5640_calc_pixel_rate>
> [    4.599930] ov5640_set_mode_direct>
> [    4.610913] ov5640_get_sysclk>
> [    4.616195] ov5640_set_framefmt>
> [    4.620424] ov5640_s_ctrl>
> [    4.623136] ov5640_set_ctrl_white_balance>
> [    4.627485] ov5640_s_ctrl>
> [    4.630185] ov5640_set_ctrl_exposure>
> [    4.634115] ov5640_s_ctrl>
> [    4.636815] ov5640_set_ctrl_gain>
> [    4.640382] ov5640_s_ctrl>
> [    4.643095] ov5640_set_ctrl_saturation>
> [    4.647412] ov5640_s_ctrl>
> [    4.650112] ov5640_set_ctrl_hue>
> [    4.653612] ov5640_s_ctrl>
> [    4.656573] ov5640_s_ctrl>
> [    4.659389] ov5640_s_ctrl>
> [    4.662105] ov5640_set_ctrl_hflip>
> [    4.665760] ov5640_s_ctrl>
> [    4.668460] ov5640_set_ctrl_vflip>
> [    4.672126] ov5640_s_ctrl>
> [    4.674826] ov5640_set_ctrl_light_freq>
> [    4.679405] ov5640_s_power>
> [    4.682211] ov5640_set_power>
> [    4.685171] ov5640_set_power_off>
> [   45.262397] ov5640_s_power>
> [   45.265189] ov5640_set_power>
> [   45.268150] ov5640_set_power_on>
> [   45.271455] ov5640_reset>
> [   45.274071] ov5640_restore_mode>
> [   45.621705] ov5640_set_mode>
> [   45.625104] ov5640_calc_pixel_rate>
> [   45.629916] ov5640_set_mode_direct>
> [   45.640983] ov5640_get_sysclk>
> [   45.646295] ov5640_set_framefmt>
> [   45.650537] ov5640_s_ctrl>
> [   45.653237] ov5640_set_ctrl_white_balance>
> [   45.657588] ov5640_s_ctrl>
> [   45.660289] ov5640_set_ctrl_exposure>
> [   45.664217] ov5640_s_ctrl>
> [   45.666918] ov5640_set_ctrl_gain>
> [   45.670483] ov5640_s_ctrl>
> [   45.673201] ov5640_set_ctrl_saturation>
> [   45.677523] ov5640_s_ctrl>
> [   45.680223] ov5640_set_ctrl_hue>
> [   45.683721] ov5640_s_ctrl>
> [   45.686684] ov5640_s_ctrl>
> [   45.689501] ov5640_s_ctrl>
> [   45.692213] ov5640_set_ctrl_hflip>
> [   45.695870] ov5640_s_ctrl>
> [   45.698570] ov5640_set_ctrl_vflip>
> [   45.702240] ov5640_s_ctrl>
> [   45.704940] ov5640_set_ctrl_light_freq>
> [   45.709474] ov5640_set_fmt>
> [   45.717852] ov5640_try_fmt_internal>
> [   45.726964] ov5640_find_mode>
> [   45.730785] ov5640_calc_pixel_rate>
> [   45.743733] ov5640_s_stream>
> [   45.751076] ov5640_set_stream_dvp>
> [   57.048902] ov5640_s_stream>
> [   57.051800] ov5640_set_stream_dvp>
> [   57.056180] ov5640_s_power>
> [   57.058967] ov5640_set_power>
> [   57.061943] ov5640_set_power_off>
> 
> Cheers,
> Prabhakar
> 

In order to commpare behaviours, I have taken the KO trace and marked 
with "--" the missing trace points compared to OK trace and with "++" 
the additional traces:

 > [    4.222662] ov5640_s_power>
 > [    4.225458] ov5640_set_power>
 > [    4.228420] ov5640_set_power_on>
 > [    4.231737] ov5640_reset>
 > [    4.234356] ov5640_restore_mode>
-- > [    4.397142] ov5640_load_regs>
-- > [    4.750263] ov5640_set_timings>
==> seems that register table load is not there. If it is a trace issue, 
please add the same trace points in code before modifications.
 > [    4.591733] ov5640_set_mode>
 > [    4.595133] ov5640_calc_pixel_rate>
-- > [    4.761513] ov5640_set_dvp_pclk>
-- > [    4.764734] ov5640_calc_pclk>
-- > [    4.767701] ov5640_calc_sys_clk>
=> missing pixel clock for DVP
 > [    4.599930] ov5640_set_mode_direct>
-- > [    4.397142] ov5640_load_regs>
-- > [    4.750263] ov5640_set_timings>
++ > [    4.610913] ov5640_get_sysclk>
 > [    4.616195] ov5640_set_framefmt>
-- > [    4.795659] ov5640_set_power_dvp>
 > [    4.620424] ov5640_s_ctrl>
 > [    4.623136] ov5640_set_ctrl_white_balance>
 > [    4.627485] ov5640_s_ctrl>
 > [    4.630185] ov5640_set_ctrl_exposure>
 > [    4.634115] ov5640_s_ctrl>
 > [    4.636815] ov5640_set_ctrl_gain>
 > [    4.640382] ov5640_s_ctrl>
 > [    4.643095] ov5640_set_ctrl_saturation>
 > [    4.647412] ov5640_s_ctrl>
 > [    4.650112] ov5640_set_ctrl_hue>
 > [    4.653612] ov5640_s_ctrl>
 > [    4.656573] ov5640_s_ctrl>
 > [    4.659389] ov5640_s_ctrl>
 > [    4.662105] ov5640_set_ctrl_hflip>
 > [    4.665760] ov5640_s_ctrl>
 > [    4.668460] ov5640_set_ctrl_vflip>
 > [    4.672126] ov5640_s_ctrl>
 > [    4.674826] ov5640_set_ctrl_light_freq>
 > [    4.679405] ov5640_s_power>
 > [    4.682211] ov5640_set_power>
-- > [    4.867824] ov5640_set_power_dvp>
 > [    4.685171] ov5640_set_power_off>
 > [   45.262397] ov5640_s_power>
 > [   45.265189] ov5640_set_power>
 > [   45.268150] ov5640_set_power_on>
 > [   45.271455] ov5640_reset>
 > [   45.274071] ov5640_restore_mode>
-- > [   36.206549] ov5640_load_regs>
-- > [   36.550255] ov5640_set_timings>
 > [   45.621705] ov5640_set_mode>
 > [   45.625104] ov5640_calc_pixel_rate>
-- > [   36.561458] ov5640_set_dvp_pclk>
-- > [   36.564679] ov5640_calc_pclk>
-- > [   36.567639] ov5640_calc_sys_clk>
 > [   45.629916] ov5640_set_mode_direct>
-- > [   36.575671] ov5640_load_regs>
-- > [   36.583205] ov5640_set_timings>
++ > [   45.640983] ov5640_get_sysclk>
 > [   45.646295] ov5640_set_framefmt>
-- > [   36.595717] ov5640_set_power_dvp>
 > [   45.650537] ov5640_s_ctrl>
 > [   45.653237] ov5640_set_ctrl_white_balance>
 > [   45.657588] ov5640_s_ctrl>
 > [   45.660289] ov5640_set_ctrl_exposure>
 > [   45.664217] ov5640_s_ctrl>
 > [   45.666918] ov5640_set_ctrl_gain>
 > [   45.670483] ov5640_s_ctrl>
 > [   45.673201] ov5640_set_ctrl_saturation>
 > [   45.677523] ov5640_s_ctrl>
 > [   45.680223] ov5640_set_ctrl_hue>
 > [   45.683721] ov5640_s_ctrl>
 > [   45.686684] ov5640_s_ctrl>
 > [   45.689501] ov5640_s_ctrl>
 > [   45.692213] ov5640_set_ctrl_hflip>
 > [   45.695870] ov5640_s_ctrl>
 > [   45.698570] ov5640_set_ctrl_vflip>
 > [   45.702240] ov5640_s_ctrl>
 > [   45.704940] ov5640_set_ctrl_light_freq>
 > [   45.709474] ov5640_set_fmt>
 > [   45.717852] ov5640_try_fmt_internal>
 > [   45.726964] ov5640_find_mode>
 > [   45.730785] ov5640_calc_pixel_rate>
 > [   45.743733] ov5640_s_stream>
 > [   45.751076] ov5640_set_stream_dvp>
 > [   57.048902] ov5640_s_stream>
 > [   57.051800] ov5640_set_stream_dvp>
 > [   57.056180] ov5640_s_power>
 > [   57.058967] ov5640_set_power>
 > [   57.061943] ov5640_set_power_off>




Best regards,
Hugues.

[PATCH v4 4/6] media: i2c: ov5640: Configure HVP lines in s_power callback

[Lad Prabhakar]

Configure HVP lines in s_power callback instead of configuring everytime
in ov5640_set_stream_dvp().

Alongside also disable MIPI in DVP mode.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 123 +++++++++++++++++--------------------
 1 file changed, 58 insertions(+), 65 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8288728d8704..6834ebc1995f 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1217,71 +1217,6 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
-	int ret;
-	unsigned int flags = sensor->ep.bus.parallel.flags;
-	u8 pclk_pol = 0;
-	u8 hsync_pol = 0;
-	u8 vsync_pol = 0;
-
-	/*
-	 * Note about parallel port configuration.
-	 *
-	 * When configured in parallel mode, the OV5640 will
-	 * output 10 bits data on DVP data lines [9:0].
-	 * If only 8 bits data are wanted, the 8 bits data lines
-	 * of the camera interface must be physically connected
-	 * on the DVP data lines [9:2].
-	 *
-	 * Control lines polarity can be configured through
-	 * devicetree endpoint control lines properties.
-	 * If no endpoint control lines properties are set,
-	 * polarity will be as below:
-	 * - VSYNC:	active high
-	 * - HREF:	active low
-	 * - PCLK:	active low
-	 */
-
-	if (on) {
-		/*
-		 * configure parallel port control lines polarity
-		 *
-		 * POLARITY CTRL0
-		 * - [5]:	PCLK polarity (0: active low, 1: active high)
-		 * - [1]:	HREF polarity (0: active low, 1: active high)
-		 * - [0]:	VSYNC polarity (mismatch here between
-		 *		datasheet and hardware, 0 is active high
-		 *		and 1 is active low...)
-		 */
-		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-			pclk_pol = 1;
-		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-			hsync_pol = 1;
-		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-			vsync_pol = 1;
-
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_POLARITY_CTRL00,
-				       (pclk_pol << 5) |
-				       (hsync_pol << 1) |
-				       vsync_pol);
-
-		if (ret)
-			return ret;
-	}
-
-	/*
-	 * powerdown MIPI TX/RX PHY & disable MIPI
-	 *
-	 * MIPI CONTROL 00
-	 * 4:	 PWDN PHY TX
-	 * 3:	 PWDN PHY RX
-	 * 2:	 MIPI enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
-	if (ret)
-		return ret;
-
 	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
 				OV5640_REG_SYS_CTRL0_SW_PWUP :
 				OV5640_REG_SYS_CTRL0_SW_PWDN);
@@ -2043,15 +1978,73 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
 
 static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
 {
+	unsigned int flags = sensor->ep.bus.parallel.flags;
+	u8 pclk_pol = 0;
+	u8 hsync_pol = 0;
+	u8 vsync_pol = 0;
 	int ret;
 
 	if (!on) {
 		/* Reset settings to their default values. */
+		ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
+		ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
 		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
 		ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00);
 		return 0;
 	}
 
+	/*
+	 * Note about parallel port configuration.
+	 *
+	 * When configured in parallel mode, the OV5640 will
+	 * output 10 bits data on DVP data lines [9:0].
+	 * If only 8 bits data are wanted, the 8 bits data lines
+	 * of the camera interface must be physically connected
+	 * on the DVP data lines [9:2].
+	 *
+	 * Control lines polarity can be configured through
+	 * devicetree endpoint control lines properties.
+	 * If no endpoint control lines properties are set,
+	 * polarity will be as below:
+	 * - VSYNC:	active high
+	 * - HREF:	active low
+	 * - PCLK:	active low
+	 */
+	/*
+	 * configure parallel port control lines polarity
+	 *
+	 * POLARITY CTRL0
+	 * - [5]:	PCLK polarity (0: active low, 1: active high)
+	 * - [1]:	HREF polarity (0: active low, 1: active high)
+	 * - [0]:	VSYNC polarity (mismatch here between
+	 *		datasheet and hardware, 0 is active high
+	 *		and 1 is active low...)
+	 */
+	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+		pclk_pol = 1;
+	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+		hsync_pol = 1;
+	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+		vsync_pol = 1;
+
+	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * powerdown MIPI TX/RX PHY & disable MIPI
+	 *
+	 * MIPI CONTROL 00
+	 * 4:	 PWDN PHY TX
+	 * 3:	 PWDN PHY RX
+	 * 2:	 MIPI enable
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
+	if (ret)
+		return ret;
+
 	/*
 	 * enable VSYNC/HREF/PCLK DVP control lines
 	 * & D[9:6] DVP data lines
-- 
2.17.1

[PATCH v4 5/6] media: i2c: ov5640: Add support for BT656 mode

[Lad Prabhakar]

Enable support for BT656 mode.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 46 ++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 6834ebc1995f..96daeb5d6a27 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -84,6 +84,7 @@
 #define OV5640_REG_VFIFO_HSIZE		0x4602
 #define OV5640_REG_VFIFO_VSIZE		0x4604
 #define OV5640_REG_JPG_MODE_SELECT	0x4713
+#define OV5640_REG_CCIR656_CTRL00	0x4730
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
@@ -1215,6 +1216,20 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 			      BIT(1), on ? 0 : BIT(1));
 }
 
+static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00,
+			       on ? 0x1 : 0x00);
+	if (ret)
+		return ret;
+
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+				OV5640_REG_SYS_CTRL0_SW_PWUP :
+				OV5640_REG_SYS_CTRL0_SW_PWDN);
+}
+
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
 	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
@@ -2020,18 +2035,21 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
 	 *		datasheet and hardware, 0 is active high
 	 *		and 1 is active low...)
 	 */
-	if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-		pclk_pol = 1;
-	if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-		hsync_pol = 1;
-	if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-		vsync_pol = 1;
+	if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
+		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+			pclk_pol = 1;
+		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+			hsync_pol = 1;
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+			vsync_pol = 1;
+
+		ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+				       (pclk_pol << 5) | (hsync_pol << 1) |
+				       vsync_pol);
 
-	ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
-			       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
-
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 
 	/*
 	 * powerdown MIPI TX/RX PHY & disable MIPI
@@ -2055,7 +2073,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
 	 * - 4:		PCLK output enable
 	 * - [3:0]:	D[9:6] output enable
 	 */
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
+			       sensor->ep.bus_type == V4L2_MBUS_PARALLEL ?
+			       0x7f : 0x1f);
 	if (ret)
 		return ret;
 
@@ -2905,6 +2925,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
 			ret = ov5640_set_stream_mipi(sensor, enable);
+		else if (sensor->ep.bus_type == V4L2_MBUS_BT656)
+			ret = ov5640_set_stream_bt656(sensor, enable);
 		else
 			ret = ov5640_set_stream_dvp(sensor, enable);
 
-- 
2.17.1

[PATCH v4 6/6] media: i2c: ov5640: Fail probe on unsupported bus_type

[Lad Prabhakar]

Fail probe if unsupported bus_type is detected.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 96daeb5d6a27..ce39abf41b48 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -3072,6 +3072,13 @@ static int ov5640_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	if (sensor->ep.bus_type != V4L2_MBUS_PARALLEL &&
+	    sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY &&
+	    sensor->ep.bus_type != V4L2_MBUS_BT656) {
+		dev_err(dev, "Unsupported bus type %d\n", sensor->ep.bus_type);
+		return -EINVAL;
+	}
+
 	/* get system clock (xclk) */
 	sensor->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(sensor->xclk)) {
-- 
2.17.1