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

[Anatolij Gustschin]

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 at 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 at 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

-- 
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: office at denx.de