linux-kernel - Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 21 Dec 2021 07:37:47 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <hugues.fruchet@...com>, <prabhakar.mahadev-lad.rj@...renesas.com>,
        <jacopo+renesas@...ndi.org>, <sakari.ailus@...ux.intel.com>,
        <laurent.pinchart@...asonboard.com>, <slongerbeam@...il.com>,
        <paul.kocialkowski@...tlin.com>
CC:     <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-renesas-soc@...r.kernel.org>, <biju.das.jz@...renesas.com>,
        <prabhakar.csengg@...il.com>, <mchehab@...nel.org>
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP
 mode unless streaming

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@...renesas.com>
>>> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
>>> Tested-by: Jacopo Mondi <jacopo@...ndi.org>
>>> ---
>>>     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