Re: [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505

[Alexey Charkov <alchark@gmail.com>]

2010/11/8 Paul Mundt <lethal@linux-sh.org>:
> On Sun, Nov 07, 2010 at 07:28:57PM +0300, Alexey Charkov wrote:
>> +static int __devinit vt8500lcd_probe(struct platform_device *pdev)
>> +{
>
> ...
>
>> +     addr = fbi;
>> +     addr = addr + sizeof(struct vt8500lcd_info);
>> +     fbi->fb.pseudo_palette  = addr;
>> +
> ...
>
>> +     fbi->palette_size       = PAGE_ALIGN(512);
>> +     fbi->palette_cpu        = dma_alloc_coherent(&pdev->dev,
>> +                                                  fbi->palette_size,
>> +                                                  &fbi->palette_phys,
>> +                                                  GFP_KERNEL);
>> +     if (fbi->fb.pseudo_palette == NULL) {
>> +             dev_err(&pdev->dev, "Failed to allocate palette buffer\n");
>> +             ret = -ENOMEM;
>> +             goto failed_free_io;
>> +     }
>> +
> This looks like a bogus test, you've already allocated enough space for
> the pseudo_palette and will have bailed out on the kmalloc() failing well
> before this. You also don't have any error handling for fbi->palette_cpu,
> which is presumably what you intended to do here.
>

True, this has to be corrected (old copy-paste error left unnoticed
somehow). It's also deallocated wrongly in the error path, which I
have just noticed.

>> +static int __devexit vt8500lcd_remove(struct platform_device *pdev)
>> +{
>> +     struct vt8500lcd_info *fbi = platform_get_drvdata(pdev);
>> +     struct resource *res;
>> +     int irq;
>> +
>> +     if (!fbi)
>> +             return 0;
>> +
>> +     unregister_framebuffer(&fbi->fb);
>> +
>> +     writel(0, fbi->regbase);
>> +
>> +     if (fbi->fb.cmap.len)
>> +             fb_dealloc_cmap(&fbi->fb.cmap);
>> +
>> +     irq = platform_get_irq(pdev, 0);
>> +     free_irq(irq, fbi);
>> +
>> +     iounmap(fbi->regbase);
>> +
>
> You're also missing a dma_free_coherent() here.
>

True, will be fixed.

>> +static int __devinit wm8505fb_probe(struct platform_device *pdev)
>> +{
>> +     fbi->fb.screen_base     = pdata->video_mem_virt;
>> +     fbi->fb.screen_size     = pdata->video_mem_len;
>> +
> ...
>
>> +failed_free_mem:
>> +     free_pages_exact(fbi->fb.screen_base, fbi->fb.screen_size);
>
> ...
>
> What in the name of all that is holy are you doing here? If you need to
> have your platform deal with virtual address allocation and freeing then
> you should pass in callbacks for that and hide the instrumentation
> details there. Presently this is tying you down to an alloc_pages_exact()
> interface buried in the board setup, which isn't going to mesh well with
> other platforms that may wish to go about this an alternate way (like
> memblock reservations).
>

Actually, this is a leftover from a previous implementation with
alloc_pages_exact(), which could not work for larger screen sizes (due
to the framebuffer growing over 4MB). Now memory is reserved via
memblock, so this should have probably been just dropped.

>> diff --git a/drivers/video/wmt_ge_rops.c b/drivers/video/wmt_ge_rops.c
>> new file mode 100644
>> index 0000000..c71f97e
>> --- /dev/null
>> +++ b/drivers/video/wmt_ge_rops.c
>> +void __iomem *regbase;
>> +
> Uhm, no. If this is only used in this driver then just make it static.
> Given that you are using the driver model here though and could
> theoretically support multiple rop engines, you're much better off making
> this private data and burying it under the appropriate per-device data
> structures.
>

Is that platform_{set,get}_drvdata() what you are talking about? Using
multiple engines seems extremely unlikely, though :)

>> +int wmt_ge_sync(struct fb_info *p)
>> +{
>> +     while (readl(regbase + GE_STATUS_OFF) & 4)
>> +             /* busy wait */;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL(wmt_ge_sync);
>> +
> While I admire your optimism in your hardware, experience suggests you
> really want a timeout here. You may also wish to insert a cpu_relax()
> here.
>

I wonder if this callback is allowed to sleep? In principle, the
hardware seems to support interrupt generation on operation
completion, so maybe wait_event_interruptible_timeout() could be used
here to remove the busy wait altogether?

Thank you for the comments, Paul!

Best regards,
Alexey