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
[off-list ref] wrote:

On Thu, 31 Jan 2008 00:49:43 +0800 Bryan Wu [off-list ref] 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;
 > +}
 > +

quoted

+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;
 > +}
 > +
 > +

quoted

+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