Hi Anatolij,
<snip>
>> 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.
The CONFIG_VIDEO_SM501 driver is completely differently configured
(boards supply tables with register settings). This will be very
hard to merge. SM50x might be a better name, but is it good style
to mix lowercase and uppercase letters in macro name?
<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>
I agree in removing dead code and structure components that will
never make sense in u-boot environment. However, should I really
delete definitions form -regs.h? This is well developed (linux)
and things might be needed for future development. I did not
touch this file compared to kernel source.
- Stefan
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
