Hello Stefan,
please see comments below:
Stefan Althoefer wrote:
> [PATCH] video: Add new driver for Silicon Motion SM501/SM502
this line duplicates the subject, so simply remove it.
> This patch adds a new driver for SM501/SM502. Compared to the
> existing driver it allows dynamic selection of resolution
> (environment: videomode).
>
> The drive is based on Vincent Sanders and Ben Dooks' linux
> kernel driver.
s/drive/driver/
> Use CONFIG_VIDEO_SM501NEW to enable the driver.
not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe
we should use s.th. like CONFIG_VIDEO_SM50x. This applies to
the file names too: sm50x.h, sm50x.c, etc. Even better would
be a merge with the existing driver.
<snip>
> The patch is against "latest" u-boot git-repository
>
> Please (still) be patient if style of submission or patches are
> offending.
These lines are patch comments which should not appear in
the commit message. The common practice is to place such
comments below "---" line.
> Signed-off-by: Stefan Althoefer <[EMAIL PROTECTED]>
> ----
So, here is the place for patch comments which should not
appear in the commit message. Also changes between patch
revisions are often summarised here.
> diff -uprN u-boot-orig//drivers/video/sm501new.h
> u-boot/drivers/video/sm501new.h
> --- u-boot-orig//drivers/video/sm501new.h 1970-01-01 01:00:00.000000000
> +0100
> +++ u-boot/drivers/video/sm501new.h 2008-12-02 18:28:42.000000000 +0100
> @@ -0,0 +1,125 @@
> +/* Large parts of this have been taken from the linux kernel source code
> + * with the following licencse:
Multi-line comment style is as follows:
/*
* This is a
* multi-line
* comment
*/
> +/* Ported to u-boot:
> + *
> + * Copyright (c) 2008 StefanAlthoefer ([EMAIL PROTECTED])
> + */
same here, fix multi-line comment here and everywhere in
the code.
<snip>
> +#define SM501FB_FLAG_USE_INIT_MODE (1<<0)
> +#define SM501FB_FLAG_DISABLE_AT_EXIT (1<<1)
> +#define SM501FB_FLAG_USE_HWCURSOR (1<<2)
> +#define SM501FB_FLAG_USE_HWACCEL (1<<3)
> +
> +struct sm501_platdata_fbsub {
> + struct fb_videomode *def_mode;
> + unsigned int def_bpp;
> + unsigned long max_mem;
> + unsigned int flags;
> +};
> +
> +enum sm501_fb_routing {
> + SM501_FB_OWN = 0, /* CRT=>CRT, Panel=>Panel */
> + SM501_FB_CRT_PANEL = 1, /* Panel=>CRT, Panel=>Panel */
> +};
> +
> +/* sm501_platdata_fb flag field bit definitions */
> +
> +#define SM501_FBPD_SWAP_FB_ENDIAN (1<<0) /* need to endian swap */
> +
> +/* sm501_platdata_fb
> + *
> + * configuration data for the framebuffer driver
> +*/
> +
> +struct sm501_platdata_fb {
> + enum sm501_fb_routing fb_route;
> + unsigned int flags;
> + struct sm501_platdata_fbsub *fb_crt;
> + struct sm501_platdata_fbsub *fb_pnl;
> +};
> +
> +/* gpio i2c */
> +
> +struct sm501_platdata_gpio_i2c {
> + unsigned int pin_sda;
> + unsigned int pin_scl;
> +};
all this stuff above starting from
"#define SM501FB_FLAG_USE_INIT_MODE"
can be removed if you remove "init_gpiop", "fb", "gpio_i2c" and
"gpio_i2c_nr" from the "struct sm501_platdata" definition below.
Also remove ".fb = &sm501_fb_pdata" from "sm501_pci_platdata"
declaration in drivers/video/sm501new.c. These structure members
aren't referenced in this U-Boot driver, so they aren't needed
(unused and nearly dead code and data).
<snip>
> +#define SM501_USE_USB_HOST (1<<0)
> +#define SM501_USE_USB_SLAVE (1<<1)
> +#define SM501_USE_SSP0 (1<<2)
> +#define SM501_USE_SSP1 (1<<3)
> +#define SM501_USE_UART0 (1<<4)
> +#define SM501_USE_UART1 (1<<5)
> +#define SM501_USE_FBACCEL (1<<6)
> +#define SM501_USE_AC97 (1<<7)
> +#define SM501_USE_I2S (1<<8)
These macros aren't used, remove them too.
> +
> +#define SM501_USE_ALL (0xffffffff)
> +
> +struct sm501_initdata {
> + struct sm501_reg_init gpio_low;
> + struct sm501_reg_init gpio_high;
> + struct sm501_reg_init misc_timing;
> + struct sm501_reg_init misc_control;
> +
> + unsigned long devices;
"devices" member is only initialized in "sm501_pci_initdata"
declaration and isn't referenced anywhere in the code, so
another candidate to remove.
> +/* sm501_platdata
> + *
> + * This is passed with the platform device to allow the board
> + * to control the behaviour of the SM501 driver(s) which attach
> + * to the device.
> + *
> +*/
> +
> +struct sm501_platdata {
> + struct sm501_initdata *init;
> + struct sm501_init_gpio *init_gpiop;
> + struct sm501_platdata_fb *fb;
> +
> + struct sm501_platdata_gpio_i2c *gpio_i2c;
> + unsigned int gpio_i2c_nr;
see above suggestion to remove init_gpiop, fb, gpio_i2c and
gpio_i2c_nr.
> diff -uprN u-boot-orig//drivers/video/sm501new-regs.h
> u-boot/drivers/video/sm501new-regs.h
> --- u-boot-orig//drivers/video/sm501new-regs.h 1970-01-01
> 01:00:00.000000000 +0100
> +++ u-boot/drivers/video/sm501new-regs.h 2008-12-02 18:28:42.000000000
> +0100
> @@ -0,0 +1,365 @@
> +/* sm501-regs.h
> + *
> + * Copyright 2006 Simtec Electronics
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Silicon Motion SM501 register definitions
> +*/
please, fix multi-line comment style.
Also check all below defined macros in this file. If some
macro isn't used in the driver code, then remove it.
> +/* System Configuration area */
> +/* System config base */
> +#define SM501_SYS_CONFIG (0x000000)
> +
> +/* config 1 */
> +#define SM501_SYSTEM_CONTROL (0x000000)
> +#define SM501_MISC_CONTROL (0x000004)
> +
> +#define SM501_MISC_BUS_SH (0x0)
> +#define SM501_MISC_BUS_PCI (0x1)
> +#define SM501_MISC_BUS_XSCALE (0x2)
> +#define SM501_MISC_BUS_NEC (0x6)
> +#define SM501_MISC_BUS_MASK (0x7)
<snip>
Best regards,
Anatolij
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
