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

[Andrew M. <ak...@li...>]

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.