RE: [omap3isp RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi David and Laurent, -Original Message- From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] Sent: Saturday, November 20, 2010 11:29 AM To: David Cohen Cc: Aguirre, Sergio; linux-media@vger.kernel.org Subject: Re: [omap3isp RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code Hi David, On Saturday 20 November 2010 12:39:56 David Cohen wrote: On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote: Since this sequence strictly touches ISP global registers, it's not really part of the same register address space than the CCDC. Do this check in main isp code instead. Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/isp.c | 24 drivers/media/video/isp/isp.h |2 ++ drivers/media/video/isp/ispccdc.c | 26 +- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c index 2e5030f..ee45eb6 100644 --- a/drivers/media/video/isp/isp.c +++ b/drivers/media/video/isp/isp.c @@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp) } } +int ispccdc_lsc_wait_prefetch(struct isp_device *isp) This is up to you, but to ensure this function now belongs to ISP core and not CCDC anymore, I would change the function name to something like isp_ccdc_lsc_wait_prefetch(). isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver. I know we have the isphist_dma_done() inside ISP core, but changing it to isp_hist_dma_done could be a good cleanup as well. But this is my opinion only. :) I agree. I plan to submit a patch at some point that will rename all non- static functions to use the omap3isp_ prefix instead of the isp_ prefix. Static functions should use the module name as prefix (ccdc_, preview_, ...). Sounds good. It will be a simple patch but will conflict with pretty much everything, so I'll try to push at at a quiet time (or at least quiet enough) to minimize disturbances. I will also see if I can use spatch [1] to generate it. Ok, so, sounds to me then this leaves my patch as-is, and we'll wait for Laurent's coccinelle-assisted changes on top in the future. Is that correct? Regards, Sergio [1] http://coccinelle.lip6.fr/ -- 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 RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi Sergio, On Monday 22 November 2010 16:29:46 Aguirre, Sergio wrote: On Saturday, November 20, 2010 11:29 AM Aguirre, Sergio wrote: On Saturday 20 November 2010 12:39:56 David Cohen wrote: On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote: Since this sequence strictly touches ISP global registers, it's not really part of the same register address space than the CCDC. Do this check in main isp code instead. Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/isp.c | 24 drivers/media/video/isp/isp.h |2 ++ drivers/media/video/isp/ispccdc.c | 26 +- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c index 2e5030f..ee45eb6 100644 --- a/drivers/media/video/isp/isp.c +++ b/drivers/media/video/isp/isp.c @@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp) } } +int ispccdc_lsc_wait_prefetch(struct isp_device *isp) This is up to you, but to ensure this function now belongs to ISP core and not CCDC anymore, I would change the function name to something like isp_ccdc_lsc_wait_prefetch(). isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver. I know we have the isphist_dma_done() inside ISP core, but changing it to isp_hist_dma_done could be a good cleanup as well. But this is my opinion only. :) I agree. I plan to submit a patch at some point that will rename all non- static functions to use the omap3isp_ prefix instead of the isp_ prefix. Static functions should use the module name as prefix (ccdc_, preview_, ...). Sounds good. It will be a simple patch but will conflict with pretty much everything, so I'll try to push at at a quiet time (or at least quiet enough) to minimize disturbances. I will also see if I can use spatch [1] to generate it. Ok, so, sounds to me then this leaves my patch as-is, and we'll wait for Laurent's coccinelle-assisted changes on top in the future. Is that correct? As you need to resubmit the serie anyway, I'd appreciate if you could already make the change. -- 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 RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi Laurent, -Original Message- From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com] Sent: Monday, November 22, 2010 9:35 AM To: Aguirre, Sergio Cc: David Cohen; linux-media@vger.kernel.org Subject: Re: [omap3isp RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code Hi Sergio, On Monday 22 November 2010 16:29:46 Aguirre, Sergio wrote: On Saturday, November 20, 2010 11:29 AM Aguirre, Sergio wrote: On Saturday 20 November 2010 12:39:56 David Cohen wrote: On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote: Since this sequence strictly touches ISP global registers, it's not really part of the same register address space than the CCDC. Do this check in main isp code instead. Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/isp.c | 24 drivers/media/video/isp/isp.h |2 ++ drivers/media/video/isp/ispccdc.c | 26 +--- -- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c index 2e5030f..ee45eb6 100644 --- a/drivers/media/video/isp/isp.c +++ b/drivers/media/video/isp/isp.c @@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp) } } +int ispccdc_lsc_wait_prefetch(struct isp_device *isp) This is up to you, but to ensure this function now belongs to ISP core and not CCDC anymore, I would change the function name to something like isp_ccdc_lsc_wait_prefetch(). isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver. I know we have the isphist_dma_done() inside ISP core, but changing it to isp_hist_dma_done could be a good cleanup as well. But this is my opinion only. :) I agree. I plan to submit a patch at some point that will rename all non- static functions to use the omap3isp_ prefix instead of the isp_ prefix. Static functions should use the module name as prefix (ccdc_, preview_, ...). Sounds good. It will be a simple patch but will conflict with pretty much everything, so I'll try to push at at a quiet time (or at least quiet enough) to minimize disturbances. I will also see if I can use spatch [1] to generate it. Ok, so, sounds to me then this leaves my patch as-is, and we'll wait for Laurent's coccinelle-assisted changes on top in the future. Is that correct? As you need to resubmit the serie anyway, I'd appreciate if you could already make the change. Ok, sure. Not a problem for me. Will add that change in v2 of this patch series. Thanks for the clarification. Regards, Sergio -- 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 RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi Sergio,
On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote:
Since this sequence strictly touches ISP global registers, it's
not really part of the same register address space than the CCDC.
Do this check in main isp code instead.
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 24
drivers/media/video/isp/isp.h |2 ++
drivers/media/video/isp/ispccdc.c | 26 +-
3 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index 2e5030f..ee45eb6 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp)
}
}
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp)
This is up to you, but to ensure this function now belongs to ISP core
and not CCDC anymore, I would change the function name to something like
isp_ccdc_lsc_wait_prefetch().
isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver.
I know we have the isphist_dma_done() inside ISP core, but changing it
to isp_hist_dma_done could be a good cleanup as well.
But this is my opinion only. :)
Br,
David Cohen
+{
+ unsigned int wait;
+
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+
+ /* timeout 1 ms */
+ for (wait = 0; wait 1000; wait++) {
+ if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
+ IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+ return 0;
+ }
+
+ rmb();
+ udelay(1);
+ }
+
+ return -ETIMEDOUT;
+}
+
+
static inline void isp_isr_dbg(struct isp_device *isp, u32 irqstatus)
{
static const char *name[] = {
diff --git a/drivers/media/video/isp/isp.h b/drivers/media/video/isp/isp.h
index 1260e9f..d0b7b0f 100644
--- a/drivers/media/video/isp/isp.h
+++ b/drivers/media/video/isp/isp.h
@@ -280,6 +280,8 @@ struct isp_device {
void isphist_dma_done(struct isp_device *isp);
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp);
+
void isp_flush(struct isp_device *isp);
int isp_pipeline_set_stream(struct isp_pipeline *pipe,
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index 4244edf..b039bce 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -223,30 +223,6 @@ static void ispccdc_lsc_setup_regs(struct
isp_ccdc_device *ccdc,
ISPCCDC_LSC_INITIAL);
}
-static int ispccdc_lsc_wait_prefetch(struct isp_ccdc_device *ccdc)
-{
- struct isp_device *isp = to_isp_device(ccdc);
- unsigned int wait;
-
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
-OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
-
- /* timeout 1 ms */
- for (wait = 0; wait 1000; wait++) {
- if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
- IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
-OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
- return 0;
- }
-
- rmb();
- udelay(1);
- }
-
- return -ETIMEDOUT;
-}
-
/*
* __ispccdc_lsc_enable - Enables/Disables the Lens Shading Compensation
module.
* @ccdc: Pointer to ISP CCDC device.
@@ -272,7 +248,7 @@ static int __ispccdc_lsc_enable(struct isp_ccdc_device
*ccdc, int enable)
ISPCCDC_LSC_ENABLE, enable ? ISPCCDC_LSC_ENABLE : 0);
if (enable) {
- if (ispccdc_lsc_wait_prefetch(ccdc) 0) {
+ if (ispccdc_lsc_wait_prefetch(isp) 0) {
isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC,
ISPCCDC_LSC_CONFIG, ISPCCDC_LSC_ENABLE);
ccdc-lsc.state = LSC_STATE_STOPPED;
--
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 RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Hi David, On Saturday 20 November 2010 12:39:56 David Cohen wrote: On Sat, Nov 20, 2010 at 12:23:49AM +0100, ext Sergio Aguirre wrote: Since this sequence strictly touches ISP global registers, it's not really part of the same register address space than the CCDC. Do this check in main isp code instead. Signed-off-by: Sergio Aguirre saagui...@ti.com --- drivers/media/video/isp/isp.c | 24 drivers/media/video/isp/isp.h |2 ++ drivers/media/video/isp/ispccdc.c | 26 +- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c index 2e5030f..ee45eb6 100644 --- a/drivers/media/video/isp/isp.c +++ b/drivers/media/video/isp/isp.c @@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp) } } +int ispccdc_lsc_wait_prefetch(struct isp_device *isp) This is up to you, but to ensure this function now belongs to ISP core and not CCDC anymore, I would change the function name to something like isp_ccdc_lsc_wait_prefetch(). isp_* is prefix for ISP core and ispccdc_* is prefix for CCDC driver. I know we have the isphist_dma_done() inside ISP core, but changing it to isp_hist_dma_done could be a good cleanup as well. But this is my opinion only. :) I agree. I plan to submit a patch at some point that will rename all non- static functions to use the omap3isp_ prefix instead of the isp_ prefix. Static functions should use the module name as prefix (ccdc_, preview_, ...). It will be a simple patch but will conflict with pretty much everything, so I'll try to push at at a quiet time (or at least quiet enough) to minimize disturbances. I will also see if I can use spatch [1] to generate it. [1] http://coccinelle.lip6.fr/ -- 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
[omap3isp RFC][PATCH 2/4] omap3isp: Move CCDC LSC prefetch wait to main isp code
Since this sequence strictly touches ISP global registers, it's
not really part of the same register address space than the CCDC.
Do this check in main isp code instead.
Signed-off-by: Sergio Aguirre saagui...@ti.com
---
drivers/media/video/isp/isp.c | 24
drivers/media/video/isp/isp.h |2 ++
drivers/media/video/isp/ispccdc.c | 26 +-
3 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/media/video/isp/isp.c b/drivers/media/video/isp/isp.c
index 2e5030f..ee45eb6 100644
--- a/drivers/media/video/isp/isp.c
+++ b/drivers/media/video/isp/isp.c
@@ -339,6 +339,30 @@ void isphist_dma_done(struct isp_device *isp)
}
}
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp)
+{
+ unsigned int wait;
+
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+ OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+
+ /* timeout 1 ms */
+ for (wait = 0; wait 1000; wait++) {
+ if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
+ IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
+ isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
+ OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
+ return 0;
+ }
+
+ rmb();
+ udelay(1);
+ }
+
+ return -ETIMEDOUT;
+}
+
+
static inline void isp_isr_dbg(struct isp_device *isp, u32 irqstatus)
{
static const char *name[] = {
diff --git a/drivers/media/video/isp/isp.h b/drivers/media/video/isp/isp.h
index 1260e9f..d0b7b0f 100644
--- a/drivers/media/video/isp/isp.h
+++ b/drivers/media/video/isp/isp.h
@@ -280,6 +280,8 @@ struct isp_device {
void isphist_dma_done(struct isp_device *isp);
+int ispccdc_lsc_wait_prefetch(struct isp_device *isp);
+
void isp_flush(struct isp_device *isp);
int isp_pipeline_set_stream(struct isp_pipeline *pipe,
diff --git a/drivers/media/video/isp/ispccdc.c
b/drivers/media/video/isp/ispccdc.c
index 4244edf..b039bce 100644
--- a/drivers/media/video/isp/ispccdc.c
+++ b/drivers/media/video/isp/ispccdc.c
@@ -223,30 +223,6 @@ static void ispccdc_lsc_setup_regs(struct isp_ccdc_device
*ccdc,
ISPCCDC_LSC_INITIAL);
}
-static int ispccdc_lsc_wait_prefetch(struct isp_ccdc_device *ccdc)
-{
- struct isp_device *isp = to_isp_device(ccdc);
- unsigned int wait;
-
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
- OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
-
- /* timeout 1 ms */
- for (wait = 0; wait 1000; wait++) {
- if (isp_reg_readl(isp, OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS)
- IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ) {
- isp_reg_writel(isp, IRQ0STATUS_CCDC_LSC_PREF_COMP_IRQ,
- OMAP3_ISP_IOMEM_MAIN, ISP_IRQ0STATUS);
- return 0;
- }
-
- rmb();
- udelay(1);
- }
-
- return -ETIMEDOUT;
-}
-
/*
* __ispccdc_lsc_enable - Enables/Disables the Lens Shading Compensation
module.
* @ccdc: Pointer to ISP CCDC device.
@@ -272,7 +248,7 @@ static int __ispccdc_lsc_enable(struct isp_ccdc_device
*ccdc, int enable)
ISPCCDC_LSC_ENABLE, enable ? ISPCCDC_LSC_ENABLE : 0);
if (enable) {
- if (ispccdc_lsc_wait_prefetch(ccdc) 0) {
+ if (ispccdc_lsc_wait_prefetch(isp) 0) {
isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC,
ISPCCDC_LSC_CONFIG, ISPCCDC_LSC_ENABLE);
ccdc-lsc.state = LSC_STATE_STOPPED;
--
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
