Re: [Linux-fbdev-devel] patch - bitblt 2D acceleration for s1d13xxxfb

[Kristoffer E. <kri...@gm...>]

Hi Andrew,

Ive sent the updated patch to you and list earlier. Is it okey to merge? 
I feel confident that it doesnt break anything and it is tested
to both compile and work. 

If needed I could run it by Russell and push it that way.

Best wishes 
Kristoffer Ericson

On Fri, 5 Jun 2009 13:59:16 -0700
Andrew Morton <ak...@li...> wrote:

> On Fri, 5 Jun 2009 19:38:53 +0200
> Kristoffer Ericson <kri...@gm...> wrote:
> 
> > Greetings,
> > 
> > Just wanted some feedback since this patch is quite large. Any comments are
> > appreciated. It patches, compiles and runs like a fox on fire.
> 
> we like firey foxes.
> 
> > This patch:
> > enables 2D rectfill and copyarea acceleration for 13506, need to look
> > at hardware sheets before I can say it works on other version but at minimal
> > 13806.
> > 
> > Also some cleans up the code.
> > 
> >
> > ...
> >
> > +/**
> > + * we make sure only one bitblt operation is running
> > + */
> > +static DEFINE_SPINLOCK(s1d13xxxfb_bitblt_lock);
> > +
> > +/**
> > + * list of card production ids
> >   */
> >  static const int s1d13xxxfb_prod_ids[] = {
> >  	S1D13505_PROD_ID,
> > @@ -59,7 +71,7 @@ static const int s1d13xxxfb_prod_ids[] = {
> >  	S1D13806_PROD_ID,
> >  };
> >  
> > -/*
> > +/**
> >   * List of card strings
> >   */
> >  static const char *s1d13xxxfb_prod_names[] = {
> > @@ -68,8 +80,8 @@ static const char *s1d13xxxfb_prod_names[] = {
> >  	"S1D13806",
> >  };
> >  
> > -/*
> > - * Here we define the default struct fb_fix_screeninfo
> > +/**
> > + * here we define the default struct fb_fix_screeninfo
> >   */
> 
> The /** token is reserved for introducing a kerneldoc-formatted
> comment, but none of the above three comments are kerneldoc comments.
> 
> > +/**
> > + *	bltbit_wait_bitset - waits for change in register value
> > + *	@info : framebuffer structure
> > + *	@bit  : value expected in register
> > + *	@timeout : ...
> > + *
> > + *	waits until value changes INTO bit
> > + */
> 
> That one is kerneldoc.
> 
> > +u8 bltbit_wait_bitset(struct fb_info *info, u8 bit, int timeout)
> 
> The patch adds lots of global functions.  Please make symbols static
> where possible.
> 
> > +{
> > +	while (!(s1d13xxxfb_readreg(info->par, S1DREG_BBLT_CTL0) & bit)) {
> > +		udelay(10);
> > +		if (!--timeout) {
> > +			dbg_blit("wait_bitset timeout\n");
> > +			break;
> > +		}
> > +	}
> > +
> > +	return timeout;
> > +}
> > +
> >
> > ...
> >
> 
> It otherwise looks OK to my inexpert eye.


-- 
Kristoffer Ericson <kri...@gm...>