Re: [RFC][PATCH 16/26] fbdev: Add JZ4740 framebuffer driver

[Andrew Morton <akpm@linux-foundation.org>]

On Wed,  2 Jun 2010 21:10:32 +0200
Lars-Peter Clausen <lars@metafoo.de> 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;
> +}
> +
>
> ...
>