[alsa-devel] [PATCH 3/4] ASoC: Blackfin: add multi-channel function support

[Mark Brown]

On Thu, Oct 23, 2008 at 05:01:59PM +0800, Bryan Wu wrote:

> This patch provides a option for users to enable multi-channel function support
> in Blackfin ASoC driver. Because Blackfin is without MMU, it is easy for us and
> the user to enable this function at compiling stage not dynamically on the fly.

> Signed-off-by: Cliff Cai <cliff.cai at analog.com>
> Signed-off-by: Bryan Wu <cooloney at kernel.org>

Acked-by: Mark Brown <broonie at opensource.wolfsonmicro.com>

A few comments below, though.

As a general comment it'd be really helpful if you could split your
changes into more patches than you tend to do - ideally, each patch
should have a single logical change in it which is fully described by
the commit message.  Sometimes it's not worth splitting stuff out if the
changes overlap but it does make review easier if you can.

For example, you've already split out the codec driver change but still
well as the advertised change this patch also renames the mmap() support
define and some coding style cleanups.

> @@ -90,7 +98,7 @@ config SND_BF5XX_HAVE_COLD_RESET
>  	depends on SND_BF5XX_AC97
>  	default y if BFIN548_EZKIT
>  	default n if !BFIN548_EZKIT
> -	
> +

Random whitespace changes.

> -		bf5xx_pcm_to_ac97(
> -			(struct ac97_frame *)sport->tx_dma_buf + sport->tx_pos,
> -			(__u32 *)runtime->dma_area + sport->tx_pos, count);
> +		bf5xx_pcm_to_ac97((struct ac97_frame *)sport->tx_dma_buf +
> +		sport->tx_pos, (__u16 *)runtime->dma_area + sport->tx_pos *
> +		runtime->channels, count, chan_mask);

I'm not sure that the reindentation here is helpful but then the line is
so long that it's hard to do anything.  When revisiting this I'd suggest
using temporary variables to calculate the pointers passed in - that
should help a lot and I'd not expect worse code from the compiler since
the value has to be calculated and stored for passing to the function
anyway.

> +#if defined(CONFIG_SND_BF5XX_MULTICHAN_SUPPORT)

Normally just ifdef.