RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
Hi, -Original Message- From: David Cohen [mailto:david.co...@nokia.com] Sent: Saturday, November 20, 2010 4:49 AM To: ext Laurent Pinchart Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent 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 saagui...@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 ? Yes, It's fine with me. I'll say that if this doesn't change things for N900, which appears to be the only community available device (I know of) that uses CCP2 cameras, then we shouldn't care. If somebody comes up with an ES1.0 in the future, and he does see problems, then we will address that. But that would be a very weird case. In fact, it will be most probably someone from TI with a hidden board buried in some box of scraps, which happens to have hacked the board for a CCP2 camera... The more I think about it, the more I'm getting convinced this shouldn't be a worry. :) Regards, Sergio 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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
On Mon, Nov 22, 2010 at 04:19:42PM +0100, ext Aguirre, Sergio wrote: Hi, -Original Message- From: David Cohen [mailto:david.co...@nokia.com] Sent: Saturday, November 20, 2010 4:49 AM To: ext Laurent Pinchart Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent 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 saagui...@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 ? Yes, It's fine with me. I'll say that if this doesn't change things for N900, which appears to be the only community available device (I know of) that uses CCP2 cameras, then we shouldn't care. If somebody comes up with an ES1.0 in the future, and he does see problems, then we will address that. But that would be a very weird case. In fact, it will be most probably someone from TI with a hidden board buried in some box of scraps, which happens to have hacked the board for a CCP2 camera... The more I think about it, the more I'm getting convinced this shouldn't be a worry. :) You're true. Maybe we could rework in future part of the driver's PM. I'm fine with this patch. :) I only suggested something to make it a little better. But as AUTOIDLE is not being set anyway, the result is the same. Br, David Regards, Sergio 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 majord...@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 majord
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
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.co...@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 saagui...@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_IDLE0x1 +#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_SHIFT12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY0x1 -#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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
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 saagui...@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 ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
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 saagui...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
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 saagui...@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_SHIFT12 +#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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
-Original Message- From: David Cohen [mailto:david.co...@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 saagui...@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_IDLE0x1 +#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_SHIFT12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY0x1 -#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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
-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.co...@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 saagui...@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_SHIFT12 +#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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[omap3isp][PATCH v2 8/9] omap3isp: ccp2: Make SYSCONFIG fields consistent
Signed-off-by: Sergio Aguirre saagui...@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); /* 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_IDLE0x1 +#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_SHIFT12 -#define ISPCSI1_MIDLEMODE_FORCESTANDBY 0x0 -#define ISPCSI1_MIDLEMODE_NOSTANDBY0x1 -#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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
