Re: [Linux-fbdev-devel] [PATCH] atmel_lcdfb: AT91/AT32 LCDController framebuffer driver

[Nicolas Ferre <nicolas.ferre@rfo.atmel.com>]

Antonino A. Daplas :
> On Mon, 2007-05-07 at 16:11 +0200, Nicolas Ferre wrote:
> > From: Nicolas Ferre <nicolas.ferre@rfo.atmel.com>

> > +
> > +static struct fb_fix_screeninfo atmel_lcdfb_fix __initdata = {
> > +	.type		= FB_TYPE_PACKED_PIXELS,
> > +	.visual		= FB_VISUAL_TRUECOLOR,
> > +	.xpanstep	= 0,
> > +	.ypanstep	= 0,
> > +	.ywrapstep	= 0,
> > +	.accel		= FB_ACCEL_NONE,
> > +};
> > +
>
> This driver has fb_pan_display() which I assume works. However, you also
> need to set ypanstep and/or xpanstep and/or ywrapstep to a nonzero
> value, depending on the hardware/drive capability.

True, this function is here because one of our products can do pan display.
The technique is not used for the moment but will be in a future version
of this driver.

> > +static u32 pseudo_palette[16] = {
> > +	0x000000,
> > +	0xaa0000,
> > +	0x00aa00,
> > +	0xaa5500,
> > +	0x0000aa,
> > +	0xaa00aa,
> > +	0x00aaaa,
> > +	0xaaaaaa,
> > +	0x555555,
> > +	0xff5555,
> > +	0x55ff55,
> > +	0xffff55,
> > +	0x5555ff,
> > +	0xff55ff,
> > +	0x55ffff,
> > +	0xffffff
> > +};
> > +
>
> Do you really need to pre-fill pseudo_palette[]? The contents are going
> to be overwritten by the console anyway (The pseudo_palette is for
> fbcon's use only).

Ok, I have tested it with fbcon and no pre-fill : colors seem ok.

> You can also include pseudo_palette[] as part of struct
> atmel_lcdfb_info, then in your probe routine:
>
> info->pseudo_palette = par->pseudo_palette;
>
> to reduce the size of the kernel image. 

Done.

> > +static inline u_int chan_to_field(u_int chan, const struct fb_bitfield *bf)
>
> Might as well change u_int to u32 or unsigned int, for consistency.

Ok, changed to unsigned int.

> > +static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo)
> > +{
> > +	struct fb_info *info = sinfo->info;
> > +	int ret = 0;
> > +
> > +	memset(info->screen_base, 0, info->fix.smem_len);
>
> memset_io?

ok, modified.


> > +	if (dev->platform_data) {
> > +		pdata_sinfo = (struct atmel_lcdfb_info *)dev->platform_data;
> > +		sinfo->default_bpp = pdata_sinfo->default_bpp;
> > +		sinfo->default_dmacon = pdata_sinfo->default_dmacon;
> > +		sinfo->default_lcdcon2 = pdata_sinfo->default_lcdcon2;
> > +		sinfo->default_monspecs = pdata_sinfo->default_monspecs;
> > +		sinfo->atmel_lcdfb_power_control = pdata_sinfo->atmel_lcdfb_power_control;
> > +		sinfo->guard_time = pdata_sinfo->guard_time;
> > +	} else {
> > +		dev_err(dev, "cannot get default configuration\n");
> > +		goto out;
>
> Wrong goto? Should be goto free_info?

Right. modified to "goto free_info".

> > +release_intmem:
> > +	if (map) {
> > +		release_mem_region(info->fix.smem_start, info->fix.smem_len);
> > +	}
>
> Unnecessary curly braces

I like curly braces... Anyway, removed.

Thanks you for your comments.

I resend a modified driver now in the following email.

Regards,
--
Nicolas Ferre


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/