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 stefan.althoefer@web.de
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 (as@janz.de)
- */
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