Hi Eugen, On Mon, Jan 3, 2022 at 11:29 AM <Eugen.Hristev@xxxxxxxxxxxxx> wrote: > > 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. > That's strange. > 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. > you mean yavta ? > Which of the init configuration set of registers your test is using? I have been testing 320x240 and 640x480. Could you give that a try please? > 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. > Let me check if I can ping OmniVision FAE. > 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
