| Message ID | 1289831401-593-9-git-send-email-saaguirre@ti.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-path: <mchehab@pedra> Envelope-to: mchehab@pedra Delivery-date: Tue, 16 Nov 2010 09:49:44 -0200 Received: from mchehab by pedra with local (Exim 4.72) (envelope-from <mchehab@pedra>) id 1PIK2i-0001bB-Dq for mchehab@pedra; Tue, 16 Nov 2010 09:49:44 -0200 Received: from casper.infradead.org [85.118.1.10] by pedra with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Tue, 16 Nov 2010 09:49:44 -0200 (BRST) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PI04P-0008T0-MD; Mon, 15 Nov 2010 14:30:10 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757152Ab0KOOaF (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Mon, 15 Nov 2010 09:30:05 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:38540 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756055Ab0KOOaD (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 15 Nov 2010 09:30:03 -0500 Received: from dlep33.itg.ti.com ([157.170.170.112]) by comal.ext.ti.com (8.13.7/8.13.7) with ESMTP id oAFEU0ic025567 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 15 Nov 2010 08:30:00 -0600 Received: from legion.dal.design.ti.com (localhost [127.0.0.1]) by dlep33.itg.ti.com (8.13.7/8.13.7) with ESMTP id oAFEU07t001994; Mon, 15 Nov 2010 08:30:00 -0600 (CST) Received: from localhost (dtx0091359-ubuntu-1.am.dhcp.ti.com [128.247.79.64]) by legion.dal.design.ti.com (8.11.7p1+Sun/8.11.7) with ESMTP id oAFEU0f09584; Mon, 15 Nov 2010 08:30:00 -0600 (CST) From: Sergio Aguirre <saaguirre@ti.com> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: linux-media@vger.kernel.org, Sergio Aguirre <saaguirre@ti.com> Subject: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent Date: Mon, 15 Nov 2010 08:30:00 -0600 Message-Id: <1289831401-593-9-git-send-email-saaguirre@ti.com> X-Mailer: git-send-email 1.7.0.4 In-Reply-To: <1289831401-593-1-git-send-email-saaguirre@ti.com> References: <1289831401-593-1-git-send-email-saaguirre@ti.com> Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org Sender: <mchehab@pedra> |
Commit Message
Aguirre Rodriguez, Sergio Alberto
Nov. 15, 2010, 2:30 p.m. UTC
Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
drivers/media/video/isp/ispccp2.c | 3 +--
drivers/media/video/isp/ispreg.h | 14 ++++++++------
2 files changed, 9 insertions(+), 8 deletions(-)
Comments
Hi Sergio, I've few comments below. On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > --- > drivers/media/video/isp/ispccp2.c | 3 +-- > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c > index fa23394..3127a74 100644 > --- a/drivers/media/video/isp/ispccp2.c > +++ b/drivers/media/video/isp/ispccp2.c > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, > config->src_ofst = 0; > } > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > - ISPCSI1_MIDLEMODE_SHIFT), > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); To make your cleanup even better, you could change isp_reg_clr_set() instead. If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an invalid 0x3 value. Despite it cannot happen with the current code, it's still better to avoid such situations for future versions. :) > > /* Hsize, Skip */ > diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h > index d885541..9b0d3ad 100644 > --- a/drivers/media/video/isp/ispreg.h > +++ b/drivers/media/video/isp/ispreg.h > @@ -141,6 +141,14 @@ > #define ISPCCP2_REVISION (0x000) > #define ISPCCP2_SYSCONFIG (0x004) > #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 << 1) > +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ > + (0x0 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ > + (0x1 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ > + (0x2 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be always set together. Regards, David Cohen > #define ISPCCP2_SYSSTATUS (0x008) > #define ISPCCP2_SYSSTATUS_RESET_DONE (1 << 0) > #define ISPCCP2_LC01_IRQENABLE (0x00C) > @@ -1309,12 +1317,6 @@ > #define ISPMMU_SIDLEMODE_SMARTIDLE 2 > #define ISPMMU_SIDLEMODE_SHIFT 3 > > -#define ISPCSI1_AUTOIDLE 0x1 > -#define ISPCSI1_MIDLEMODE_SHIFT 12 > -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 > -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 > -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 > - > /* ----------------------------------------------------------------------------- > * CSI2 receiver registers (ES2.0) > */ > -- > 1.7.0.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Cohen [mailto:david.cohen@nokia.com] > Sent: Friday, November 19, 2010 4:06 AM > To: Aguirre, Sergio > Cc: Laurent Pinchart; linux-media@vger.kernel.org > Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > fields consistent > > Hi Sergio, Hi David, Thanks for the review. > > I've few comments below. > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > --- > > drivers/media/video/isp/ispccp2.c | 3 +-- > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/media/video/isp/ispccp2.c > b/drivers/media/video/isp/ispccp2.c > > index fa23394..3127a74 100644 > > --- a/drivers/media/video/isp/ispccp2.c > > +++ b/drivers/media/video/isp/ispccp2.c > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > isp_ccp2_device *ccp2, > > config->src_ofst = 0; > > } > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > - ISPCSI1_MIDLEMODE_SHIFT), > > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > To make your cleanup even better, you could change isp_reg_clr_set() > instead. > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an > invalid 0x3 value. Despite it cannot happen with the current code, it's > still better to avoid such situations for future versions. :) Hmm you're right, I guess I didn't do any real functional changes, just defines renaming. I can repost an updated patch with this suggestion. No problem. > > > > > /* Hsize, Skip */ > > diff --git a/drivers/media/video/isp/ispreg.h > b/drivers/media/video/isp/ispreg.h > > index d885541..9b0d3ad 100644 > > --- a/drivers/media/video/isp/ispreg.h > > +++ b/drivers/media/video/isp/ispreg.h > > @@ -141,6 +141,14 @@ > > #define ISPCCP2_REVISION (0x000) > > #define ISPCCP2_SYSCONFIG (0x004) > > #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 << 1) > > +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ > > + (0x0 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ > > + (0x1 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ > > + (0x2 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be > always set together. Sure, will do. Thanks and Regards, Sergio > > Regards, > > David Cohen > > > #define ISPCCP2_SYSSTATUS (0x008) > > #define ISPCCP2_SYSSTATUS_RESET_DONE (1 << 0) > > #define ISPCCP2_LC01_IRQENABLE (0x00C) > > @@ -1309,12 +1317,6 @@ > > #define ISPMMU_SIDLEMODE_SMARTIDLE 2 > > #define ISPMMU_SIDLEMODE_SHIFT 3 > > > > -#define ISPCSI1_AUTOIDLE 0x1 > > -#define ISPCSI1_MIDLEMODE_SHIFT 12 > > -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 > > -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 > > -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 > > - > > /* -------------------------------------------------------------------- > --------- > > * CSI2 receiver registers (ES2.0) > > */ > > -- > > 1.7.0.4 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" > in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Aguirre, Sergio > Sent: Friday, November 19, 2010 9:44 AM > To: 'David Cohen' > Cc: Laurent Pinchart; linux-media@vger.kernel.org > Subject: RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > fields consistent > > > > -----Original Message----- > > From: David Cohen [mailto:david.cohen@nokia.com] > > Sent: Friday, November 19, 2010 4:06 AM > > To: Aguirre, Sergio > > Cc: Laurent Pinchart; linux-media@vger.kernel.org > > Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > > fields consistent > > > > Hi Sergio, > > Hi David, > > Thanks for the review. > > > > > I've few comments below. > > > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > > --- > > > drivers/media/video/isp/ispccp2.c | 3 +-- > > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/media/video/isp/ispccp2.c > > b/drivers/media/video/isp/ispccp2.c > > > index fa23394..3127a74 100644 > > > --- a/drivers/media/video/isp/ispccp2.c > > > +++ b/drivers/media/video/isp/ispccp2.c > > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > > isp_ccp2_device *ccp2, > > > config->src_ofst = 0; > > > } > > > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > > - ISPCSI1_MIDLEMODE_SHIFT), > > > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > > > To make your cleanup even better, you could change isp_reg_clr_set() > > instead. > > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an > > invalid 0x3 value. Despite it cannot happen with the current code, it's > > still better to avoid such situations for future versions. :) > > Hmm you're right, I guess I didn't do any real functional changes, just > defines renaming. > > I can repost an updated patch with this suggestion. No problem. David, Geez I guess I wasn't paying much attention to this either. Sorry. The case you mention about it potentially become 0x3 is not possible, because, I'm basically overwriting the whole register with (0x2 << 12) Notice the isp_reg_write, there's no OR operation... Now, this means that we have been implicitly setting other fields as Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to test with :(. Regards, Sergio > > > > > > > > > /* Hsize, Skip */ > > > diff --git a/drivers/media/video/isp/ispreg.h > > b/drivers/media/video/isp/ispreg.h > > > index d885541..9b0d3ad 100644 > > > --- a/drivers/media/video/isp/ispreg.h > > > +++ b/drivers/media/video/isp/ispreg.h > > > @@ -141,6 +141,14 @@ > > > #define ISPCCP2_REVISION (0x000) > > > #define ISPCCP2_SYSCONFIG (0x004) > > > #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 << 1) > > > +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ > > > + (0x0 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ > > > + (0x1 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ > > > + (0x2 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > > You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be > > always set together. > > Sure, will do. > > Thanks and Regards, > Sergio > > > > > Regards, > > > > David Cohen > > > > > #define ISPCCP2_SYSSTATUS (0x008) > > > #define ISPCCP2_SYSSTATUS_RESET_DONE (1 << 0) > > > #define ISPCCP2_LC01_IRQENABLE (0x00C) > > > @@ -1309,12 +1317,6 @@ > > > #define ISPMMU_SIDLEMODE_SMARTIDLE 2 > > > #define ISPMMU_SIDLEMODE_SHIFT 3 > > > > > > -#define ISPCSI1_AUTOIDLE 0x1 > > > -#define ISPCSI1_MIDLEMODE_SHIFT 12 > > > -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 > > > -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 > > > -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 > > > - > > > /* ------------------------------------------------------------------ > -- > > --------- > > > * CSI2 receiver registers (ES2.0) > > > */ > > > -- > > > 1.7.0.4 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-media" > > in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: > > > > -----Original Message----- > > From: Aguirre, Sergio > > Sent: Friday, November 19, 2010 9:44 AM > > To: 'David Cohen' > > Cc: Laurent Pinchart; linux-media@vger.kernel.org > > Subject: RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > > fields consistent > > > > > > > -----Original Message----- > > > From: David Cohen [mailto:david.cohen@nokia.com] > > > Sent: Friday, November 19, 2010 4:06 AM > > > To: Aguirre, Sergio > > > Cc: Laurent Pinchart; linux-media@vger.kernel.org > > > Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG > > > fields consistent > > > > > > Hi Sergio, > > > > Hi David, > > > > Thanks for the review. > > > > > > > > I've few comments below. > > > > > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > > > --- > > > > drivers/media/video/isp/ispccp2.c | 3 +-- > > > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/media/video/isp/ispccp2.c > > > b/drivers/media/video/isp/ispccp2.c > > > > index fa23394..3127a74 100644 > > > > --- a/drivers/media/video/isp/ispccp2.c > > > > +++ b/drivers/media/video/isp/ispccp2.c > > > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > > > isp_ccp2_device *ccp2, > > > > config->src_ofst = 0; > > > > } > > > > > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > > > - ISPCSI1_MIDLEMODE_SHIFT), > > > > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > > > > > To make your cleanup even better, you could change isp_reg_clr_set() > > > instead. > > > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an > > > invalid 0x3 value. Despite it cannot happen with the current code, it's > > > still better to avoid such situations for future versions. :) > > > > Hmm you're right, I guess I didn't do any real functional changes, just > > defines renaming. > > > > I can repost an updated patch with this suggestion. No problem. > > David, > > Geez I guess I wasn't paying much attention to this either. Sorry. > > The case you mention about it potentially become 0x3 is not possible, because, I'm basically overwriting the whole register with (0x2 << 12) > Notice the isp_reg_write, there's no OR operation... That's correct. My mistake. IMO ISP power settings still need a better way to be setup, but it definitively does not belong to your cleanup patch. I'm fine with your changes. > > Now, this means that we have been implicitly setting other fields as Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. > > Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to test > with :(. I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable so far. It should be set to 0 until somebody proves it can be set to 1 :) Br, David > > Regards, > Sergio > > > > > > > > > > > > > > /* Hsize, Skip */ > > > > diff --git a/drivers/media/video/isp/ispreg.h > > > b/drivers/media/video/isp/ispreg.h > > > > index d885541..9b0d3ad 100644 > > > > --- a/drivers/media/video/isp/ispreg.h > > > > +++ b/drivers/media/video/isp/ispreg.h > > > > @@ -141,6 +141,14 @@ > > > > #define ISPCCP2_REVISION (0x000) > > > > #define ISPCCP2_SYSCONFIG (0x004) > > > > #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 << 1) > > > > +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 > > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 > > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ > > > > + (0x0 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ > > > > + (0x1 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > > +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ > > > > + (0x2 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) > > > > > > You can add some ISPCCP2_SYSCONFIG_MSTANDY_MODE_MASK, as 2 bits must be > > > always set together. > > > > Sure, will do. > > > > Thanks and Regards, > > Sergio > > > > > > > > Regards, > > > > > > David Cohen > > > > > > > #define ISPCCP2_SYSSTATUS (0x008) > > > > #define ISPCCP2_SYSSTATUS_RESET_DONE (1 << 0) > > > > #define ISPCCP2_LC01_IRQENABLE (0x00C) > > > > @@ -1309,12 +1317,6 @@ > > > > #define ISPMMU_SIDLEMODE_SMARTIDLE 2 > > > > #define ISPMMU_SIDLEMODE_SHIFT 3 > > > > > > > > -#define ISPCSI1_AUTOIDLE 0x1 > > > > -#define ISPCSI1_MIDLEMODE_SHIFT 12 > > > > -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 > > > > -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 > > > > -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 > > > > - > > > > /* ------------------------------------------------------------------ > > -- > > > --------- > > > > * CSI2 receiver registers (ES2.0) > > > > */ > > > > -- > > > > 1.7.0.4 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-media" > > > in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Saturday 20 November 2010 11:00:30 David Cohen wrote: > On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: > > On Friday, November 19, 2010 9:44 AM Aguirre, Sergio wrote: > > > On Friday, November 19, 2010 4:06 AM David Cohen wrote: > > > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > > > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > > > > --- > > > > > > > > > > drivers/media/video/isp/ispccp2.c | 3 +-- > > > > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/media/video/isp/ispccp2.c > > > > > > > > b/drivers/media/video/isp/ispccp2.c > > > > > > > > > index fa23394..3127a74 100644 > > > > > --- a/drivers/media/video/isp/ispccp2.c > > > > > +++ b/drivers/media/video/isp/ispccp2.c > > > > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > > > > > > > > isp_ccp2_device *ccp2, > > > > > > > > > config->src_ofst = 0; > > > > > > > > > > } > > > > > > > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > > > > - ISPCSI1_MIDLEMODE_SHIFT), > > > > > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > > > > > > > > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > > > > > > > To make your cleanup even better, you could change isp_reg_clr_set() > > > > instead. > > > > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an > > > > invalid 0x3 value. Despite it cannot happen with the current code, > > > > it's still better to avoid such situations for future versions. :) > > > > > > Hmm you're right, I guess I didn't do any real functional changes, just > > > defines renaming. > > > > > > I can repost an updated patch with this suggestion. No problem. > > > > David, > > > > Geez I guess I wasn't paying much attention to this either. Sorry. > > > > The case you mention about it potentially become 0x3 is not possible, > > because, I'm basically overwriting the whole register with (0x2 << 12) > > Notice the isp_reg_write, there's no OR operation... > > That's correct. My mistake. > IMO ISP power settings still need a better way to be setup, but it > definitively does not belong to your cleanup patch. > I'm fine with your changes. > > > Now, this means that we have been implicitly setting other fields as > > Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. > > > > Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to > > test with :(. > > I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable > so far. It should be set to 0 until somebody proves it can be set to 1 :) To summarize things, we're only setting the CCP2 AUTOIDLE bit on ES 1.0 devices, and we're clearing it anyway ispccp2_mem_configure(). For the sake of correctness we should replace isp_reg_writel() by isp_reg_clr_set() in ispccp2_mem_configure(), which will only make a difference on ES 1.0 devices that have basically no users. Is that OK with both of you ?
On Sat, Nov 20, 2010 at 11:34:04AM +0100, ext Laurent Pinchart wrote: > Hi, > > On Saturday 20 November 2010 11:00:30 David Cohen wrote: > > On Sat, Nov 20, 2010 at 12:10:11AM +0100, ext Aguirre, Sergio wrote: > > > On Friday, November 19, 2010 9:44 AM Aguirre, Sergio wrote: > > > > On Friday, November 19, 2010 4:06 AM David Cohen wrote: > > > > > On Mon, Nov 15, 2010 at 03:30:00PM +0100, ext Sergio Aguirre wrote: > > > > > > Signed-off-by: Sergio Aguirre <saaguirre@ti.com> > > > > > > --- > > > > > > > > > > > > drivers/media/video/isp/ispccp2.c | 3 +-- > > > > > > drivers/media/video/isp/ispreg.h | 14 ++++++++------ > > > > > > 2 files changed, 9 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/drivers/media/video/isp/ispccp2.c > > > > > > > > > > b/drivers/media/video/isp/ispccp2.c > > > > > > > > > > > index fa23394..3127a74 100644 > > > > > > --- a/drivers/media/video/isp/ispccp2.c > > > > > > +++ b/drivers/media/video/isp/ispccp2.c > > > > > > @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct > > > > > > > > > > isp_ccp2_device *ccp2, > > > > > > > > > > > config->src_ofst = 0; > > > > > > > > > > > > } > > > > > > > > > > > > - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << > > > > > > - ISPCSI1_MIDLEMODE_SHIFT), > > > > > > + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, > > > > > > > > > > > > OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); > > > > > > > > > > To make your cleanup even better, you could change isp_reg_clr_set() > > > > > instead. > > > > > If CCP2 MSTANDY_MODE is set to NOSTANDY, the result is going to be an > > > > > invalid 0x3 value. Despite it cannot happen with the current code, > > > > > it's still better to avoid such situations for future versions. :) > > > > > > > > Hmm you're right, I guess I didn't do any real functional changes, just > > > > defines renaming. > > > > > > > > I can repost an updated patch with this suggestion. No problem. > > > > > > David, > > > > > > Geez I guess I wasn't paying much attention to this either. Sorry. > > > > > > The case you mention about it potentially become 0x3 is not possible, > > > because, I'm basically overwriting the whole register with (0x2 << 12) > > > Notice the isp_reg_write, there's no OR operation... > > > > That's correct. My mistake. > > IMO ISP power settings still need a better way to be setup, but it > > definitively does not belong to your cleanup patch. > > I'm fine with your changes. > > > > > Now, this means that we have been implicitly setting other fields as > > > Zeroes (SOFT_RESET = 0, and AUTO_IDLE = 0) aswell. > > > > > > Has AUTOIDLE in CCP2 been tried in N900? I don't have a CCP2 sensor to > > > test with :(. > > > > I have no idea. Indeed, AUTOIDLE in ISP doesn't seem to be much reliable > > so far. It should be set to 0 until somebody proves it can be set to 1 :) > > To summarize things, we're only setting the CCP2 AUTOIDLE bit on ES 1.0 > devices, and we're clearing it anyway ispccp2_mem_configure(). For the sake of > correctness we should replace isp_reg_writel() by isp_reg_clr_set() in > ispccp2_mem_configure(), which will only make a difference on ES 1.0 devices > that have basically no users. Is that OK with both of you ? I'm fine with both solutions. IMO power settings could be improved and not be hardcoded. It could come from platform data. But that's another case. :) Br, David > > -- > Regards, > > Laurent Pinchart > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/isp/ispccp2.c b/drivers/media/video/isp/ispccp2.c index fa23394..3127a74 100644 --- a/drivers/media/video/isp/ispccp2.c +++ b/drivers/media/video/isp/ispccp2.c @@ -419,8 +419,7 @@ static void ispccp2_mem_configure(struct isp_ccp2_device *ccp2, config->src_ofst = 0; } - isp_reg_writel(isp, (ISPCSI1_MIDLEMODE_SMARTSTANDBY << - ISPCSI1_MIDLEMODE_SHIFT), + isp_reg_writel(isp, ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_SYSCONFIG); /* Hsize, Skip */ diff --git a/drivers/media/video/isp/ispreg.h b/drivers/media/video/isp/ispreg.h index d885541..9b0d3ad 100644 --- a/drivers/media/video/isp/ispreg.h +++ b/drivers/media/video/isp/ispreg.h @@ -141,6 +141,14 @@ #define ISPCCP2_REVISION (0x000) #define ISPCCP2_SYSCONFIG (0x004) #define ISPCCP2_SYSCONFIG_SOFT_RESET (1 << 1) +#define ISPCCP2_SYSCONFIG_AUTO_IDLE 0x1 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT 12 +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_FORCE \ + (0x0 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_NO \ + (0x1 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) +#define ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SMART \ + (0x2 << ISPCCP2_SYSCONFIG_MSTANDBY_MODE_SHIFT) #define ISPCCP2_SYSSTATUS (0x008) #define ISPCCP2_SYSSTATUS_RESET_DONE (1 << 0) #define ISPCCP2_LC01_IRQENABLE (0x00C) @@ -1309,12 +1317,6 @@ #define ISPMMU_SIDLEMODE_SMARTIDLE 2 #define ISPMMU_SIDLEMODE_SHIFT 3 -#define ISPCSI1_AUTOIDLE 0x1 -#define ISPCSI1_MIDLEMODE_SHIFT 12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY 0x1 -#define ISPCSI1_MIDLEMODE_SMARTSTANDBY 0x2 - /* ----------------------------------------------------------------------------- * CSI2 receiver registers (ES2.0) */