pxa168fb: Add .remove function

While fb isn't active, we should clear CFG_GRA_ENA bit. The existing code can't clear this bit.

Signed-off-by Haojian Zhuang <[email protected]>

---
 drivers/video/pxa168fb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxa168fb.c +0 -0

index ec2ddb2..3f112c9 100644

While fb isn't active, we should clear CFG_GRA_ENA bit. The existing code can't clear this bit.

Signed-off-by Haojian Zhuang <[email protected]>

---
?drivers/video/pxa168fb.c | ? ?4 ++--
?1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxa168fb.c +0 -0

index ec2ddb2..3f112c9 100644
@@ -298,8 +298,8 @@ static void set_dma_control0(struct pxa168fb_info *fbi)

? ? ? ? * Set bit to enable graphics DMA.
? ? ? ? */
? ? ? ?x = readl(fbi->reg_base + LCD_SPU_DMA_CTRL0);
- ? ? ? x |= fbi->active ? 0x00000100 : 0;
- ? ? ? fbi->active = 0;
+ ? ? ? x &= ~CFG_GRA_ENA_MASK;
+ ? ? ? x |= CFG_GRA_ENA(!!fbi->active);

? ? ? ?/*
? ? ? ? * If we are in a pseudo-color mode, we need to enable --
1.5.6.5

Quoted 13 lines · Haojian Zhuang

While fb isn't active, we should clear CFG_GRA_ENA bit. The existing code can't clear this bit.

Signed-off-by Haojian Zhuang <[email protected]>

---
?drivers/video/pxa168fb.c | ? ?4 ++--
?1 files changed, 2 insertions(+), 2 deletions(-)

drivers/video/pxa168fb.c +0 -0

index ec2ddb2..3f112c9 100644
@@ -298,8 +298,8 @@ static void set_dma_control0(struct pxa168fb_info *fbi)

Quoted 7 lines

? ? ? ? * Set bit to enable graphics DMA.
? ? ? ? */
? ? ? ?x = readl(fbi->reg_base + LCD_SPU_DMA_CTRL0);
- ? ? ? x |= fbi->active ? 0x00000100 : 0;
- ? ? ? fbi->active = 0;
+ ? ? ? x &= ~CFG_GRA_ENA_MASK;
+ ? ? ? x |= CFG_GRA_ENA(!!fbi->active);

This isn't very readable. And since CFG_GRA_ENA is actually a 1-bit field, I'd recommend to change it to something like below:

drivers/video/pxa168fb.c +0 -0

index 5d786bd..59fa751 100644
@@ -298,8 +298,11 @@ static void set_dma_control0(struct pxa168fb_info *fbi)

? ? ? ? * Set bit to enable graphics DMA.
? ? ? ? */
? ? ? ?x = readl(fbi->reg_base + LCD_SPU_DMA_CTRL0);
- ? ? ? x |= fbi->active ? 0x00000100 : 0;
- ? ? ? fbi->active = 0;
+
+ ? ? ? if (fbi->active)
+ ? ? ? ? ? ? ? x &= ~CFG_GRA_ENA;
+ ? ? ? else
+ ? ? ? ? ? ? ? x |= CFG_GRA_ENA;

? ? ? ?/*
? ? ? ? * If we are in a pseudo-color mode, we need to enable

drivers/video/pxa168fb.h +0 -0

index eee0927..e5c34a5 100644
@@ -240,8 +240,7 @@

?#define ? ? CFG_GRA_SWAPYU_MASK ? ? ? ? ? ? ? ? ? ? ? ?0x00000400 ?#define ? ? CFG_YUV2RGB_GRA(cvrt) ? ? ? ? ? ? ?((cvrt) << 9) ?#define ? ? CFG_YUV2RGB_GRA_MASK ? ? ? ? ? ? ? 0x00000200

drivers/video/pxa168fb.h +1 -2

-#define ? ? CFG_GRA_ENA(gra) ? ? ? ? ? ? ? ? ? ((gra) << 8)
-#define ? ? CFG_GRA_ENA_MASK ? ? ? ? ? ? ? ? ? 0x00000100
+#define ? ? CFG_GRA_ENA ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x10000100

?/* for video part */
?#define ? ? CFG_DMA_FTOGGLE(toggle) ? ? ? ? ? ?((toggle) << 7) ?#define ? ? CFG_DMA_FTOGGLE_MASK ? ? ? ? ? ? ? 0x00000080

Quoted 7 lines

? ? ? ?/*
? ? ? ? * If we are in a pseudo-color mode, we need to enable --
1.5.6.5

I'm OK. Should I submit this patch again?

The pxa168fb driver is missing .remove function so the framebuffer isn't correctly shut down when the module is removed.

Signed-off-by Marek Vasut <[email protected]>

Signed-off-by Haojian Zhuang <[email protected]>

---
 drivers/video/pxa168fb.c |   47

++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-)

drivers/video/pxa168fb.c +9 -0

index c91a7f7..ec2ddb2 100644
@@ -784,12 +784,53 @@ failed:
 	return ret;
 }
 
+static int __devexit pxa168fb_remove(struct platform_device *pdev)
+{
+	struct pxa168fb_info *fbi = platform_get_drvdata(pdev);
+	struct fb_info *info;
+	int irq;
+	unsigned int data;
+
+	if (!fbi)
+		return 0;

drivers/video/pxa168fb.c +28 -0

+	writel(GRA_FRAME_IRQ0_ENA(0x0), fbi->reg_base + SPU_IRQ_ENA);
+
+	if (info->cmap.len)
+		fb_dealloc_cmap(&info->cmap);
+
+	irq = platform_get_irq(pdev, 0);
+	free_irq(irq, fbi);
+
+	dma_free_writecombine(fbi->dev, PAGE_ALIGN(info->fix.smem_len),
+				info->screen_base, info->fix.smem_start);
+
+	iounmap(fbi->reg_base);
+
+	clk_disable(fbi->clk);
+	clk_put(fbi->clk);
+
+	framebuffer_release(info);
+
+	return 0;
+}
+
 static struct platform_driver pxa168fb_driver = {
 	.driver		= {
 		.name	= "pxa168-fb",
 		.owner	= THIS_MODULE,
 	},
 	.probe		= pxa168fb_probe,
+	.remove		= __devexit_p(pxa168fb_remove),
 };
 
 static int __devinit pxa168fb_init(void)

@@ -798,6 +839,12 @@ static int __devinit pxa168fb_init(void)
 }
 module_init(pxa168fb_init);
 
+static void __exit pxa168fb_exit(void)
+{
+	platform_driver_unregister(&pxa168fb_driver);
+}
+module_exit(pxa168fb_exit);
+
 MODULE_AUTHOR("Lennert Buytenhek <[email protected]> "
 	      "Green Wan <[email protected]>");
 MODULE_DESCRIPTION("Framebuffer driver for PXA168/910");

Dne Po 30. srpna 2010 02:32:16 Haojian Zhuang napsal(a):

Quoted 27 lines

The pxa168fb driver is missing .remove function so the framebuffer isn't correctly shut down when the module is removed.

Signed-off-by Marek Vasut <[email protected]>

Signed-off-by Haojian Zhuang <[email protected]>

---
?drivers/video/pxa168fb.c | ? 47 ++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 47 insertions(+), 0 deletions(-)

drivers/video/pxa168fb.c +9 -0

index c91a7f7..ec2ddb2 100644
@@ -784,12 +784,53 @@ failed:
 ? ? ? return ret;
 ?}
 
+static int __devexit pxa168fb_remove(struct platform_device *pdev)
+{
+ ? ? struct pxa168fb_info *fbi = platform_get_drvdata(pdev);
+ ? ? struct fb_info *info;
+ ? ? int irq;
+ ? ? unsigned int data;
+
+ ? ? if (!fbi)
+ ? ? ? ? ? ? return 0;

drivers/video/pxa168fb.c failed: · line 805

Quoted 10 lines

drivers/video/pxa168fb.c +10 -0

+
+ ? ? /* disable DMA transfer */
+ ? ? data = readl(fbi->reg_base + LCD_SPU_DMA_CTRL0);
+ ? ? data &= ~CFG_GRA_ENA_MASK;
+ ? ? writel(data, fbi->reg_base + LCD_SPU_DMA_CTRL0);
+
+ ? ? info = fbi->info;
+
+ ? ? unregister_framebuffer(info);
+

Haojian, is this line below correct?

If so, for this modified version, find my:

Acked-by Marek Vasut <[email protected]>