Re: [Linux-fbdev-devel] [GEODE] Add framebuffer support for the AMD Geode LX

[Ondrej Zajicek <santiago@crfreenet.org>]

On Mon, Jul 09, 2007 at 11:32:11AM -0600, Jordan Crouse wrote:
> +const struct fb_videomode geode_modedb[] __initdata = {
> +	/* 640x480-60 */
> +	{ NULL, 60, 640, 480, 39682, 48, 8, 25, 2, 88, 2,
> +	  FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
> +	  FB_VMODE_NONINTERLACED, 0 },
> +	/* 640x400-70 */
> +	{ NULL, 70, 640, 400, 39770, 40, 8, 28, 5, 96, 2,
> +	  FB_SYNC_HOR_HIGH_ACT,
> +	  FB_VMODE_NONINTERLACED, 0 },
> +	/* 640x480-70 */

This looks like standard timings, is there any reason why not
use modes from modedb.c?

> +static int lxfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
> +{
> +	if (var->xres > 1920 || var->yres > 1440)
> +		return -EINVAL;
> +
> +	if (var->bits_per_pixel == 32) {
> +		var->red.offset   = 16; var->red.length   = 8;
> +		var->green.offset =  8; var->green.length = 8;
> +		var->blue.offset  =  0; var->blue.length  = 8;
> +	} else if (var->bits_per_pixel == 16) {
> +		var->red.offset   = 11; var->red.length   = 5;
> +		var->green.offset =  5; var->green.length = 6;
> +		var->blue.offset  =  0; var->blue.length  = 5;
> +	} else if (var->bits_per_pixel == 8) {
> +		var->red.offset   = 0; var->red.length   = 8;
> +		var->green.offset = 0; var->green.length = 8;
> +		var->blue.offset  = 0; var->blue.length  = 8;
> +	} else
> +		return -EINVAL;

Clear var->{color}.msb_right ?

If don't support panning, it should ensure that
xres_virtual == xres and yres_virtual == yres

> +static int __init lxfb_map_video_memory(struct fb_info *info,
> +					struct pci_dev *dev)
> +{
> +	struct lxfb_par *par = info->par;
> +	int ret;
> +
> +	ret = pci_enable_device(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_request_region(dev, 0, "lxfb-framebuffer");
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_request_region(dev, 1, "lxfb-gp");
> +
> +	if (ret)
> +		return ret;
> +
...

Free allocated regions in a case of a error?

> +
> +	ret = -ENOMEM;
> +
> +	if (info->screen_base == NULL)
> +		return ret;
> +
> +	par->gp_regs = ioremap(pci_resource_start(dev, 1),
> +				pci_resource_len(dev, 1));

pci_iomap(dev, 1, 0) ?

> +static struct fb_info * __init lxfb_init_fbinfo(struct device *dev)
> +{
> +	struct lxfb_par *par;
> +	struct fb_info *info;
> +
> +	/* Alloc enough space for the pseudo palette. */
> +	info = framebuffer_alloc(sizeof(struct lxfb_par) + sizeof(u32) * 16,
> +				 dev);

What about adding u32 pseudo_palette[16] at the end of struct lxfb_par
instead of that trick?

> +module_param(mode_option, charp, 0);
> +MODULE_PARM_DESC(mode_option, "video mode (<x>x<y>[-<bpp>][@<refr>])");
> +
> +module_param(fbsize, int, 0);
> +MODULE_PARM_DESC(fbsize, "video memory size");

What about adding module params for noclear, nopanel and nocrt ?


> +static void lx_set_clock(struct fb_info *info)
> +{
> +	unsigned int diff, min, best = 0;
> +	unsigned int freq, i;
> +
> +	freq = (unsigned int) (0x3b9aca00 / info->var.pixclock);

Here using 0x3b9aca00 instead of 1000000000 is obscuring.

-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org, jabber: santiago@njs.netlab.cz)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."

Attachment: signature.asc
Description: Digital signature