Messages in this thread | | | Date | Thu, 6 Mar 2008 08:50:32 -0800 | From | "Bryan Wu" <> | Subject | Re: [Linux-fbdev-devel] [PATCH 1/1 try#2] [VIDEO/FRAMEBUFFER]: add BF52x EZkit Display driver |
| |
On Wed, Mar 5, 2008 at 11:02 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 31 Jan 2008 00:49:43 +0800 Bryan Wu <bryan.wu@analog.com> wrote: > > > > bah, January. > > I'm trolling the fbdev list only now - I guess I'll slip this one into > 2.6.25. >
You finally caught this, -;)
> > > +static int bfin_t350mcqb_request_ports(int action) > > +{ > > + u16 ppi0_req_8[] = {P_PPI0_CLK, P_PPI0_FS1, P_PPI0_FS2, > > + P_PPI0_D0, P_PPI0_D1, P_PPI0_D2, > > + P_PPI0_D3, P_PPI0_D4, P_PPI0_D5, > > + P_PPI0_D6, P_PPI0_D7, 0}; > > This array will be assembled on the stack at runtime. If > peripheral_request_list() doesn't alter it then it would be better to > create it at compile-time by adding `static'. >
Exactly, I will do it.
> > > + if (action) { > > + if (peripheral_request_list(ppi0_req_8, DRIVER_NAME)) { > > + printk(KERN_ERR "Requesting Peripherals faild\n"); > > + return -EFAULT; > > + } > > + } else > > + peripheral_free_list(ppi0_req_8); > > + > > + return 0; > > +} > > + > > > +static int bfin_t350mcqb_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > > +{ > > + struct bfin_t350mcqbfb_info *fbi = info->par; > > + > > + if (fbi->lq043_mmap) > > + return -1; > > + > > + spin_lock(&fbi->lock); > > + fbi->lq043_mmap = 1; > > + spin_unlock(&fbi->lock); > > the locking here probably doesn't do a lot. >
Actually, this locking is necessary here for trying to mmap by two application, IMO.
> > > + vma->vm_start = (unsigned long)(fbi->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET); > > + > > + vma->vm_end = vma->vm_start + info->fix.smem_len; > > + /* For those who don't understand how mmap works, go read > > + * Documentation/nommu-mmap.txt. > > + * For those that do, you will know that the VM_MAYSHARE flag > > + * must be set in the vma->vm_flags structure on noMMU > > + * Other flags can be set, and are documented in > > + * include/linux/mm.h > > + */ > > + vma->vm_flags |= VM_MAYSHARE; > > + > > + return 0; > > +} > > + > > + > > > > +static int __init bfin_t350mcqb_probe(struct platform_device *pdev) > > +{ > > + struct bfin_t350mcqbfb_info *info; > > + struct fb_info *fbinfo; > > + int ret; > > + > > + printk(KERN_INFO DRIVER_NAME ": %dx%d %d-bit RGB FrameBuffer initializing...\n", > > + LCD_X_RES, LCD_Y_RES, LCD_BPP); > > + > > + if (request_dma(CH_PPI, "CH_PPI") < 0) { > > + printk(KERN_ERR DRIVER_NAME > > + ": couldn't request CH_PPI DMA\n"); > > + ret = -EFAULT; > > + goto out1; > > + } > > + > > + fbinfo = > > + framebuffer_alloc(sizeof(struct bfin_t350mcqbfb_info), &pdev->dev); > > + if (!fbinfo) { > > + ret = -ENOMEM; > > + goto out2; > > + } > > + > > + info = fbinfo->par; > > + info->fb = fbinfo; > > + info->dev = &pdev->dev; > > + > > + platform_set_drvdata(pdev, fbinfo); > > + > > + strcpy(fbinfo->fix.id, driver_name); > > + > > + fbinfo->fix.type = FB_TYPE_PACKED_PIXELS; > > + fbinfo->fix.type_aux = 0; > > + fbinfo->fix.xpanstep = 0; > > + fbinfo->fix.ypanstep = 0; > > + fbinfo->fix.ywrapstep = 0; > > + fbinfo->fix.accel = FB_ACCEL_NONE; > > + fbinfo->fix.visual = FB_VISUAL_TRUECOLOR; > > + > > + fbinfo->var.nonstd = 0; > > + fbinfo->var.activate = FB_ACTIVATE_NOW; > > + fbinfo->var.height = -1; > > + fbinfo->var.width = -1; > > + fbinfo->var.accel_flags = 0; > > + fbinfo->var.vmode = FB_VMODE_NONINTERLACED; > > + > > + fbinfo->var.xres = LCD_X_RES; > > + fbinfo->var.xres_virtual = LCD_X_RES; > > + fbinfo->var.yres = LCD_Y_RES; > > + fbinfo->var.yres_virtual = LCD_Y_RES; > > + fbinfo->var.bits_per_pixel = LCD_BPP; > > + > > + fbinfo->var.red.offset = 0; > > + fbinfo->var.green.offset = 8; > > + fbinfo->var.blue.offset = 16; > > + fbinfo->var.transp.offset = 0; > > + fbinfo->var.red.length = 8; > > + fbinfo->var.green.length = 8; > > + fbinfo->var.blue.length = 8; > > + fbinfo->var.transp.length = 0; > > + fbinfo->fix.smem_len = LCD_X_RES * LCD_Y_RES * LCD_BPP / 8; > > + > > + fbinfo->fix.line_length = fbinfo->var.xres_virtual * > > + fbinfo->var.bits_per_pixel / 8; > > + > > + > > + fbinfo->fbops = &bfin_t350mcqb_fb_ops; > > + fbinfo->flags = FBINFO_FLAG_DEFAULT; > > + > > + info->fb_buffer = > > + dma_alloc_coherent(NULL, fbinfo->fix.smem_len, &info->dma_handle, > > + GFP_KERNEL); > > + > > + if (NULL == info->fb_buffer) { > > + printk(KERN_ERR DRIVER_NAME > > + ": couldn't allocate dma buffer.\n"); > > + ret = -ENOMEM; > > + goto out3; > > + } > > + > > + memset(info->fb_buffer, 0, fbinfo->fix.smem_len); > > + > > + fbinfo->screen_base = (void *)info->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET; > > + fbinfo->fix.smem_start = (int)info->fb_buffer + ACTIVE_VIDEO_MEM_OFFSET; > > + > > + fbinfo->fbops = &bfin_t350mcqb_fb_ops; > > + > > + fbinfo->pseudo_palette = kmalloc(sizeof(u32) * 16, GFP_KERNEL); > > + if (!fbinfo->pseudo_palette) { > > + printk(KERN_ERR DRIVER_NAME > > + "Fail to allocate pseudo_palette\n"); > > + > > + ret = -ENOMEM; > > + goto out4; > > + } > > + > > + memset(fbinfo->pseudo_palette, 0, sizeof(u32) * 16); > > You just invented kzalloc! >
Right
> > > > + if (fb_alloc_cmap(&fbinfo->cmap, BFIN_LCD_NBR_PALETTE_ENTRIES, 0) > > + < 0) { > > + printk(KERN_ERR DRIVER_NAME > > + "Fail to allocate colormap (%d entries)\n", > > + BFIN_LCD_NBR_PALETTE_ENTRIES); > > + ret = -EFAULT; > > + goto out5; > > + } > > + > > + if (bfin_t350mcqb_request_ports(1)) { > > + printk(KERN_ERR DRIVER_NAME ": couldn't request gpio port.\n"); > > + ret = -EFAULT; > > + goto out6; > > + } > > + > > + info->irq = platform_get_irq(pdev, 0); > > + if (info->irq < 0) { > > + ret = -EINVAL; > > + goto out7; > > + } > > + > > + if (request_irq(info->irq, (void *)bfin_t350mcqb_irq_error, IRQF_DISABLED, > > + "PPI ERROR", info) < 0) { > > + printk(KERN_ERR DRIVER_NAME > > + ": unable to request PPI ERROR IRQ\n"); > > + ret = -EFAULT; > > EFAULT? >
It should return the request_irq return value and other similar function calls should be checked like this, right? Michael.
-Bryan
| |