[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 9 Sep 2020 16:16:01 +0000
From: Hugues FRUCHET <hugues.fruchet@...com>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Jacopo Mondi <jacopo+renesas@...ndi.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Steve Longerbeam <slongerbeam@...il.com>,
Paul <paul.kocialkowski@...tlin.com>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-renesas-soc@...r.kernel.org"
<linux-renesas-soc@...r.kernel.org>,
"Biju Das" <biju.das.jz@...renesas.com>,
Prabhakar <prabhakar.csengg@...il.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP
mode unless streaming
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.
Powered by blists - more mailing lists