On Tue, Feb 05, 2013 at 03:51:55PM +0800, Aaron.Chen 陈俊杰 wrote:

This is an initial patch for Siliconmotion Graphics chips. It add SM750 chip support with 2d accelerate and hw curser support. It is a complete new driver. So the patch is a little bit big. Is it OK for review?

Adding a new fbdev driver while established consensus pretty much boils down to fbdev being a dead subsystem which should only be used for legacy applications and drivers: Not good. You should implement this as a drm modesetting driver, and if required base your 2d acceleration on top of the drm fb helpers. Or just implement a drm/gem driver to expose this.

Additionally you should include the appropriate mailing lists, which is linux-fbdev for fbdev drivers.

On a very quick read the patch has serious issues:

- checkpatch.pl:

total: 2 errors, 779 warnings, 7961 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or scripts/cleanfile

- Remants of hungarian notation (dunno whether checkpatch checks for those, too much other stuff flying by, but it really stuck out).

- Not using the linux i2c layer in the ddk750_hwi2c.c file.

- Reimplementing i2c bit-banging code in the ddk750_swi2c.c

- Adding a new implementation of a sil 164 dvi driver, we already have at least two in drivers/gpu/drm/i2c/sil164_drv.c and drivers/gpu/drm/i915/dvo_sil164.c. This should be unified and probably use the drm encoder slave framework.

At that point I've given up on further review, since there's clearly a lot of work left to do. Please review SubmittingPatches from the kernel documentation carefully. Also cc'ing our dear staging maintainer, he should be able to point you at further useful resources for getting this going.

Cheers, Daniel

Dear Aaron,

thank you for your patch. It is much appreciated. Here are some more comments about formal stuff in addition to what Daniel replied.

Am Dienstag, den 05.02.2013, 15:51 +0800 schrieb Aaron.Chen 陈俊杰:

[…]

diff --git a/drivers/video/lynxfb/lynx_drv.c b/drivers/video/lynxfb/lynx_drv.c new file mode 100644 index 0000000..556902d --- /dev/null +++ b/drivers/video/lynxfb/lynx_drv.c

[…]

•   if (!info)
  

•           LEAVE(-EINVAL);
  

•   ret = 0;
  

•   par = info->par;
  

•   share = par->share;
  

•   crtc = &par->crtc;
  

•   output = &par->output;
  

•   var = &info->var;
  

•   fix = &info->fix;
  

•   /* fix structur is not so FIX ... */
  

structur*e*

Please elaborate the comment.

•   line_length = var->xres_virtual * var->bits_per_pixel / 8;
  

•   line_length = PADDING(crtc->line_pad, line_length);
  

•   fix->line_length = line_length;
  

•   err_msg("fix->line_length = %d\n", fix->line_length);
  

•   /* var->red, green, blue, transp are need to be set by driver
  

need*ed*

•    *and these data should be set before setcolreg routine
  

Missing space after *.

•    **/
  

•   switch (var->bits_per_pixel) {
  

•   case 8:
  

•           fix->visual = FB_VISUAL_PSEUDOCOLOR;
  

•           var->red.offset = 0;
  

•           var->red.length = 8;
  

•           var->green.offset = 0;
  

•           var->green.length = 8;
  

•           var->blue.offset = 0;
  

•           var->blue.length = 8;
  

•           var->transp.length = 0;
  

•           var->transp.offset = 0;
  

•           break;
  

•   case 16:
  

•           var->red.offset = 11;
  

•           var->red.length = 5;
  

•           var->green.offset = 5;
  

•           var->green.length = 6;
  

•           var->blue.offset = 0;
  

•           var->blue.length = 5;
  

•           var->transp.length = 0;
  

•           var->transp.offset = 0;
  

•           fix->visual = FB_VISUAL_TRUECOLOR;
  

•           break;
  

•   case 24:
  

•   case 32:
  

•           var->red.offset = 16;
  

•           var->red.length = 8;
  

•           var->green.offset = 8;
  

•           var->green.length = 8;
  

•           var->blue.offset = 0 ;
  

•           var->blue.length = 8;
  

•           fix->visual = FB_VISUAL_TRUECOLOR;
  

•           break;
  

•   default:
  

•           ret = -EINVAL;
  

•           break;
  

•   }
  

•   var->height = var->width = -1;
  

•   var->accel_flags = FB_ACCELF_TEXT;
  

•   if (ret) {
  

•           err_msg("pixel bpp format not satisfied\n.");
  

•           LEAVE(ret);
  

•   }
  

•   ret = crtc->proc_setMode(crtc, var, fix);
  

•   if (!ret)
  

•           ret = output->proc_setMode(output, var, fix);
  

•   LEAVE(ret);
  

+} +static inline unsigned int chan_to_field(unsigned int chan, struct fb_bitfield *bf) +{

•   chan &= 0xffff;
  

•   chan >>= 16 - bf->length;
  

•   return chan << bf->offset;
  

+}

+static int lynxfb_ops_setcolreg(unsigned regno, unsigned red,

•           unsigned green, unsigned blue,
  

•           unsigned transp, struct fb_info *info)
  

+{

•   struct lynxfb_par *par;
  

•   struct lynxfb_crtc *crtc;
  

•   struct fb_var_screeninfo *var;
  

•   int ret;
  

•   par = info->par;
  

•   crtc = &par->crtc;
  

•   var = &info->var;
  

•   ret = 0;
  

•   /*dbg_msg("regno=%d, red=%d, green=%d, blue=%d\n", regno, red, green, blue);*/
  

Missing spaces.

•   if (regno > 256) {
  

•           err_msg("regno = %d\n", regno);
  

•           LEAVE(-EINVAL);
  

•   }
  

•   if (info->var.grayscale)
  

•           red = green = blue = (red * 77 + green * 151 + blue * 28) >> 8;
  

•   if (var->bits_per_pixel == 8 && info->fix.visual == FB_VISUAL_PSEUDOCOLOR) {
  

•           red >>= 8;
  

•           green >>= 8;
  

•           blue >>= 8;
  

•           ret = crtc->proc_setColReg(crtc, regno, red, green, blue);
  

•           goto exit;
  

•   }
  

•   if (info->fix.visual == FB_VISUAL_TRUECOLOR && regno < 256) {
  

•           u32 val;
  

•           if (var->bits_per_pixel == 16 ||
  

•                           var->bits_per_pixel == 32 ||
  

•                           var->bits_per_pixel == 24) {
  

•                   val = chan_to_field(red, &var->red);
  

•                   val |= chan_to_field(green, &var->green);
  

•                   val |= chan_to_field(blue, &var->blue);
  

•                   par->pseudo_palette[regno] = val;
  

•                   goto exit;
  

•           }
  

•   }
  

•   ret = -EINVAL;
  

+exit:

•   return ret;
  

•   LEAVE(ret);
  

+}

+static int lynxfb_ops_blank(int blank, struct fb_info *info) +{

•   struct lynxfb_par *par;
  

•   struct lynxfb_output *output;
  

•   ENTER();
  

•   dbg_msg("blank = %d.\n", blank);
  

•   par = info->par;
  

•   output = &par->output;
  

•   LEAVE(output->proc_setBLANK(output, blank));
  

+} +static int sm750fb_set_drv(struct lynxfb_par *par) +{

•   int ret;
  

•   struct lynx_share *share;
  

•   struct sm750_share *spec_share;
  

•   struct lynxfb_output *output;
  

•   struct lynxfb_crtc *crtc;
  

•   ENTER();
  

•   ret = 0;
  

•   share = par->share;
  

•   spec_share = container_of(share, struct sm750_share, share);
  

•   output = &par->output;
  

•   crtc = &par->crtc;
  

•   crtc->vidmem_size = (share->dual) ? share->vidmem_size>>1 : share->vidmem_size;
  

•   /* setup crtc and output member */
  

•   spec_share->hwCursor = g_hwcursor;
  

•   crtc->proc_setMode = hw_sm750_crtc_setMode;
  

•   crtc->proc_checkMode = hw_sm750_crtc_checkMode;
  

•   crtc->proc_setColReg = hw_sm750_setColReg;
  

•   crtc->proc_panDisplay = hw_sm750_pan_display;
  

•   crtc->clear = hw_sm750_crtc_clear;
  

•   crtc->line_pad = 16;
  

•   /*crtc->xpanstep = crtc->ypanstep = crtc->ywrapstep = 0;*/
  

Missing spaces. Why is it commented out? Either explain why in the code or remove commented out code.

•   crtc->xpanstep = 8;
  

•   crtc->ypanstep = 1;
  

•   crtc->ywrapstep = 0;
  

•   output->proc_setMode = hw_sm750_output_setMode;
  

•   output->proc_checkMode = hw_sm750_output_checkMode;
  

•   output->proc_setBLANK = (share->revid == SM750LE_REVISION_ID) ? hw_sm750le_setBLANK : hw_sm750_setBLANK;
  

•   output->clear = hw_sm750_output_clear;
  

•   /* chip specific phase */
  

•   share->accel.de_wait = (share->revid == SM750LE_REVISION_ID) ? hw_sm750le_deWait : hw_sm750_deWait;
  

•   switch (spec_share->state.dataflow) {
  

•   case sm750_simul_pri:
  

•                   output->paths = sm750_pnc;
  

•                   crtc->channel = sm750_primary;
  

•                   crtc->oScreen = 0;
  

•                   crtc->vScreen = share->pvMem;
  

•                   inf_msg("use simul primary mode\n");
  

•                   break;
  

•   case sm750_simul_sec:
  

•                   output->paths = sm750_pnc;
  

•                   crtc->channel = sm750_secondary;
  

•                   crtc->oScreen = 0;
  

•                   crtc->vScreen = share->pvMem;
  

•                   break;
  

•   case sm750_dual_normal:
  

•                   if (par->index == 0) {
  

•                           output->paths = sm750_panel;
  

•                           crtc->channel = sm750_primary;
  

•                           crtc->oScreen = 0;
  

•                           crtc->vScreen = share->pvMem;
  

•                   } else{
  

•                           output->paths = sm750_crt;
  

•                           crtc->channel = sm750_secondary;
  

•                           /* not consider of padding stuffs for oScreen, need fix*/
  

•                           crtc->oScreen = (share->vidmem_size >> 1);
  

•                           crtc->vScreen = share->pvMem + crtc->oScreen;
  

•                   }
  

•                   break;
  

•   case sm750_dual_swap:
  

•                   if (par->index == 0) {
  

•                           output->paths = sm750_panel;
  

•                           crtc->channel = sm750_secondary;
  

•                           crtc->oScreen = 0;
  

•                           crtc->vScreen = share->pvMem;
  

•                   } else{
  

•                           output->paths = sm750_crt;
  

•                           crtc->channel = sm750_primary;
  

•                           /* not consider of padding stuffs for oScreen, need fix*/
  

•                           crtc->oScreen = (share->vidmem_size >> 1);
  

•                           crtc->vScreen = share->pvMem + crtc->oScreen;
  

•                   }
  

•                   break;
  

•   default:
  

•                   ret = -EINVAL;
  

•   }
  

•   LEAVE(ret);
  

+}

+static int __devinit lynxfb_set_fbinfo(struct fb_info *info, int index) +{

•   int i;
  

•   struct lynxfb_par *par;
  

•   struct lynx_share *share;
  

•   struct lynxfb_crtc *crtc;
  

•   struct lynxfb_output *output;
  

•   struct fb_var_screeninfo *var;
  

•   struct fb_fix_screeninfo *fix;
  

•   const struct fb_videomode *pdb[] = {
  

•           NULL, NULL, vesa_modes,
  

•   };
  

•   int cdb[] = {0, 0, VESA_MODEDB_SIZE};
  

•   static const char *mdb_desc[] = {
  

•           "driver prepared modes",
  

•           "kernel prepared default modedb",
  

•           "kernel HELPERS prepared vesa_modes",
  

•   };
  

+#define sm502_ext lynx750_ext

•   static const struct fb_videomode *ext_table[] = {lynx750_ext, NULL, sm502_ext};
  

•   static size_t ext_size[] = {ARRAY_SIZE(lynx750_ext), 0, ARRAY_SIZE(sm502_ext)};
  

•   static const char *fixId[][2] = {
  

•           {"sm750_fb1", "sm750_fb2"},
  

•   };
  

•   int ret, line_length;
  

•   ENTER();
  

•   ret = 0;
  

•   par = (struct lynxfb_par *)info->par;
  

•   share = par->share;
  

•   crtc = &par->crtc;
  

•   output = &par->output;
  

•   var = &info->var;
  

•   fix = &info->fix;
  

•   /* set index */
  

•   par->index = index;
  

•   output->channel = &crtc->channel;
  

•   sm750fb_set_drv(par);
  

•   lynxfb_ops.fb_pan_display = lynxfb_ops_pan_display;
  

•   /* set current cursor variable and proc pointer,
  

•    *must be set after crtc member initialized */
  

Missing space.

•   crtc->cursor.offset = crtc->oScreen + crtc->vidmem_size - 1024;
  

•   crtc->cursor.mmio = share->pvReg + 0x800f0 + (int)crtc->channel * 0x140;
  

•   inf_msg("crtc->cursor.mmio = %p\n", crtc->cursor.mmio);
  

•   crtc->cursor.maxH = crtc->cursor.maxW = 64;
  

•   crtc->cursor.size = crtc->cursor.maxH*crtc->cursor.maxW*2/8;
  

•   crtc->cursor.disable = hw_cursor_disable;
  

•   crtc->cursor.enable = hw_cursor_enable;
  

•   crtc->cursor.setColor = hw_cursor_setColor;
  

•   crtc->cursor.setPos = hw_cursor_setPos;
  

•   crtc->cursor.setSize = hw_cursor_setSize;
  

•   crtc->cursor.setData = hw_cursor_setData;
  

•   crtc->cursor.vstart = share->pvMem + crtc->cursor.offset;
  

•   crtc->cursor.share = share;
  

•   memset(crtc->cursor.vstart, 0, crtc->cursor.size);
  

•   if (!g_hwcursor) {
  

•           lynxfb_ops.fb_cursor = NULL;
  

•           crtc->cursor.disable(&crtc->cursor);
  

•   }
  

+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 10)

•           /* hardware cursor broken under low version kernel*/
  

Missing space.

•           lynxfb_ops.fb_cursor = soft_cursor;
  

+#endif

•           /* set info->fbops, must be set before fb_find_mode */
  

•           if (!share->accel_off) {
  

•                   /* use 2d acceleration */
  

•                   lynxfb_ops.fb_fillrect = lynxfb_ops_fillrect;
  

•                   lynxfb_ops.fb_copyarea = lynxfb_ops_copyarea;
  

•                   lynxfb_ops.fb_imageblit = lynxfb_ops_imageblit;
  

•           }
  

•           info->fbops = &lynxfb_ops;
  

•           if (!g_fbmode[index]) {
  

•                   g_fbmode[index] = g_def_fbmode;
  

•                   if (index)
  

•                           g_fbmode[index] = g_fbmode[0];
  

•           }
  

•           pdb[0] = ext_table[g_specId];
  

•           cdb[0] = ext_size[g_specId];
  

•           for (i = 0; i < 3; i++) {
  

•                   /* no NULL pointer passed to fb_find_mode @4 */
  

•                   if (pdb[i] == NULL) {
  

+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 10)

•                           pdb[i] = &modedb2[0];
  

•                           cdb[i] = nmodedb2;
  

+#endif

•                   }
  

•                   ret = fb_find_mode(var, info, g_fbmode[index],
  

•                                   pdb[i], cdb[i], NULL, 8);
  

•                   if (ret == 1) {
  

•                           inf_msg("success! use specified mode:%s in %s\n",
  

•                                           g_fbmode[index],
  

•                                           mdb_desc[i]);
  

•                           break;
  

•                   } else if (ret == 2) {
  

•                           war_msg("use specified mode:%s in %s, with an ignored refresh rate\n",
  

Missing space after `:`.

•                                           g_fbmode[index],
  

•                                           mdb_desc[i]);
  

•                           break;
  

•                   } else if (ret == 3) {
  

•                           war_msg("wanna use default mode\n");
  

•                           /*                      break;*/
  

•                   } else if (ret == 4) {
  

•                           war_msg("fall back to any valid mode\n");
  

•                   } else{
  

•                           war_msg("ret = %d, fb_find_mode failed, with %s\n", ret, mdb_desc[i]);
  

•                   }
  

•           }
  

•           /* some member of info->var had been set by fb_find_mode */
  

•           inf_msg("Member of info->var is :\n\
  

Remove space before `:`.

•                           xres=%d\n\
  

•                           yres=%d\n\
  

•                           xres_virtual=%d\n\
  

•                           yres_virtual=%d\n\
  

•                           xoffset=%d\n\
  

•                           yoffset=%d\n\
  

•                           bits_per_pixel=%d\n \
  

•                           ...\n", var->xres, var->yres, var->xres_virtual, var->yres_virtual,
  

•                           var->xoffset, var->yoffset, var->bits_per_pixel);
  

•           /* set par */
  

•           par->info = info;
  

•           /* set info */
  

•           line_length = PADDING(crtc->line_pad,
  

•                           (var->xres_virtual * var->bits_per_pixel/8));
  

•           info->pseudo_palette = &par->pseudo_palette[0];
  

•           info->screen_base = crtc->vScreen;
  

•           dbg_msg("screen_base vaddr = %p\n", info->screen_base);
  

+#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 10)

•           info->screen_size = line_length * var->yres_virtual;
  

+#endif

•           info->flags = FBINFO_FLAG_DEFAULT|0;
  

•           /* set info->fix */
  

•           fix->type = FB_TYPE_PACKED_PIXELS;
  

•           fix->type_aux = 0;
  

•           fix->xpanstep = crtc->xpanstep;
  

•           fix->ypanstep = crtc->ypanstep;
  

•           fix->ywrapstep = crtc->ywrapstep;
  

•           fix->accel = FB_ACCEL_NONE;
  

•           strlcpy(fix->id, fixId[g_specId][index], sizeof(fix->id));
  

•           fix->smem_start = crtc->oScreen + share->vidmem_start;
  

•           inf_msg("fix->smem_start = %lx\n", fix->smem_start);
  

•           /* according to mmap experiment from user space application,
  

Which one?

•            *fix->mmio_len should not larger than virtual size
  

not *be* larger

•            *(xres_virtual x yres_virtual x ByPP)
  

•            *Below line maybe buggy when user mmap fb dev node and write
  

write*s*

•            *data into the bound over virtual size
  

Missing spaces.

•            **/
  

•           fix->smem_len = crtc->vidmem_size;
  

•           inf_msg("fix->smem_len = %x\n", fix->smem_len);
  

+#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 10)

•           info->screen_size = fix->smem_len;
  

+#endif

•           fix->line_length = line_length;
  

•           fix->mmio_start = share->vidreg_start;
  

•           inf_msg("fix->mmio_start = %lx\n", fix->mmio_start);
  

•           fix->mmio_len = share->vidreg_size;
  

•           inf_msg("fix->mmio_len = %x\n", fix->mmio_len);
  

•           switch (var->bits_per_pixel) {
  

•           case 8:
  

•                           fix->visual = FB_VISUAL_PSEUDOCOLOR;
  

•                           break;
  

•           case 16:
  

•           case 32:
  

•                           fix->visual = FB_VISUAL_TRUECOLOR;
  

•                           break;
  

•           }
  

•           /* set var */
  

•           var->activate = FB_ACTIVATE_NOW;
  

•           var->accel_flags = 0;
  

•           var->vmode = FB_VMODE_NONINTERLACED;
  

•           dbg_msg("#1 show info->cmap : \nstart=%d, len=%d, red=%p, green=%p, blue=%p, transp=%p\n",
  

Remove space in front of `:`.

•                           info->cmap.start, info->cmap.len,
  

•                           info->cmap.red, info->cmap.green, info->cmap.blue,
  

•                           info->cmap.transp);
  

•           ret = fb_alloc_cmap(&info->cmap, 256, 0);
  

•           if (ret < 0) {
  

•                   err_msg("Could not allcate memory for cmap.\n");
  

all*o*cate

•                   goto exit;
  

•           }
  

•           dbg_msg("#2 show info->cmap : \nstart=%d, len=%d, red=%p, green=%p, blue=%p, transp=%p\n",
  

Remove space.

•                           info->cmap.start, info->cmap.len,
  

•                           info->cmap.red, info->cmap.green, info->cmap.blue,
  

•                           info->cmap.transp);
  

+exit:

•           lynxfb_ops_check_var(var, info);
  

•           /*    lynxfb_ops_set_par(info);*/
  

Missing explanation and incorrect white space.

•           LEAVE(ret);
  

•   }
  

•   static int __devinit lynxfb_pci_probe(struct pci_dev *pdev,
  

•                   const struct pci_device_id *ent)
  

•   {
  

•           struct fb_info *info[] = {NULL, NULL};
  

•           struct lynx_share *share = NULL;
  

•           void *spec_share = NULL;
  

•           size_t spec_offset = 0;
  

•           int fbidx;
  

•           ENTER();
  

•           /* enable device */
  

•           if (pci_enable_device(pdev)) {
  

•                   err_msg("can not enable device.\n");
  

•                   goto err_enable;
  

•           }
  

•           switch (ent->device) {
  

•           case PCI_DEVID_LYNX_EXP:
  

•           case PCI_DEVID_LYNX_SE:
  

•                   g_specId = SPC_SM750;
  

•                           /* though offset of share in sm750_share is 0,
  

•                            *we use this marcro as the same */
  

1. Space. 2. macro. 3. Elaborate comment.

•                   spec_offset = offsetof(struct sm750_share, share);
  

•                   break;
  

•           default:
  

•                   break;
  

•           }
  

•           dbg_msg("spec_offset = %d\n", spec_offset);
  

•           spec_share = kzalloc(spec_size[g_specId], GFP_KERNEL);
  

•           if (!spec_share) {
  

•                   err_msg("Could not allocate memory for share.\n");
  

•                   goto err_share;
  

•           }
  

•           /* setting share structure */
  

•           share = (struct lynx_share *)(spec_share + spec_offset);
  

•           share->fbinfo[0] = share->fbinfo[1] = NULL;
  

•           share->devid = pdev->device;
  

+#if LINUX_VERSION_CODE <= KERNEL_VERSION(2, 6, 22)

•           u32 temp;
  

•           pci_read_config_dword(pdev, PCI_CLASS_REVISION, &temp);
  

•           share->revid = temp&0xFF;
  

+#else

•           share->revid = pdev->revision;
  

+#endif

•           inf_msg("share->revid = %02x\n", share->revid);
  

•           share->pdev = pdev;
  

+#ifdef CONFIG_MTRR

•           share->mtrr_off = g_nomtrr;
  

•           share->mtrr.vram = 0;
  

•           share->mtrr.vram_added = 0;
  

+#endif

•           share->accel_off = g_noaccel;
  

•           share->dual = g_dualview;
  

•           spin_lock_init(&share->slock);
  

•           if (!share->accel_off) {
  

•                   /* hook deInit and 2d routines, notes that below hw_xxx
  

note without s.

•                    *routine can work on most of lynx chips
  

•                    *if some chip need specific function, please hook it in smXXX_set_drv
  

need*s* *a* specific

•                    *routine */
  

Missing spaces.

•                   share->accel.de_init = hw_de_init;
  

•                   share->accel.de_fillrect = hw_fillrect;
  

•                   share->accel.de_copyarea = hw_copyarea;
  

•                   share->accel.de_imageblit = hw_imageblit;
  

•                   inf_msg("enable 2d acceleration\n");
  

•           } else{
  

•                   inf_msg("disable 2d acceleration\n");
  

•           }
  

•           /* call chip specific setup routine  */
  

•           (*setup_rout[g_specId])(share, g_settings);
  

•           /* call chip specific mmap routine */
  

•           if ((*map_rout[g_specId])(share, pdev)) {
  

•                   err_msg("Memory map failed\n");
  

•                   goto err_map;
  

•           }
  

+#ifdef CONFIG_MTRR

•           if (!share->mtrr_off) {
  

•                   inf_msg("enable mtrr\n");
  

•                   share->mtrr.vram = mtrr_add(share->vidmem_start,
  

•                                   share->vidmem_size,
  

•                                   MTRR_TYPE_WRCOMB, 1);
  

•                   if (share->mtrr.vram < 0) {
  

•                           /* don't block driver with the failure of MTRR */
  

•                           err_msg("Unable to setup MTRR.\n");
  

•                   } else{
  

•                           share->mtrr.vram_added = 1;
  

•                           inf_msg("MTRR added succesfully\n");
  

succes*s*fully

•                   }
  

•           }
  

+#endif

•           memset(share->pvMem, 0, share->vidmem_size);
  

•           inf_msg("sm%3x mmio address = %p\n", share->devid, share->pvReg);
  

•           pci_set_drvdata(pdev, share);
  

•           /* call chipInit routine */
  

•           (*inithw_rout[g_specId])(share, pdev);
  

•           /* detect 502 need no disp driver
  

•            *beware that other chips except 502 should not touch g_502nodisp
  

•            *(remain g_502nodisp always 0)
  

•            *so regularily, below if line will not affect other chips' behaviour
  

regularly

Missing spaces.

•            **/
  

•   /*      if (!g_no502disp) {*/
  

Why commented out?

•                   /* allocate frame buffer info structor according to g_dualview */
  

struct*ur*e

•                   fbidx = 0;
  

+ALLOC_FB:

•                   info[fbidx] = framebuffer_alloc(sizeof(struct lynxfb_par), &pdev->dev);
  

•                   if (!info[fbidx]) {
  

•                           err_msg("Could not allocate framebuffer #%d.\n", fbidx);
  

•                           if (fbidx == 0)
  

•                                   goto err_info0_alloc;
  

•                           else
  

•                                   goto err_info1_alloc;
  

•                   } else{
  

•                           struct lynxfb_par *par;
  

•                           inf_msg("framebuffer #%d alloc okay\n", fbidx);
  

•                           share->fbinfo[fbidx] = info[fbidx];
  

•                           par = info[fbidx]->par;
  

•                           par->share = share;
  

•                           /* set fb_info structure */
  

•                           if (lynxfb_set_fbinfo(info[fbidx], fbidx)) {
  

•                                   err_msg("Failed to initial fb_info #%d.\n", fbidx);
  

•                                   if (fbidx == 0)
  

•                                           goto err_info0_set;
  

•                                   else
  

•                                           goto err_info1_set;
  

•                           }
  

•                           /* register frame buffer*/
  

•                           inf_msg("Ready to register framebuffer #%d.\n", fbidx);
  

•                           int errno = register_framebuffer(info[fbidx]);
  

•                           if (errno < 0) {
  

•                                   err_msg("Failed to register fb_info #%d. err %d\n", fbidx, errno);
  

•                                   if (fbidx == 0)
  

•                                           goto err_register0;
  

•                                   else
  

•                                           goto err_register1;
  

•                           }
  

•                           inf_msg("Accomplished register framebuffer #%d.\n", fbidx);
  

•                   }
  

•                   /* no dual view by far */
  

Elaborate.

•                   fbidx++;
  

•                   if (share->dual && fbidx < 2)
  

•                           goto ALLOC_FB;
  

+/* }*/

•           LEAVE(0);
  

+err_register1: +err_info1_set:

•           framebuffer_release(info[1]);
  

+err_info1_alloc:

•           unregister_framebuffer(info[0]);
  

+err_register0: +err_info0_set:

•           framebuffer_release(info[0]);
  

+err_info0_alloc: +err_map:

•           kfree(spec_share);
  

+err_share: +err_enable:

•           LEAVE(-ENODEV);
  

•   }
  

+static void __devexit lynxfb_pci_remove(struct pci_dev *pdev) +{

•   struct fb_info *info;
  

•   struct lynx_share *share;
  

•   void *spec_share;
  

•   struct lynxfb_par *par;
  

•   int cnt;
  

•   ENTER();
  

•   cnt = 2;
  

•   share = pci_get_drvdata(pdev);
  

•   while (cnt-- > 0) {
  

•           info = share->fbinfo[cnt];
  

•           if (!info)
  

•                   continue;
  

•           par = info->par;
  

•           unregister_framebuffer(info);
  

•           /* clean crtc & output allocations*
  

Missing space at end.

•           par->crtc.clear(&par->crtc);
  

•           par->output.clear(&par->output);
  

•           /* release frame buffer*/
  

•           framebuffer_release(info);
  

•   }
  

+#ifdef CONFIG_MTRR

•   if (share->mtrr.vram_added)
  

•           mtrr_del(share->mtrr.vram, share->vidmem_start, share->vidmem_size);
  

+#endif

•   /*      pci_release_regions(pdev);*/
  

•   iounmap(share->pvReg);
  

•   iounmap(share->pvMem);
  

•   switch (share->devid) {
  

•   case PCI_DEVID_LYNX_EXP:
  

•   case PCI_DEVID_LYNX_SE:
  

•                   spec_share = container_of(share, struct sm750_share, share);
  

•                   break;
  

•   default:
  

•                   spec_share = share;
  

•   }
  

•   kfree(g_settings);
  

•   kfree(spec_share);
  

•   pci_set_drvdata(pdev, NULL);
  

•   LEAVE();
  

+}

+/* chip specific g_option configuration routine */

Remove some spaces.

+static void sm750fb_setup(struct lynx_share *share, char *src) +{

•   struct sm750_share *spec_share;
  

•   char *opt;
  

+#ifdef CAP_EXPENSION

•   char *exp_res;
  

+#endif

•   int swap;
  

•   ENTER();
  

•   spec_share = container_of(share, struct sm750_share, share);
  

+#ifdef CAP_EXPENSIION

•   exp_res = NULL;
  

+#endif

•   swap = 0;
  

•   spec_share->state.initParm.chip_clk = 0;
  

•   spec_share->state.initParm.mem_clk = 0;
  

•   spec_share->state.initParm.master_clk = 0;
  

•   spec_share->state.initParm.powerMode = 0;
  

•   spec_share->state.initParm.setAllEngOff = 0;
  

•   spec_share->state.initParm.resetMemory = 1;
  

•   /*defaultly turn g_hwcursor on for both view */
  

by default Missing space.

•   g_hwcursor = 3;
  

•   if (!src || !*src) {
  

•           war_msg("no specific g_option.\n");
  

•           goto NO_PARAM;
  

•   }
  

•   while ((opt = strsep(&src, ":")) != NULL && *opt != NULL) {
  

•           err_msg("opt=%s\n", opt);
  

•           err_msg("src=%s\n", src);
  

•           if (!strncmp(opt, "swap", strlen("swap")))
  

•                   swap = 1;
  

•           else if (!strncmp(opt, "nocrt", strlen("nocrt")))
  

•                   spec_share->state.nocrt = 1;
  

•           else if (!strncmp(opt, "36bit", strlen("36bit")))
  

•                   spec_share->state.pnltype = sm750_doubleTFT;
  

•           else if (!strncmp(opt, "18bit", strlen("18bit")))
  

•                   spec_share->state.pnltype = sm750_dualTFT;
  

•           else if (!strncmp(opt, "24bit", strlen("24bit")))
  

•                   spec_share->state.pnltype = sm750_24TFT;
  

+#ifdef CAP_EXPANSION

•           else if (!strncmp(opt, "exp:", strlen("exp:")))
  

•                   exp_res = opt + strlen("exp:");
  

+#endif

•           else if (!strncmp(opt, "nohwc0", strlen("nohwc0")))
  

•                   g_hwcursor &= ~0x1;
  

•           else if (!strncmp(opt, "nohwc1", strlen("nohwc1")))
  

•                   g_hwcursor &= ~0x2;
  

•           else if (!strncmp(opt, "nohwc", strlen("nohwc")))
  

•                   g_hwcursor = 0;
  

•           else{
  

•                   if (!g_fbmode[0]) {
  

•                           g_fbmode[0] = opt;
  

•                           inf_msg("find fbmode0 : %s\n", g_fbmode[0]);
  

•                   } else if (!g_fbmode[1]) {
  

•                           g_fbmode[1] = opt;
  

•                           inf_msg("find fbmode1 : %s\n", g_fbmode[1]);
  

•                   } else{
  

•                           war_msg("How many view you wann set?\n");
  

•                   }
  

•           }
  

•   }
  

+#ifdef CAP_EXPANSION

•   if (getExpRes(exp_res, &spec_share->state.xLCD, &spec_share->state.yLCD)) {
  

•           /* seems exp_res is not valid*/
  

Missing space. Report an error?

•           spec_share->state.xLCD = spec_share->state.yLCD = 0;
  

•   }
  

+#endif

+NO_PARAM:

•   if (share->revid != SM750LE_REVISION_ID) {
  

•           if (share->dual) {
  

•                   if (swap)
  

•                           spec_share->state.dataflow = sm750_dual_swap;
  

•                   else
  

•                           spec_share->state.dataflow = sm750_dual_normal;
  

•           } else{
  

•                   if (swap)
  

•                           spec_share->state.dataflow = sm750_simul_sec;
  

•                   else
  

•                           spec_share->state.dataflow = sm750_simul_pri;
  

•           }
  

•   } else{
  

•           /* SM750LE only have one crt channel */
  

•           spec_share->state.dataflow = sm750_simul_sec;
  

•           /* sm750le do not have complex attributes*/
  

Missing space.

•           spec_share->state.nocrt = 0;
  

•   }
  

•   LEAVE();
  

+}

+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 10) +static int __init lynxfb_setup(char *options) +#else +int __init lynxfb_setup(char *options) +#endif +{

•   int len;
  

•   char *opt, *tmp;
  

•   ENTER();
  

•   if (!options || !*options) {
  

•           war_msg("no options.\n");
  

•           LEAVE(0);
  

•   }
  

•   inf_msg("options:%s\n", options);
  

•   len = strlen(options) + 1;
  

•   g_settings = kmalloc(len, GFP_KERNEL);
  

•   if (!g_settings)
  

•           LEAVE(-ENOMEM);
  

•   memset(g_settings, 0, len);
  

•   tmp = g_settings;
  

•   /*      Notes:
  

•           char *strsep(char **s, const char *ct);
  

•           @s: the string to be searched
  

•           @ct :the characters to search for
  

Space.

•           strsep() updates @options to pointer after the first found token
  

•           it also returns the pointer ahead the token.
  

•           */
  

•   while ((opt = strsep(&options, ":")) != NULL) {
  

•           /* options that mean for any lynx chips are configured here */
  

•           if (!strncmp(opt, "noaccel", strlen("noaccel")))
  

•                   g_noaccel = 1;
  

+#ifdef CONFIG_MTRR

•           else if (!strncmp(opt, "nomtrr", strlen("nomtrr")))
  

•                   g_nomtrr = 1;
  

+#endif

•           else if (!strncmp(opt, "dual", strlen("dual")))
  

•                   g_dualview = 1;
  

•           else{
  

•                   strcat(tmp, opt);
  

•                   tmp += strlen(opt);
  

•                   if (options != NULL)
  

•                           *tmp++ = ':';
  

•                   else
  

•                           *tmp++ = 0;
  

•           }
  

•   }
  

•   /* misc g_settings are transport to chip specific routines */
  

s,are transport,are transported,

•   inf_msg("parameter left for chip specific analysis:%s\n", g_settings);
  

Missing space in front of `%s`.

•   LEAVE(0);
  

+}

+static void claim(void) +{

•   inf_msg("+-------------SMI Driver Information------------+");
  

•   inf_msg("Release type : " RELEASE_TYPE "\n");
  

•   inf_msg("Driver version: v" _version_ "\n");
  

•   inf_msg("Support products:\n"
  

•                   SUPPORT_CHIP);
  

•   inf_msg("Support OS:\n"
  

•                   SUPPORT_OS);
  

•   inf_msg("Support ARCH: " SUPPORT_ARCH "\n");
  

•   inf_msg("+-----------------------------------------------+");
  

+} +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 10) +static int __init lynxfb_init() +{

•   char *option ;
  

•   int ret;
  

•   smi_indent = 0;
  

•   ENTER();
  

•   claim();
  

+#ifdef MODULE

•   option = g_option;
  

+#else

•   if (fb_get_options("lynxfb", &option))
  

•           LEAVE(-ENODEV);
  

+#endif

•   lynxfb_setup(option);
  

•   ret = pci_register_driver(&lynxfb_driver);
  

•   LEAVE(ret);
  

+} +#else /* kernel version >= 2.6.10*/

Missing space at end in front of `*/`.

+int __init lynxfb_init(void) +{

•   char *option;
  

•   int ret;
  

•   smi_indent = 0;
  

•   ENTER();
  

•   claim();
  

+#ifdef MODULE

•   option = g_option;
  

•   lynxfb_setup(option);
  

+#else

•   /* do nothing */
  

+#endif

•   ret = pci_register_driver(&lynxfb_driver);
  

•   LEAVE(ret);
  

+} +#endif

•   module_init(lynxfb_init);
  

+#ifdef MODULE +static void __exit lynxfb_exit() +{

•   ENTER();
  

•   inf_msg(_moduleName_ " exit\n");
  

•   pci_unregister_driver(&lynxfb_driver);
  

•   LEAVE();
  

+}

•   module_exit(lynxfb_exit);
  

+#endif

+#ifdef MODULE +#if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 10)

•   module_param(g_option, charp, S_IRUGO);
  

+#else

•   /* be ware that PARM and param */
  

s,ware,aware,

The comment does not make any sense to me. Please rephrase.

•   MODULE_PARM(g_option, "s");
  

+#endif

•   MODULE_PARM_DESC(g_option,
  

•                   "\n\t\tCommon options:\n"
  

•                   "\t\tnoaccel:disable 2d capabilities\n"
  

•                   "\t\tnomtrr:disable MTRR attribute for video memory\n"
  

•                   "\t\tdualview:dual frame buffer feature enabled\n"
  

•                   "\t\tnohwc:disable hardware cursor\n"
  

Missing spaces after `:`.

•                   "\t\tUsual example:\n"
  

•                   "\t\tinsmod ./lynxfb.ko g_option=\"noaccel, nohwc, 1280x1024-8@60\"\n"
  

•                   "\t\tFor more detail chip specific options, please refer to \"Lynxfb User Mnual\" or readme\n"
  

For more detail*ed* chip specific options, please refer to "Lynxfb User M*a*nul" re README*.*

Please add an URL and the path to the README.

•                   );
  

+#endif

•   MODULE_AUTHOR("monk liu<monk.liu@siliconmotion.com>"); 
  

1. Missing space in front of email address `<`. 2. Please capitalize names correctly: Monk Liu. You could also put the Chinese(?) characters in parentheses after it.

Applying your patch, Git complains about white space errors.

$ git am /tmp/0001-Siliconmotion-initial-patch.patch Applying: Siliconmotion initial patch /src/linux/.git/rebase-apply/patch:86: trailing whitespace. sm750/sm718/sm712/sm722/sm502. Say Y if you have such a /src/linux/.git/rebase-apply/patch:88: trailing whitespace. To compile this driver as a module, choose M here: the /src/linux/.git/rebase-apply/patch:802: new blank line at EOF. + /src/linux/.git/rebase-apply/patch:1207: new blank line at EOF. + /src/linux/.git/rebase-apply/patch:1456: new blank line at EOF. + warning: squelched 8 whitespace errors warning: 13 lines add whitespace errors.

You have to fix those. Especially if tools warn you about this. You can enable colors in Git [1] and `git diff` or `git show` will mark white space issues with red.

If this is supposed to be the final version, please be more thorough next time. White space issues and spelling mistakes can be easily fixed beforehand by using Git correctly and a spell checker!

Thanks,

Paul

[1] http://git-scm.com/book/ch7-1.html