Re: [PATCH 2/3] ASoC Samsung S3C24xx audio DMA

[Takashi Iwai <tiwai@suse.de>]

At Fri, 02 Feb 2007 18:03:23 +0000,
Liam Girdwood wrote:
> 
> diff -r 0f9c7c3b7657 soc/s3c24xx/s3c24xx-pcm.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/soc/s3c24xx/s3c24xx-pcm.h	Fri Feb 02 17:50:25 2007 +0000
> @@ -0,0 +1,25 @@
> +/*
> + * linux/sound/arm/s3c24xx-pcm.h -- ALSA PCM interface for the Samsung S3C24xx CPU
> + */
> +
> +#ifndef _S3C24XX_PCM_H
> +#define _S3C24XX_PCM_H
> +
> +#define ST_RUNNING		(1<<0)
> +#define ST_OPENED		(1<<1)
> +
> +typedef struct {
> +	struct s3c2410_dma_client *client;			/* stream identifier */
> +	int channel;						/* Channel ID */
> +	dma_addr_t dma_addr;
> +} s3c24xx_pcm_dma_params_t;

Avoid typedefs as much as possible.

> +static int s3c24xx_pcm_hw_params(struct snd_pcm_substream *substream,
> +	struct snd_pcm_hw_params *params)
> +{
(snip)
> +	spin_lock_irqsave(&prtd->lock, flags);

The hw_params callback is always sleepable, so it's safe to use
spin_lock_irq() here.

> +static int s3c24xx_pcm_hw_free(struct snd_pcm_substream *substream)
> +{
> +	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
> +
> +	DBG("Entered %s\n", __FUNCTION__);
> +
> +	// TODO - do we need to ensure DMA flushed

Avoid C++ style comments.

> +static int s3c24xx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct s3c24xx_runtime_data *prtd = substream->runtime->private_data;
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	DBG("Entered %s\n", __FUNCTION__);
> +
> +	spin_lock_irqsave(&prtd->lock, flags);

The trigger and pointer callbacks are, on the contrary, always
atomic.  You can skip irqsave, just use spin_lock().

> +static int s3c24xx_pcm_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct s3c24xx_runtime_data *prtd;
> +
> +	int ret;
> +
> +	DBG("Entered %s\n", __FUNCTION__);
> +
> +	snd_soc_set_runtime_hwparams(substream, &s3c24xx_pcm_hardware);
> +
> +	if((prtd = kzalloc(sizeof(struct s3c24xx_runtime_data),
> +		GFP_KERNEL)) == NULL) {

Split to two lines.

> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	runtime->private_data = prtd;
> +	return 0;
> +
> +out:
> +	return ret;
> +}
> +
> +static int s3c24xx_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct s3c24xx_runtime_data *prtd = runtime->private_data;
> +
> +	DBG("Entered %s\n", __FUNCTION__);
> +
> +	if(prtd)
> +		kfree(prtd);
> +	else {
> +		DBG("s3c24xx_pcm_close called with prtd == NULL\n");
> +	}

You don't need braces.

> +struct snd_pcm_ops s3c24xx_pcm_ops = {

Missing static?

> +static u64 s3c24xx_pcm_dmamask = 0xffffffff;

Use DMA_*BIT_MASK instead.

> +int s3c24xx_pcm_new(struct snd_card *card, struct snd_soc_codec_dai *dai,
> +	struct snd_pcm *pcm)

Missing static?


Takashi

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642