[alsa-devel] [PATCH] Asoc Davinci Voicecodec: Added support based on copy_from_user instead of DMA

[Mark Brown]

On Wed, Jul 14, 2010 at 04:28:10PM +0200, Raffaele Recalcati wrote:
> From: Davide Bonfanti <davide.bonfanti at bticino.it>

>     Since DM365 has the same DMA event for McBSP and Voicecodec this two
>     peripherals cannot be used at the same time.

Please try to format your patches as documented in SubmittingPatches -
you don't want all this indentation you're introducing in the start of
the changelog.  There's also other stuff with regard to coding style and
so on that it'd be useful for you to look at.

>     This driver implements Voicecodec without the use of a DMA but with
>     a copy_from_user.

Why is this specific to the voice CODEC?  Shouldn't this be generally
usable, and wouldn't it be better if the DMA driver were able to do some
automatic fallback here so that in cases where DMA can be used it will
be?

I had thought from the discussion on original submission that the two
devices were mutually exclusive for other reasons anyway.

> +/* Timer register offsets */
> +#define PID12                   0x00
> +#define TIM12                   0x10
> +#define TIM34                   0x14
> +#define PRD12                   0x18
> +#define PRD34                   0x1c
> +#define TCR                     0x20
> +#define TGCR                    0x24

This timer stuff all looks rather like it should be in whatever other
code manages the timers - as it stands it'll get into a fight with
anything else trying to use them.  I'd expect something like this to use
hrtimers to get high resolution time rather than banging the timer
hardware directly.

> +int pointer_sub;
> +u16 local_buffer[BUF_SIZE/2];

Should BUF_SIZE not be a configuration option, or dynamically configured
at runtime?

> +/*
> + *	ppcm = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
> + *		&pcm_hardware_playback : &pcm_hardware_capture;
> + */

Remove this if it's not used.

> +	__raw_writel(0x0, IO_ADDRESS(0x01D0C008));
> +	__raw_writel(0x7400, IO_ADDRESS(0x01D0C004));

This could do with being substantially clearer...  There's quite a few
other magic numbers in the code which need fixing.

> +static int davinci_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct clk *clk;
> +
> +	clk = clk_get(NULL, TIMER);
> +	if (!IS_ERR(clk))
> +		clk_disable(clk);

As with your previous patch you're going to be leaking clocks all over -
you should be balancing your clk_get() with clk_put().

> +struct snd_soc_platform davinci_soc_platform_copy = {
> +	.name =		"davinci-audio-copy",
> +	.pcm_ops =	&davinci_pcm_ops,
> +	.pcm_new =	davinci_pcm_new,
> +}; EXPORT_SYMBOL_GPL(davinci_soc_platform_copy);

Fix your formatting here.

> +++ b/sound/soc/soc-core.c
> @@ -801,7 +801,7 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>  	}
>  	return 0;
>  }
> -
> +#if 0
>  /* ASoC PCM operations */
>  static struct snd_pcm_ops soc_pcm_ops = {
>  	.open		= soc_pcm_open,
> @@ -811,7 +811,7 @@ static struct snd_pcm_ops soc_pcm_ops = {
>  	.prepare	= soc_pcm_prepare,
>  	.trigger	= soc_pcm_trigger,
>  };
> -
> +#endif

Um....?  I'm not entirely sure what this and the rest of the changes in
the file are supposed to do but they weren't mentioned at all in the
changelog.  If this is needed it should be a separate change with a
proper changelog explaining what's going on.