Hi Hugues, Eugen, On Tue, Dec 21, 2021 at 07:37:47AM +0000, Eugen.Hristev@xxxxxxxxxxxxx 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@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. 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
