On 12/21/21 5:11 PM, Lad, Prabhakar wrote: > Hi Eugen, > > On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@xxxxxxxxxxxxx> 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@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. >>>> >>> 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 >
