[U-Boot] [PATCH] video: Add new driver for Silicon Motion SM501/SM502 Part 1/2

[Stefan Althoefer]

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