[PATCH v2 0/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[PATCH v2 0/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi everybody,

Here's the second version of the non-GRBG Bayer patterns support for the OMAP3
ISP preview engine. Compared to v1, the CFA table can be reconfigured at
runtime, which resulted in several cleanup patches.

The first patch is a v3.5 regression fix, I'll send a separate pull request.

Jean-Philippe, could you please test this patch set on your hardware ? It's
based on top of the latest linuxtv/staging/for_v3.6 branch.

Laurent Pinchart (6):
  omap3isp: preview: Fix contrast and brightness handling
  omap3isp: preview: Remove lens shading compensation support
  omap3isp: preview: Pass a prev_params pointer to configuration
    functions
  omap3isp: preview: Reorder configuration functions
  omap3isp: preview: Merge gamma correction and gamma bypass
  omap3isp: preview: Add support for non-GRBG Bayer patterns

 drivers/media/video/omap3isp/isppreview.c |  706 ++++++++++++++---------------
 drivers/media/video/omap3isp/isppreview.h |    1 +
 2 files changed, 346 insertions(+), 361 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/6] omap3isp: preview: Fix contrast and brightness handling

[Laurent Pinchart]

Commit bac387efbb88cf0e8df6f46a38387897cea464ee ("omap3isp: preview:
Simplify configuration parameters access") added three fields to the
preview_update structure, but failed to properly update the related
initializers. Fix this.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 8a4935e..aec9860 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -888,12 +888,12 @@ static const struct preview_update update_attrs[] = {
 		preview_config_contrast,
 		NULL,
 		offsetof(struct prev_params, contrast),
-		0, true,
+		0, 0, true,
 	}, /* OMAP3ISP_PREV_BRIGHTNESS */ {
 		preview_config_brightness,
 		NULL,
 		offsetof(struct prev_params, brightness),
-		0, true,
+		0, 0, true,
 	},
 };
 
-- 
1.7.8.6

[PATCH v2 2/6] omap3isp: preview: Remove lens shading compensation support

[Laurent Pinchart]

The feature isn't fully implemented and doesn't work, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index aec9860..4cdcc48 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -215,22 +215,6 @@ preview_enable_drkframe(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_config_drkf_shadcomp - Configures shift value in shading comp.
- * @scomp_shtval: 3bit value of shift used in shading compensation.
- */
-static void
-preview_config_drkf_shadcomp(struct isp_prev_device *prev,
-			     const void *scomp_shtval)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const u32 *shtval = scomp_shtval;
-
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			ISPPRV_PCR_SCOMP_SFT_MASK,
-			*shtval << ISPPRV_PCR_SCOMP_SFT_SHIFT);
-}
-
-/*
  * preview_enable_hmed - Enables/Disables of the Horizontal Median Filter.
  * @enable: 1 - Enables Horizontal Median Filter.
  */
@@ -870,7 +854,7 @@ static const struct preview_update update_attrs[] = {
 		NULL,
 		preview_enable_drkframe,
 	}, /* OMAP3ISP_PREV_LENS_SHADING */ {
-		preview_config_drkf_shadcomp,
+		NULL,
 		preview_enable_drkframe,
 	}, /* OMAP3ISP_PREV_NF */ {
 		preview_config_noisefilter,
-- 
1.7.8.6

[PATCH v2 3/6] omap3isp: preview: Pass a prev_params pointer to configuration functions

[Laurent Pinchart]

Instead of using void pointers and offset arithmetics to compute a
pointer to configuration parameters in a generic way, pass the complete
parameters structure to configuration functions and let them access the
parameters they need.

Also modify the enable functions to use a bool enable parameter instead
of a u8.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  200 ++++++++++++-----------------
 1 files changed, 83 insertions(+), 117 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 4cdcc48..1ac48b7 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -157,11 +157,9 @@ static u32 luma_enhance_table[] = {
 };
 
 /*
- * preview_enable_invalaw - Enable/Disable Inverse A-Law module in Preview.
- * @enable: 1 - Reverse the A-Law done in CCDC.
+ * preview_enable_invalaw - Enable/disable Inverse A-Law decompression
  */
-static void
-preview_enable_invalaw(struct isp_prev_device *prev, u8 enable)
+static void preview_enable_invalaw(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -174,15 +172,10 @@ preview_enable_invalaw(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_drkframe_capture - Enable/Disable of the darkframe capture.
- * @prev -
- * @enable: 1 - Enable, 0 - Disable
- *
- * NOTE: PRV_WSDR_ADDR and PRV_WADD_OFFSET must be set also
- * The process is applied for each captured frame.
+ * preview_enable_drkframe_capture - Enable/disable Dark Frame Capture
  */
 static void
-preview_enable_drkframe_capture(struct isp_prev_device *prev, u8 enable)
+preview_enable_drkframe_capture(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -195,14 +188,9 @@ preview_enable_drkframe_capture(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_drkframe - Enable/Disable of the darkframe subtract.
- * @enable: 1 - Acquires memory bandwidth since the pixels in each frame is
- *          subtracted with the pixels in the current frame.
- *
- * The process is applied for each captured frame.
+ * preview_enable_drkframe - Enable/disable Dark Frame Subtraction
  */
-static void
-preview_enable_drkframe(struct isp_prev_device *prev, u8 enable)
+static void preview_enable_drkframe(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -215,11 +203,9 @@ preview_enable_drkframe(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_hmed - Enables/Disables of the Horizontal Median Filter.
- * @enable: 1 - Enables Horizontal Median Filter.
+ * preview_enable_hmed - Enable/disable the Horizontal Median Filter
  */
-static void
-preview_enable_hmed(struct isp_prev_device *prev, u8 enable)
+static void preview_enable_hmed(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -232,15 +218,13 @@ preview_enable_hmed(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_config_hmed - Configures the Horizontal Median Filter.
- * @prev_hmed: Structure containing the odd and even distance between the
- *             pixels in the image along with the filter threshold.
+ * preview_config_hmed - Configure the Horizontal Median Filter
  */
-static void
-preview_config_hmed(struct isp_prev_device *prev, const void *prev_hmed)
+static void preview_config_hmed(struct isp_prev_device *prev,
+				const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_hmed *hmed = prev_hmed;
+	const struct omap3isp_prev_hmed *hmed = &params->hmed;
 
 	isp_reg_writel(isp, (hmed->odddist == 1 ? 0 : ISPPRV_HMED_ODDDIST) |
 		       (hmed->evendist == 1 ? 0 : ISPPRV_HMED_EVENDIST) |
@@ -249,15 +233,14 @@ preview_config_hmed(struct isp_prev_device *prev, const void *prev_hmed)
 }
 
 /*
- * preview_config_noisefilter - Configures the Noise Filter.
- * @prev_nf: Structure containing the noisefilter table, strength to be used
- *           for the noise filter and the defect correction enable flag.
+ * preview_config_noisefilter - Configure the Noise Filter
  */
 static void
-preview_config_noisefilter(struct isp_prev_device *prev, const void *prev_nf)
+preview_config_noisefilter(struct isp_prev_device *prev,
+			   const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_nf *nf = prev_nf;
+	const struct omap3isp_prev_nf *nf = &params->nf;
 	unsigned int i;
 
 	isp_reg_writel(isp, nf->spread, OMAP3_ISP_IOMEM_PREV, ISPPRV_NF);
@@ -270,14 +253,14 @@ preview_config_noisefilter(struct isp_prev_device *prev, const void *prev_nf)
 }
 
 /*
- * preview_config_dcor - Configures the defect correction
- * @prev_dcor: Structure containing the defect correct thresholds
+ * preview_config_dcor - Configure Couplet Defect Correction
  */
 static void
-preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor)
+preview_config_dcor(struct isp_prev_device *prev,
+		    const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_dcor *dcor = prev_dcor;
+	const struct omap3isp_prev_dcor *dcor = &params->dcor;
 
 	isp_reg_writel(isp, dcor->detect_correct[0],
 		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR0);
@@ -293,15 +276,14 @@ preview_config_dcor(struct isp_prev_device *prev, const void *prev_dcor)
 }
 
 /*
- * preview_config_cfa - Configures the CFA Interpolation parameters.
- * @prev_cfa: Structure containing the CFA interpolation table, CFA format
- *            in the image, vertical and horizontal gradient threshold.
+ * preview_config_cfa - Configure CFA Interpolation
  */
 static void
-preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
+preview_config_cfa(struct isp_prev_device *prev,
+		   const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_cfa *cfa = prev_cfa;
+	const struct omap3isp_prev_cfa *cfa = &params->cfa;
 	unsigned int i;
 
 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
@@ -323,14 +305,14 @@ preview_config_cfa(struct isp_prev_device *prev, const void *prev_cfa)
 }
 
 /*
- * preview_config_gammacorrn - Configures the Gamma Correction table values
- * @gtable: Structure containing the table for red, blue, green gamma table.
+ * preview_config_gammacorrn - Configure the Gamma Correction tables
  */
 static void
-preview_config_gammacorrn(struct isp_prev_device *prev, const void *gtable)
+preview_config_gammacorrn(struct isp_prev_device *prev,
+			  const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_gtables *gt = gtable;
+	const struct omap3isp_prev_gtables *gt = &params->gamma;
 	unsigned int i;
 
 	isp_reg_writel(isp, ISPPRV_REDGAMMA_TABLE_ADDR,
@@ -353,15 +335,14 @@ preview_config_gammacorrn(struct isp_prev_device *prev, const void *gtable)
 }
 
 /*
- * preview_config_luma_enhancement - Sets the Luminance Enhancement table.
- * @ytable: Structure containing the table for Luminance Enhancement table.
+ * preview_config_luma_enhancement - Configure the Luminance Enhancement table
  */
 static void
 preview_config_luma_enhancement(struct isp_prev_device *prev,
-				const void *ytable)
+				const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_luma *yt = ytable;
+	const struct omap3isp_prev_luma *yt = &params->luma;
 	unsigned int i;
 
 	isp_reg_writel(isp, ISPPRV_YENH_TABLE_ADDR,
@@ -373,16 +354,14 @@ preview_config_luma_enhancement(struct isp_prev_device *prev,
 }
 
 /*
- * preview_config_chroma_suppression - Configures the Chroma Suppression.
- * @csup: Structure containing the threshold value for suppression
- *        and the hypass filter enable flag.
+ * preview_config_chroma_suppression - Configure Chroma Suppression
  */
 static void
 preview_config_chroma_suppression(struct isp_prev_device *prev,
-				  const void *csup)
+				  const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_csup *cs = csup;
+	const struct omap3isp_prev_csup *cs = &params->csup;
 
 	isp_reg_writel(isp,
 		       cs->gain | (cs->thres << ISPPRV_CSUP_THRES_SHIFT) |
@@ -391,11 +370,10 @@ preview_config_chroma_suppression(struct isp_prev_device *prev,
 }
 
 /*
- * preview_enable_noisefilter - Enables/Disables the Noise Filter.
- * @enable: 1 - Enables the Noise Filter.
+ * preview_enable_noisefilter - Enable/disable the Noise Filter
  */
 static void
-preview_enable_noisefilter(struct isp_prev_device *prev, u8 enable)
+preview_enable_noisefilter(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -408,11 +386,9 @@ preview_enable_noisefilter(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_dcor - Enables/Disables the defect correction.
- * @enable: 1 - Enables the defect correction.
+ * preview_enable_dcor - Enable/disable Couplet Defect Correction
  */
-static void
-preview_enable_dcor(struct isp_prev_device *prev, u8 enable)
+static void preview_enable_dcor(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -425,12 +401,13 @@ preview_enable_dcor(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_gammabypass - Enables/Disables the GammaByPass
- * @enable: 1 - Bypasses Gamma - 10bit input is cropped to 8MSB.
- *          0 - Goes through Gamma Correction. input and output is 10bit.
+ * preview_enable_gammabypass - Enable/disable Gamma Bypass
+ *
+ * When gamma bypass is enabled, the output of the gamma correction is the 8 MSB
+ * of the 10-bit input .
  */
 static void
-preview_enable_gammabypass(struct isp_prev_device *prev, u8 enable)
+preview_enable_gammabypass(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -443,11 +420,10 @@ preview_enable_gammabypass(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_luma_enhancement - Enables/Disables Luminance Enhancement
- * @enable: 1 - Enable the Luminance Enhancement.
+ * preview_enable_luma_enhancement - Enable/disable Luminance Enhancement
  */
 static void
-preview_enable_luma_enhancement(struct isp_prev_device *prev, u8 enable)
+preview_enable_luma_enhancement(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -460,11 +436,10 @@ preview_enable_luma_enhancement(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_enable_chroma_suppression - Enables/Disables Chrominance Suppr.
- * @enable: 1 - Enable the Chrominance Suppression.
+ * preview_enable_chroma_suppression - Enable/disable Chrominance Suppression
  */
 static void
-preview_enable_chroma_suppression(struct isp_prev_device *prev, u8 enable)
+preview_enable_chroma_suppression(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
@@ -477,17 +452,16 @@ preview_enable_chroma_suppression(struct isp_prev_device *prev, u8 enable)
 }
 
 /*
- * preview_config_whitebalance - Configures the White Balance parameters.
- * @prev_wbal: Structure containing the digital gain and white balance
- *             coefficient.
+ * preview_config_whitebalance - Configure White Balance parameters
  *
  * Coefficient matrix always with default values.
  */
 static void
-preview_config_whitebalance(struct isp_prev_device *prev, const void *prev_wbal)
+preview_config_whitebalance(struct isp_prev_device *prev,
+			    const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_wbal *wbal = prev_wbal;
+	const struct omap3isp_prev_wbal *wbal = &params->wbal;
 	u32 val;
 
 	isp_reg_writel(isp, wbal->dgain, OMAP3_ISP_IOMEM_PREV, ISPPRV_WB_DGAIN);
@@ -519,15 +493,14 @@ preview_config_whitebalance(struct isp_prev_device *prev, const void *prev_wbal)
 }
 
 /*
- * preview_config_blkadj - Configures the Black Adjustment parameters.
- * @prev_blkadj: Structure containing the black adjustment towards red, green,
- *               blue.
+ * preview_config_blkadj - Configure Black Adjustment
  */
 static void
-preview_config_blkadj(struct isp_prev_device *prev, const void *prev_blkadj)
+preview_config_blkadj(struct isp_prev_device *prev,
+		      const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_blkadj *blkadj = prev_blkadj;
+	const struct omap3isp_prev_blkadj *blkadj = &params->blkadj;
 
 	isp_reg_writel(isp, (blkadj->blue << ISPPRV_BLKADJOFF_B_SHIFT) |
 		       (blkadj->green << ISPPRV_BLKADJOFF_G_SHIFT) |
@@ -536,15 +509,14 @@ preview_config_blkadj(struct isp_prev_device *prev, const void *prev_blkadj)
 }
 
 /*
- * preview_config_rgb_blending - Configures the RGB-RGB Blending matrix.
- * @rgb2rgb: Structure containing the rgb to rgb blending matrix and the rgb
- *           offset.
+ * preview_config_rgb_blending - Configure RGB-RGB Blending
  */
 static void
-preview_config_rgb_blending(struct isp_prev_device *prev, const void *rgb2rgb)
+preview_config_rgb_blending(struct isp_prev_device *prev,
+			    const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_rgbtorgb *rgbrgb = rgb2rgb;
+	const struct omap3isp_prev_rgbtorgb *rgbrgb = &params->rgb2rgb;
 	u32 val;
 
 	val = (rgbrgb->matrix[0][0] & 0xfff) << ISPPRV_RGB_MAT1_MTX_RR_SHIFT;
@@ -575,15 +547,14 @@ preview_config_rgb_blending(struct isp_prev_device *prev, const void *rgb2rgb)
 }
 
 /*
- * Configures the color space conversion (RGB toYCbYCr) matrix
- * @prev_csc: Structure containing the RGB to YCbYCr matrix and the
- *            YCbCr offset.
+ * preview_config_csc - Configure Color Space Conversion (RGB to YCbYCr)
  */
 static void
-preview_config_csc(struct isp_prev_device *prev, const void *prev_csc)
+preview_config_csc(struct isp_prev_device *prev,
+		   const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_csc *csc = prev_csc;
+	const struct omap3isp_prev_csc *csc = &params->csc;
 	u32 val;
 
 	val = (csc->matrix[0][0] & 0x3ff) << ISPPRV_CSC0_RY_SHIFT;
@@ -631,19 +602,19 @@ preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
 }
 
 /*
- * preview_config_contrast - Configures the Contrast.
- * @params: Contrast value (u8 pointer, U8Q0 format).
+ * preview_config_contrast - Configure the Contrast
  *
  * Value should be programmed before enabling the module.
  */
 static void
-preview_config_contrast(struct isp_prev_device *prev, const void *params)
+preview_config_contrast(struct isp_prev_device *prev,
+			const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_CNT_BRT,
 			0xff << ISPPRV_CNT_BRT_CNT_SHIFT,
-			*(u8 *)params << ISPPRV_CNT_BRT_CNT_SHIFT);
+			params->contrast << ISPPRV_CNT_BRT_CNT_SHIFT);
 }
 
 /*
@@ -669,28 +640,28 @@ preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
 }
 
 /*
- * preview_config_brightness - Configures the brightness.
- * @params: Brightness value (u8 pointer, U8Q0 format).
+ * preview_config_brightness - Configure the Brightness
  */
 static void
-preview_config_brightness(struct isp_prev_device *prev, const void *params)
+preview_config_brightness(struct isp_prev_device *prev,
+			  const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
 	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_CNT_BRT,
 			0xff << ISPPRV_CNT_BRT_BRT_SHIFT,
-			*(u8 *)params << ISPPRV_CNT_BRT_BRT_SHIFT);
+			params->brightness << ISPPRV_CNT_BRT_BRT_SHIFT);
 }
 
 /*
- * preview_config_yc_range - Configures the max and min Y and C values.
- * @yclimit: Structure containing the range of Y and C values.
+ * preview_config_yc_range - Configure the max and min Y and C values
  */
 static void
-preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
+preview_config_yc_range(struct isp_prev_device *prev,
+			const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_yclimit *yc = yclimit;
+	const struct omap3isp_prev_yclimit *yc = &params->yclimit;
 
 	isp_reg_writel(isp,
 		       yc->maxC << ISPPRV_SETUP_YC_MAXC_SHIFT |
@@ -771,8 +742,8 @@ static void preview_params_switch(struct isp_prev_device *prev)
 
 /* preview parameters update structure */
 struct preview_update {
-	void (*config)(struct isp_prev_device *, const void *);
-	void (*enable)(struct isp_prev_device *, u8);
+	void (*config)(struct isp_prev_device *, const struct prev_params *);
+	void (*enable)(struct isp_prev_device *, bool);
 	unsigned int param_offset;
 	unsigned int param_size;
 	unsigned int config_offset;
@@ -871,13 +842,11 @@ static const struct preview_update update_attrs[] = {
 	}, /* OMAP3ISP_PREV_CONTRAST */ {
 		preview_config_contrast,
 		NULL,
-		offsetof(struct prev_params, contrast),
-		0, 0, true,
+		0, 0, 0, true,
 	}, /* OMAP3ISP_PREV_BRIGHTNESS */ {
 		preview_config_brightness,
 		NULL,
-		offsetof(struct prev_params, brightness),
-		0, 0, true,
+		0, 0, 0, true,
 	},
 };
 
@@ -972,7 +941,6 @@ static void preview_setup_hw(struct isp_prev_device *prev, u32 update,
 		const struct preview_update *attr = &update_attrs[i];
 		struct prev_params *params;
 		unsigned int bit = 1 << i;
-		void *param_ptr;
 
 		if (!(update & bit))
 			continue;
@@ -980,15 +948,13 @@ static void preview_setup_hw(struct isp_prev_device *prev, u32 update,
 		params = &prev->params.params[!(active & bit)];
 
 		if (params->features & bit) {
-			if (attr->config) {
-				param_ptr = (void *)params + attr->param_offset;
-				attr->config(prev, param_ptr);
-			}
+			if (attr->config)
+				attr->config(prev, params);
 			if (attr->enable)
-				attr->enable(prev, 1);
+				attr->enable(prev, true);
 		} else {
 			if (attr->enable)
-				attr->enable(prev, 0);
+				attr->enable(prev, false);
 		}
 	}
 }
-- 
1.7.8.6

[PATCH v2 4/6] omap3isp: preview: Reorder configuration functions

[Laurent Pinchart]

Reorder the configuration and enable functions to match the parameters
order.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  452 ++++++++++++++--------------
 1 files changed, 226 insertions(+), 226 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 1ac48b7..c325e79 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -157,64 +157,53 @@ static u32 luma_enhance_table[] = {
 };
 
 /*
- * preview_enable_invalaw - Enable/disable Inverse A-Law decompression
- */
-static void preview_enable_invalaw(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_WIDTH | ISPPRV_PCR_INVALAW);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_WIDTH | ISPPRV_PCR_INVALAW);
-}
-
-/*
- * preview_enable_drkframe_capture - Enable/disable Dark Frame Capture
+ * preview_config_luma_enhancement - Configure the Luminance Enhancement table
  */
 static void
-preview_enable_drkframe_capture(struct isp_prev_device *prev, bool enable)
+preview_config_luma_enhancement(struct isp_prev_device *prev,
+				const struct prev_params *params)
 {
 	struct isp_device *isp = to_isp_device(prev);
+	const struct omap3isp_prev_luma *yt = &params->luma;
+	unsigned int i;
 
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DRKFCAP);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DRKFCAP);
+	isp_reg_writel(isp, ISPPRV_YENH_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+	for (i = 0; i < OMAP3ISP_PREV_YENH_TBL_SIZE; i++) {
+		isp_reg_writel(isp, yt->table[i],
+			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
+	}
 }
 
 /*
- * preview_enable_drkframe - Enable/disable Dark Frame Subtraction
+ * preview_enable_luma_enhancement - Enable/disable Luminance Enhancement
  */
-static void preview_enable_drkframe(struct isp_prev_device *prev, bool enable)
+static void
+preview_enable_luma_enhancement(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
 	if (enable)
 		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DRKFEN);
+			    ISPPRV_PCR_YNENHEN);
 	else
 		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DRKFEN);
+			    ISPPRV_PCR_YNENHEN);
 }
 
 /*
- * preview_enable_hmed - Enable/disable the Horizontal Median Filter
+ * preview_enable_invalaw - Enable/disable Inverse A-Law decompression
  */
-static void preview_enable_hmed(struct isp_prev_device *prev, bool enable)
+static void preview_enable_invalaw(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
 
 	if (enable)
 		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_HMEDEN);
+			    ISPPRV_PCR_WIDTH | ISPPRV_PCR_INVALAW);
 	else
 		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_HMEDEN);
+			    ISPPRV_PCR_WIDTH | ISPPRV_PCR_INVALAW);
 }
 
 /*
@@ -233,46 +222,18 @@ static void preview_config_hmed(struct isp_prev_device *prev,
 }
 
 /*
- * preview_config_noisefilter - Configure the Noise Filter
- */
-static void
-preview_config_noisefilter(struct isp_prev_device *prev,
-			   const struct prev_params *params)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_nf *nf = &params->nf;
-	unsigned int i;
-
-	isp_reg_writel(isp, nf->spread, OMAP3_ISP_IOMEM_PREV, ISPPRV_NF);
-	isp_reg_writel(isp, ISPPRV_NF_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-	for (i = 0; i < OMAP3ISP_PREV_NF_TBL_SIZE; i++) {
-		isp_reg_writel(isp, nf->table[i],
-			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
-	}
-}
-
-/*
- * preview_config_dcor - Configure Couplet Defect Correction
+ * preview_enable_hmed - Enable/disable the Horizontal Median Filter
  */
-static void
-preview_config_dcor(struct isp_prev_device *prev,
-		    const struct prev_params *params)
+static void preview_enable_hmed(struct isp_prev_device *prev, bool enable)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_dcor *dcor = &params->dcor;
 
-	isp_reg_writel(isp, dcor->detect_correct[0],
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR0);
-	isp_reg_writel(isp, dcor->detect_correct[1],
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR1);
-	isp_reg_writel(isp, dcor->detect_correct[2],
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR2);
-	isp_reg_writel(isp, dcor->detect_correct[3],
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR3);
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			ISPPRV_PCR_DCCOUP,
-			dcor->couplet_mode_en ? ISPPRV_PCR_DCCOUP : 0);
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_HMEDEN);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_HMEDEN);
 }
 
 /*
@@ -305,55 +266,6 @@ preview_config_cfa(struct isp_prev_device *prev,
 }
 
 /*
- * preview_config_gammacorrn - Configure the Gamma Correction tables
- */
-static void
-preview_config_gammacorrn(struct isp_prev_device *prev,
-			  const struct prev_params *params)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_gtables *gt = &params->gamma;
-	unsigned int i;
-
-	isp_reg_writel(isp, ISPPRV_REDGAMMA_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
-		isp_reg_writel(isp, gt->red[i], OMAP3_ISP_IOMEM_PREV,
-			       ISPPRV_SET_TBL_DATA);
-
-	isp_reg_writel(isp, ISPPRV_GREENGAMMA_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
-		isp_reg_writel(isp, gt->green[i], OMAP3_ISP_IOMEM_PREV,
-			       ISPPRV_SET_TBL_DATA);
-
-	isp_reg_writel(isp, ISPPRV_BLUEGAMMA_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
-		isp_reg_writel(isp, gt->blue[i], OMAP3_ISP_IOMEM_PREV,
-			       ISPPRV_SET_TBL_DATA);
-}
-
-/*
- * preview_config_luma_enhancement - Configure the Luminance Enhancement table
- */
-static void
-preview_config_luma_enhancement(struct isp_prev_device *prev,
-				const struct prev_params *params)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_luma *yt = &params->luma;
-	unsigned int i;
-
-	isp_reg_writel(isp, ISPPRV_YENH_TABLE_ADDR,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
-	for (i = 0; i < OMAP3ISP_PREV_YENH_TBL_SIZE; i++) {
-		isp_reg_writel(isp, yt->table[i],
-			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
-	}
-}
-
-/*
  * preview_config_chroma_suppression - Configure Chroma Suppression
  */
 static void
@@ -370,72 +282,6 @@ preview_config_chroma_suppression(struct isp_prev_device *prev,
 }
 
 /*
- * preview_enable_noisefilter - Enable/disable the Noise Filter
- */
-static void
-preview_enable_noisefilter(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_NFEN);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_NFEN);
-}
-
-/*
- * preview_enable_dcor - Enable/disable Couplet Defect Correction
- */
-static void preview_enable_dcor(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DCOREN);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_DCOREN);
-}
-
-/*
- * preview_enable_gammabypass - Enable/disable Gamma Bypass
- *
- * When gamma bypass is enabled, the output of the gamma correction is the 8 MSB
- * of the 10-bit input .
- */
-static void
-preview_enable_gammabypass(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_GAMMA_BYPASS);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_GAMMA_BYPASS);
-}
-
-/*
- * preview_enable_luma_enhancement - Enable/disable Luminance Enhancement
- */
-static void
-preview_enable_luma_enhancement(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_YNENHEN);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_YNENHEN);
-}
-
-/*
  * preview_enable_chroma_suppression - Enable/disable Chrominance Suppression
  */
 static void
@@ -579,26 +425,175 @@ preview_config_csc(struct isp_prev_device *prev,
 }
 
 /*
- * preview_update_contrast - Updates the contrast.
- * @contrast: Pointer to hold the current programmed contrast value.
+ * preview_config_yc_range - Configure the max and min Y and C values
+ */
+static void
+preview_config_yc_range(struct isp_prev_device *prev,
+			const struct prev_params *params)
+{
+	struct isp_device *isp = to_isp_device(prev);
+	const struct omap3isp_prev_yclimit *yc = &params->yclimit;
+
+	isp_reg_writel(isp,
+		       yc->maxC << ISPPRV_SETUP_YC_MAXC_SHIFT |
+		       yc->maxY << ISPPRV_SETUP_YC_MAXY_SHIFT |
+		       yc->minC << ISPPRV_SETUP_YC_MINC_SHIFT |
+		       yc->minY << ISPPRV_SETUP_YC_MINY_SHIFT,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SETUP_YC);
+}
+
+/*
+ * preview_config_dcor - Configure Couplet Defect Correction
+ */
+static void
+preview_config_dcor(struct isp_prev_device *prev,
+		    const struct prev_params *params)
+{
+	struct isp_device *isp = to_isp_device(prev);
+	const struct omap3isp_prev_dcor *dcor = &params->dcor;
+
+	isp_reg_writel(isp, dcor->detect_correct[0],
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR0);
+	isp_reg_writel(isp, dcor->detect_correct[1],
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR1);
+	isp_reg_writel(isp, dcor->detect_correct[2],
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR2);
+	isp_reg_writel(isp, dcor->detect_correct[3],
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_CDC_THR3);
+	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			ISPPRV_PCR_DCCOUP,
+			dcor->couplet_mode_en ? ISPPRV_PCR_DCCOUP : 0);
+}
+
+/*
+ * preview_enable_dcor - Enable/disable Couplet Defect Correction
+ */
+static void preview_enable_dcor(struct isp_prev_device *prev, bool enable)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DCOREN);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DCOREN);
+}
+
+/*
+ * preview_enable_gammabypass - Enable/disable Gamma Bypass
  *
- * Value should be programmed before enabling the module.
+ * When gamma bypass is enabled, the output of the gamma correction is the 8 MSB
+ * of the 10-bit input .
  */
 static void
-preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
+preview_enable_gammabypass(struct isp_prev_device *prev, bool enable)
 {
-	struct prev_params *params;
-	unsigned long flags;
+	struct isp_device *isp = to_isp_device(prev);
 
-	spin_lock_irqsave(&prev->params.lock, flags);
-	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
-	       ? &prev->params.params[0] : &prev->params.params[1];
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_GAMMA_BYPASS);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_GAMMA_BYPASS);
+}
 
-	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
-		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
-		params->update |= OMAP3ISP_PREV_CONTRAST;
+/*
+ * preview_enable_drkframe_capture - Enable/disable Dark Frame Capture
+ */
+static void
+preview_enable_drkframe_capture(struct isp_prev_device *prev, bool enable)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DRKFCAP);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DRKFCAP);
+}
+
+/*
+ * preview_enable_drkframe - Enable/disable Dark Frame Subtraction
+ */
+static void preview_enable_drkframe(struct isp_prev_device *prev, bool enable)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DRKFEN);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_DRKFEN);
+}
+
+/*
+ * preview_config_noisefilter - Configure the Noise Filter
+ */
+static void
+preview_config_noisefilter(struct isp_prev_device *prev,
+			   const struct prev_params *params)
+{
+	struct isp_device *isp = to_isp_device(prev);
+	const struct omap3isp_prev_nf *nf = &params->nf;
+	unsigned int i;
+
+	isp_reg_writel(isp, nf->spread, OMAP3_ISP_IOMEM_PREV, ISPPRV_NF);
+	isp_reg_writel(isp, ISPPRV_NF_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+	for (i = 0; i < OMAP3ISP_PREV_NF_TBL_SIZE; i++) {
+		isp_reg_writel(isp, nf->table[i],
+			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
 	}
-	spin_unlock_irqrestore(&prev->params.lock, flags);
+}
+
+/*
+ * preview_enable_noisefilter - Enable/disable the Noise Filter
+ */
+static void
+preview_enable_noisefilter(struct isp_prev_device *prev, bool enable)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (enable)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_NFEN);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_NFEN);
+}
+
+/*
+ * preview_config_gammacorrn - Configure the Gamma Correction tables
+ */
+static void
+preview_config_gammacorrn(struct isp_prev_device *prev,
+			  const struct prev_params *params)
+{
+	struct isp_device *isp = to_isp_device(prev);
+	const struct omap3isp_prev_gtables *gt = &params->gamma;
+	unsigned int i;
+
+	isp_reg_writel(isp, ISPPRV_REDGAMMA_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
+		isp_reg_writel(isp, gt->red[i], OMAP3_ISP_IOMEM_PREV,
+			       ISPPRV_SET_TBL_DATA);
+
+	isp_reg_writel(isp, ISPPRV_GREENGAMMA_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
+		isp_reg_writel(isp, gt->green[i], OMAP3_ISP_IOMEM_PREV,
+			       ISPPRV_SET_TBL_DATA);
+
+	isp_reg_writel(isp, ISPPRV_BLUEGAMMA_TABLE_ADDR,
+		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
+	for (i = 0; i < OMAP3ISP_PREV_GAMMA_TBL_SIZE; i++)
+		isp_reg_writel(isp, gt->blue[i], OMAP3_ISP_IOMEM_PREV,
+			       ISPPRV_SET_TBL_DATA);
 }
 
 /*
@@ -618,57 +613,62 @@ preview_config_contrast(struct isp_prev_device *prev,
 }
 
 /*
- * preview_update_brightness - Updates the brightness in preview module.
- * @brightness: Pointer to hold the current programmed brightness value.
+ * preview_config_brightness - Configure the Brightness
+ */
+static void
+preview_config_brightness(struct isp_prev_device *prev,
+			  const struct prev_params *params)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_CNT_BRT,
+			0xff << ISPPRV_CNT_BRT_BRT_SHIFT,
+			params->brightness << ISPPRV_CNT_BRT_BRT_SHIFT);
+}
+
+/*
+ * preview_update_contrast - Updates the contrast.
+ * @contrast: Pointer to hold the current programmed contrast value.
  *
+ * Value should be programmed before enabling the module.
  */
 static void
-preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
+preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
 {
 	struct prev_params *params;
 	unsigned long flags;
 
 	spin_lock_irqsave(&prev->params.lock, flags);
-	params = (prev->params.active & OMAP3ISP_PREV_BRIGHTNESS)
+	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
 	       ? &prev->params.params[0] : &prev->params.params[1];
 
-	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
-		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
-		params->update |= OMAP3ISP_PREV_BRIGHTNESS;
+	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
+		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
+		params->update |= OMAP3ISP_PREV_CONTRAST;
 	}
 	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /*
- * preview_config_brightness - Configure the Brightness
+ * preview_update_brightness - Updates the brightness in preview module.
+ * @brightness: Pointer to hold the current programmed brightness value.
+ *
  */
 static void
-preview_config_brightness(struct isp_prev_device *prev,
-			  const struct prev_params *params)
+preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
 {
-	struct isp_device *isp = to_isp_device(prev);
-
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_CNT_BRT,
-			0xff << ISPPRV_CNT_BRT_BRT_SHIFT,
-			params->brightness << ISPPRV_CNT_BRT_BRT_SHIFT);
-}
+	struct prev_params *params;
+	unsigned long flags;
 
-/*
- * preview_config_yc_range - Configure the max and min Y and C values
- */
-static void
-preview_config_yc_range(struct isp_prev_device *prev,
-			const struct prev_params *params)
-{
-	struct isp_device *isp = to_isp_device(prev);
-	const struct omap3isp_prev_yclimit *yc = &params->yclimit;
+	spin_lock_irqsave(&prev->params.lock, flags);
+	params = (prev->params.active & OMAP3ISP_PREV_BRIGHTNESS)
+	       ? &prev->params.params[0] : &prev->params.params[1];
 
-	isp_reg_writel(isp,
-		       yc->maxC << ISPPRV_SETUP_YC_MAXC_SHIFT |
-		       yc->maxY << ISPPRV_SETUP_YC_MAXY_SHIFT |
-		       yc->minC << ISPPRV_SETUP_YC_MINC_SHIFT |
-		       yc->minY << ISPPRV_SETUP_YC_MINY_SHIFT,
-		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SETUP_YC);
+	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
+		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
+		params->update |= OMAP3ISP_PREV_BRIGHTNESS;
+	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 static u32
-- 
1.7.8.6

[PATCH v2 5/6] omap3isp: preview: Merge gamma correction and gamma bypass

[Laurent Pinchart]

Enabling gamma bypass disables gamma correction and vice versa. Merge
the two parameters.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |   42 ++++++++++++++--------------
 1 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index c325e79..71ce0f4 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -481,25 +481,6 @@ static void preview_enable_dcor(struct isp_prev_device *prev, bool enable)
 }
 
 /*
- * preview_enable_gammabypass - Enable/disable Gamma Bypass
- *
- * When gamma bypass is enabled, the output of the gamma correction is the 8 MSB
- * of the 10-bit input .
- */
-static void
-preview_enable_gammabypass(struct isp_prev_device *prev, bool enable)
-{
-	struct isp_device *isp = to_isp_device(prev);
-
-	if (enable)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_GAMMA_BYPASS);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_GAMMA_BYPASS);
-}
-
-/*
  * preview_enable_drkframe_capture - Enable/disable Dark Frame Capture
  */
 static void
@@ -597,6 +578,25 @@ preview_config_gammacorrn(struct isp_prev_device *prev,
 }
 
 /*
+ * preview_enable_gammacorrn - Enable/disable Gamma Correction
+ *
+ * When gamma correction is disabled, the module is bypassed and its output is
+ * the 8 MSB of the 10-bit input .
+ */
+static void
+preview_enable_gammacorrn(struct isp_prev_device *prev, bool enable)
+{
+	struct isp_device *isp = to_isp_device(prev);
+
+	if (enable)
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_GAMMA_BYPASS);
+	else
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_GAMMA_BYPASS);
+}
+
+/*
  * preview_config_contrast - Configure the Contrast
  *
  * Value should be programmed before enabling the module.
@@ -817,7 +817,7 @@ static const struct preview_update update_attrs[] = {
 		offsetof(struct omap3isp_prev_update_config, dcor),
 	}, /* OMAP3ISP_PREV_GAMMABYPASS */ {
 		NULL,
-		preview_enable_gammabypass,
+		NULL,
 	}, /* OMAP3ISP_PREV_DRK_FRM_CAPTURE */ {
 		NULL,
 		preview_enable_drkframe_capture,
@@ -835,7 +835,7 @@ static const struct preview_update update_attrs[] = {
 		offsetof(struct omap3isp_prev_update_config, nf),
 	}, /* OMAP3ISP_PREV_GAMMA */ {
 		preview_config_gammacorrn,
-		NULL,
+		preview_enable_gammacorrn,
 		offsetof(struct prev_params, gamma),
 		FIELD_SIZEOF(struct prev_params, gamma),
 		offsetof(struct omap3isp_prev_update_config, gamma),
-- 
1.7.8.6

[PATCH v2 6/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Rearrange the CFA interpolation coefficients table based on the Bayer
pattern. Support for non-Bayer CFA patterns is dropped as they were not
correctly supported, and have never been tested.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  134 ++++++++++++++++++-----------
 drivers/media/video/omap3isp/isppreview.h |    1 +
 2 files changed, 85 insertions(+), 50 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 71ce0f4..475908b 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -236,20 +236,30 @@ static void preview_enable_hmed(struct isp_prev_device *prev, bool enable)
 			    ISPPRV_PCR_HMEDEN);
 }
 
+#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
+
 /*
- * preview_config_cfa - Configure CFA Interpolation
- */
-static void
-preview_config_cfa(struct isp_prev_device *prev,
-		   const struct prev_params *params)
-{
-	struct isp_device *isp = to_isp_device(prev);
+ * preview_config_cfa - Configure CFA Interpolation for Bayer formats
+ *
+ * The CFA table is organised in four blocks, one per Bayer component. The
+ * hardware expects blocks to follow the Bayer order of the input data, while
+ * the driver stores the table in GRBG order in memory. The blocks need to be
+ * reordered to support non-GRBG Bayer patterns.
+ */
+static void preview_config_cfa(struct isp_prev_device *prev,
+			       const struct prev_params *params)
+{
+	static const unsigned int cfa_coef_order[4][4] = {
+		{ 0, 1, 2, 3 }, /* GRBG */
+		{ 1, 0, 3, 2 }, /* RGGB */
+		{ 2, 3, 0, 1 }, /* BGGR */
+		{ 3, 2, 1, 0 }, /* GBRG */
+	};
+	const unsigned int *order = cfa_coef_order[prev->params.cfa_order];
 	const struct omap3isp_prev_cfa *cfa = &params->cfa;
+	struct isp_device *isp = to_isp_device(prev);
 	unsigned int i;
-
-	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			ISPPRV_PCR_CFAFMT_MASK,
-			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
+	unsigned int j;
 
 	isp_reg_writel(isp,
 		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
@@ -259,9 +269,13 @@ preview_config_cfa(struct isp_prev_device *prev,
 	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
 		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
 
-	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
-		isp_reg_writel(isp, cfa->table[i],
-			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
+	for (i = 0; i < 4; ++i) {
+		const __u32 *block = cfa->table
+				   + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
+
+		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
+			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
+				       ISPPRV_SET_TBL_DATA);
 	}
 }
 
@@ -993,42 +1007,62 @@ preview_config_ycpos(struct isp_prev_device *prev,
 static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	struct prev_params *params;
-	int reg = 0;
 
-	params = (prev->params.active & OMAP3ISP_PREV_CFA)
-	       ? &prev->params.params[0] : &prev->params.params[1];
-
-	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
-		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
-		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
-		      average;
-	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
-		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
-		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
-		      average;
-	isp_reg_writel(isp, reg, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
+	isp_reg_writel(isp, ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
+		       ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
+		       average, OMAP3_ISP_IOMEM_PREV, ISPPRV_AVE);
 }
 
+
+#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
+
 /*
  * preview_config_input_format - Configure the input format
  * @prev: The preview engine
  * @format: Format on the preview engine sink pad
  *
- * Enable CFA interpolation for Bayer formats and disable it for greyscale
- * formats.
+ * Enable and configure CFA interpolation for Bayer formats and disable it for
+ * greyscale formats.
+ *
+ * The CFA table is organised in four blocks, one per Bayer component. The
+ * hardware expects blocks to follow the Bayer order of the input data, while
+ * the driver stores the table in GRBG order in memory. The blocks need to be
+ * reordered to support non-GRBG Bayer patterns.
  */
 static void preview_config_input_format(struct isp_prev_device *prev,
 					const struct v4l2_mbus_framefmt *format)
 {
 	struct isp_device *isp = to_isp_device(prev);
+	struct prev_params *params;
 
-	if (format->code != V4L2_MBUS_FMT_Y10_1X10)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_CFAEN);
-	else
+	switch (format->code) {
+	case V4L2_MBUS_FMT_SGRBG10_1X10:
+		prev->params.cfa_order = 0;
+		break;
+	case V4L2_MBUS_FMT_SRGGB10_1X10:
+		prev->params.cfa_order = 1;
+		break;
+	case V4L2_MBUS_FMT_SBGGR10_1X10:
+		prev->params.cfa_order = 2;
+		break;
+	case V4L2_MBUS_FMT_SGBRG10_1X10:
+		prev->params.cfa_order = 3;
+		break;
+	default:
+		/* Disable CFA for non-Bayer formats. */
 		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
 			    ISPPRV_PCR_CFAEN);
+		return;
+	}
+
+	isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR, ISPPRV_PCR_CFAEN);
+	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			ISPPRV_PCR_CFAFMT_MASK, ISPPRV_PCR_CFAFMT_BAYER);
+
+	params = (prev->params.active & OMAP3ISP_PREV_CFA)
+	       ? &prev->params.params[0] : &prev->params.params[1];
+
+	preview_config_cfa(prev, params);
 }
 
 /*
@@ -1371,22 +1405,6 @@ static void preview_configure(struct isp_prev_device *prev)
 	active = prev->params.active;
 	spin_unlock_irqrestore(&prev->params.lock, flags);
 
-	preview_setup_hw(prev, update, active);
-
-	if (prev->output & PREVIEW_OUTPUT_MEMORY)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_SDRPORT);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_SDRPORT);
-
-	if (prev->output & PREVIEW_OUTPUT_RESIZER)
-		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_RSZPORT);
-	else
-		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
-			    ISPPRV_PCR_RSZPORT);
-
 	/* PREV_PAD_SINK */
 	format = &prev->formats[PREV_PAD_SINK];
 
@@ -1401,10 +1419,26 @@ static void preview_configure(struct isp_prev_device *prev)
 		preview_config_inlineoffset(prev,
 				ALIGN(format->width, 0x20) * 2);
 
+	preview_setup_hw(prev, update, active);
+
 	/* PREV_PAD_SOURCE */
 	format = &prev->formats[PREV_PAD_SOURCE];
 
 	if (prev->output & PREVIEW_OUTPUT_MEMORY)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_SDRPORT);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_SDRPORT);
+
+	if (prev->output & PREVIEW_OUTPUT_RESIZER)
+		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_RSZPORT);
+	else
+		isp_reg_clr(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
+			    ISPPRV_PCR_RSZPORT);
+
+	if (prev->output & PREVIEW_OUTPUT_MEMORY)
 		preview_config_outlineoffset(prev,
 				ALIGN(format->width, 0x10) * 2);
 
diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index 6663ab6..f669234 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -144,6 +144,7 @@ struct isp_prev_device {
 	struct isp_video video_out;
 
 	struct {
+		unsigned int cfa_order;
 		struct prev_params params[2];
 		u32 active;
 		spinlock_t lock;
-- 
1.7.8.6

Re: [PATCH v2 5/6] omap3isp: preview: Merge gamma correction and gamma bypass

[Sakari Ailus]

Hi Laurent,

Thanks for the patches.

Laurent Pinchart wrote:
...
> @@ -817,7 +817,7 @@ static const struct preview_update update_attrs[] = {
>  		offsetof(struct omap3isp_prev_update_config, dcor),
>  	}, /* OMAP3ISP_PREV_GAMMABYPASS */ {
>  		NULL,
> -		preview_enable_gammabypass,
> +		NULL,
>  	}, /* OMAP3ISP_PREV_DRK_FRM_CAPTURE */ {
>  		NULL,
>  		preview_enable_drkframe_capture,
> @@ -835,7 +835,7 @@ static const struct preview_update update_attrs[] = {
>  		offsetof(struct omap3isp_prev_update_config, nf),
>  	}, /* OMAP3ISP_PREV_GAMMA */ {
>  		preview_config_gammacorrn,
> -		NULL,
> +		preview_enable_gammacorrn,
>  		offsetof(struct prev_params, gamma),
>  		FIELD_SIZEOF(struct prev_params, gamma),
>  		offsetof(struct omap3isp_prev_update_config, gamma),

Doesn't this change the behaviour of the user space API?

I'm not sure if we _really_ need to worry about that _too_ much, but I
think that if OMAP3ISP_PREV_GAMMABYPASS is no longer used anywhere the
definition should be removed as well to prevent anyone accidentally from
using it.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2 5/6] omap3isp: preview: Merge gamma correction and gamma bypass

[Laurent Pinchart]

Hi Sakari,

On Friday 13 July 2012 12:51:36 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> ...
> 
> > @@ -817,7 +817,7 @@ static const struct preview_update update_attrs[] = {
> >  		offsetof(struct omap3isp_prev_update_config, dcor),
> >  	}, /* OMAP3ISP_PREV_GAMMABYPASS */ {
> >  		NULL,
> > -		preview_enable_gammabypass,
> > +		NULL,
> >  	}, /* OMAP3ISP_PREV_DRK_FRM_CAPTURE */ {
> >  		NULL,
> >  		preview_enable_drkframe_capture,
> > @@ -835,7 +835,7 @@ static const struct preview_update update_attrs[] = {
> >  		offsetof(struct omap3isp_prev_update_config, nf),
> >  	}, /* OMAP3ISP_PREV_GAMMA */ {
> >  		preview_config_gammacorrn,
> > -		NULL,
> > +		preview_enable_gammacorrn,
> >  		offsetof(struct prev_params, gamma),
> >  		FIELD_SIZEOF(struct prev_params, gamma),
> >  		offsetof(struct omap3isp_prev_update_config, gamma),
> 
> Doesn't this change the behaviour of the user space API?

That's correct, it does.

> I'm not sure if we _really_ need to worry about that _too_ much, but I
> think that if OMAP3ISP_PREV_GAMMABYPASS is no longer used anywhere the
> definition should be removed as well to prevent anyone accidentally from
> using it.

Good point. I'll replace it with a comment that warns not to use bit 11.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

Laurent Pinchart wrote:
> Rearrange the CFA interpolation coefficients table based on the Bayer
> pattern. Support for non-Bayer CFA patterns is dropped as they were not
> correctly supported, and have never been tested.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |  134 ++++++++++++++++++-----------
>  drivers/media/video/omap3isp/isppreview.h |    1 +
>  2 files changed, 85 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index 71ce0f4..475908b 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -236,20 +236,30 @@ static void preview_enable_hmed(struct isp_prev_device *prev, bool enable)
>  			    ISPPRV_PCR_HMEDEN);
>  }
>  
> +#define OMAP3ISP_PREV_CFA_BLK_SIZE	(OMAP3ISP_PREV_CFA_TBL_SIZE / 4)
> +
>  /*
> - * preview_config_cfa - Configure CFA Interpolation
> - */
> -static void
> -preview_config_cfa(struct isp_prev_device *prev,
> -		   const struct prev_params *params)
> -{
> -	struct isp_device *isp = to_isp_device(prev);
> + * preview_config_cfa - Configure CFA Interpolation for Bayer formats
> + *
> + * The CFA table is organised in four blocks, one per Bayer component. The
> + * hardware expects blocks to follow the Bayer order of the input data, while
> + * the driver stores the table in GRBG order in memory. The blocks need to be
> + * reordered to support non-GRBG Bayer patterns.
> + */
> +static void preview_config_cfa(struct isp_prev_device *prev,
> +			       const struct prev_params *params)
> +{
> +	static const unsigned int cfa_coef_order[4][4] = {
> +		{ 0, 1, 2, 3 }, /* GRBG */
> +		{ 1, 0, 3, 2 }, /* RGGB */
> +		{ 2, 3, 0, 1 }, /* BGGR */
> +		{ 3, 2, 1, 0 }, /* GBRG */
> +	};
> +	const unsigned int *order = cfa_coef_order[prev->params.cfa_order];
>  	const struct omap3isp_prev_cfa *cfa = &params->cfa;
> +	struct isp_device *isp = to_isp_device(prev);
>  	unsigned int i;
> -
> -	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> -			ISPPRV_PCR_CFAFMT_MASK,
> -			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
> +	unsigned int j;
>  
>  	isp_reg_writel(isp,
>  		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> @@ -259,9 +269,13 @@ preview_config_cfa(struct isp_prev_device *prev,
>  	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
>  		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
>  
> -	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
> -		isp_reg_writel(isp, cfa->table[i],
> -			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> +	for (i = 0; i < 4; ++i) {
> +		const __u32 *block = cfa->table
> +				   + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> +
> +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> +				       ISPPRV_SET_TBL_DATA);
>  	}
>  }
>  

I think struct omap3isp_prev_cfa would benefit from more detailed
definition of the gamma table. That would also change the API albeit not
ABI... unless you use an anonymous union which then requires a GCC newer
than or equal to 4.4, I think.

The table should be two-dimensional array, not one-dimensional.

Now that we're making changes to the user space API anyway, what would
you think about this? I think it'd make the code much nicer.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH v2 6/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[Laurent Pinchart]

Hi Sakari,

Thanks for the review.

On Friday 13 July 2012 13:12:27 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > Rearrange the CFA interpolation coefficients table based on the Bayer
> > pattern. Support for non-Bayer CFA patterns is dropped as they were not
> > correctly supported, and have never been tested.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> > +static void preview_config_cfa(struct isp_prev_device *prev,
> > +			       const struct prev_params *params)
> > +{
> > +	static const unsigned int cfa_coef_order[4][4] = {
> > +		{ 0, 1, 2, 3 }, /* GRBG */
> > +		{ 1, 0, 3, 2 }, /* RGGB */
> > +		{ 2, 3, 0, 1 }, /* BGGR */
> > +		{ 3, 2, 1, 0 }, /* GBRG */
> > +	};
> > +	const unsigned int *order = cfa_coef_order[prev->params.cfa_order];
> >  	const struct omap3isp_prev_cfa *cfa = &params->cfa;
> > +	struct isp_device *isp = to_isp_device(prev);
> >  	unsigned int i;
> > -
> > -	isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> > -			ISPPRV_PCR_CFAFMT_MASK,
> > -			cfa->format << ISPPRV_PCR_CFAFMT_SHIFT);
> > +	unsigned int j;
> > 
> >  	isp_reg_writel(isp,
> >  		(cfa->gradthrs_vert << ISPPRV_CFA_GRADTH_VER_SHIFT) |
> > @@ -259,9 +269,13 @@ preview_config_cfa(struct isp_prev_device *prev,
> >  	isp_reg_writel(isp, ISPPRV_CFA_TABLE_ADDR,
> >  		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_ADDR);
> > 
> > -	for (i = 0; i < OMAP3ISP_PREV_CFA_TBL_SIZE; i++) {
> > -		isp_reg_writel(isp, cfa->table[i],
> > -			       OMAP3_ISP_IOMEM_PREV, ISPPRV_SET_TBL_DATA);
> > +	for (i = 0; i < 4; ++i) {
> > +		const __u32 *block = cfa->table
> > +				   + order[i] * OMAP3ISP_PREV_CFA_BLK_SIZE;
> > +
> > +		for (j = 0; j < OMAP3ISP_PREV_CFA_BLK_SIZE; ++j)
> > +			isp_reg_writel(isp, block[j], OMAP3_ISP_IOMEM_PREV,
> > +				       ISPPRV_SET_TBL_DATA);
> >  	}
> >  }
> 
> I think struct omap3isp_prev_cfa would benefit from more detailed definition
> of the gamma table.

I suppose you mean the CFA table.

> That would also change the API albeit not ABI... unless you use an anonymous
> union which then requires a GCC newer than or equal to 4.4, I think.
> 
> The table should be two-dimensional array, not one-dimensional.
> 
> Now that we're making changes to the user space API anyway, what would
> you think about this? I think it'd make the code much nicer.

It's a good idea. I'll fix that. I'd like to document the table contents as 
well, but the information is unfortunately not available publicly :-(

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] omap3isp: preview: Add support for non-GRBG Bayer patterns

[jean-philippe francois]

2012/7/6 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi everybody,
>
> Here's the second version of the non-GRBG Bayer patterns support for the OMAP3
> ISP preview engine. Compared to v1, the CFA table can be reconfigured at
> runtime, which resulted in several cleanup patches.
>
> The first patch is a v3.5 regression fix, I'll send a separate pull request.
>
> Jean-Philippe, could you please test this patch set on your hardware ? It's
> based on top of the latest linuxtv/staging/for_v3.6 branch.

Hi Laurent,

Sorry for the delay, but I was on vacation last week, and will be next week,
so I won't be able to test anything before  July 23rd.
I will report here when tested.

Regards,
Jean-Philippe François

>
> Laurent Pinchart (6):
>   omap3isp: preview: Fix contrast and brightness handling
>   omap3isp: preview: Remove lens shading compensation support
>   omap3isp: preview: Pass a prev_params pointer to configuration
>     functions
>   omap3isp: preview: Reorder configuration functions
>   omap3isp: preview: Merge gamma correction and gamma bypass
>   omap3isp: preview: Add support for non-GRBG Bayer patterns
>
>  drivers/media/video/omap3isp/isppreview.c |  706 ++++++++++++++---------------
>  drivers/media/video/omap3isp/isppreview.h |    1 +
>  2 files changed, 346 insertions(+), 361 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>