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.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
