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

[Alexey Charkov]

2010/11/8 Paul Mundt <lethal@xxxxxxxxxxxx>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/