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@xxxxxxxxxxxxxx>
>> Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>> Tested-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>> ---
>> 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
