Andrew Morton wrote:
> 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.
>
>
Ok.
>> ...
>>
>> +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.
>
>
It is needed, because the pseudo_palette member of struct fb_info is a
void pointer.
>> + 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..
>
>
Hm, ok.
>> + 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;
>> +}
>> +
>>
>> ...
>>
>>
>
>
