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.
WARNING: multiple messages have this Message-ID (diff)
From: lethal@linux-sh.org (Paul Mundt) To: linux-arm-kernel@lists.infradead.org Subject: [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: 184+ 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 ` 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 16:28 ` Alexey Charkov 2010-11-07 23:08 ` Alan Cox 2010-11-07 23:08 ` Alan Cox 2010-11-08 0:58 ` [PATCH 2/6 v3] " Alexey Charkov 2010-11-08 0:58 ` Alexey Charkov 2010-11-08 10:46 ` Alan Cox 2010-11-08 10:46 ` Alan Cox 2010-11-08 17:33 ` [PATCH 2/6 v4] " Alexey Charkov 2010-11-08 17:33 ` 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-07 16:28 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 22:54 ` Alexey Charkov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-12 23:30 ` Dmitry Torokhov 2010-11-13 0:00 ` Alexey Charkov 2010-11-13 0:00 ` Alexey Charkov 2010-12-22 21:41 ` [PATCH 3/6 v3] " Alexey Charkov 2010-12-22 21:41 ` 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 ` 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-07 16:28 ` Alexey Charkov 2010-11-12 22:53 ` Alexey Charkov 2010-11-12 22:53 ` Alexey Charkov 2010-11-13 12:14 ` Lars-Peter Clausen 2010-11-13 12:14 ` Lars-Peter Clausen 2010-11-13 23:56 ` [PATCH 5/6 v3] " Alexey Charkov 2010-11-13 23:56 ` Alexey Charkov 2010-11-14 15:50 ` Lars-Peter Clausen 2010-11-14 15:50 ` Lars-Peter Clausen 2010-11-14 17:00 ` [PATCH 5/6 v4] " Alexey Charkov 2010-11-14 17:00 ` Alexey Charkov 2010-11-23 19:17 ` Alexey Charkov 2010-11-23 19:17 ` Alexey Charkov 2010-11-24 19:23 ` Lars-Peter Clausen 2010-11-24 19:23 ` Lars-Peter Clausen 2010-11-24 22:47 ` [PATCH 5/6 v5] " Alexey Charkov 2010-11-24 22:47 ` 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-07 16:28 ` Alexey Charkov 2010-11-08 4:17 ` Paul Mundt 2010-11-08 4:17 ` Paul Mundt 2010-11-08 12:56 ` Alexey Charkov 2010-11-08 12:56 ` Alexey Charkov 2010-11-08 14:14 ` [PATCH 6/6 v3] " Alexey Charkov 2010-11-08 14:14 ` Alexey Charkov 2010-11-08 20:43 ` Paul Mundt [this message] 2010-11-08 20:43 ` Paul Mundt 2010-11-08 21:15 ` Alexey Charkov 2010-11-08 21:15 ` Alexey Charkov 2010-11-08 21:30 ` Paul Mundt 2010-11-08 21:30 ` Paul Mundt 2010-11-08 23:42 ` [PATCH 6/6 v4] " Alexey Charkov 2010-11-08 23:42 ` Alexey Charkov 2010-11-08 23:54 ` Paul Mundt 2010-11-08 23:54 ` Paul Mundt 2010-11-09 0:03 ` Alexey Charkov 2010-11-09 0:03 ` Alexey Charkov 2010-11-09 7:36 ` Guennadi Liakhovetski 2010-11-09 7:36 ` Guennadi Liakhovetski 2010-11-09 9:39 ` Paul Mundt 2010-11-09 9:39 ` Paul Mundt 2010-11-09 9:49 ` Alexey Charkov 2010-11-09 9:49 ` Alexey Charkov 2010-11-09 10:33 ` [PATCH 6/6 v3] " Russell King - ARM Linux 2010-11-09 10:33 ` Russell King - ARM Linux 2010-11-09 10:51 ` Paul Mundt 2010-11-09 10:51 ` Paul Mundt 2010-11-09 11:04 ` Russell King - ARM Linux 2010-11-09 11:04 ` Russell King - ARM Linux 2010-11-09 13:02 ` Geert Uytterhoeven 2010-11-09 13:02 ` Geert Uytterhoeven 2010-11-09 13:33 ` Arnd Bergmann 2010-11-09 13:33 ` Arnd Bergmann 2010-11-09 16:20 ` Paul Mundt 2010-11-09 16:20 ` Paul Mundt 2010-11-08 8:47 ` [PATCH 6/6 v2] " Arnd Bergmann 2010-11-08 8:47 ` Arnd Bergmann 2010-11-09 10:23 ` Alexey Charkov 2010-11-09 10:23 ` Alexey Charkov 2010-11-09 15:03 ` Arnd Bergmann 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 16:57 ` Russell King - ARM Linux 2010-11-07 17:08 ` Alexey Charkov 2010-11-07 17:08 ` Alexey Charkov 2010-11-07 17:17 ` Russell King - ARM Linux 2010-11-07 17:17 ` Russell King - ARM Linux 2010-11-07 18:25 ` [PATCH 1/6 v3] " Alexey Charkov 2010-11-07 18:25 ` Alexey Charkov 2010-11-08 17:19 ` [PATCH 1/6 v4] " Alexey Charkov 2010-11-08 17:19 ` Alexey Charkov 2010-11-10 15:16 ` saeed bishara 2010-11-10 15:16 ` saeed bishara 2010-11-10 15:18 ` Russell King - ARM Linux 2010-11-10 15:18 ` Russell King - ARM Linux 2010-11-10 15:20 ` saeed bishara 2010-11-10 15:20 ` saeed bishara 2010-11-11 21:23 ` [PATCH 1/6 v5] " Alexey Charkov 2010-11-11 21:23 ` Alexey Charkov 2010-11-11 23:49 ` Russell King - ARM Linux 2010-11-11 23:49 ` Russell King - ARM Linux 2010-11-12 16:54 ` [PATCH 1/6 v6] " Alexey Charkov 2010-11-12 16:54 ` Alexey Charkov 2010-11-12 20:14 ` Alexey Charkov 2010-11-12 20:14 ` Alexey Charkov 2010-11-23 19:50 ` [PATCH 1/6 v7] " Alexey Charkov 2010-11-23 19:50 ` Alexey Charkov 2010-12-19 17:40 ` [PATCH 1/6 v8] " Alexey Charkov 2010-12-19 17:40 ` Alexey Charkov 2010-12-20 18:15 ` Arnd Bergmann 2010-12-20 18:15 ` Arnd Bergmann 2010-12-20 19:15 ` Russell King - ARM Linux 2010-12-20 19:15 ` Russell King - ARM Linux 2010-12-20 19:26 ` Alexey Charkov 2010-12-20 19:26 ` Alexey Charkov 2010-12-20 19:54 ` [PATCH 1/6 v9] " Alexey Charkov 2010-12-20 19:54 ` Alexey Charkov 2010-12-20 20:50 ` Ryan Mallon 2010-12-20 20:50 ` Ryan Mallon 2010-12-20 21:48 ` Alexey Charkov 2010-12-20 21:48 ` Alexey Charkov 2010-12-20 22:23 ` Ryan Mallon 2010-12-20 22:23 ` Ryan Mallon 2010-12-20 23:00 ` Alexey Charkov 2010-12-20 23:00 ` Alexey Charkov 2010-12-20 23:22 ` Ryan Mallon 2010-12-20 23:22 ` Ryan Mallon 2010-12-20 23:49 ` Alexey Charkov 2010-12-20 23:49 ` Alexey Charkov 2010-12-21 0:09 ` Alexey Charkov 2010-12-21 0:09 ` Alexey Charkov 2010-12-21 2:32 ` Ryan Mallon 2010-12-21 2:32 ` Ryan Mallon 2010-12-21 10:00 ` Alexey Charkov 2010-12-21 10:00 ` Alexey Charkov 2010-12-21 12:05 ` Arnd Bergmann 2010-12-21 12:05 ` Arnd Bergmann 2010-12-21 19:39 ` Ryan Mallon 2010-12-21 19:39 ` Ryan Mallon 2010-12-22 21:18 ` [PATCH 1/6 v10] " Alexey Charkov 2010-12-22 21:18 ` Alexey Charkov 2010-12-22 21:52 ` Ryan Mallon 2010-12-22 21:52 ` Ryan Mallon 2010-12-22 22:02 ` Alexey Charkov 2010-12-22 22:02 ` Alexey Charkov 2010-12-22 22:32 ` Ryan Mallon 2010-12-22 22:32 ` Ryan Mallon 2010-12-22 22:25 ` [PATCH 1/6 v11] " Alexey Charkov 2010-12-22 22:25 ` Alexey Charkov 2010-12-22 22:59 ` Ryan Mallon 2010-12-22 22:59 ` Ryan Mallon 2010-12-22 23:38 ` [PATCH 1/6 v12] " Alexey Charkov 2010-12-22 23:38 ` Alexey Charkov 2010-12-28 14:52 ` Alexey Charkov 2010-12-28 14:52 ` Alexey Charkov 2011-01-15 0:53 ` Alexey Charkov 2011-01-15 0:53 ` Alexey Charkov 2011-01-15 0:58 ` Russell King - ARM Linux 2011-01-15 0:58 ` Russell King - ARM Linux 2011-01-15 1:13 ` Alexey Charkov 2011-01-15 1:13 ` Alexey Charkov 2011-01-21 10:43 ` Russell King - ARM Linux 2011-01-21 10:43 ` Russell King - ARM Linux 2011-07-06 12:34 ` [PATCH 1/6 v8] " Russell King - ARM Linux 2011-07-06 12:34 ` Russell King - ARM Linux 2011-07-07 7:13 ` Alexey Charkov 2011-07-07 7:13 ` Alexey Charkov 2011-07-07 7:54 ` Arnd Bergmann 2011-07-07 7:54 ` Arnd Bergmann 2011-07-07 7:59 ` Russell King - ARM Linux 2011-07-07 7:59 ` Russell King - ARM Linux 2011-07-07 8:05 ` Arnd Bergmann 2011-07-07 8:05 ` Arnd Bergmann 2010-11-07 17:00 ` [PATCH 1/6 v2] " Russell King - ARM Linux 2010-11-07 17:00 ` Russell King - ARM Linux 2010-11-07 17:16 ` Alexey Charkov 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.