Message ID | 20221216134409.6868-4-j-luthra@ti.com (mailing list archive) |
---|---|
State | Superseded |
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 1p6B0v-00Epgh-3O; Fri, 16 Dec 2022 13:44:37 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230020AbiLPNof (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 16 Dec 2022 08:44:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33400 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229627AbiLPNod (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 16 Dec 2022 08:44:33 -0500 Received: from fllv0016.ext.ti.com (fllv0016.ext.ti.com [198.47.19.142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 87E0B15822 for <linux-media@vger.kernel.org>; Fri, 16 Dec 2022 05:44:31 -0800 (PST) Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0016.ext.ti.com (8.15.2/8.15.2) with ESMTP id 2BGDiPKD117784; Fri, 16 Dec 2022 07:44:25 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1671198265; bh=Bj1fZQNN5IprYVjl0f6bzyscJXjpQtaZ88sTePrrqNI=; h=From:To:CC:Subject:Date:In-Reply-To:References; b=D1vutf/gJ+VHGIvSnWEH2Y2nZrSBgDOAMW/T4fohberV2edoZunlc+8vSliVpKJSY u1/dpASeSVd9CvARi18g09WLU2yVyYKDCh1Wz9mriPOL9XXPjk1r+4iRT+V2bXGdZ7 tjm6Rc1TxSACN3jBisDv5cRT8e1MFLKkxK3dw9zM= Received: from DLEE102.ent.ti.com (dlee102.ent.ti.com [157.170.170.32]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 2BGDiPqZ131043 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 16 Dec 2022 07:44:25 -0600 Received: from DLEE112.ent.ti.com (157.170.170.23) by DLEE102.ent.ti.com (157.170.170.32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.16; Fri, 16 Dec 2022 07:44:25 -0600 Received: from fllv0039.itg.ti.com (10.64.41.19) by DLEE112.ent.ti.com (157.170.170.23) 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; Fri, 16 Dec 2022 07:44:25 -0600 Received: from localhost (ileaxei01-snat.itg.ti.com [10.180.69.5]) by fllv0039.itg.ti.com (8.15.2/8.15.2) with ESMTP id 2BGDiOlC071235; Fri, 16 Dec 2022 07:44:24 -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> CC: <linux-media@vger.kernel.org>, Nishanth Menon <nm@ti.com> Subject: [PATCH 3/3] media: ov5640: Honor power on time in init_setting Date: Fri, 16 Dec 2022 19:14:09 +0530 Message-ID: <20221216134409.6868-4-j-luthra@ti.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20221216134409.6868-1-j-luthra@ti.com> References: <20221216134409.6868-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. 16, 2022, 1:44 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> --- drivers/media/i2c/ov5640.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi Jai On Fri, Dec 16, 2022 at 07:14:09PM +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> > --- > drivers/media/i2c/ov5640.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index fa84e60de0db..ff2a2c9358e7 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -608,7 +608,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}, Two observations: as per the description of register 0x3008 3008 default value = 0x02 3008[7] = Software Reset 3008[6] = Software Power Down The init_settings[] register table has these entries at the very beginning {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, and ends with the entry you have modified {0x3008, 0x02, 0, 5} As I read from the 2.8 section of the datasheet A reset can also be initiated through the SCCB interface by setting register 0x3008[7] to high. So I presume the first two registers entries: {0x3008, 0x82, 0, 5} -> Start a SW reset and wait 5 msec for the chip to resume {0x3008, 0x42, 0, 0} -> SW standby mode SW standby mode is described as: Executing a software standby through the SCCB interface suspends internal circuit activity but does not halt the device clock. All register content is maintained in standby mode. I presume that the first {0x3008, 0x42, 0, 0} exists from SW standby mode to program the chip and the last {0x3008, 0x02, 0, 5} puts the chip in sw standby at the end of init_settings[]. Software standby is then exited by ov5640_set_stream_dvp() for DVP and by clearing 0x300e[4:3] in MIPI mode, as the datasheet reports: To initiate hardware standby mode, the PWDN pin must be tied to high (while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDN pin is set to high) My second observation is that those entries in the init_settings[] table performs SW reset/standby regardless if there's a GPIO or not installed to control the reset and pwdn lines. Would it be worth in your opinion trying to modify ov5640_power() and ov5640_reset() to use either SW or HW standby/reset conditionally on the avialability of sensor->reset_gpio and sensor->pwdn_gpio and remove the initial SW standby/reset from init_setting[] ? > }; > > static const struct reg_value ov5640_setting_low_res[] = { > -- > 2.17.1 >
Hi Jacopo, Thank you for the comments. On 19/12/22 15:40, Jacopo Mondi wrote: > Hi Jai > > On Fri, Dec 16, 2022 at 07:14:09PM +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> >> --- >> drivers/media/i2c/ov5640.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index fa84e60de0db..ff2a2c9358e7 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -608,7 +608,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}, > > Two observations: > > as per the description of register 0x3008 > > 3008 default value = 0x02 > 3008[7] = Software Reset > 3008[6] = Software Power Down > > The init_settings[] register table has these entries at the very > beginning > > {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, > > and ends with the entry you have modified > > {0x3008, 0x02, 0, 5} > > As I read from the 2.8 section of the datasheet > > A reset can also be initiated through the SCCB interface by setting > register 0x3008[7] to high. > > So I presume the first two registers entries: > > {0x3008, 0x82, 0, 5} -> Start a SW reset and wait 5 msec > for the chip to resume > {0x3008, 0x42, 0, 0} -> SW standby mode > > SW standby mode is described as: > > Executing a software standby through the SCCB interface > suspends internal circuit activity but does not halt the > device clock. All register content is maintained in standby > mode. I agree with everything above. > > I presume that the first > > {0x3008, 0x42, 0, 0} > > exists from SW standby mode to program the chip and the last Here I'm a little confused. Isn't it the opposite according to the datasheet? Setting the register to 0x42 (bit 6 = 1) initiates SW standby, which I assume stops all other functions on the chip but still allows programming registers. > > {0x3008, 0x02, 0, 5} > > puts the chip in sw standby at the end of init_settings[] Then setting 0x02 (bit 6 = 0) exits from SW standby in case of MIPI. For DVP though, I see that we skip setting 0x02 to that register in ov5640_load_regs(), so sensor stays in SW standby mode until ov5640_set_stream_dvp() is called. > Software standby is then exited by ov5640_set_stream_dvp() for DVP and > by clearing 0x300e[4:3] in MIPI mode, as the datasheet reports: > > To initiate hardware standby mode, the PWDN pin must be tied to high > (while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDN > pin is set to high > > My second observation is that those entries in the init_settings[] > table performs SW reset/standby regardless if there's a GPIO or not > installed to control the reset and pwdn lines. > > Would it be worth in your opinion trying to modify ov5640_power() > and ov5640_reset() to use either SW or HW standby/reset conditionally > on the avialability of sensor->reset_gpio and sensor->pwdn_gpio and > remove the initial SW standby/reset from init_setting[] ? I don't think we can use the SCCB register pwdn & reset for the initial sensor power up sequence, as sensor SCCB depends on the correct sequence being followed on hardware pins. From Section 2.8: Manually applying a hard reset upon power up is required even though on-chip reset is included. But I agree, if some module does not wire the pins at all then we might have to ignore the datasheet here and try it using the register. > >> }; >> >> static const struct reg_value ov5640_setting_low_res[] = { >> -- >> 2.17.1 >> Thanks, Jai
Hi Jai On Mon, Dec 19, 2022 at 06:38:54PM +0530, Jai Luthra wrote: > Hi Jacopo, > > Thank you for the comments. > > On 19/12/22 15:40, Jacopo Mondi wrote: > > Hi Jai > > > > On Fri, Dec 16, 2022 at 07:14:09PM +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> > > > --- > > > drivers/media/i2c/ov5640.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > > index fa84e60de0db..ff2a2c9358e7 100644 > > > --- a/drivers/media/i2c/ov5640.c > > > +++ b/drivers/media/i2c/ov5640.c > > > @@ -608,7 +608,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}, > > > > Two observations: > > > > as per the description of register 0x3008 > > > > 3008 default value = 0x02 > > 3008[7] = Software Reset > > 3008[6] = Software Power Down > > > > The init_settings[] register table has these entries at the very > > beginning > > > > {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, > > > > and ends with the entry you have modified > > > > {0x3008, 0x02, 0, 5} > > > > As I read from the 2.8 section of the datasheet > > > > A reset can also be initiated through the SCCB interface by setting > > register 0x3008[7] to high. > > > > So I presume the first two registers entries: > > > > {0x3008, 0x82, 0, 5} -> Start a SW reset and wait 5 msec > > for the chip to resume > > {0x3008, 0x42, 0, 0} -> SW standby mode > > > > SW standby mode is described as: > > > > Executing a software standby through the SCCB interface > > suspends internal circuit activity but does not halt the > > device clock. All register content is maintained in standby > > mode. > > I agree with everything above. > > > > > I presume that the first > > > > {0x3008, 0x42, 0, 0} > > > > exists from SW standby mode to program the chip and the last > > Here I'm a little confused. Isn't it the opposite according to the > datasheet? > > Setting the register to 0x42 (bit 6 = 1) initiates SW standby, which I > assume stops all other functions on the chip but still allows programming > registers. > I haven't found any mention about the BIT(6) polarity. IOW I haven't found documented if setting it to 1 enters or exists SW standby mode. Your reasoning is valid, I'm just a bit surprised the SCCB is active and registers can be programmed while in standby mode. However, this #define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 #define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 seems to confirm what you said. > > > > {0x3008, 0x02, 0, 5} > > > > puts the chip in sw standby at the end of init_settings[] > > Then setting 0x02 (bit 6 = 0) exits from SW standby in case of MIPI. > > For DVP though, I see that we skip setting 0x02 to that register in > ov5640_load_regs(), so sensor stays in SW standby mode until > ov5640_set_stream_dvp() is called. > That's what confused me. init_settings[] are programmed for both DVP and MIPI. If the last write to 0x3008 is 0x02 there shouldn't be any need to re-program it. However since ov5640_set_stream_dvp() is called in the s_stream() call path it means SW standby is entered between every streaming session. Please note that no 5msec delay is respected when existing/entering sw standby between streaming session. > > Software standby is then exited by ov5640_set_stream_dvp() for DVP and > > by clearing 0x300e[4:3] in MIPI mode, as the datasheet reports: > > > > To initiate hardware standby mode, the PWDN pin must be tied to high > > (while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDN > > pin is set to high > > > My second observation is that those entries in the init_settings[] > > table performs SW reset/standby regardless if there's a GPIO or not > > installed to control the reset and pwdn lines. > > > > Would it be worth in your opinion trying to modify ov5640_power() > > and ov5640_reset() to use either SW or HW standby/reset conditionally > > on the avialability of sensor->reset_gpio and sensor->pwdn_gpio and > > remove the initial SW standby/reset from init_setting[] ? > > I don't think we can use the SCCB register pwdn & reset for the initial > sensor power up sequence, as sensor SCCB depends on the correct sequence > being followed on hardware pins. From Section 2.8: I was suggesting to use sw reset/powerup if no gpios are registered. > > Manually applying a hard reset upon power up is required even > though on-chip reset is included. > Do you interpret "Manually applying a reset upon power up" to imply "software reset" ? As I read it it doesn't require the reset to be SW or HW. > But I agree, if some module does not wire the pins at all then we might have > to ignore the datasheet here and try it using the register. > That's what I was suggesting. If you have gpios you shouldn't need initial SW resets and powerup. If you don't have gpios, use sw reset so that the reset/powerup routines are centralized and not spread between register tables and functions ? What setup are you testing with ? DVP or MIPI ? With gpios or without ? I can test on a 2-data lanes MIPI setup with HW gpio lines if it helps. Thanks j > > > > > }; > > > > > > static const struct reg_value ov5640_setting_low_res[] = { > > > -- > > > 2.17.1 > > > > > Thanks, > Jai
Hi Jacopo, On 19/12/22 18:55, Jacopo Mondi wrote: > Hi Jai > > On Mon, Dec 19, 2022 at 06:38:54PM +0530, Jai Luthra wrote: >> Hi Jacopo, >> >> Thank you for the comments. >> >> On 19/12/22 15:40, Jacopo Mondi wrote: >>> Hi Jai >>> >>> On Fri, Dec 16, 2022 at 07:14:09PM +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> >>>> --- >>>> drivers/media/i2c/ov5640.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>>> index fa84e60de0db..ff2a2c9358e7 100644 >>>> --- a/drivers/media/i2c/ov5640.c >>>> +++ b/drivers/media/i2c/ov5640.c >>>> @@ -608,7 +608,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}, >>> >>> Two observations: >>> >>> as per the description of register 0x3008 >>> >>> 3008 default value = 0x02 >>> 3008[7] = Software Reset >>> 3008[6] = Software Power Down >>> >>> The init_settings[] register table has these entries at the very >>> beginning >>> >>> {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0}, >>> >>> and ends with the entry you have modified >>> >>> {0x3008, 0x02, 0, 5} >>> >>> As I read from the 2.8 section of the datasheet >>> >>> A reset can also be initiated through the SCCB interface by setting >>> register 0x3008[7] to high. >>> >>> So I presume the first two registers entries: >>> >>> {0x3008, 0x82, 0, 5} -> Start a SW reset and wait 5 msec >>> for the chip to resume >>> {0x3008, 0x42, 0, 0} -> SW standby mode >>> >>> SW standby mode is described as: >>> >>> Executing a software standby through the SCCB interface >>> suspends internal circuit activity but does not halt the >>> device clock. All register content is maintained in standby >>> mode. >> >> I agree with everything above. >> >>> >>> I presume that the first >>> >>> {0x3008, 0x42, 0, 0} >>> >>> exists from SW standby mode to program the chip and the last >> >> Here I'm a little confused. Isn't it the opposite according to the >> datasheet? >> >> Setting the register to 0x42 (bit 6 = 1) initiates SW standby, which I >> assume stops all other functions on the chip but still allows programming >> registers. >> > > I haven't found any mention about the BIT(6) polarity. IOW I haven't > found documented if setting it to 1 enters or exists SW standby mode. > > Your reasoning is valid, I'm just a bit surprised the SCCB is active > and registers can be programmed while in standby mode. > > However, this > > #define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42 > #define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02 > > seems to confirm what you said. > >>> >>> {0x3008, 0x02, 0, 5} >>> >>> puts the chip in sw standby at the end of init_settings[] >> >> Then setting 0x02 (bit 6 = 0) exits from SW standby in case of MIPI. >> >> For DVP though, I see that we skip setting 0x02 to that register in >> ov5640_load_regs(), so sensor stays in SW standby mode until >> ov5640_set_stream_dvp() is called. >> > > That's what confused me. init_settings[] are programmed for both DVP > and MIPI. If the last write to 0x3008 is 0x02 there shouldn't be any > need to re-program it. However since ov5640_set_stream_dvp() is called > in the s_stream() call path it means SW standby is entered between > every streaming session. > > Please note that no 5msec delay is respected when > existing/entering sw standby between streaming session. Good catch, I wonder if that is what causes failure to stream here https://lore.kernel.org/all/3b54ab9b-4ffd-ab32-d495-fad6132ea504@microchip.com/ Will fix in v2. > > >>> Software standby is then exited by ov5640_set_stream_dvp() for DVP and >>> by clearing 0x300e[4:3] in MIPI mode, as the datasheet reports: >>> >>> To initiate hardware standby mode, the PWDN pin must be tied to high >>> (while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDN >>> pin is set to high > >>> My second observation is that those entries in the init_settings[] >>> table performs SW reset/standby regardless if there's a GPIO or not >>> installed to control the reset and pwdn lines. >>> >>> Would it be worth in your opinion trying to modify ov5640_power() >>> and ov5640_reset() to use either SW or HW standby/reset conditionally >>> on the avialability of sensor->reset_gpio and sensor->pwdn_gpio and >>> remove the initial SW standby/reset from init_setting[] ? >> >> I don't think we can use the SCCB register pwdn & reset for the initial >> sensor power up sequence, as sensor SCCB depends on the correct sequence >> being followed on hardware pins. From Section 2.8: > > I was suggesting to use sw reset/powerup if no gpios are registered. > >> >> Manually applying a hard reset upon power up is required even >> though on-chip reset is included. >> > > Do you interpret "Manually applying a reset upon power up" to imply > "software reset" ? As I read it it doesn't require the reset to be > SW or HW. It's the second phrase "though on-chip reset is included" that makes me believe they require it to be HW only. > >> But I agree, if some module does not wire the pins at all then we might have >> to ignore the datasheet here and try it using the register. >> > > That's what I was suggesting. If you have gpios you shouldn't need > initial SW resets and powerup. If you don't have gpios, use sw reset > so that the reset/powerup routines are centralized and not spread between > register tables and functions ? Makes sense, I will try this out and spin it as a separate series. > > What setup are you testing with ? DVP or MIPI ? With gpios or without > ? I can test on a 2-data lanes MIPI setup with HW gpio lines if it > helps. Thanks, my module is similar with MIPI but only has the PWUP gpio line. Might send you the patches separately if it doesn't work on my setup. > > Thanks > j > Thanks, Jai > > >>> >>>> }; >>>> >>>> static const struct reg_value ov5640_setting_low_res[] = { >>>> -- >>>> 2.17.1 >>>> >> >> Thanks, >> Jai
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index fa84e60de0db..ff2a2c9358e7 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -608,7 +608,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[] = {