Re: [PATCH]: frame buffer driver for 2700G LCD controller

[Bernhard Fischer <yght@users.sourceforge.net>]

Could some kind soul do a recursive s/timming/timing/g in
drivers/video, please? TIA.

A few cosmetic remarks regarding the mbx driver below..

On Tue, Jul 11, 2006 at 02:10:53PM +0200, Mike Rapoport wrote:
>--- a/drivers/video/Kconfig
>+++ b/drivers/video/Kconfig
>@@ -1518,6 +1518,16 @@ config FB_PXA_PARAMETERS
> 
> 	  <file:Documentation/fb/pxafb.txt> describes the available parameters.
> 
>+config FB_MBX
>+	tristate "2700G LCD framebuffer support"
>+	depends on FB && ARCH_PXA
>+	select FB_CFB_FILLRECT
>+	select FB_CFB_COPYAREA
>+	select FB_CFB_IMAGEBLIT
>+	---help---
>+

A tad more description would be nice:
	  Intel 2700G (Marathon) Graphics Accelerator Frame Buffer
	  Driver

>+	  If unsure, say N.
>+
> config FB_W100
> 	tristate "W100 frame buffer support"
> 	depends on FB && PXA_SHARPSL

>diff --git a/drivers/video/mbx/mbxfb.c b/drivers/video/mbx/mbxfb.c
>new file mode 100644
>index 0000000..c41a678
>--- /dev/null
>+++ b/drivers/video/mbx/mbxfb.c

>+#define MAX_PALETTES	16
>+
>+/* FIXME: take care of different chip reivsions with different sizes

s/reivsions/revisions/

>+#ifdef CONFIG_PM
>+/*
>+ * Power management hooks.  Note that we won't be called from IRQ context,
>+ * unlike the blank functions above, so we may sleep.
>+ */
>+static int mbxfb_suspend(struct platform_device *dev, pm_message_t state)
>+{
>+	/* make frame buffer memory enter self-refresh mode */
>+	writel(LMPWR_MC_PWR_SRM, LMPWR);
>+	while (LMPWRSTAT != LMPWRSTAT_MC_PWR_SRM)
>+		; /* empty statement */
>+
>+	/* reset the device, since it's initial state is 'mostly sleeping' */
>+	writel(SYSRST_RST, SYSRST);
>+	return 0;
>+}
>+
>+static int mbxfb_resume(struct platform_device *dev)
>+{
>+	struct fb_info *fbi = (struct fb_info *)platform_get_drvdata(dev);
>+
>+	enable_clocks(fbi);
>+/* 	setup_graphics(fbi); */
>+/* 	setup_display(fbi); */
>+
>+	writel((readl(DSCTRL) | DSCTRL_SYNCGEN_EN), DSCTRL);
>+	return 0;
>+}

IIRC it was already stated that it is better to only set the .suspend
and .resume ops depending on CONFIG_PM (see below instead of the #if
#else #endif stanza here).
>+#else
>+#define mbxfb_suspend	NULL
>+#define mbxfb_resume	NULL
>+#endif

>+static struct platform_driver mbxfb_driver = {
>+	.probe = mbxfb_probe,
>+	.remove = mbxfb_remove,

#ifdef CONFIG_PM

>+	.suspend = mbxfb_suspend,
>+	.resume = mbxfb_resume,

#endif /* CONFIG_PM */

>+	.driver = {
>+		.name = "mbx-fb",
>+	},
>+};

cheers,


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642