Hello Stefan,

Stefan Althoefer wrote:
> [PATCH] video: Add new driver for Silicon Motion SM501/SM502
> 
> 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.
> 
> Use CONFIG_VIDEO_SM501NEW to enable the driver.
> 
> This has been tested on Janz emPC-A400. On this platform
> the SM501 is connected via PCI.
> 
> 
> The patch is against "latest" u-boot git-repository
> 
> Please (still) be patient if style of submission or patches are
> offending.
> 
> Signed-off-by: Stefan Althoefer <[EMAIL PROTECTED]>
> ----

see patch header comments in previous email, same applies here.

<snip>

> diff -uprN u-boot-orig//drivers/video/sm501new.c 
> u-boot/drivers/video/sm501new.c
> --- u-boot-orig//drivers/video/sm501new.c     1970-01-01 01:00:00.000000000 
> +0100
> +++ u-boot/drivers/video/sm501new.c   2008-12-03 11:47:22.000000000 +0100
> @@ -0,0 +1,1533 @@
> +/* Large parts of this have been taken from the linux kernel source code
> + * with the following licencse:
> + *
> + * Copyright (c) 2006 Simtec Electronics
> + *      Vincent Sanders <[EMAIL PROTECTED]>
> + *      Ben Dooks <[EMAIL PROTECTED]>
> + *
> + * 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.
> + *
> + * Framebuffer driver for the Silicon Motion SM501
> + */
> +
> +/* Ported to u-boot:
> + * 
> + * Copyright (c) 2008 StefanAlthoefer ([EMAIL PROTECTED])
> + */
> +
> +
> +#include <common.h>
> +#include <exports.h>

is exports.h needed here?

> +#include <config.h>
> +#include <watchdog.h>

is watchdog.h really needed?

> +#include <command.h>
> +#include <image.h>

image.h ?

> +#include <asm/byteorder.h>
> +#include <pci.h>
> +#include <video_fb.h>
> +#include <video.h>
> +#include "videomodes.h"
> +#include "sm501new-regs.h"
> +#include "sm501new.h"
> +
> +
> +#ifdef CONFIG_VIDEO_SM501NEW

please, remove above #ifdef and corresponding #endif. Conditional
compiling is already handled by Makefile.

> +#undef DEBUG
> +
> +/* this should be in pci_ids.h */
> +#define PCI_DEVICE_ID_SMI_501           0x0501

please, remove above comment and move PCI_DEVICE_ID_SMI_501
definition to include/pci_ids.h

> +#define BIG_ENDIAN_HOST
> +#define VIDEO_MEM_SIZE           (4*1024*1024)
> +
> +#define SM501_FRAMEBUFFER_ADDR   0
> +
> +#define HEAD_CRT     0
> +#define HEAD_PANEL   1
> +
> +#if defined(BIG_ENDIAN_HOST)
> +static inline unsigned int LONGSWAP(unsigned int x)
> +{
> +     return (
> +             ((x<<24) & 0xff000000) |
> +             ((x<< 8) & 0x00ff0000) |
> +             ((x>> 8) & 0x0000ff00) |
> +             ((x>>24) & 0x000000ff) );
> +}
> +static inline unsigned int readl(void *addr)
> +{
> +     return LONGSWAP((*(volatile unsigned int *)(addr)));
> +}
> +static inline void writel(unsigned int data, void *addr)
> +{
> +     /*printf("%p <- %x\n", addr, data); */
> +     *(volatile unsigned int *)(addr) = LONGSWAP(data);
> +}
> +#else
> +#define readl(addr) (*(volatile unsigned int*)(addr))
> +#define writel(b,addr) ((*(volatile unsigned int *) (addr)) = (b))
> +#endif

please, use existing code for readl, writel and byte swapping.

<snip>
> +struct sm501_devdata {
> +     struct sm501_platdata            *platdata;
> +     unsigned long                    pm_misc;
> +     int                              unit_power[20];
> +     unsigned int                     pdev_id;
> +     unsigned int                     irq;
> +     void                             *regs;
> +     void                             *dc;
> +     void                             *vmem;

pm_misc, pdev_id and irq are not used in the code, so
remove them.

> +     GraphicDevice                    *gd;

gd is asigned but not dereferenced in the code, another
candidate to remove?

> +     struct ctfb_res_modes            *mode;
> +     int                              bits_per_pixel;
> +     int                              xres_virtual;
> +     int                              yres_virtual;
> +};
> +
> +struct sm501_devdata smi_devd;
> +
> +
> +static void mdelay(unsigned int delay)
> +{
> +     while( delay-- > 0 ){
> +             udelay(1000);
> +     }
> +}
> +
> +#define MHZ (1000 * 1000)
> +
> +
> +#ifdef DEBUG
> +#define dev_dbg(XXX,...) printf(__VA_ARGS__)
> +#define dev_info(XXX,...) printf(__VA_ARGS__)
> +#define dev_err(XXX,...) printf(__VA_ARGS__)

please, use existing debug() macro for debugging purposes,
it's in include/common.h.

Debug is debug, info is info, error is error. It is a good idea
to report errors unconditionally, I mean independent of DEBUG
definition. Errors should always be reported, so I suggest using
printf() for error messages. Many dev_info() calls in this driver
should be changed to debug() calls, see appropriate comments below.
For some short info messages (chip version info, video memory size)
printf() is enough, I think. Also, first argument isn't used, so
simply drop it.

<snip>
> +static void sm501_dump_regs(struct sm501_devdata *sm)
> +{

sm501_dump_regs() is defined but not used. Please, use it or drop it
entirely.

> +     void *regs = sm->regs;
> +
> +     dev_info(sm->dev, "System Control   %08x\n",
> +                     readl(regs + SM501_SYSTEM_CONTROL));

Actually, this is debug output, so please use debug() macro.
This comment applies if sm501_dump_regs() is going to be used.

<snip>
> +static void sm501_dump_gate(struct sm501_devdata *sm)
> +{
> +     dev_info(sm->dev, "CurrentGate      %08x\n",
> +                     readl(sm->regs + SM501_CURRENT_GATE));
> +     dev_info(sm->dev, "CurrentClock     %08x\n",
> +                     readl(sm->regs + SM501_CURRENT_CLOCK));
> +     dev_info(sm->dev, "PowerModeControl %08x\n",
> +                     readl(sm->regs + SM501_POWER_MODE_CONTROL));

please, use debug() instead of dev_info() here.

<snip>
> +static inline void sm501_mdelay(struct sm501_devdata *sm, unsigned int delay)
> +{
> +     mdelay(delay);
> +}

please, drop sm501_mdelay() and simply use mdelay().

<snip>
> +unsigned long sm501_gpio_get(struct sm501_devdata *sm,
> +                          unsigned long gpio)
> +{

unused code, please remove it.

<snip>
> +void sm501_gpio_set(struct sm501_devdata *sm,
> +                 unsigned long gpio,
> +                 unsigned int to,
> +                 unsigned int dir)
> +{

ditto.

<snip>
> +/* sm501_init_dev
> + *
> + * Common init code for an SM501
> +*/
> +
> +static int sm501_init_dev(struct sm501_devdata *sm)
> +{
> +     unsigned long mem_avail;
> +     unsigned long dramctrl;
> +     unsigned long devid;
> +     int ret;
> +
> +     devid = readl(sm->regs + SM501_DEVICEID);
> +
> +     if ((devid & SM501_DEVICEID_IDMASK) != SM501_DEVICEID_SM501) {
> +             dev_err(sm->dev, "incorrect device id %08lx\n", devid);

printf() for error messages is ok.

> +             return -1;
> +     }
> +
> +     dramctrl = readl(sm->regs + SM501_DRAM_CONTROL);
> +     mem_avail = sm501_mem_local[(dramctrl >> 13) & 0x7];
> +
> +     dev_info(sm->dev, "SM501 At %p: Version %08lx, %ld Mb, IRQ %d\n",
> +              sm->regs, devid, (unsigned long)mem_avail >> 20, sm->irq);

IRQ doesn't make sense here, so I suggest to drop it. Also use debug()
instead of dev_info().

> +     sm501_dump_gate(sm);
> +     sm501_dump_clk(sm);
> +
> +     /* check to see if we have some device initialisation */
> +
> +     if (sm->platdata) {
> +             struct sm501_platdata *pdata = sm->platdata;
> +
> +             if (pdata->init) {
> +                     sm501_init_regs(sm, sm->platdata->init);
> +             }
> +     }
> +
> +     ret = sm501_check_clocks(sm);
> +     if (ret) {
> +             dev_err(sm->dev, "M1X and M clocks sourced from different "
> +                                     "PLLs\n");
> +             return -1;
> +     }
> +
> +
> +     return 0;
> +}
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

please remove empty lines, max. 2 empty lines allowed.

> +/* Initialisation data for PCI devices */
> +
> +
> +static struct sm501_initdata sm501_pci_initdata = {
> +     .gpio_high      = {
> +             .set    = 0x3F000000,           /* 24bit panel */
> +             .mask   = 0x0,
> +     },
> +     .misc_timing    = {
> +             .set    = 0x010100,             /* SDRAM timing */
> +             .mask   = 0x1F1F00,
> +     },
> +     .misc_control   = {
> +             .set    = SM501_MISC_PNL_24BIT,
> +             .mask   = 0,
> +     },
> +
> +     .devices        = SM501_USE_ALL,

"devices" isn't used in the code, a candidate to remove?

> +
> +     /* Errata AB-3 says that 72MHz is the fastest available
> +      * for 33MHZ PCI with proper bus-mastering operation */
> +
> +     .mclk           = 72 * MHZ,
> +     .m1xclk         = 144 * MHZ,
> +};
> +
> +static struct sm501_platdata_fbsub sm501_pdata_fbsub = {
> +     .flags          = (SM501FB_FLAG_USE_INIT_MODE |
> +                        SM501FB_FLAG_USE_HWCURSOR |
> +                        SM501FB_FLAG_USE_HWACCEL |
> +                        SM501FB_FLAG_DISABLE_AT_EXIT),
> +};
> +
> +static struct sm501_platdata_fb sm501_fb_pdata = {
> +     .fb_route       = SM501_FB_OWN,
> +     .fb_crt         = &sm501_pdata_fbsub,
> +     .fb_pnl         = &sm501_pdata_fbsub,
> +};
> +
> +static struct sm501_platdata sm501_pci_platdata = {
> +     .init           = &sm501_pci_initdata,
> +     .fb             = &sm501_fb_pdata,
> +};

sm501_fb_pdata isn't referenced in the code, so please
remove ".fb = &sm501_fb_pdata," in the sm501_pci_platdata
declaration. Also drop sm501_fb_pdata and sm501_pdata_fbsub
declarations above.

> +
> +
> +

max. 2 empty lines.

<snip>
> +static int sm501fb_set_par_common(struct sm501_devdata *sm, int head)
> +{
> +     struct ctfb_res_modes *var = sm->mode;
> +     unsigned long pixclock;      /* pixelclock in Hz */
> +     unsigned long sm501pixclock; /* pixelclock the 501 can achive in Hz */
> +     /*unsigned int mem_type;*/
> +     unsigned int clock_type;
> +     unsigned int head_addr;
> +
> +     dev_dbg(fbi->dev, "%s: %dx%d, bpp = %d, virtual %dx%d\n",
> +             __func__, var->xres, var->yres, sm->bits_per_pixel,
> +             sm->xres_virtual, sm->yres_virtual);
> +
> +     switch (head) {
> +     case HEAD_CRT:
> +             /*mem_type = SM501_MEMF_CRT;*/
> +             clock_type = SM501_CLOCK_V2XCLK;
> +             head_addr = SM501_DC_CRT_FB_ADDR;
> +             break;
> +
> +     case HEAD_PANEL:
> +             /*mem_type = SM501_MEMF_PANEL;*/
> +             clock_type = SM501_CLOCK_P2XCLK;
> +             head_addr = SM501_DC_PANEL_FB_ADDR;
> +             break;
> +
> +     default:
> +             /*mem_type = 0;*/               /* stop compiler warnings */
> +             head_addr = 0;
> +             clock_type = 0;
> +     }
> +
> +#if 0
> +     switch (sm->bits_per_pixel) {
> +     case 8:
> +             info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> +             break;
> +
> +     case 16:
> +             info->fix.visual = FB_VISUAL_DIRECTCOLOR;
> +             break;
> +
> +     case 32:
> +             info->fix.visual = FB_VISUAL_TRUECOLOR;
> +             break;
> +     }
> +
> +     /* allocate fb memory within 501 */
> +     info->fix.line_length = (var->xres_virtual * var->bits_per_pixel)/8;
> +     info->fix.smem_len    = info->fix.line_length * var->yres_virtual;
> +
> +     dev_dbg(fbi->dev, "%s: line length = %u\n", __func__,
> +             info->fix.line_length);
> +
> +     if (sm501_alloc_mem(fbi, &par->screen, mem_type,
> +                         info->fix.smem_len)) {
> +             dev_err(fbi->dev, "no memory available\n");
> +             return -ENOMEM;
> +     }
> +
> +     info->fix.smem_start = fbi->fbmem_res->start + par->screen.sm_addr;
> +
> +     info->screen_base = fbi->fbmem + par->screen.sm_addr;
> +     info->screen_size = info->fix.smem_len;
> +#endif

please remove unused code (between #if 0 and #endif).

<snip>
> +/* sm501fb_pan_crt
> + *
> + * pan the CRT display output within an virtual framebuffer
> +*/
> +#if 0
> +static int sm501fb_pan_crt(struct sm501_devdata *sm)
> +{
> +     struct ctfb_res_modes *var = sm->mode;
> +     unsigned int bytes_pixel = sm->bits_per_pixel / 8;
> +     unsigned long reg;
> +     unsigned long xoffs;
> +
> +     xoffs = /*var->xoffset*/0 * bytes_pixel;
> +
> +     reg = readl(sm->dc + SM501_DC_CRT_CONTROL);
> +
> +     reg &= ~SM501_DC_CRT_CONTROL_PIXEL_MASK;
> +     reg |= ((xoffs & 15) / bytes_pixel) << 4;
> +     writel(reg, sm->dc + SM501_DC_CRT_CONTROL);
> +
> +     reg = (par->screen.sm_addr + xoffs +
> +            /*var->yoffset*/0 * info->fix.line_length);
> +     writel(reg | SM501_ADDR_FLIP, fbi->dc + SM501_DC_CRT_FB_ADDR);
> +
> +     sm501fb_sync_regs(sm);
> +     return 0;
> +}
> +#endif

unused code, please remove.

> +
> +/* sm501fb_pan_pnl
> + *
> + * pan the panel display output within an virtual framebuffer
> +*/
> +static int sm501fb_pan_pnl(struct sm501_devdata *sm)
> +{
> +     /*struct ctfb_res_modes *var = sm->mode;*/

drop above unused code.

> +     unsigned long reg;
> +
> +     reg = /*var->xoffset*/0 | (sm->xres_virtual << 16);

remove /*var->xoffset*/.

> +     writel(reg, sm->dc + SM501_DC_PANEL_FB_WIDTH);
> +
> +     reg = /*var->yoffset*/0 | (sm->yres_virtual << 16);

Ditto.

> +     writel(reg, sm->dc + SM501_DC_PANEL_FB_HEIGHT);
> +
> +     sm501fb_sync_regs(sm);
> +     return 0;
> +}
> +
> +/* sm501fb_set_par_crt
> + *
> + * Set the CRT video mode from the fb_info structure
> +*/
> +
> +static int sm501fb_set_par_crt(struct sm501_devdata *sm)
> +{
> +     struct ctfb_res_modes *var = sm->mode;
> +     unsigned long control;       /* control register */
> +     int ret;
> +
> +     /* activate new configuration */
> +
> +     dev_dbg(fbi->dev, "%s(%p)\n", __func__, sm);
> +
> +     /* enable CRT DAC - note 0 is on!*/
> +     sm501_misc_control(sm, 0, SM501_MISC_DAC_POWER);
> +
> +     control = readl(sm->dc + SM501_DC_CRT_CONTROL);
> +     dev_dbg(fbi->dev, "old control is %08lx\n", control);
> +
> +     control &= (SM501_DC_CRT_CONTROL_PIXEL_MASK |
> +                 SM501_DC_CRT_CONTROL_GAMMA |
> +                 SM501_DC_CRT_CONTROL_BLANK |
> +                 SM501_DC_CRT_CONTROL_SEL |
> +                 SM501_DC_CRT_CONTROL_CP |
> +                 SM501_DC_CRT_CONTROL_TVP);
> +
> +     /* set the sync polarities before we check data source  */
> +
> +     if ((var->sync & FB_SYNC_HOR_HIGH_ACT) == 0)
> +             control |= SM501_DC_CRT_CONTROL_HSP;
> +
> +     if ((var->sync & FB_SYNC_VERT_HIGH_ACT) == 0)
> +             control |= SM501_DC_CRT_CONTROL_VSP;
> +
> +     if ((control & SM501_DC_CRT_CONTROL_SEL) == 0) {
> +             /* the head is displaying panel data... */
> +
> +             dev_dbg(fbi->dev, "%s CRT display panel data\n", __func__);
> +             /*sm501_alloc_mem(fbi, &par->screen, SM501_MEMF_CRT, 0);*/

drop commented out sm501_alloc_mem code (elsewhere in the file too).

<snip>
> +/* sm501fb_set_par_pnl
> + *
> + * Set the panel video mode from the fb_info structure
> +*/
> +
> +static int sm501fb_set_par_pnl(struct sm501_devdata *sm)
> +{
> +     struct ctfb_res_modes *var = sm->mode;
> +     unsigned long control;
> +     unsigned long reg;
> +     int ret;
> +
> +     dev_dbg(fbi->dev, "%s(%p)\n", __func__, sm);
> +
> +     /* activate this new configuration */
> +
> +     ret = sm501fb_set_par_common(sm, HEAD_PANEL);
> +     if (ret)
> +             return ret;
> +
> +     sm501fb_pan_pnl(sm);
> +     sm501fb_set_par_geometry(sm, HEAD_PANEL);
> +
> +     /* update control register */
> +
> +     control = readl(sm->dc + SM501_DC_PANEL_CONTROL);
> +     control &= (SM501_DC_PANEL_CONTROL_GAMMA |
> +                 SM501_DC_PANEL_CONTROL_VDD  |
> +                 SM501_DC_PANEL_CONTROL_DATA |
> +                 SM501_DC_PANEL_CONTROL_BIAS |
> +                 SM501_DC_PANEL_CONTROL_FPEN |
> +                 SM501_DC_PANEL_CONTROL_CP |
> +                 SM501_DC_PANEL_CONTROL_CK |
> +                 SM501_DC_PANEL_CONTROL_HP |
> +                 SM501_DC_PANEL_CONTROL_VP |
> +                 SM501_DC_PANEL_CONTROL_HPD |
> +                 SM501_DC_PANEL_CONTROL_VPD);
> +
> +     control |= SM501_FIFO_3;        /* fill if >3 free slots */
> +
> +     switch(sm->bits_per_pixel) {
> +     case 8:
> +             control |= SM501_DC_PANEL_CONTROL_8BPP;
> +             break;
> +
> +     case 16:
> +             control |= SM501_DC_PANEL_CONTROL_16BPP;
> +             break;
> +
> +     case 32:
> +             control |= SM501_DC_PANEL_CONTROL_32BPP;
> +             /*sm501fb_setup_gamma(fbi, SM501_DC_PANEL_PALETTE);*/

drop commented out sm501fb_setup_gamma, also elsewhere in the file.

> +             break;
> +
> +     default:
> +             dev_dbg(fbi->dev, "unkown pixel format\n");
> +     }
> +
> +     writel(0x0, sm->dc + SM501_DC_PANEL_PANNING_CONTROL);
> +
> +     /* panel plane top left and bottom right location */
> +
> +     writel(0x00, sm->dc + SM501_DC_PANEL_TL_LOC);
> +
> +     reg  = var->xres - 1;
> +     reg |= (var->yres - 1) << 16;
> +
> +     writel(reg, sm->dc + SM501_DC_PANEL_BR_LOC);
> +
> +     /* program panel control register */
> +
> +     control |= SM501_DC_PANEL_CONTROL_TE;   /* enable PANEL timing */
> +     control |= SM501_DC_PANEL_CONTROL_EN;   /* enable PANEL gfx plane */
> +
> +     if ((var->sync & FB_SYNC_HOR_HIGH_ACT) == 0)
> +             control |= SM501_DC_PANEL_CONTROL_HSP;
> +
> +     if ((var->sync & FB_SYNC_VERT_HIGH_ACT) == 0)
> +             control |= SM501_DC_PANEL_CONTROL_VSP;
> +
> +     writel(control, sm->dc + SM501_DC_PANEL_CONTROL);
> +     sm501fb_sync_regs(sm);
> +
> +     /* power the panel up */
> +     sm501fb_panel_power(sm, 1);
> +     return 0;
> +}
> +
> +void video_set_lut(
> +     unsigned int index,           /* color number */
> +     unsigned char r,              /* red */
> +     unsigned char g,              /* green */
> +     unsigned char b               /* blue */
> +     )
> +{
> +     /* FIXME: to be done */
> +}
> +
> +/*******************************************************************************
> + *
> + * Init video chip with common Linux graphic modes (lilo)
> + */
> +void *video_hw_init(void)
> +{
> +     GraphicDevice *pGD = (GraphicDevice *)&smi;
> +     unsigned short device_id;
> +     pci_dev_t devbusfn;
> +     int videomode;
> +     unsigned long t1, hsynch, vsynch;
> +     unsigned int pci_mem_base, *vm;
> +     unsigned int pci_reg_base;
> +     char *penv;
> +     int tmp, i, bits_per_pixel;
> +     int vmem_size;
> +     struct ctfb_res_modes *res_mode;
> +     struct ctfb_res_modes var_mode;
> +
> +     /* Search for video chip */
> +     printf("Video: ");
> +
> +     if ((devbusfn = pci_find_devices(supported, 0)) < 0)
> +     {
> +             printf ("Controller not found !\n");
> +             return (NULL);
> +     }
> +
> +     /* PCI setup */
> +     pci_write_config_dword (devbusfn, PCI_COMMAND, (PCI_COMMAND_MEMORY | 
> PCI_COMMAND_IO));
> +     pci_read_config_word (devbusfn, PCI_DEVICE_ID, &device_id);
> +     pci_read_config_dword (devbusfn, PCI_BASE_ADDRESS_0, &pci_mem_base);
> +     /*pci_mem_base = pci_mem_to_phys (devbusfn, pci_mem_base);*/
> +     pci_read_config_dword (devbusfn, PCI_BASE_ADDRESS_1, &pci_reg_base);
> +     /*pci_reg_base = pci_mem_to_phys (devbusfn, pci_reg_base);*/
> +
> +     dev_dbg(xxx, "mem_base=0x%x reg_base=0x%x\n", pci_mem_base, 
> pci_reg_base);
> +     
> +     tmp = 0;
> +
> +     videomode = CONFIG_SYS_DEFAULT_VIDEO_MODE;
> +     /* get video mode via environment */
> +     if ((penv = getenv ("videomode")) != NULL) {
> +             /* deceide if it is a string */
> +             if (penv[0] <= '9') {
> +                     videomode = (int) simple_strtoul (penv, NULL, 16);
> +                     tmp = 1;
> +             }
> +     } else {
> +             tmp = 1;
> +     }
> +     if (tmp) {
> +             /* parameter are vesa modes */
> +             /* search params */
> +             for (i = 0; i < VESA_MODES_COUNT; i++) {
> +                     if (vesa_modes[i].vesanr == videomode)
> +                             break;
> +             }
> +             if (i == VESA_MODES_COUNT) {
> +                     printf ("no VESA Mode found, switching to mode 0x%x ", 
> CONFIG_SYS_DEFAULT_VIDEO_MODE);
above line too long, max. 80 characters please.

> +                     i = 0;
> +             }
> +             res_mode =
> +                     (struct ctfb_res_modes *) &res_mode_init[vesa_modes[i].
> +                                                              resindex];
> +             bits_per_pixel = vesa_modes[i].bits_per_pixel;
> +     } else {
> +
> +             res_mode = (struct ctfb_res_modes *) &var_mode;
> +             bits_per_pixel = video_get_params (res_mode, penv);
> +     }
> +
> +     /* calculate hsynch and vsynch freq (info only) */
> +     t1 = (res_mode->left_margin + res_mode->xres +
> +           res_mode->right_margin + res_mode->hsync_len) / 8;
> +     t1 *= 8;
> +     t1 *= res_mode->pixclock;
> +     t1 /= 1000;
> +     hsynch = 1000000000L / t1;
> +     t1 *=
> +             (res_mode->upper_margin + res_mode->yres +
> +              res_mode->lower_margin + res_mode->vsync_len);
> +     t1 /= 1000;
> +     vsynch = 1000000000L / t1;
> +
> +     /* fill in Graphic device struct */
> +     sprintf (pGD->modeIdent, "%dx%dx%d %ldkHz %ldHz", res_mode->xres,
> +              res_mode->yres, bits_per_pixel, (hsynch / 1000),
> +              (vsynch / 1000));
> +     printf ("%s\n", pGD->modeIdent);
> +     pGD->winSizeX = res_mode->xres;
> +     pGD->winSizeY = res_mode->yres;
> +     pGD->plnSizeX = res_mode->xres;
> +     pGD->plnSizeY = res_mode->yres;
> +     switch (bits_per_pixel) {
> +     case 8:
> +             pGD->gdfBytesPP = 1;
> +             pGD->gdfIndex = GDF__8BIT_INDEX;
> +             break;
> +     case 15:
> +             pGD->gdfBytesPP = 2;
> +             pGD->gdfIndex = GDF_15BIT_555RGB;
> +             break;
> +     case 16:
> +             pGD->gdfBytesPP = 2;
> +             pGD->gdfIndex = GDF_16BIT_565RGB;
> +             break;
> +     case 24:
> +             pGD->gdfBytesPP = 4;
> +             pGD->gdfIndex = GDF_32BIT_X888RGB;
> +             break;
> +     case 32:
> +             pGD->gdfBytesPP = 4;
> +             pGD->gdfIndex = GDF_32BIT_X888RGB;
> +             break;
> +     }
> +
> +     pGD->isaBase = 0;
> +     pGD->pciBase = pci_mem_base;
> +     pGD->dprBase = pci_reg_base;
> +     pGD->vprBase = 0;
> +     pGD->cprBase = 0;
> +     pGD->frameAdrs = pci_mem_base;
> +     pGD->memSize = VIDEO_MEM_SIZE;
> +     
> +     smi_devd.platdata = &sm501_pci_platdata;
> +     smi_devd.regs = (void *)pci_reg_base;
> +     smi_devd.dc   = (void *)pci_reg_base + 0x80000;
> +     smi_devd.vmem = (void *)pci_mem_base;
> +     smi_devd.gd = pGD;

"smi_devd.gd" isn't referenced elsewhere in the code,
a candidate to remove?

> +     smi_devd.mode = res_mode;
> +     switch (bits_per_pixel) {
> +     case  8: smi_devd.bits_per_pixel =  8; break;
> +     case 16: smi_devd.bits_per_pixel = 16; break;
> +     case 24: smi_devd.bits_per_pixel = 32; break;
> +     default: smi_devd.bits_per_pixel = 32; break;
> +     }
> +     smi_devd.xres_virtual = pGD->plnSizeX;
> +     smi_devd.yres_virtual = pGD->plnSizeY;
> +
> +     switch( (readl(smi_devd.regs + SM501_DRAM_CONTROL) >> 13) & 0x7 ){
> +     case 0: vmem_size = 4*1024*1024; break;
> +     case 1: vmem_size = 8*1024*1024; break;
> +     case 2: vmem_size = 16*1024*1024; break;
> +     case 3: vmem_size = 32*1024*1024; break;
> +     case 4: vmem_size = 64*1024*1024; break;
> +     case 5: vmem_size = 2*1024*1024; break;
> +     default: vmem_size = 2*1024*1024; break;
> +     }
> +     printf("SM501: %d MB Video memory\n", vmem_size/(1024*1024));
> +     
> +     /* Blank video memory */
> +     vm = (unsigned int *)pGD->frameAdrs;
> +     for(i=0; i < vmem_size/sizeof(int); i++){
> +             *vm++ = 0;
> +     }
> +     
> +     sm501_init_dev(&smi_devd);
> +     
> +     /* enable display controller */
> +     sm501_unit_power(&smi_devd, SM501_GATE_DISPLAY, 1);
> +
> +#if 0
> +     /* setup cursors */
> +
> +     sm501_init_cursor(info->fb[HEAD_CRT], SM501_DC_CRT_HWC_ADDR);
> +     sm501_init_cursor(info->fb[HEAD_PANEL], SM501_DC_PANEL_HWC_ADDR);
> +#endif

unused code, please remove it.

> +
> +     /*
> +      * Panel setup
> +      */
> +     sm501fb_set_par_pnl(&smi_devd);
> +
> +     /*
> +      * CRT setup (we want CRT display panel data)
> +      */
> +     sm501fb_set_par_crt(&smi_devd);
> +     
> +     return ((void*)&smi);
> +}
> +
> +
> +#if 0
> +int t_smi (int argc, char *argv[])
> +{
> +     int i;
> +     ulong ret;
> +     ulong t,t1;
> +
> +     app_startup(argv);
> +
> +     pci_init();
> +     drv_video_init();
> +
> +#if 0
> +     video_puts("loading ...\n");
> +     video_puts("be patient\n");
> +#endif
> +             
> +#if 0
> +     if (argc == 1) {
> +             /* Print the ABI version */
> +             
> +             printf("Video hw Init\n");
> +             pci_init();
> +             video_hw_init();
> +             
> +             return 0;
> +     } else {
> +             printf("Video Init\n");
> +             pci_init();
> +             drv_video_init();
> +     }
> +#endif
> +}
> +#endif

unused code, please drop it.

Also check for proper multi-line comment style, please.

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
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to