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
