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@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 > Gentle ping. I would like to send a revert patch if nobody cares about this driver/patch except me. Thanks, Eugen
