Re: [PATCH] ov772x: add edge contrl support

2009-03-23 Thread Guennadi Liakhovetski 
On Mon, 23 Mar 2009, Kuninori Morimoto wrote:

 
 Signed-off-by: Kuninori Morimoto morimoto.kunin...@renesas.com
 ---
 This patch is 1st step for extra settings
 
  drivers/media/video/ov772x.c |   34 ++
  include/media/ov772x.h   |   25 +
  2 files changed, 59 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
 index 34c9819..a951327 100644
 --- a/drivers/media/video/ov772x.c
 +++ b/drivers/media/video/ov772x.c
 @@ -358,6 +358,15 @@
  #define VOSZ_VGA0xF0
  #define VOSZ_QVGA   0x78
  
 +/* EDGE CTRL
 + * see alse
 + *ov772x.h :: for Edge ctrl
 + */
 +#define EDGE0CTRL(param) (((param)  24)  0x1F)
 +#define EDGE1CTRL(param) (((param)  16)  0x0F)
 +#define EDGE2CTRL(param) (((param)   8)  0xFF)
 +#define EDGE3CTRL(param) (((param)   0)  0xFF)
 +
  /*
   * ID
   */
 @@ -816,6 +825,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, 
 u32 width, u32 height,
   ov772x_reset(priv-client);
  
   /*
 +  * set Edge Ctrl if platform has edgectrl
 +  */
 + if (priv-info-edgectrl  OV772X_EDGECTRL_ENABLE) {
 + ret = ov772x_mask_set(priv-client,
 + EDGE0, 0x1F, EDGE0CTRL(priv-info-edgectrl));
 + if (ret  0)
 + goto ov772x_set_fmt_error;
 +
 + ret = ov772x_mask_set(priv-client,
 + EDGE1, 0x0F, EDGE1CTRL(priv-info-edgectrl));
 + if (ret  0)
 + goto ov772x_set_fmt_error;
 +
 + ret = ov772x_mask_set(priv-client,
 + EDGE2, 0xFF, EDGE2CTRL(priv-info-edgectrl));
 + if (ret  0)
 + goto ov772x_set_fmt_error;
 +
 + ret = ov772x_mask_set(priv-client,
 + EDGE3, 0xFF, EDGE3CTRL(priv-info-edgectrl));
 + if (ret  0)
 + goto ov772x_set_fmt_error;
 + }
 +
 + /*
* set size format
*/
   ret = ov772x_write_array(priv-client, priv-win-regs);
 diff --git a/include/media/ov772x.h b/include/media/ov772x.h
 index 57db48d..5b083dc 100644
 --- a/include/media/ov772x.h
 +++ b/include/media/ov772x.h
 @@ -17,9 +17,34 @@
  #define OV772X_FLAG_VFLIP 0x0001 /* Vertical flip image */
  #define OV772X_FLAG_HFLIP 0x0002 /* Horizontal flip image */
  
 +/*
 + * for Edge ctrl
 + *
 + * strength  : (for EDGE0) Edge enhancement strength control
 + * threshold : (for EDGE1) Edge enhancement threshold control
 + * low   : (for EDGE2) Edge enhancement strength Low point control
 + * high  : (for EDGE3) Edge enhancement strength High point control
 + *
 + * Meaning of edgectrl bit
 + *
 + * Exx0       
 + *
 + * E: use edgectrl or not (OV772X_EDGECTRL_ENABLE)
 + * 0: for Edge0 ctrl
 + * 1: for Edge1 ctrl
 + * 2: for Edge2 ctrl
 + * 3: for Edge3 ctrl
 + */
 +#define OV772X_EDGECTRL_ENABLE   0x8000
 +#define OV772X_EDGECTRL(strength, threshold, low, high) \
 + (OV772X_EDGECTRL_ENABLE | \
 +  (strength  0x1F)  24 | (threshold  0x0F)  16 | \
 +  (low  0xFF)  8 | (high  0xFF)  0)
 +
  struct ov772x_camera_info {
   unsigned long  buswidth;
   unsigned long  flags;
 + unsigned long  edgectrl;

Wouldn't it be easier to use

unsigned char   edge_strength;
unsigned char   edge_threshold;
unsigned char   edge_low;
unsigned char   edge_high;

and thus avoid all that shifting?

   struct soc_camera_link link;
  };
  
 -- 
 1.5.6.3

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
--
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: [PATCH] ov772x: add edge contrl support

2009-03-23 Thread morimoto . kuninori 

Dear Guennadi

Thank you for checking

 Wouldn't it be easier to use
 
   unsigned char   edge_strength;
   unsigned char   edge_threshold;
   unsigned char   edge_low;
   unsigned char   edge_high;
 
 and thus avoid all that shifting?

Yes. it is easier to use.
But, driver should judge whether to use this setting or not.
Because this setting is optional.

Because user setting might be 0,
we can not judge it like this.
  if (edge_xxx)
   ov772x_mask_set(  )

So, we can use un-used bit to judge whether to use it.
and edge_strength and edge_threshold have un-used bit.
But edge_low and edge_high doesn't have un-used bit.

Another way to judge it is to use pointer or to add another variable.
But I don't like these style.
What do you think about this ?

Best regards
--
Kuninori Morimoto
 
--
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: [PATCH] ov772x: add edge contrl support

2009-03-23 Thread Guennadi Liakhovetski 
On Mon, 23 Mar 2009, morimoto.kunin...@renesas.com wrote:

 Dear Guennadi
 
 Thank you for checking
 
  Wouldn't it be easier to use
  
  unsigned char   edge_strength;
  unsigned char   edge_threshold;
  unsigned char   edge_low;
  unsigned char   edge_high;
  
  and thus avoid all that shifting?
 
 Yes. it is easier to use.
 But, driver should judge whether to use this setting or not.
 Because this setting is optional.
 
 Because user setting might be 0,
 we can not judge it like this.
   if (edge_xxx)
ov772x_mask_set(  )
 
 So, we can use un-used bit to judge whether to use it.
 and edge_strength and edge_threshold have un-used bit.
 But edge_low and edge_high doesn't have un-used bit.
 
 Another way to judge it is to use pointer or to add another variable.
 But I don't like these style.
 What do you think about this ?

Is edge_strength == 0 a useful edge configuration? Cannot you use it as a 
test whether to set all edge parameters or not? If you cannot, well, just 
do the same as what you have done with 32-bits - use one bit in strength 
as edge enable - just exactly in the same way as in your patch. Like

if (edge_strength  EDGE_ENABLE) {
set_strength;
set_threshold;
set_low;
set_high;
}

Thanks
Guennadi
---
Guennadi Liakhovetski
--
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: [PATCH] ov772x: add edge contrl support

2009-03-23 Thread morimoto . kuninori 

Dear Guennadi

 Is edge_strength == 0 a useful edge configuration? Cannot you use it as a 
 test whether to set all edge parameters or not? If you cannot, well, just 
 do the same as what you have done with 32-bits - use one bit in strength 
 as edge enable - just exactly in the same way as in your patch. Like
 
   if (edge_strength  EDGE_ENABLE) {
   set_strength;
   set_threshold;
   set_low;
   set_high;
   }

Hmm..
edge_threshold has 4 un-used bit.
we can use it for judge.

And sorry, I don't like this style
unsigned char   edge_strength;
unsigned char   edge_threshold;
unsigned char   edge_low;
unsigned char   edge_high;

I will create new struct ov772x_edge for it.

Best regards
--
Kuninori Morimoto
 
--
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

[PATCH] ov772x: add edge contrl support

2009-03-22 Thread Kuninori Morimoto 

Signed-off-by: Kuninori Morimoto morimoto.kunin...@renesas.com
---
This patch is 1st step for extra settings

 drivers/media/video/ov772x.c |   34 ++
 include/media/ov772x.h   |   25 +
 2 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 34c9819..a951327 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -358,6 +358,15 @@
 #define VOSZ_VGA0xF0
 #define VOSZ_QVGA   0x78
 
+/* EDGE CTRL
+ * see alse
+ *ov772x.h :: for Edge ctrl
+ */
+#define EDGE0CTRL(param) (((param)  24)  0x1F)
+#define EDGE1CTRL(param) (((param)  16)  0x0F)
+#define EDGE2CTRL(param) (((param)   8)  0xFF)
+#define EDGE3CTRL(param) (((param)   0)  0xFF)
+
 /*
  * ID
  */
@@ -816,6 +825,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 
width, u32 height,
ov772x_reset(priv-client);
 
/*
+* set Edge Ctrl if platform has edgectrl
+*/
+   if (priv-info-edgectrl  OV772X_EDGECTRL_ENABLE) {
+   ret = ov772x_mask_set(priv-client,
+   EDGE0, 0x1F, EDGE0CTRL(priv-info-edgectrl));
+   if (ret  0)
+   goto ov772x_set_fmt_error;
+
+   ret = ov772x_mask_set(priv-client,
+   EDGE1, 0x0F, EDGE1CTRL(priv-info-edgectrl));
+   if (ret  0)
+   goto ov772x_set_fmt_error;
+
+   ret = ov772x_mask_set(priv-client,
+   EDGE2, 0xFF, EDGE2CTRL(priv-info-edgectrl));
+   if (ret  0)
+   goto ov772x_set_fmt_error;
+
+   ret = ov772x_mask_set(priv-client,
+   EDGE3, 0xFF, EDGE3CTRL(priv-info-edgectrl));
+   if (ret  0)
+   goto ov772x_set_fmt_error;
+   }
+
+   /*
 * set size format
 */
ret = ov772x_write_array(priv-client, priv-win-regs);
diff --git a/include/media/ov772x.h b/include/media/ov772x.h
index 57db48d..5b083dc 100644
--- a/include/media/ov772x.h
+++ b/include/media/ov772x.h
@@ -17,9 +17,34 @@
 #define OV772X_FLAG_VFLIP 0x0001 /* Vertical flip image */
 #define OV772X_FLAG_HFLIP 0x0002 /* Horizontal flip image */
 
+/*
+ * for Edge ctrl
+ *
+ * strength  : (for EDGE0) Edge enhancement strength control
+ * threshold : (for EDGE1) Edge enhancement threshold control
+ * low   : (for EDGE2) Edge enhancement strength Low point control
+ * high  : (for EDGE3) Edge enhancement strength High point control
+ *
+ * Meaning of edgectrl bit
+ *
+ * Exx0       
+ *
+ * E: use edgectrl or not (OV772X_EDGECTRL_ENABLE)
+ * 0: for Edge0 ctrl
+ * 1: for Edge1 ctrl
+ * 2: for Edge2 ctrl
+ * 3: for Edge3 ctrl
+ */
+#define OV772X_EDGECTRL_ENABLE 0x8000
+#define OV772X_EDGECTRL(strength, threshold, low, high) \
+   (OV772X_EDGECTRL_ENABLE | \
+(strength  0x1F)  24 | (threshold  0x0F)  16 | \
+(low  0xFF)  8 | (high  0xFF)  0)
+
 struct ov772x_camera_info {
unsigned long  buswidth;
unsigned long  flags;
+   unsigned long  edgectrl;
struct soc_camera_link link;
 };
 
-- 
1.5.6.3

--
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