[alsa-devel] ALSA:ATMEL: common arch for audio via ssc(patch 2/3: adding directories)

[Mark Brown]

On Fri, Sep 26, 2008 at 11:24:07AM +0200, Sedji Gaouaou wrote:
> This patch contains:
> The Kconfig and Makefile, plus the interface files(atmel_ssc_dai and 
> atmel-pcm).

This all looks very good - I've got some comments below but they're all
fairly minor coding style issues.  The only ones that really need to be
addressed are the commented out code and the use of printk KERN_DEBUG
rather than pr_dbg().  However, your mailer appears to have word wrapped
the patch so it won't apply - you'll need to reconfigure your MUA to
avoid this.

You also need to include a Signed-off-by: line in your message when
submitting patches, it's required for code going into the Linux kernel.
Documentation/SubmittingPatches contains an explanation of what that's
for and the text you'd be agreeing to.

Please also include a more detailed changelog entry in your patch
explaining that you're unifying the existing Ateml .  
Documentation/SubmittingPatches has some guidance on this.

> +config SND_ATMEL_SOC_SSC
> +	tristate
> +	depends on SND_ATMEL_SOC
> +	help
> +	  Say Y or M if you want to add support for codecs the

...connected to an...

> +	  ATMEL SSC interface. You will also needs to select the individual
> +	  machine drivers to support below.

> +	buf->dev.dev = pcm->card->dev;
> +	buf->private_data = NULL;
> +	buf->area = dma_alloc_writecombine(pcm->card->dev, size,
> +					   &buf->addr, GFP_KERNEL);
> +	/*buf->area = dma_alloc_coherent(pcm->card->dev, size,
> +					  &buf->addr, GFP_KERNEL);*/

Please remove commented code like this.

> +	printk(KERN_DEBUG "atmel-pcm:"
> +		"preallocate_dma_buffer: area=%p, addr=%p, size=%d\n",
> +		(void *) buf->area,
> +		(void *) buf->addr,
> +		size);

It's probably better to use pr_dbg() for this and most of the other
KERN_DEBUG prints - I'd expect the current code would be too verbose
for normal operation.

> +static int atmel_pcm_mmap(struct snd_pcm_substream *substream,
> +	struct vm_area_struct *vma)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	return dma_mmap_writecombine(substream->pcm->card->dev, vma,
> +				     runtime->dma_area,
> +				     runtime->dma_addr,
> +				     runtime->dma_bytes);
> +	/*return remap_pfn_range(vma, vma->vm_start,
> +		       substream->dma_buffer.addr >> PAGE_SHIFT,
> +		       vma->vm_end - vma->vm_start, vma->vm_page_prot);*/
> +}

Again, please remove the old, commented code.  There's some other
instances of this in the patch.

> +	/*
> +	 * Currently, there is only one set of dma params for
> +	 * each direction.  If more are added, this code will
> +	 * have to be changed to select the proper set.
> +	 */
> +	dir = ((substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
> +		0 : 1);

I'd really prefer

	if (substream->stream == SND_PCM_STREAM_PLAYBACK)
		dir = 1;
	else
		dir = 0;

for clarity.

> +		 *  For single channel data, one sample is transferred
> +		 * on the falling edge of the LRC clock.
> +		 * For two channel data, one sample is
> +		 * transferred on both edges of the LRC clock.
> +		 */
> +		start_event = ((channels == 1)
> +				? SSC_START_FALLING_RF
> +				: SSC_START_EDGE_RF);

Please expand this to

	if (channels == 1)
		start_event = SSC_START_FALLING_RF;
	else
		start_event = SSC_START_EDGE_RF;

> +#define ATMEL_SSC_RATES (SNDRV_PCM_RATE_8000  | SNDRV_PCM_RATE_11025 |\
> +			SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |\
> +			SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
> +			SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
> +			SNDRV_PCM_RATE_96000)

Might be clearer to say SNDRV_PCM_RATE_8000_96000 unless there are some
rates which that includes that are missing in there but it's not
important either way.