From: Alexey Charkov <alchark@gmail.com> To: Paul Mundt <lethal@linux-sh.org> 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 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Date: Mon, 8 Nov 2010 12:56:42 +0000 [thread overview] Message-ID: <AANLkTimkrK9T7G3=K_Ob1SuK64DrGJD3WNoangPp_ryW@mail.gmail.com> (raw) In-Reply-To: <20101108041721.GA11605@linux-sh.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: alchark@gmail.com (Alexey Charkov) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Date: Mon, 8 Nov 2010 12:56:42 +0000 [thread overview] Message-ID: <AANLkTimkrK9T7G3=K_Ob1SuK64DrGJD3WNoangPp_ryW@mail.gmail.com> (raw) In-Reply-To: <20101108041721.GA11605@linux-sh.org> 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
next prev parent reply other threads:[~2010-11-08 12:56 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 [this message] 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 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='AANLkTimkrK9T7G3=K_Ob1SuK64DrGJD3WNoangPp_ryW@mail.gmail.com' \ --to=alchark@gmail.com \ --cc=FlorianSchandinat@gmx.de \ --cc=akpm@linux-foundation.org \ --cc=davem@davemloft.net \ --cc=g.liakhovetski@gmx.de \ --cc=lethal@linux-sh.org \ --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.