Re: [PATCH 09/10] ov772x: Compute window size registers at runtime

[Guennadi Liakhovetski]

Hi Laurent

On Fri, 6 Jul 2012, Laurent Pinchart wrote:

 Instead of hardcoding register arrays, compute the values at runtime.

Great to see this register-array magic go! Just one nitpick:

 
 Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
 ---
  drivers/media/video/ov772x.c |  149 -
  1 files changed, 58 insertions(+), 91 deletions(-)
 
 diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
 index 07ff709..98b1bdf 100644
 --- a/drivers/media/video/ov772x.c
 +++ b/drivers/media/video/ov772x.c

[snip]

 @@ -574,24 +514,26 @@ static const struct ov772x_color_format ov772x_cfmts[] 
 = {
  
  #define VGA_WIDTH   640
  #define VGA_HEIGHT  480
 -#define QVGA_WIDTH  320
 -#define QVGA_HEIGHT 240
 -#define MAX_WIDTH   VGA_WIDTH
 -#define MAX_HEIGHT  VGA_HEIGHT

You removed QVGA_* macros, because they only were used at one location, 
but you kept VGA_*.

  
  static const struct ov772x_win_size ov772x_win_sizes[] = {
   {
   .name = VGA,
 - .width= VGA_WIDTH,
 - .height   = VGA_HEIGHT,
   .com7_bit = SLCT_VGA,
 - .regs = ov772x_vga_regs,
 + .rect = {
 + .left = 140,
 + .top = 14,
 + .width = 640,
 + .height = 480,

...but here you hard-code .width and .height. I'd propose to use some 
symbolic names for all these sizes.

 + },
   }, {
   .name = QVGA,
 - .width= QVGA_WIDTH,
 - .height   = QVGA_HEIGHT,
   .com7_bit = SLCT_QVGA,
 - .regs = ov772x_qvga_regs,
 + .rect = {
 + .left = 252,
 + .top = 6,
 + .width = 320,
 + .height = 240,
 + },
   },
  };
  

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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 09/10] ov772x: Compute window size registers at runtime

[Laurent Pinchart]

Instead of hardcoding register arrays, compute the values at runtime.

Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
---
 drivers/media/video/ov772x.c |  149 -
 1 files changed, 58 insertions(+), 91 deletions(-)

diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
index 07ff709..98b1bdf 100644
--- a/drivers/media/video/ov772x.c
+++ b/drivers/media/video/ov772x.c
@@ -317,8 +317,15 @@
 #define SGLF_ON_OFF 0x02   /* Single frame ON/OFF selection */
 #define SGLF_TRIG   0x01   /* Single frame transfer trigger */
 
+/* HREF */
+#define HREF_VSTART_SHIFT  6   /* VSTART LSB */
+#define HREF_HSTART_SHIFT  4   /* HSTART 2 LSBs */
+#define HREF_VSIZE_SHIFT   2   /* VSIZE LSB */
+#define HREF_HSIZE_SHIFT   0   /* HSIZE 2 LSBs */
+
 /* EXHCH */
-#define VSIZE_LSB   0x04   /* Vertical data output size LSB */
+#define EXHCH_VSIZE_SHIFT  2   /* VOUTSIZE LSB */
+#define EXHCH_HSIZE_SHIFT  0   /* HOUTSIZE 2 LSBs */
 
 /* DSP_CTRL1 */
 #define FIFO_ON 0x80   /* FIFO enable/disable selection */
@@ -344,30 +351,6 @@
 #define DSP_OFMT_RAW8  0x02
 #define DSP_OFMT_RAW10 0x03
 
-/* HSTART */
-#define HST_VGA 0x23
-#define HST_QVGA0x3F
-
-/* HSIZE */
-#define HSZ_VGA 0xA0
-#define HSZ_QVGA0x50
-
-/* VSTART */
-#define VST_VGA 0x07
-#define VST_QVGA0x03
-
-/* VSIZE */
-#define VSZ_VGA 0xF0
-#define VSZ_QVGA0x78
-
-/* HOUTSIZE */
-#define HOSZ_VGA0xA0
-#define HOSZ_QVGA   0x50
-
-/* VOUTSIZE */
-#define VOSZ_VGA0xF0
-#define VOSZ_QVGA   0x78
-
 /* DSPAUTO (DSP Auto Function ON/OFF Control) */
 #define AWB_ACTRL   0x80 /* AWB auto threshold control */
 #define DENOISE_ACTRL   0x40 /* De-noise auto threshold control */
@@ -386,10 +369,6 @@
 /*
  * struct
  */
-struct regval_list {
-   unsigned char reg_num;
-   unsigned char value;
-};
 
 struct ov772x_color_format {
enum v4l2_mbus_pixelcode code;
@@ -402,10 +381,8 @@ struct ov772x_color_format {
 
 struct ov772x_win_size {
char *name;
-   __u32 width;
-   __u32 height;
unsigned char com7_bit;
-   const struct regval_list *regs;
+   struct v4l2_rect  rect;
 };
 
 struct ov772x_priv {
@@ -421,31 +398,6 @@ struct ov772x_priv {
unsigned shortband_filter;
 };
 
-#define ENDMARKER { 0xff, 0xff }
-
-/*
- * register setting for window size
- */
-static const struct regval_list ov772x_qvga_regs[] = {
-   { HSTART,   HST_QVGA },
-   { HSIZE,HSZ_QVGA },
-   { VSTART,   VST_QVGA },
-   { VSIZE,VSZ_QVGA  },
-   { HOUTSIZE, HOSZ_QVGA },
-   { VOUTSIZE, VOSZ_QVGA },
-   ENDMARKER,
-};
-
-static const struct regval_list ov772x_vga_regs[] = {
-   { HSTART,   HST_VGA },
-   { HSIZE,HSZ_VGA },
-   { VSTART,   VST_VGA },
-   { VSIZE,VSZ_VGA },
-   { HOUTSIZE, HOSZ_VGA },
-   { VOUTSIZE, VOSZ_VGA },
-   ENDMARKER,
-};
-
 /*
  * general function
  */
@@ -465,18 +417,6 @@ static inline int ov772x_write(struct i2c_client *client, 
u8 addr, u8 value)
return i2c_smbus_write_byte_data(client, addr, value);
 }
 
-static int ov772x_write_array(struct i2c_client*client,
- const struct regval_list *vals)
-{
-   while (vals-reg_num != 0xff) {
-   int ret = ov772x_write(client, vals-reg_num, vals-value);
-   if (ret  0)
-   return ret;
-   vals++;
-   }
-   return 0;
-}
-
 static int ov772x_mask_set(struct i2c_client *client, u8  command, u8  mask,
   u8  set)
 {
@@ -574,24 +514,26 @@ static const struct ov772x_color_format ov772x_cfmts[] = {
 
 #define VGA_WIDTH   640
 #define VGA_HEIGHT  480
-#define QVGA_WIDTH  320
-#define QVGA_HEIGHT 240
-#define MAX_WIDTH   VGA_WIDTH
-#define MAX_HEIGHT  VGA_HEIGHT
 
 static const struct ov772x_win_size ov772x_win_sizes[] = {
{
.name = VGA,
-   .width= VGA_WIDTH,
-   .height   = VGA_HEIGHT,
.com7_bit = SLCT_VGA,
-   .regs = ov772x_vga_regs,
+   .rect = {
+   .left = 140,
+   .top = 14,
+   .width = 640,
+   .height = 480,
+   },
}, {
.name = QVGA,
-   .width= QVGA_WIDTH,
-   .height   = QVGA_HEIGHT,
.com7_bit = SLCT_QVGA,
-   .regs = ov772x_qvga_regs,
+   .rect = {
+   .left = 252,
+   .top = 6,
+   .width = 320,
+   .height = 240,
+   },
},
 };
 
@@ -602,8 +544,8 @@ static const struct