Re: [PATCH] mb862xxfb: add acceleration support forCoral-P/Coral-PA. * imageblt * copyarea * fillrect
From: Daniel Walker
Date: Mon Oct 19 2009 - 08:47:34 EST
Your code has lots of style issues .. I commented below in some of the
areas of code with some errors messages from scripts/checkpatch.pl
On Mon, 2009-10-19 at 14:52 +0400, Valentin Sitdikov wrote:
> Signed-off-by: Valentin Sitdikov <valentin.sitdikov@xxxxxxxxxxx>
> ---
> drivers/video/mb862xx/Makefile | 2 +-
> drivers/video/mb862xx/mb862xxfb.c | 16 ++-
> drivers/video/mb862xx/mb862xxfb.h | 2 +
> drivers/video/mb862xx/mb862xxfb_accel.c | 318 +++++++++++++++++++++++++++++++
> drivers/video/mb862xx/mb862xxfb_accel.h | 203 ++++++++++++++++++++
> 5 files changed, 539 insertions(+), 2 deletions(-)
> create mode 100644 drivers/video/mb862xx/mb862xxfb_accel.c
> create mode 100644 drivers/video/mb862xx/mb862xxfb_accel.h
>
> diff --git a/drivers/video/mb862xx/Makefile b/drivers/video/mb862xx/Makefile
> index 0766481..d777771 100644
> --- a/drivers/video/mb862xx/Makefile
> +++ b/drivers/video/mb862xx/Makefile
> @@ -2,4 +2,4 @@
> # Makefile for the MB862xx framebuffer driver
> #
>
> -obj-$(CONFIG_FB_MB862XX) := mb862xxfb.o
> +obj-$(CONFIG_FB_MB862XX) := mb862xxfb.o mb862xxfb_accel.o
> diff --git a/drivers/video/mb862xx/mb862xxfb.c b/drivers/video/mb862xx/mb862xxfb.c
> index a28e3cf..2cd7456 100644
> --- a/drivers/video/mb862xx/mb862xxfb.c
> +++ b/drivers/video/mb862xx/mb862xxfb.c
> @@ -214,7 +214,9 @@ static int mb862xxfb_set_par(struct fb_info *fbi)
> unsigned long reg, sc;
>
> dev_dbg(par->dev, "%s\n", __func__);
> -
> + if ( par->type == BT_CORALP ) {
> + mb862xxfb_init_accel(fbi, fbi->var.xres);
> + }
ERROR: space prohibited after that open parenthesis '('
#69: FILE: drivers/video/mb862xx/mb862xxfb.c:217:
+ if ( par->type == BT_CORALP ) {
This line should be
if (par->type == BT_CORALP) {
> +static void mb862xxfb_write_fifo(u32 count, u32 *data, struct fb_info *info)
> +{
> + struct mb862xxfb_par *par = info->par;
> + static u32 free = 0;
ERROR: do not initialise statics to 0 or NULL
#143: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:31:
+ static u32 free = 0;
Pretty clean, no need to initialized this.
> + u32 total = 0;
> + while ( total < count) {
> + if ( free ) {
> + outreg(geo, GDC_GEO_REG_INPUT_FIFO, data[total] );
> + total++;
> + free--;
> + }
> + else {
> + free = (u32)inreg(draw, GDC_REG_FIFO_COUNT);
> + }
ERROR: else should follow close brace '}'
#152: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:40:
+ }
+ else {
Should be,
} else {
> + }
> +}
> +
> +static void mb86290fb_copyarea(struct fb_info *info, const struct fb_copyarea *area)
> +{
> + __u32 cmd[6];
> +
> + cmd[0] = (GDC_TYPE_SETREGISTER << 24) | (1 << 16) | GDC_REG_MODE_BITMAP;
> + cmd[1] = (2 << 7) | (GDC_ROP_COPY << 9); /* Set raster operation */
> + cmd[2] = GDC_TYPE_BLTCOPYP << 24;
> +
> + if (area->sx >= area->dx && area->sy >= area->dy)
> + cmd[2] |= GDC_CMD_BLTCOPY_TOP_LEFT << 16;
> + else if (area->sx >= area->dx && area->sy <= area->dy)
> + cmd[2] |= GDC_CMD_BLTCOPY_BOTTOM_LEFT << 16;
> + else if (area->sx <= area->dx && area->sy >= area->dy)
> + cmd[2] |= GDC_CMD_BLTCOPY_TOP_RIGHT << 16;
> + else
> + cmd[2] |= GDC_CMD_BLTCOPY_BOTTOM_RIGHT << 16;
WARNING: suspect code indent for conditional statements (8, 8)
#166: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:54:
+ if (area->sx >= area->dx && area->sy >= area->dy)
+ cmd[2] |= GDC_CMD_BLTCOPY_TOP_LEFT << 16;
The above lines need tabs added like this,
if (area->sx >= area->dx && area->sy >= area->dy)
cmd[2] |= GDC_CMD_BLTCOPY_TOP_LEFT << 16;
else if (area->sx >= area->dx && area->sy <= area->dy)
cmd[2] |= GDC_CMD_BLTCOPY_BOTTOM_LEFT << 16;
else ...
> + cmd[3] = (area->sy << 16) | area->sx;
> + cmd[4] = (area->dy << 16) | area->dx ;
> + cmd[5] = (area->height << 16) | area->width;
> + mb862xxfb_write_fifo(6, cmd, info);
> +}
> +
> +/* Fill in the cmd array /GDC FIFO commands/ to draw a 1bit image.
> + * Make sure cmd has enough room! */
> +static void mb86290fb_imageblit1(u32 *cmd, u16 step, u16 dx, u16 dy,
> + u16 width, u16 height, u32 fgcolor, u32 bgcolor,
> + const struct fb_image *image, struct fb_info *info)
> +{
> + int i;
> + unsigned const char *line;
> + u16 bytes;
> +
> +
> + /* set colors and raster operation regs */
> + cmd[0] = (GDC_TYPE_SETREGISTER << 24) | (1 << 16) | GDC_REG_MODE_BITMAP;
> + cmd[1] = (2 << 7) | (GDC_ROP_COPY << 9); /* Set raster operation */
> +
> + cmd[2] = (GDC_TYPE_SETCOLORREGISTER << 24) | (GDC_CMD_BODY_FORE_COLOR << 16);
> + cmd[3] = fgcolor;
> + cmd[4] = (GDC_TYPE_SETCOLORREGISTER << 24) | (GDC_CMD_BODY_BACK_COLOR << 16);
> + cmd[5] = bgcolor;
> +
> + i = 0;
> + line = image->data;
> + bytes = (image->width + 7) >> 3;
> +
> + /* and the image */
> + cmd[6] = (GDC_TYPE_DRAWBITMAPP << 24) |
> + (GDC_CMD_BITMAP << 16) |
> + (2 + (step * height));
> + cmd[7] = (dy << 16) | dx;
> + cmd[8] = (height << 16) | width;
> +
> + while (i < height) {
> + memcpy(&cmd[9 + i * step], line, step << 2);
> +#ifdef __LITTLE_ENDIAN
> + {
> + int k=0;
> + for (k=0; k<step;k++)
ERROR: spaces required around that '=' (ctx:VxV)
#216: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:104:
+ int k=0;
^
ERROR: spaces required around that '=' (ctx:VxV)
#217: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:105:
+ for (k=0; k<step;k++)
^
ERROR: spaces required around that '<' (ctx:VxV)
#217: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:105:
+ for (k=0; k<step;k++)
^
ERROR: space required after that ';' (ctx:VxV)
#217: FILE: drivers/video/mb862xx/mb862xxfb_accel.c:105:
+ for (k=0; k<step;k++)
^
You just need to add spaces around the equals and other operators like
this,
for (k = 0; k < step; k++)
Your code has some other errors, 23 total. You can run
scripts/checkpatch.pl for a full list of the errors.
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/