Message ID | 20221227173634.5752-4-j-luthra@ti.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Sakari Ailus |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1pADt3-006vkG-IF; Tue, 27 Dec 2022 17:37:13 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231915AbiL0RhD (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 27 Dec 2022 12:37:03 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45858 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230475AbiL0Rg6 (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 27 Dec 2022 12:36:58 -0500 Received: from lelv0143.ext.ti.com (lelv0143.ext.ti.com [198.47.23.248]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5A84560C5 for <linux-media@vger.kernel.org>; Tue, 27 Dec 2022 09:36:57 -0800 (PST) Received: from fllv0034.itg.ti.com ([10.64.40.246]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id 2BRHaoxn120891; Tue, 27 Dec 2022 11:36:50 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1672162610; bh=YnLsop6rYJk8qMc1mj3i5VfDFfWGjlUUI6QMnfrsnB0=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=v93Ev/uJcOGsQiHjhbgPpwVfWoI4PJE+ZFjrYWSLY2etuST5KfuDg03pH5d/cal8n ZzEH1ZBrDXkbOWU9edPaVHkFe/jP91qUOEAMGzrqMylfXug561IzBdbKKokpalfJq0 eLYfHJCTShkdhLgEUPfiToDvpy+Npc9Y0rGcivRc= Received: from DLEE100.ent.ti.com (dlee100.ent.ti.com [157.170.170.30]) by fllv0034.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 2BRHao8j007937 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 27 Dec 2022 11:36:50 -0600 Received: from DLEE110.ent.ti.com (157.170.170.21) by DLEE100.ent.ti.com (157.170.170.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16; Tue, 27 Dec 2022 11:36:50 -0600 Received: from lelv0327.itg.ti.com (10.180.67.183) by DLEE110.ent.ti.com (157.170.170.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16 via Frontend Transport; Tue, 27 Dec 2022 11:36:50 -0600 Received: from localhost (ileaxei01-snat2.itg.ti.com [10.180.69.6]) by lelv0327.itg.ti.com (8.15.2/8.15.2) with ESMTP id 2BRHanWo019342; Tue, 27 Dec 2022 11:36:49 -0600 From: Jai Luthra <j-luthra@ti.com> To: Steve Longerbeam <slongerbeam@gmail.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, Jacopo Mondi <jacopo.mondi@ideasonboard.com> CC: <linux-media@vger.kernel.org>, Nishanth Menon <nm@ti.com>, Jai Luthra <j-luthra@ti.com> Subject: [PATCH v3 3/3] media: ov5640: Honor power on time in init_setting Date: Tue, 27 Dec 2022 23:06:34 +0530 Message-ID: <20221227173634.5752-4-j-luthra@ti.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20221227173634.5752-1-j-luthra@ti.com> References: <20221227173634.5752-1-j-luthra@ti.com> MIME-Version: 1.0 Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
media: ov5640: Fix power up sequence delays
|
|
Commit Message
Jai Luthra
Dec. 27, 2022, 5:36 p.m. UTC
From: Nishanth Menon <nm@ti.com> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences that is expected during various initialization steps. Note the power on time includes t0 + t1 + t2 >= 5ms, delay for poweron. As indicated in section 2.8, the PWDN assertion can either be via external pin control OR via the register 0x3008 bit 6 (see table 7-1 in [1]) [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") Signed-off-by: Nishanth Menon <nm@ti.com> Signed-off-by: Jai Luthra <j-luthra@ti.com> --- drivers/media/i2c/ov5640.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Comments
Hi Jai On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote: > From: Nishanth Menon <nm@ti.com> > > OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences > that is expected during various initialization steps. Note the power > on time includes t0 + t1 + t2 >= 5ms, delay for poweron. > > As indicated in section 2.8, the PWDN assertion can either be via > external pin control OR via the register 0x3008 bit 6 (see table 7-1 in > [1]) > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > Signed-off-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > drivers/media/i2c/ov5640.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5df16fb6f0a0..bd7cc294cfe6 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { > {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, > {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, > {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, > - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300}, > + {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, > }; > > static const struct reg_value ov5640_setting_low_res[] = { > @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) > > static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > { > - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > - OV5640_REG_SYS_CTRL0_SW_PWUP : > - OV5640_REG_SYS_CTRL0_SW_PWDN); > + int ret; > + > + if (on) { > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWUP); > + usleep_range(5000, 6000); > + } else { > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWDN); > + } > + return ret; Nipicking, I prefer maintaining the original version with just an additional usleep int ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? OV5640_REG_SYS_CTRL0_SW_PWUP : OV5640_REG_SYS_CTRL0_SW_PWDN); if (on) usleep_range(5000, 6000); return ret; As you prefer Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > } > > static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > -- > 2.17.1 >
Hi again On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote: > From: Nishanth Menon <nm@ti.com> > > OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences > that is expected during various initialization steps. Note the power > on time includes t0 + t1 + t2 >= 5ms, delay for poweron. > > As indicated in section 2.8, the PWDN assertion can either be via > external pin control OR via the register 0x3008 bit 6 (see table 7-1 in > [1]) > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > Signed-off-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Jai Luthra <j-luthra@ti.com> > --- > drivers/media/i2c/ov5640.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5df16fb6f0a0..bd7cc294cfe6 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { > {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, > {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, > {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, > - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300}, > + {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, This might also allow to remove the /* remain in power down mode for DVP */ if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && val == OV5640_REG_SYS_CTRL0_SW_PWUP && !ov5640_is_csi2(sensor)) continue; in ov5640_load_regs() Prabhakar since you have introduced it, coudl you test if you still need it with Nishanth's patch applied ? Thanks j > }; > > static const struct reg_value ov5640_setting_low_res[] = { > @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) > > static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > { > - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > - OV5640_REG_SYS_CTRL0_SW_PWUP : > - OV5640_REG_SYS_CTRL0_SW_PWDN); > + int ret; > + > + if (on) { > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWUP); > + usleep_range(5000, 6000); > + } else { > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > + OV5640_REG_SYS_CTRL0_SW_PWDN); > + } > + return ret; > } > > static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > -- > 2.17.1 >
Hello again On Thu, Dec 29, 2022 at 07:10:36PM +0100, Jacopo Mondi wrote: > Hi again > > On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote: > > From: Nishanth Menon <nm@ti.com> > > > > OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences > > that is expected during various initialization steps. Note the power > > on time includes t0 + t1 + t2 >= 5ms, delay for poweron. > > > > As indicated in section 2.8, the PWDN assertion can either be via > > external pin control OR via the register 0x3008 bit 6 (see table 7-1 in > > [1]) > > > > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf > > > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > > Signed-off-by: Nishanth Menon <nm@ti.com> > > Signed-off-by: Jai Luthra <j-luthra@ti.com> > > --- > > drivers/media/i2c/ov5640.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 5df16fb6f0a0..bd7cc294cfe6 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { > > {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, > > {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, > > {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, > > - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300}, > > + {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, > > This might also allow to remove the > > /* remain in power down mode for DVP */ > if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && > val == OV5640_REG_SYS_CTRL0_SW_PWUP && > !ov5640_is_csi2(sensor)) > continue; > > in ov5640_load_regs() > > Prabhakar since you have introduced it, coudl you test if you still > need it with Nishanth's patch applied ? No, it doesn't, sorry for the confusion. The following patch would allow to remove the above quirk by removing {0x3008, 0x02} from the init_sequence[]. Unfortunately the first 3 images are then black when running in CSI-2 mode. ------------------------------------------------------------------------------- diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 96b986051414..bfb60648c72a 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, + {0x3a1f, 0x14, 0, 0}, {0x3c00, 0x04, 0, 300}, }; static const struct reg_value ov5640_setting_low_res[] = { @@ -1808,7 +1808,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) BIT(1), on ? 0 : BIT(1)); } -static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) { int ret; @@ -3725,11 +3725,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) sensor->pending_fmt_change = false; } - if (ov5640_is_csi2(sensor)) + if (ov5640_is_csi2(sensor)) { ret = ov5640_set_stream_mipi(sensor, enable); - else - ret = ov5640_set_stream_dvp(sensor, enable); + if (ret) + goto out; + } + ret = ov5640_set_stream(sensor, enable); if (!ret) sensor->streaming = enable; } ------------------------------------------------------------------------------- I do however now question the patch utility itself. SW PWDN is the software standby state, while Figure 2.3 and 2.4 you mention in the commit message report: t2 = 5 ms: delay from DVDD stable to sensor power up stable I doubt this apply to SW standby as it the delay is probably required to have the internal circuitry stabilize after the power rail has been enabled. Does this patch make any practical difference in your setup ? I'm asking as in my case it doesn't, but I'm on a CSI-2 setup. > > Thanks > j > > > > }; > > > > static const struct reg_value ov5640_setting_low_res[] = { > > @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) > > > > static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > > { > > - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? > > - OV5640_REG_SYS_CTRL0_SW_PWUP : > > - OV5640_REG_SYS_CTRL0_SW_PWDN); > > + int ret; > > + > > + if (on) { > > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > > + OV5640_REG_SYS_CTRL0_SW_PWUP); > > + usleep_range(5000, 6000); > > + } else { > > + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > > + OV5640_REG_SYS_CTRL0_SW_PWDN); > > + } > > + return ret; > > } > > > > static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) > > -- > > 2.17.1 > >
Hi Jacopo, On 02/01/23 18:29, Jacopo Mondi wrote: > Hello again > > On Thu, Dec 29, 2022 at 07:10:36PM +0100, Jacopo Mondi wrote: >> Hi again >> >> On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote: >>> From: Nishanth Menon <nm@ti.com> >>> >>> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences >>> that is expected during various initialization steps. Note the power >>> on time includes t0 + t1 + t2 >= 5ms, delay for poweron. >>> >>> As indicated in section 2.8, the PWDN assertion can either be via >>> external pin control OR via the register 0x3008 bit 6 (see table 7-1 in >>> [1]) >>> >>> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf >>> >>> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") >>> Signed-off-by: Nishanth Menon <nm@ti.com> >>> Signed-off-by: Jai Luthra <j-luthra@ti.com> >>> --- >>> drivers/media/i2c/ov5640.c | 16 ++++++++++++---- >>> 1 file changed, 12 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>> index 5df16fb6f0a0..bd7cc294cfe6 100644 >>> --- a/drivers/media/i2c/ov5640.c >>> +++ b/drivers/media/i2c/ov5640.c >>> @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { >>> {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, >>> {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, >>> {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, >>> - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300}, >>> + {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, >> >> This might also allow to remove the >> >> /* remain in power down mode for DVP */ >> if (regs->reg_addr == OV5640_REG_SYS_CTRL0 && >> val == OV5640_REG_SYS_CTRL0_SW_PWUP && >> !ov5640_is_csi2(sensor)) >> continue; >> >> in ov5640_load_regs() >> >> Prabhakar since you have introduced it, coudl you test if you still >> need it with Nishanth's patch applied ? > > No, it doesn't, sorry for the confusion. > > The following patch would allow to remove the above quirk by removing > {0x3008, 0x02} > from the init_sequence[]. > > Unfortunately the first 3 images are then black when running in CSI-2 > mode. > > ------------------------------------------------------------------------------- > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 96b986051414..bfb60648c72a 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { > {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, > {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, > {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, > - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, > + {0x3a1f, 0x14, 0, 0}, {0x3c00, 0x04, 0, 300}, > }; > > static const struct reg_value ov5640_setting_low_res[] = { > @@ -1808,7 +1808,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) > BIT(1), on ? 0 : BIT(1)); > } > > -static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) > +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on) > { > int ret; > > @@ -3725,11 +3725,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable) > sensor->pending_fmt_change = false; > } > > - if (ov5640_is_csi2(sensor)) > + if (ov5640_is_csi2(sensor)) { > ret = ov5640_set_stream_mipi(sensor, enable); > - else > - ret = ov5640_set_stream_dvp(sensor, enable); > + if (ret) > + goto out; > + } > > + ret = ov5640_set_stream(sensor, enable); > if (!ret) > sensor->streaming = enable; > } > ------------------------------------------------------------------------------- > > I do however now question the patch utility itself. SW PWDN is the > software standby state, while Figure 2.3 and 2.4 you mention in the > commit message report: > > t2 = 5 ms: delay from DVDD stable to sensor power up stable > > I doubt this apply to SW standby as it the delay is probably required > to have the internal circuitry stabilize after the power rail has been > enabled. Good catch. > > Does this patch make any practical difference in your setup ? I'm > asking as in my case it doesn't, but I'm on a CSI-2 setup. I tested today with this patch removed, and it does not make any difference to the probe success rate. We missed this during development as we first tried the SW powerdown and reset timing changes, and later the hardware reset timing changes [1/3]. I will drop this patch in the next series. Thanks for pointing this out. > >> >> Thanks >> j >> >> >>> }; >>> >>> static const struct reg_value ov5640_setting_low_res[] = { >>> @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) >>> >>> static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) >>> { >>> - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? >>> - OV5640_REG_SYS_CTRL0_SW_PWUP : >>> - OV5640_REG_SYS_CTRL0_SW_PWDN); >>> + int ret; >>> + >>> + if (on) { >>> + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, >>> + OV5640_REG_SYS_CTRL0_SW_PWUP); >>> + usleep_range(5000, 6000); >>> + } else { >>> + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, >>> + OV5640_REG_SYS_CTRL0_SW_PWDN); >>> + } >>> + return ret; >>> } >>> >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on) >>> -- >>> 2.17.1 >>> Thanks, Jai
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5df16fb6f0a0..bd7cc294cfe6 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = { {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0}, {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0}, {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0}, - {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300}, + {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300}, }; static const struct reg_value ov5640_setting_low_res[] = { @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on) static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on) { - return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? - OV5640_REG_SYS_CTRL0_SW_PWUP : - OV5640_REG_SYS_CTRL0_SW_PWDN); + int ret; + + if (on) { + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, + OV5640_REG_SYS_CTRL0_SW_PWUP); + usleep_range(5000, 6000); + } else { + ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, + OV5640_REG_SYS_CTRL0_SW_PWDN); + } + return ret; } static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)