Message-ID: <450850f6-9296-e505-4b92-c71ed190b95f@microchip.com>
Date:   Tue, 21 Dec 2021 15:01:57 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <prabhakar.csengg@...il.com>
CC:     <hugues.fruchet@...com>, <prabhakar.mahadev-lad.rj@...renesas.com>,
        <jacopo+renesas@...ndi.org>, <sakari.ailus@...ux.intel.com>,
        <laurent.pinchart@...asonboard.com>, <slongerbeam@...il.com>,
        <paul.kocialkowski@...tlin.com>, <linux-media@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-renesas-soc@...r.kernel.org>, <biju.das.jz@...renesas.com>,
        <mchehab@...nel.org>
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP
 mode unless streaming

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@...rochip.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@...renesas.com>
>>>>> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
>>>>> Tested-by: Jacopo Mondi <jacopo@...ndi.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
> 

Powered by blists - more mailing lists