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

["Antonino A. Daplas" <adaplas@gmail.com>]

Mike Rapoport wrote:
> This patch adds frame buffer driver for 2700G LCD controller present on
> CompuLab CM-X270 computer module.
> This pacth is versus current Linus git tree.
> 
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> 
> 

> +#include <linux/config.h>

config.h is not needed, remove that particular line.

> +
> +static struct fb_var_screeninfo mbxfb_default = {

__devinitdata or __initdata

> +
> +static struct fb_fix_screeninfo mbxfb_fix = {

__devinitdata or __initdata

> +	.id =		"MBX",
> +	.type =		FB_TYPE_PACKED_PIXELS,
> +	.visual =	FB_VISUAL_TRUECOLOR,
> +	.xpanstep =	0,
> +	.ypanstep =	0,
> +	.ywrapstep =	0,
> +	.accel =	FB_ACCEL_NONE,
> +};
> +
> +struct pixclock_div {
> +	u8 m;
> +	u8 n;
> +	u8 p;
> +};
> +
> +static unsigned int mbxfb_get_pixclock(unsigned int pixclock_ps, struct pixclock_div *div)

80-column limit

> +{
> +	u8  m, n, p;
> +	unsigned int err = 0;
> +	unsigned int min_err = ~0x0;
> +	unsigned int clk;
> +	unsigned int best_clk = 0;
> +	unsigned int ref_clk = 13000; /* FIXME: take from platform data */
> +	unsigned int pixclock;
> +
> +	/* convert pixclock to KHz */
> +	pixclock = PICOS2KHZ(pixclock_ps);
> +
> +	for ( m = 1; m < 64; m++ ) {

Coding style, no space after "(" and before ")".

> +		for ( n = 1; n < 8; n++ ) {
> +			for ( p = 0; p < 8; p++ ) {
> +				clk = (ref_clk * m) / (n * (1 << p));
> +				err = (clk > pixclock) ? (clk - pixclock) : 
> +					(pixclock - clk);
> +				if ( err < min_err ) {
> +					min_err = err;
> +					best_clk = clk;
> +					div->m = m;
> +					div->n = n;
> +					div->p = p;
> +				}
> +			}
> +		}
> +	}
> +	return KHZ2PICOS(best_clk);
> +}
> +
> +static int
> +mbxfb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
> +		   u_int trans, struct fb_info *info)

Coding style, "static int" and "mbxfb_setcolreg" in one line.

> +{
> +	uint val, ret = 1;

If you can, avoid u_int and family and just use u32 and family.

> +
> +	if ( regno < 255 ) {
> +		val = (red & 0xff) << 16;
> +		val |= (green & 0xff) << 8;
> +		val |= (blue & 0xff) << 0;
> +		GPLUT = Gplut_Lutadr(regno) | Gplut_Lutdata(val);
> +		udelay(1000);
> +		ret = 0;

If you want this to work as a console driver, make sure you also
write 'val' to info->pseudo_palette (for bpp 16 and above) and only
for regno's 0-15.

> +		switch ( info->var.bits_per_pixel ) {
> +			case 16:

Kernel coding style. the "case" statements must begin at the same column
as the "switch", ie:

		 switch (info->var.bits_per_pixel) {
		 case 16:
			if (info->var.green.length == 5)
				GSCTRL |= GSCTRL_GPIXFMT_ARGB1555;
			else
				GSCTRL |= GSCTRL_GPIXFMT_RGB565;
			break;

> +static void enable_clocks(struct fb_info* fbi)
> +{
> +	/* enable clocks */
> +	SYSCLKSRC = SYSCLKSRC_PLL_2; udelay(1000);

No multiple statements in one line.

> +static int mbxfb_suspend(struct platform_device *dev, pm_message_t state)
> +{
> +	/* make frame buffer memory enter self-refresh mode */
> +	LMPWR = LMPWR_MC_PWR_SRM;
> +	while ( LMPWRSTAT != LMPWRSTAT_MC_PWR_SRM );

Linus hates this style, do it like this:

	while (LMPWRSTAT != LMPWRSTAT_MC_PWR_SRM)
	      ; /* empty statement */


> +#include "mbxsysfs.c"
> +
> +#define res_size(_r) (((_r)->end - (_r)->start) + 1)

No arch-specific function/macro's to determine the resource length?

> +
> +static int mbxfb_probe(struct platform_device *dev)

__init or __devinit. And that includes other functions that are
called only during init.

> +static int mbxfb_remove(struct platform_device *dev)

__exit or __devexit.

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

Redundant #ifdef, suspend and resume are already defined as NULL if
CONFIG_PM is false.

> +	.suspend	= mbxfb_suspend,
> +	.resume		= mbxfb_resume,
> +#endif
> +	.driver		= {
> +		.name	= "mbx-fb",
> +	},
> +};
> +
> diff --git a/drivers/video/mbx/mbxsysfs.c b/drivers/video/mbx/mbxsysfs.c
> new file mode 100644
> index 0000000..4b9571a
> --- /dev/null
> +++ b/drivers/video/mbx/mbxsysfs.c
> @@ -0,0 +1,129 @@

Most of the attributes violate the "one file, one value" rule. GregKH won't
like it. Try using debugfs if you cannot separate them.

Tony

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