From: Paul Mundt <lethal@linux-sh.org>
To: Alexey Charkov <alchark@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
vt8500-wm8505-linux-kernel@googlegroups.com,
Andrew Morton <akpm@linux-foundation.org>,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
Florian Tobias Schandinat <FlorianSchandinat@gmx.de>,
Ralf Baechle <ralf@linux-mips.org>,
"David S. Miller" <davem@davemloft.net>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/6 v3] ARM: Add support for the display controllers in VT8500 and WM8505
Date: Tue, 9 Nov 2010 05:43:16 +0900 [thread overview]
Message-ID: <20101108204315.GA12050@linux-sh.org> (raw)
In-Reply-To: <20101108141407.GA25739@alchark-u3s.lan>
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.
> +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.
> +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.
> + unregister_framebuffer(&fbi->fb);
> +
You're missing a device_remove_file().
> +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.
> 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.
> +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(...);
...
> + 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?
> +MODULE_AUTHOR("Alexey Charkov <alchark@gmail.com");
> +MODULE_DESCRIPTION("Accelerators for raster operations using "
> + "WonderMedia Graphics Engine");
> +MODULE_LICENSE("GPL");
Your other drivers appear to be lacking AUTHOR and DESCRIPTION
definitions.
next prev parent reply other threads:[~2010-11-08 20:43 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-07 16:28 [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Alexey Charkov
2010-11-07 16:28 ` [PATCH 2/6 v2] serial: Add support for UART on VIA VT8500 and compatibles Alexey Charkov
2010-11-07 23:08 ` Alan Cox
2010-11-08 0:58 ` [PATCH 2/6 v3] " Alexey Charkov
2010-11-08 10:46 ` Alan Cox
2010-11-08 17:33 ` [PATCH 2/6 v4] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 3/6 v2] input: Add support for VIA VT8500 and compatibles in i8042 Alexey Charkov
2010-11-12 22:54 ` Alexey Charkov
2010-11-12 23:30 ` Dmitry Torokhov
2010-11-13 0:00 ` Alexey Charkov
2010-12-22 21:41 ` [PATCH 3/6 v3] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 4/6 v2] usb: Add support for VIA VT8500 and compatibles in EHCI HCD Alexey Charkov
2010-11-07 16:28 ` [PATCH 5/6 v2] rtc: Add support for the RTC in VIA VT8500 and compatibles Alexey Charkov
2010-11-12 22:53 ` Alexey Charkov
2010-11-13 12:14 ` Lars-Peter Clausen
2010-11-13 23:56 ` [PATCH 5/6 v3] " Alexey Charkov
2010-11-14 15:50 ` Lars-Peter Clausen
2010-11-14 17:00 ` [PATCH 5/6 v4] " Alexey Charkov
2010-11-23 19:17 ` Alexey Charkov
2010-11-24 19:23 ` Lars-Peter Clausen
2010-11-24 22:47 ` [PATCH 5/6 v5] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Alexey Charkov
2010-11-08 4:17 ` Paul Mundt
2010-11-08 12:56 ` Alexey Charkov
2010-11-08 14:14 ` [PATCH 6/6 v3] " Alexey Charkov
2010-11-08 20:43 ` Paul Mundt [this message]
2010-11-08 21:15 ` Alexey Charkov
2010-11-08 21:30 ` Paul Mundt
2010-11-08 23:42 ` [PATCH 6/6 v4] " Alexey Charkov
2010-11-08 23:54 ` Paul Mundt
2010-11-09 0:03 ` Alexey Charkov
2010-11-09 7:36 ` Guennadi Liakhovetski
2010-11-09 9:39 ` Paul Mundt
2010-11-09 9:49 ` Alexey Charkov
2010-11-09 10:33 ` [PATCH 6/6 v3] " Russell King - ARM Linux
2010-11-09 10:51 ` Paul Mundt
2010-11-09 11:04 ` Russell King - ARM Linux
2010-11-09 13:02 ` Geert Uytterhoeven
2010-11-09 13:33 ` Arnd Bergmann
2010-11-09 16:20 ` Paul Mundt
2010-11-08 8:47 ` [PATCH 6/6 v2] " Arnd Bergmann
2010-11-09 10:23 ` Alexey Charkov
2010-11-09 15:03 ` Arnd Bergmann
2010-11-07 16:57 ` [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Russell King - ARM Linux
2010-11-07 17:08 ` Alexey Charkov
2010-11-07 17:17 ` Russell King - ARM Linux
2010-11-07 18:25 ` [PATCH 1/6 v3] " Alexey Charkov
2010-11-08 17:19 ` [PATCH 1/6 v4] " Alexey Charkov
2010-11-10 15:16 ` saeed bishara
2010-11-10 15:18 ` Russell King - ARM Linux
2010-11-10 15:20 ` saeed bishara
2010-11-11 21:23 ` [PATCH 1/6 v5] " Alexey Charkov
2010-11-11 23:49 ` Russell King - ARM Linux
2010-11-12 16:54 ` [PATCH 1/6 v6] " Alexey Charkov
2010-11-12 20:14 ` Alexey Charkov
2010-11-23 19:50 ` [PATCH 1/6 v7] " Alexey Charkov
2010-12-19 17:40 ` [PATCH 1/6 v8] " Alexey Charkov
2010-12-20 18:15 ` Arnd Bergmann
2010-12-20 19:15 ` Russell King - ARM Linux
2010-12-20 19:26 ` Alexey Charkov
2010-12-20 19:54 ` [PATCH 1/6 v9] " Alexey Charkov
2010-12-20 20:50 ` Ryan Mallon
2010-12-20 21:48 ` Alexey Charkov
2010-12-20 22:23 ` Ryan Mallon
2010-12-20 23:00 ` Alexey Charkov
2010-12-20 23:22 ` Ryan Mallon
2010-12-20 23:49 ` Alexey Charkov
2010-12-21 0:09 ` Alexey Charkov
2010-12-21 2:32 ` Ryan Mallon
2010-12-21 10:00 ` Alexey Charkov
2010-12-21 12:05 ` Arnd Bergmann
2010-12-21 19:39 ` Ryan Mallon
2010-12-22 21:18 ` [PATCH 1/6 v10] " Alexey Charkov
2010-12-22 21:52 ` Ryan Mallon
2010-12-22 22:02 ` Alexey Charkov
2010-12-22 22:32 ` Ryan Mallon
2010-12-22 22:25 ` [PATCH 1/6 v11] " Alexey Charkov
2010-12-22 22:59 ` Ryan Mallon
2010-12-22 23:38 ` [PATCH 1/6 v12] " Alexey Charkov
2010-12-28 14:52 ` Alexey Charkov
2011-01-15 0:53 ` Alexey Charkov
2011-01-15 0:58 ` Russell King - ARM Linux
2011-01-15 1:13 ` Alexey Charkov
2011-01-21 10:43 ` Russell King - ARM Linux
2011-07-06 12:34 ` [PATCH 1/6 v8] " Russell King - ARM Linux
2011-07-07 7:13 ` Alexey Charkov
2011-07-07 7:54 ` Arnd Bergmann
2011-07-07 7:59 ` Russell King - ARM Linux
2011-07-07 8:05 ` Arnd Bergmann
2010-11-07 17:00 ` [PATCH 1/6 v2] " Russell King - ARM Linux
2010-11-07 17:16 ` Alexey Charkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101108204315.GA12050@linux-sh.org \
--to=lethal@linux-sh.org \
--cc=FlorianSchandinat@gmx.de \
--cc=akpm@linux-foundation.org \
--cc=alchark@gmail.com \
--cc=davem@davemloft.net \
--cc=g.liakhovetski@gmx.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ralf@linux-mips.org \
--cc=vt8500-wm8505-linux-kernel@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).