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

[Alexey Charkov]

2010/11/8 Paul Mundt <lethal@xxxxxxxxxxxx>:
> On Mon, Nov 08, 2010 at 05:14:07PM +0300, Alexey Charkov wrote:
>> This incorporates fixes to the issues that Paul has identified.
>> MMIO register pointer in wmt_ge_rops was just made static, as I
>> could not find any immediately obvious way to pass drvdata around
>> (the whole functionality of this driver is in exported functions
>> that are called from a display driver context, which does not know
>> about the rop engine specific data structures).
>>
> Looking better.. just a few minor things left.
>
>> diff --git a/drivers/video/vt8500lcdfb.c b/drivers/video/vt8500lcdfb.c
>> new file mode 100644
>> index 0000000..640d8a3
>> --- /dev/null
>> +++ b/drivers/video/vt8500lcdfb.c
>> +#include <asm/irq.h>
>> +
> linux/irq.h is preferred here.
>
>> diff --git a/drivers/video/wm8505fb.c b/drivers/video/wm8505fb.c
>> new file mode 100644
>> index 0000000..560c926
>> --- /dev/null
>> +++ b/drivers/video/wm8505fb.c
>> +#include <asm/irq.h>
>> +
> Likewise.
>

Ok.

>> +static int wm8505fb_pan_display(struct fb_var_screeninfo *var,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct fb_info *info)
>> +{
>> + Â Â struct wm8505fb_info *fbi = container_of(info,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct wm8505fb_info,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â fb);
>> +
> Sice you are open-coding this container_of() all over the place you may
> simply want to make a wrapper for this. ie,
>
> #define to_wm8505fb_info(info) Âcontainer_of(info, struct wm8505fb_info, fb)
>
> and then just doing struct wm855fb_info *fbi = to_wm8505fb_info(info);
>
> This wiwll also save you from having that ugly multi-line split in the
> container_of() and keep you well within 80 characters.
>

That's true, thanks.

>> +static int __devinit wm8505fb_probe(struct platform_device *pdev)
>> +{
> ...
>
>> + Â Â ret = register_framebuffer(&fbi->fb);
>> + Â Â if (ret < 0) {
>> + Â Â Â Â Â Â dev_err(&pdev->dev,
>> + Â Â Â Â Â Â Â Â Â Â "Failed to register framebuffer device: %d\n", ret);
>> + Â Â Â Â Â Â goto failed_free_cmap;
>> + Â Â }
>> +
>> + Â Â ret = device_create_file(&pdev->dev, &dev_attr_contrast);
>> + Â Â if (ret < 0) {
>> + Â Â Â Â Â Â printk(KERN_WARNING "fb%d: failed to register attributes (%d)\n",
>> + Â Â Â Â Â Â Â Â Â Â fbi->fb.node, ret);
>> + Â Â }
>> +
> ...
>> +}
>> +
>> +static int __devexit wm8505fb_remove(struct platform_device *pdev)
>> +{
>> + Â Â struct wm8505fb_info *fbi = platform_get_drvdata(pdev);
>> + Â Â struct resource *res;
>> +
>> + Â Â if (!fbi)
>> + Â Â Â Â Â Â return 0;
>> +
> I would kill this test as well. If this ever triggers, something horribly
> wrong has happened and you likely have bigger things to worry about.
>

But a couple of extra instructions for error handling to hold in the
kernel binary should not hurt, should they? Are there benefits aside
from code compaction?

>> + Â Â unregister_framebuffer(&fbi->fb);
>> +
> You're missing a device_remove_file().
>

True, will be fixed.

>> +static struct platform_driver wm8505fb_driver = {
>> +   .probe     Â= wm8505fb_probe,
>> +   .remove     = wm8505fb_remove,
>
> As your remove function is flagged devexit, this ought to be wrapped with
> __devexit_p(). This will save you a bit of space as the kernel will
> discard the remove function outright if you're not using this as a module
> or with hotplug. The same applies to your other remove functions too.
>

Ok, thanks for pointing out!

>> diff --git a/drivers/video/wmt_ge_rops.c b/drivers/video/wmt_ge_rops.c
>> new file mode 100644
>> index 0000000..b201a60
>> --- /dev/null
>> +++ b/drivers/video/wmt_ge_rops.c
>> +EXPORT_SYMBOL(wmt_ge_fillrect);
>> +EXPORT_SYMBOL(wmt_ge_copyarea);
>> +EXPORT_SYMBOL(wmt_ge_sync);
>> +
> ...
>
> Is there a particular reason why you are favouring EXPORT_SYMBOL? In
> general we prefer that new infrastructure patches and the like stick with
> EXPORT_SYMBOL_GPL, as this discourages use by non-GPLed modules going
> forward.
>

Well, I have no personal preference towards these, so I just took what
was in cfb*.c as a guidance. If the *_GPL variant is more welcome, it
can be changed.

>> +static int __devinit wmt_ge_rops_probe(struct platform_device *pdev)
>> +{
> ...
>> + Â Â regbase = ioremap(res->start, resource_size(res));
>> + Â Â if (regbase == NULL) {
>> + Â Â Â Â Â Â dev_err(&pdev->dev, "failed to map I/O memory\n");
>> + Â Â Â Â Â Â ret = -EBUSY;
>> + Â Â Â Â Â Â goto error;
>> + Â Â }
>> +
> You might also want to do something like:
>
> Â Â Â Â/* Only one ROP engine is presently supported. */
> Â Â Â Âif (unlikely(regbase)) {
> Â Â Â Â Â Â Â ÂWARN_ON(1);
> Â Â Â Â Â Â Â Âreturn -EBUSY;
> Â Â Â Â}
>
> Â Â Â Âregbase = ioremap(...);
> Â Â Â Â...
>

But for that I'd have to initialize regbase to NULL (so as not to use
an uninitialized variable), wouldn't I? checkpatch.pl complains on
that...

>> + Â Â writel(1, regbase + GE_ENABLE_OFF);
>> + Â Â printk(KERN_INFO "Enabled support for WMT GE raster acceleration\n");
>> +
>> + Â Â return 0;
>> +
>> +error:
>> + Â Â return ret;
>> +}
>> +
>> +static int __devexit wmt_ge_rops_remove(struct platform_device *pdev)
>> +{
>> + Â Â iounmap(regbase);
>> + Â Â return 0;
>> +}
>> +
> You're missing a:
>
> Â Â Â Âwritel(0, regbase + GE_ENABLE_OFF);
>
> here?
>

In fact, this module only uses a subset of GE functions, so I'm
somewhat reluctant to disable the hardware altogether when unloading
the module. And should the hardware really be disabled when the driver
is removed?

>> +MODULE_AUTHOR("Alexey Charkov <alchark@xxxxxxxxx");
>> +MODULE_DESCRIPTION("Accelerators for raster operations using "
>> + Â Â Â Â Â Â Â Â"WonderMedia Graphics Engine");
>> +MODULE_LICENSE("GPL");
>
> Your other drivers appear to be lacking AUTHOR and DESCRIPTION
> definitions.
>

Will be fixed.

Thanks,
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/