On Wed, 2 Jun 2010 21:10:32 +0200
Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> This patch adds support for the LCD controller on JZ4740 SoCs.
>
>
> ...
>
> +struct jzfb_framedesc {
> + uint32_t next;
> + uint32_t addr;
> + uint32_t id;
> + uint32_t cmd;
> +} __attribute__((packed));
We have a little __packed helper.
>
> ...
>
> +static int jzfb_setcolreg(unsigned regno, unsigned red, unsigned green,
> + unsigned blue, unsigned transp, struct fb_info *fb)
> +{
> + uint32_t color;
> +
> + if (regno >= 16)
> + return -EINVAL;
> +
> + color = jzfb_convert_color_to_hw(red, fb->var.red.length);
> + color |= jzfb_convert_color_to_hw(green, fb->var.green.length);
> + color |= jzfb_convert_color_to_hw(blue, fb->var.blue.length);
> + color |= jzfb_convert_color_to_hw(transp, fb->var.transp.length);
> +
> + ((uint32_t *)(fb->pseudo_palette))[regno] = color;
The typecast apepars to be unneeded. If it _was_ needed then this code
would look rather fishy from an endianness POV.
> + return 0;
> +}
> +
>
> ...
>
> +static int __devinit jzfb_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct jzfb *jzfb;
> + struct fb_info *fb;
> + struct jz4740_fb_platform_data *pdata = pdev->dev.platform_data;
> + struct resource *mem;
> +
> + if (!pdata) {
> + dev_err(&pdev->dev, "Missing platform data\n");
> + return -ENOENT;
> + }
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem) {
> + dev_err(&pdev->dev, "Failed to get register memory resource\n");
> + return -ENOENT;
> + }
> +
> + mem = request_mem_region(mem->start, resource_size(mem), pdev->name);
> + if (!mem) {
> + dev_err(&pdev->dev, "Failed to request register memory region\n");
> + return -EBUSY;
> + }
> +
> + fb = framebuffer_alloc(sizeof(struct jzfb), &pdev->dev);
> + if (!fb) {
> + dev_err(&pdev->dev, "Failed to allocate framebuffer device\n");
> + ret = -ENOMEM;
> + goto err_release_mem_region;
> + }
> +
> + fb->fbops = &jzfb_ops;
> + fb->flags = FBINFO_DEFAULT;
> +
> + jzfb = fb->par;
> + jzfb->pdev = pdev;
> + jzfb->pdata = pdata;
> + jzfb->mem = mem;
> +
> + jzfb->ldclk = clk_get(&pdev->dev, "lcd");
> + if (IS_ERR(jzfb->ldclk)) {
> + ret = PTR_ERR(jzfb->ldclk);
> + dev_err(&pdev->dev, "Failed to get lcd clock: %d\n", ret);
> + goto err_framebuffer_release;
> + }
> +
> + jzfb->lpclk = clk_get(&pdev->dev, "lcd_pclk");
> + if (IS_ERR(jzfb->lpclk)) {
> + ret = PTR_ERR(jzfb->lpclk);
> + dev_err(&pdev->dev, "Failed to get lcd pixel clock: %d\n", ret);
> + goto err_put_ldclk;
> + }
> +
> + jzfb->base = ioremap(mem->start, resource_size(mem));
> +
> + if (!jzfb->base) {
> + dev_err(&pdev->dev, "Failed to ioremap register memory region\n");
> + ret = -EBUSY;
> + goto err_put_lpclk;
> + }
> +
> + platform_set_drvdata(pdev, jzfb);
> +
> + fb_videomode_to_modelist(pdata->modes, pdata->num_modes,
> + &fb->modelist);
> + fb->mode = pdata->modes;
> +
> + fb_videomode_to_var(&fb->var, fb->mode);
> + fb->var.bits_per_pixel = pdata->bpp;
> + jzfb_check_var(&fb->var, fb);
> +
> + ret = jzfb_alloc_devmem(jzfb);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to allocate video memory\n");
> + goto err_iounmap;
> + }
> +
> + fb->fix = jzfb_fix;
> + fb->fix.line_length = fb->var.bits_per_pixel * fb->var.xres / 8;
> + fb->fix.mmio_start = mem->start;
> + fb->fix.mmio_len = resource_size(mem);
> + fb->fix.smem_start = jzfb->vidmem_phys;
> + fb->fix.smem_len = fb->fix.line_length * fb->var.yres;
> + fb->screen_base = jzfb->vidmem;
> + fb->pseudo_palette = jzfb->pseudo_palette;
> +
> + fb_alloc_cmap(&fb->cmap, 256, 0);
> +
> + mutex_init(&jzfb->lock);
I'd suggest initializing the mutex much earlier. THings like
jzfb_check_var() and jzfb_alloc_devmem() don't currently take that
lock, but who knows what might happen in the future..
> + clk_enable(jzfb->ldclk);
> + jzfb->is_enabled = 1;
> +
> + writel(jzfb->framedesc->next, jzfb->base + JZ_REG_LCD_DA0);
> + jzfb_set_par(fb);
> +
> + jz_gpio_bulk_request(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> + jz_gpio_bulk_request(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> +
> + ret = register_framebuffer(fb);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register framebuffer: %d\n", ret);
> + goto err_free_devmem;
> + }
> +
> + jzfb->fb = fb;
> +
> + return 0;
> +
> +err_free_devmem:
> + jz_gpio_bulk_free(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
> + jz_gpio_bulk_free(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
> +
> + fb_dealloc_cmap(&fb->cmap);
> + jzfb_free_devmem(jzfb);
> +err_iounmap:
> + iounmap(jzfb->base);
> +err_put_lpclk:
> + clk_put(jzfb->lpclk);
> +err_put_ldclk:
> + clk_put(jzfb->ldclk);
> +err_framebuffer_release:
> + framebuffer_release(fb);
> +err_release_mem_region:
> + release_mem_region(mem->start, resource_size(mem));
> + return ret;
> +}
> +
>
> ...
>
