Hi, On Fri, Jun 19, 2009 at 15:05:52, Krzysztof Helt wrote: > HI, > > This version of the driver is much cleaner however major issues still remains. > > 1. The driver seems not able to change graphics mode (depth or resolution) > but check_var() and set_par() are defined. The set_par() function does not > modify any register. You should move this few settings done there to > lcd initialization functions and just remove the check_var() and set_par().
Forgot to remove this in my last version. I'll do it this time. > > 2. The driver does not set pseudo palette entries but uses true color mode > (16-bit). See other driver show to do this (I advice tdfxfb.c). Sorry, but I didn't understand this comment. By default this driver uses PSEUDOCOLOR mode and this controller does not need palette data for TRUECOLOR mode. > > 3. The request_mem_region() should be accompanied by the release_mem_region() > (I have not noticed it in my previous review). > > 4. Check error paths in the probing functions. Not all already requested/allocated > resources are released correctly (e.g. the in one path there is no clk_put() called). > Will take care of both the above comments. > Minor issues are in comments below. > > If any of above issues is false let me know. I am not specialist in all types of > embedded lcd controllers. > > Kind regards, > Krzysztof > > On Thu, 18 Jun 2009 10:42:34 -0400 > "Rajashekhara, Sudhakar" <[email protected]> wrote: > > > Adds LCD controller (LCDC) driver for TI's DA8xx/OMAP-L1xx architecture. > > LCDC specifications can be found at http://www.ti.com/litv/pdf/sprufm0a. > > > > LCDC on DA8xx consists of two independent controllers, the Raster Controller > > and the LCD Interface Display Driver (LIDD) controller. LIDD further supports > > character and graphic displays. > > > > This patch adds support for the graphic display (Sharp LQ035Q3DG01) found on > > the DA830 based EVM. The EVM details can be found at: > > http://support.spectrumdigital.com/boards/dskda830/revc/. > > > > Signed-off-by: Sudhakar Rajashekhara <[email protected]> > > Signed-off-by: Pavel Kiryukhin <[email protected]> > > Signed-off-by: Steve Chen <[email protected]> > > --- [. . . ] > > + > > +/* Palette Initialization */ > > +static int init_palette(struct fb_info *info) > > +{ > > + struct da8xx_fb_par *par = info->par; > > + unsigned short *palette = (unsigned short *)par->p_palette_base; > > + unsigned short i, size; > > + > > + /* Palette Size */ > > + size = (info->cmap.len / sizeof(*palette)); > > + > > + /* Clear the Palette */ > > + memset(palette, 0, info->cmap.len); > > + > > + /* Initialization of Palette for Default values */ > > + for (i = 0; i < size; i++) > > + *(unsigned short *)(palette + i) = i; > > + > > + /* Setup the BPP */ > > + switch (info->var.bits_per_pixel) { > > + case 1: > > + palette[0] |= BIT(11); > > + break; > > + case 2: > > + palette[0] |= BIT(12); > > + break; > > + case 4: > > + palette[0] |= (2 << 12); > > + break; > > + case 8: > > + palette[0] |= (3 << 12); > > + break; > > + case 16: > > + palette[0] |= (4 << 12); > > + break; > > + default: > > + dev_dbg(info->dev, "GLCD: Unsupported Video BPP %d!\n", > > + info->var.bits_per_pixel); > > + return -EINVAL; > > + } > > + > > + for (i = 0; i < size; i++) > > + par->pseudo_palette[i] = i; > > + > > The pseudo palette entries do not need to be initialized. Just handle them > correctly in the fb_setcolreg. Do you mean to say this whole function is not needed or just the above line? > > > + return 0; > > +} [. . .] > > + > > + > > +static int __init fb_probe(struct platform_device *device) > > +{ > > + struct da8xx_lcdc_platform_data *fb_pdata = > > + device->dev.platform_data; > > + struct lcd_ctrl_config *lcd_cfg; > > + struct da8xx_panel *lcdc_info; > > + struct fb_info *da8xx_fb_info; > > + struct resource *lcdc_regs; > > + struct clk *fb_clk = NULL; > > + struct da8xx_fb_par *par; > > + resource_size_t len; > > + int ret, i; > > + > > + if (fb_pdata == NULL) { > > + dev_err(&device->dev, "Can not get platform data\n"); > > + return -ENOENT; > > + } > > + > > + lcdc_regs = platform_get_resource(device, IORESOURCE_MEM, 0); > > + if (!lcdc_regs) { > > + dev_err(&device->dev, > > + "Can not get memory resource for LCD controller\n"); > > + return -ENOENT; > > + } > > + > > + len = lcdc_regs->end - lcdc_regs->start + 1; > > + > > + lcdc_regs = request_mem_region(lcdc_regs->start, len, lcdc_regs->name); > > + if (!lcdc_regs) > > + return -EBUSY; > > + > > + da8xx_fb_reg_base = (resource_size_t)ioremap(lcdc_regs->start, len); > > + > > + fb_clk = clk_get(&device->dev, NULL); > > + if (IS_ERR(fb_clk)) { > > + dev_err(&device->dev, "Can not get device clock\n"); > > + return -ENODEV; > > + } > > + ret = clk_enable(fb_clk); > > + if (ret) > > + goto err_clk_put; > > + > > + for (i = 0, lcdc_info = known_lcd_panels; > > + i < ARRAY_SIZE(known_lcd_panels); > > + i++, lcdc_info++) { > > + if (strcmp(fb_pdata->type, lcdc_info->name) == 0) > > + break; > > + } > > + > > + if (i == ARRAY_SIZE(known_lcd_panels)) { > > + dev_err(&device->dev, "GLCD: No valid panel found\n"); > > + return -ENODEV; > > Missing the clk_put()/clk_disable() here. I'll correct this. > > > + } else > > + dev_info(&device->dev, "GLCD: Found %s panel\n", > > + fb_pdata->type); > > + > > + lcd_cfg = (struct lcd_ctrl_config *)fb_pdata->controller_data; > > + > > + da8xx_fb_info = framebuffer_alloc(sizeof(struct da8xx_fb_par), > > + &device->dev); > > + if (!da8xx_fb_info) { > > + dev_dbg(&device->dev, "Memory allocation failed for fb_info\n"); > > + ret = -ENOMEM; > > + goto err_clk_disable; > > + } > > + > > + par = da8xx_fb_info->par; > > + > > + if (lcd_init(par, lcd_cfg, lcdc_info) < 0) { > > + dev_err(&device->dev, "lcd_init failed\n"); > > + ret = -EFAULT; > > The da8xx_fb_info should be released as well. OK. > > > + goto err_clk_disable; > > + } > > + > > + /* allocate frame buffer */ > > + da8xx_fb_info->screen_base = dma_alloc_coherent(NULL, > > + par->databuf_sz + PAGE_SIZE, > > + (resource_size_t *) > > + &da8xx_fb_info->fix.smem_start, > > + GFP_KERNEL | GFP_DMA); > > + > > + if (!da8xx_fb_info->screen_base) { > > + dev_err(&device->dev, > > + "GLCD: kmalloc for frame buffer failed\n"); > > + ret = -EINVAL; > > + goto err_release_fb; > > + } > > + > > + /* move palette base pointer by (PAGE_SIZE - palette_sz) bytes */ > > + par->v_palette_base = da8xx_fb_info->screen_base + > > + (PAGE_SIZE - par->palette_sz); > > + par->p_palette_base = da8xx_fb_info->fix.smem_start + > > + (PAGE_SIZE - par->palette_sz); > > + > > + /* the rest of the frame buffer is pixel data */ > > + da8xx_fb_info->screen_base = par->v_palette_base + par->palette_sz; > > + da8xx_fb_info->fix.smem_start = par->p_palette_base + par->palette_sz; > > + da8xx_fb_info->fix.smem_len = par->databuf_sz - par->palette_sz; > > + > > + par->lcdc_clk = fb_clk; > > + > > + da8xx_fb_fix.smem_start = (unsigned long)da8xx_fb_info->fix.smem_start; > > + da8xx_fb_fix.smem_len = da8xx_fb_info->fix.smem_len; > > + > > Do fb_info->fix = da8xx_fb_fix earlier so these lines becomes redundant. OK. > > > + init_waitqueue_head(&par->da8xx_wq); > > + > > + par->irq = platform_get_irq(device, 0); > > + if (par->irq < 0) { > > + ret = -ENOENT; > > + goto err_release_fb_mem; > > + } > > + > > + ret = request_irq(par->irq, lcdc_irq_handler, 0, DRIVER_NAME, par); > > + if (ret) > > + goto err_free_irq; > > The irq handler has not been registered in case of error so the free_irq() call > is not required. OK. > > > + > > + /* Initialize par */ > > + da8xx_fb_info->var.bits_per_pixel = lcd_cfg->bpp; > > + > > + da8xx_fb_var.xres = lcdc_info->width; > > + da8xx_fb_var.xres_virtual = lcdc_info->width; > > + > > + da8xx_fb_var.yres = lcdc_info->height; > > + da8xx_fb_var.yres_virtual = lcdc_info->height; > > + > > + da8xx_fb_var.grayscale = > > + lcd_cfg->p_disp_panel->panel_shade == MONOCHROME ? 1 : 0; > > + da8xx_fb_var.bits_per_pixel = lcd_cfg->bpp; > > + > > + da8xx_fb_var.hsync_len = lcdc_info->hsw; > > + da8xx_fb_var.vsync_len = lcdc_info->vsw; > > + > > + /* Initialize fbinfo */ > > + da8xx_fb_info->flags = FBINFO_FLAG_DEFAULT; > > + da8xx_fb_info->fix = da8xx_fb_fix; > > + da8xx_fb_info->var = da8xx_fb_var; > > + da8xx_fb_info->fbops = &da8xx_fb_ops; > > + da8xx_fb_info->pseudo_palette = par->pseudo_palette; > > + > > + ret = fb_alloc_cmap(&da8xx_fb_info->cmap, PALETTE_SIZE, 0); > > + if (ret) > > + goto err_free_irq; > > + > > + /* First palette_sz byte of the frame buffer is the palette */ > > + da8xx_fb_info->cmap.len = par->palette_sz; > > + > > + /* Initialize the Palette */ > > + ret = init_palette(da8xx_fb_info); > > + if (ret < 0) > > + goto err_free_irq; > > One should free cmap memory here. OK. > > > + > > + /* Flush the buffer to the screen. */ > > + lcd_blit(LOAD_DATA, par); > > + > > + /* initialize var_screeninfo */ > > + da8xx_fb_var.activate = FB_ACTIVATE_FORCE; > > + fb_set_var(da8xx_fb_info, &da8xx_fb_var); > > + > > + dev_set_drvdata(&device->dev, da8xx_fb_info); > > + /* Register the Frame Buffer */ > > + if (register_framebuffer(da8xx_fb_info) < 0) { > > + dev_err(&device->dev, > > + "GLCD: Frame Buffer Registration Failed!\n"); > > + ret = -EINVAL; > > + goto err_dealloc_cmap; > > + } > > + > > + /* enable raster engine */ > > + lcdc_write(lcdc_read(LCD_RASTER_CTRL_REG) | > > + LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG); > > + > > + return 0; > > + > > +err_dealloc_cmap: > > + fb_dealloc_cmap(&da8xx_fb_info->cmap); > > + > > +err_free_irq: > > + free_irq(par->irq, NULL); > > + > > +err_release_fb_mem: > > + dma_free_coherent(NULL, par->databuf_sz + PAGE_SIZE, > > + da8xx_fb_info->screen_base, > > + da8xx_fb_info->fix.smem_start); > > + > > +err_release_fb: > > + framebuffer_release(da8xx_fb_info); > > + > > +err_clk_disable: > > + clk_disable(fb_clk); > > + > > +err_clk_put: > > + clk_put(fb_clk); > > + > > + return ret; > > +} > > + > > +#ifdef CONFIG_PM > > +static int fb_suspend(struct platform_device *dev, pm_message_t state) > > +{ > > + return -EBUSY; > > +} > > +static int fb_resume(struct platform_device *dev) > > +{ > > + return -EBUSY; > > +} > > +#else > > +#define fb_suspend NULL > > +#define fb_resume NULL > > +#endif > > + > > +static struct platform_driver da8xx_fb_driver = { > > + .probe = fb_probe, > > + .remove = fb_remove, > > + .suspend = fb_suspend, > > + .resume = fb_resume, > > + .driver = { > > + .name = DRIVER_NAME, > > + .owner = THIS_MODULE, > > + }, > > +}; > > + > > +static int __init da8xx_fb_init(void) > > +{ > > + return platform_driver_register(&da8xx_fb_driver); > > +} > > + > > +static void __exit da8xx_fb_cleanup(void) > > +{ > > + platform_driver_unregister(&da8xx_fb_driver); > > +} > > + > > +module_init(da8xx_fb_init); > > +module_exit(da8xx_fb_cleanup); > > + > > +MODULE_DESCRIPTION("Framebuffer driver for TI da8xx/omap-l1xx"); > > +MODULE_AUTHOR("Texas Instruments"); > > +MODULE_LICENSE("GPL"); I'll submit the updated version of this patch soon. Thanks, Sudhakar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
