Re: [patch] Add w100 framebuffer driver

[Kronos <kronos@people.it>]

Il Mon, Jan 03, 2005 at 07:47:33PM +0000, Richard Purdie ha scritto: 
> Add a framebuffer driver for the ATI w100 as found on several Sharp PDAs

Some minor comments:

> --- /dev/null
> +++ linux-2.6.10/drivers/video/w100fb.c
> @@ -0,0 +1,1848 @@
> +/*
> + * linux/drivers/video/w100fb.c
> + *
> + * Frame Buffer Device for ATI Imageon w100 (Wallaby)
> + *
> + * Copyright (C) 2002, ATI Corp.
> + * Copyright (C) 2004-2005 Richard Purdie
> + *
> + * Rewritten for 2.6 by Richard Purdie <rpurdie@rpsys.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <asm/io.h>
> +#include <asm/uaccess.h>
> +#include <linux/delay.h> 
> +#include <linux/fb.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/device.h>
> +#include <linux/string.h>
> +#include <linux/proc_fs.h>
> +#include <video/w100fb.h>
> +#include "w100fb.h"

Usualy asm headers are included after linux ones.

> +/* General frame buffer data structures */
> +static struct fb_info w100info;

fb layers is (slowly) migrating to dinamically allocated fb_info in
order to integrate with the driver model. Please use framebuffer_alloc
and framebuffer_release.

> +static int w100fb_blank(int blank_mode, struct fb_info *info)
> +{
> +	switch(blank_mode) {
> +
> + 	case FB_BLANK_NORMAL: /* Normal blanking */
> +	case FB_BLANK_VSYNC_SUSPEND: /* VESA blank (vsync off) */
> +	case FB_BLANK_HSYNC_SUSPEND: /* VESA blank (hsync off) */
> + 	case FB_BLANK_POWERDOWN: /* Poweroff */  
> +  		if (current_par.blanking_flag == 0) {
> +    		w100fb_save_buffer();
> +    		lcdtg_suspend();
> +    		current_par.blanking_flag = 1;
> +  		}
> +  		break;

Ops, block indentation has gone fubar.

> + 	
> + 	case FB_BLANK_UNBLANK: /* Unblanking */
> +  		if (current_par.blanking_flag != 0) {
> +    		w100fb_restore_buffer();
> +    		lcdtg_resume();
> +    		current_par.blanking_flag = 0;    		
> +  		}
> +  		break;

Ditto.

> +static int w100fb_check_var(struct fb_var_screeninfo *var, struct fb_info 
> *info)
> +{
> +	if (var->xres < var->yres) { /* Portrait mode */
> +		if ((var->xres > 480) || (var->yres > 640)) {
> +			return -EINVAL;
> +		} else if ((var->xres > 240) || (var->yres > 320)) {
> +			var->xres = 480;
> +			var->yres = 640;
> +		} else {
> +			var->xres = 240;
> +			var->yres = 320;
> +		}	
> +	} else { /* Landscape mode */
> +		if ((var->xres > 640) || (var->yres > 480)) {
> +			return -EINVAL;
> +		} else if ((var->xres > 320) || (var->yres > 240)) {
> +			var->xres = 640;
> +			var->yres = 480;
> +		} else {
> +			var->xres = 320;
> +			var->yres = 240;
> +		}	
> +	} 
> +
> +	var->xres_virtual = max(var->xres_virtual, var->xres);
> +	var->yres_virtual = max(var->yres_virtual, var->yres);
> +
> +	if (var->bits_per_pixel > BITS_PER_PIXEL) return -EINVAL;
> +	else var->bits_per_pixel = BITS_PER_PIXEL;

Please use something like this:

if (cond)
        exp1;
else
        exp2;

it's much easier to read. I've noticed more of this below...

> +static int w100fb_set_par(struct fb_info *info)
> +{
> +	current_par.xres = info->var.xres;
> +	current_par.yres = info->var.yres;
> +
> +	info->fix.visual = FB_VISUAL_TRUECOLOR;
> +
> +	info->fix.ypanstep = 0;
> +	info->fix.ywrapstep = 0;
> +	
> +	if (current_par.blanking_flag) w100fb_clear_buffer();

;)

> +static void w100fb_save_buffer(void)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < 640; i++) {
> +		if (gSaveImagePtr[i] != NULL) {
> +			kfree(gSaveImagePtr[i]);
> +			gSaveImagePtr[i] = NULL;
> +		}
> +		if (i < current_par.yres) {
> +			gSaveImagePtr[i] = kmalloc(current_par.xres * BITS_PER_PIXEL / 8, 
> GFP_KERNEL);

Hmmm, you may end up allocating up to 640 blocks of 960 bytes each (or
480 blocks, 1280 bytes each). It may be better to vmalloc a big chunk
and use that. Not sure about this though...

> +			if (gSaveImagePtr[i] != NULL) {
> +				for (j = 0; j < (current_par.xres); j++)
> +					*(gSaveImagePtr[i] + j) = readw(remapped_fbuf + (2*j)+ 
> (i*current_par.xres*2));
> +			} else {
> +				printk("can't alloc pre-off image buffer %d\n", i);

Missing prefix, maybe KERN_WARN?

> +int __init w100fb_probe(struct device *dev)
> +{
> +	struct w100fb_mach_info *inf;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct resource *mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	
> +	if (!mem) return -EINVAL;

Indentation.

> +
> +	/* remap the areas we're going to use */
> +	remapped_base = ioremap_nocache(mem->start+W100_CFG_BASE, W100_CFG_LEN);
> +	if (remapped_base == NULL) return -EIO;

Ditto.

> +static int w100fb_remove(struct device *dev)
> +{
> +     device_remove_file(dev, &dev_attr_fastsysclk);
> +     device_remove_file(dev, &dev_attr_reg_read);
> +     device_remove_file(dev, &dev_attr_reg_write);
> +     device_remove_file(dev, &dev_attr_rotation);
> +
> +     kfree(w100info.pseudo_palette);

Clean up should be done _after_ unregister_framebuffer.

> +     w100fb_clear_buffer();
> +
> +     unregister_framebuffer(&w100info);
> +
> +     iounmap(remapped_base);
> +     iounmap(remapped_regs);
> +     iounmap(remapped_fbuf);
> +
> +     return 0;
> +}


> +static power_state_t gPowerState;

mixedCaseVarsAndFuncAreEvil ;)

> +static uint8_t w100_GetPLLTestCount(uint8_t testclk_sel)
> +{
> +	udelay(5);
> +
> +	gPowerState.clk_test_cntl.f.start_check_freq = 0x0;
> +	gPowerState.clk_test_cntl.f.testclk_sel = testclk_sel;
> +	gPowerState.clk_test_cntl.f.tstcount_rst = 0x1;	/*reset test count */
> +	writel((uint32_t) (gPowerState.clk_test_cntl.val), remapped_regs + 
> mmCLK_TEST_CNTL);
> +	gPowerState.clk_test_cntl.f.tstcount_rst = 0x0;
> +	writel((uint32_t) (gPowerState.clk_test_cntl.val), remapped_regs + 
> mmCLK_TEST_CNTL);
> +
> +	gPowerState.clk_test_cntl.f.start_check_freq = 0x1;
> +	writel((uint32_t) (gPowerState.clk_test_cntl.val), remapped_regs + 
> mmCLK_TEST_CNTL);
> +
> +	udelay(20);	
> +
> +	gPowerState.clk_test_cntl.val = readl(remapped_regs + mmCLK_TEST_CNTL);
> +	gPowerState.clk_test_cntl.f.start_check_freq = 0x0;
> +	writel((uint32_t) (gPowerState.clk_test_cntl.val), remapped_regs + 
> mmCLK_TEST_CNTL);
> +
> +	return(gPowerState.clk_test_cntl.f.test_count);

return is not a function.

> +/* data structure definitions */
> +
> +typedef struct _wrap_top_dir_t {
> +     unsigned long top_addr         : 23;
> +     unsigned long    				: 9;
> +     } wrap_top_dir_t;

Are you sure that the compiler does the right thing? Don't you need
__attribute__((packed)) for this and the following structures?


Luca
-- 
Home: http://kronoz.cjb.net
"Vorrei morire ucciso dagli agi. Vorrei che di me si dicesse: ``Com'è
morto?'' ``Gli è scoppiato il portafogli''" -- Marcello Marchesi


-------------------------------------------------------
The SF.Net email is sponsored by: Beat the post-holiday blues
Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek.
It's fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt