diff mbox

[13/15] ASoC: SND_S3C_DMA_LEGACY needs S3C24XX_DMA

Message ID 1398770316-19715-14-git-send-email-kaixu.xia@linaro.org
State New
Headers show

Commit Message

Xia Kaixu April 29, 2014, 11:18 a.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

SND_S3C_DMA_LEGACY can only be set on S3C24xx, which does not
(yet) support the dmaengine framework, so samsung_dma_get_ops()
fails to link if S3C24XX_DMA is disabled:

sound/built-in.o: In function `dma_hw_params':
:(.text+0x7f310): undefined reference to `s3c_dma_get_ops'
sound/built-in.o: In function `s3c_ac97_trigger':
:(.text+0x7f7f0): undefined reference to `s3c_dma_get_ops'
sound/built-in.o: In function `s3c_ac97_mic_trigger':
:(.text+0x7f884): undefined reference to `s3c_dma_get_ops'
sound/built-in.o: In function `s3c2412_i2s_trigger':
:(.text+0x80944): undefined reference to `s3c2410_dma_ctrl'

This makes sure S3C24XX_DMA is always enabled when we need
it, just like we do it for the same dependency in SND_S3C24XX_I2S,
which has the same problem. Selecting "S3C2410_DMA" as we did
before does not actually have the intended effect, since
that one is only used on the s3c2410 and s3c2442 SoCs of the
s3c24xx family, but not the others, so we can remove this
from Kconfig.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Xia Kaixu <kaixu.xia@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Ben Dooks <ben-linux@fluff.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Sangbeom Kim <sbkim73@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: alsa-devel@alsa-project.org
---
 sound/soc/samsung/Kconfig |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Mark Brown May 1, 2014, 7:11 p.m. UTC | #1
On Tue, Apr 29, 2014 at 07:18:34PM +0800, Xia Kaixu wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> SND_S3C_DMA_LEGACY can only be set on S3C24xx, which does not
> (yet) support the dmaengine framework, so samsung_dma_get_ops()
> fails to link if S3C24XX_DMA is disabled:

Why is the fix for this not to ensure that s3c24xx always enables the
DMA controller - how likely is it that it would be sane to build a
kernel without DMA after all?
Heiko Stuebner May 2, 2014, 10:25 a.m. UTC | #2
Hi Arnd,

Am Freitag, 2. Mai 2014, 00:35:21 schrieb Arnd Bergmann:
> On Thursday 01 May 2014 12:11:25 Mark Brown wrote:
> > On Tue, Apr 29, 2014 at 07:18:34PM +0800, Xia Kaixu wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > > 
> > > SND_S3C_DMA_LEGACY can only be set on S3C24xx, which does not
> > > (yet) support the dmaengine framework, so samsung_dma_get_ops()
> > 
> > > fails to link if S3C24XX_DMA is disabled:
> > Why is the fix for this not to ensure that s3c24xx always enables the
> > DMA controller - how likely is it that it would be sane to build a
> > kernel without DMA after all?
> 
> S3C24XX_DMA is only needed for ASoC and for s3cmci. The latter
> uses 'depends on S3C24XX_DMA'. arch/arm/configs/tct_hammer_defconfig
> is an example of a configuration that does not include DMA because
> it uses neither of the two drivers.
> 
> On a related topic, I looked at the overall DMA situation for
> plat-samsung again now, and it looks much nicer now than it used to,
> with mach-s3c64xx fully migrated to dmaengine, so there might be an
> even better way to deal with this.
> 
> From all I can tell, sound/soc/samsung is the only remaining user
> of the plat/dma.h interfaces, and there is only one other  driver
> using the s3c24xx_dma interface directly, drivers/mmc/host/s3cmci.c.
> 
> How about the patch below?
> 8<-------
> ASoC: s3c24xx: use legacy DMA interface directly
> 
> The samsung platform code has a wrapper around the legacy s3c24xx
> DMA API as an alternative to the dmaengine API. This is only used
> by the legacy s3c24xx sound support, which is never used in combination
> with the dmaengine API.

Just to mention, we have a dmaengine driver for s3c24xx :-) . Correct 
platform-data is present for all s3c24xx socs (in mach-s3c24xx/common.c) .

Mark already removed support for the legacy API from the s3c64xx spi driver 
(used by s3c2416 and s3c2443), so I guess to way forward would be to "simply" 
convert asoc and s3cmci to dmaengine and get rid of it altogether.

I just never had hardware using the old mci driver or with any previously 
working sound and didn't trust my experience to be enough to be able to do 
such a conversion "on the fly" like you below in the opposite direction ;-)


Heiko

> 
> We can simplify the ASoC code significantly, and thereby completely
> obsoleting arch/arm/plat-samsung/dma-ops.c,
> arch/arm/plat-samsung/s3c-dma-ops.c and
> arch/arm/plat-samsung/include/plat/dma-ops.h.
> 
> This patch achieves this by open-coding the implementation of this
> wrapper, which ends up saving us more code than we have to add.
> s3c24xx does not support cyclic DMA, so anything referring to that
> gets deleted in the process.
> 
> The plat-samsung dma wrapper code is now dead code and can be removed
> subsequently.
> 
> Signed-off--by: Arnd Bergmann <arnd@arndb.de>
> ---
> 
>  sound/soc/samsung/ac97.c | 10 ++----
>  sound/soc/samsung/dma.c  | 79
> +++++++++++++++--------------------------------- sound/soc/samsung/dma.h  |
>  2 --
>  3 files changed, 27 insertions(+), 64 deletions(-)
> 
> diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
> index 76b072b..28fe097 100644
> --- a/sound/soc/samsung/ac97.c
> +++ b/sound/soc/samsung/ac97.c
> @@ -253,10 +253,7 @@ static int s3c_ac97_trigger(struct snd_pcm_substream
> *substream, int cmd,
> 
>  	writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
> 
> -	if (!dma_data->ops)
> -		dma_data->ops = samsung_dma_get_ops();
> -
> -	dma_data->ops->started(dma_data->channel);
> +	s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);
> 
>  	return 0;
>  }
> @@ -287,10 +284,7 @@ static int s3c_ac97_mic_trigger(struct
> snd_pcm_substream *substream,
> 
>  	writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
> 
> -	if (!dma_data->ops)
> -		dma_data->ops = samsung_dma_get_ops();
> -
> -	dma_data->ops->started(dma_data->channel);
> +	s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);
> 
>  	return 0;
>  }
> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
> index dc09b71..3c6e2f2 100644
> --- a/sound/soc/samsung/dma.c
> +++ b/sound/soc/samsung/dma.c
> @@ -54,8 +54,6 @@ struct runtime_data {
>  	struct s3c_dma_params *params;
>  };
> 
> -static void audio_buffdone(void *data);
> -
>  /* dma_enqueue
>   *
>   * place a dma buffer onto the queue for the dma system
> @@ -66,7 +64,6 @@ static void dma_enqueue(struct snd_pcm_substream
> *substream) struct runtime_data *prtd = substream->runtime->private_data;
>  	dma_addr_t pos = prtd->dma_pos;
>  	unsigned int limit;
> -	struct samsung_dma_prep dma_info;
> 
>  	pr_debug("Entered %s\n", __func__);
> 
> @@ -75,33 +72,11 @@ static void dma_enqueue(struct snd_pcm_substream
> *substream) pr_debug("%s: loaded %d, limit %d\n",
>  				__func__, prtd->dma_loaded, limit);
> 
> -	dma_info.cap = (samsung_dma_has_circular() ? DMA_CYCLIC : DMA_SLAVE);
> -	dma_info.direction =
> -		(substream->stream == SNDRV_PCM_STREAM_PLAYBACK
> -		? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM);
> -	dma_info.fp = audio_buffdone;
> -	dma_info.fp_param = substream;
> -	dma_info.period = prtd->dma_period;
> -	dma_info.len = prtd->dma_period*limit;
> -
> -	if (dma_info.cap == DMA_CYCLIC) {
> -		dma_info.buf = pos;
> -		prtd->params->ops->prepare(prtd->params->ch, &dma_info);
> -		prtd->dma_loaded += limit;
> -		return;
> -	}
> -
>  	while (prtd->dma_loaded < limit) {
>  		pr_debug("dma_loaded: %d\n", prtd->dma_loaded);
> 
> -		if ((pos + dma_info.period) > prtd->dma_end) {
> -			dma_info.period  = prtd->dma_end - pos;
> -			pr_debug("%s: corrected dma len %ld\n",
> -					__func__, dma_info.period);
> -		}
> -
> -		dma_info.buf = pos;
> -		prtd->params->ops->prepare(prtd->params->ch, &dma_info);
> +		s3c2410_dma_enqueue(prtd->params->channel, substream, pos,
> +				    prtd->dma_period * limit);
> 
>  		prtd->dma_loaded++;
>  		pos += prtd->dma_period;
> @@ -112,7 +87,8 @@ static void dma_enqueue(struct snd_pcm_substream
> *substream) prtd->dma_pos = pos;
>  }
> 
> -static void audio_buffdone(void *data)
> +static void audio_buffdone(struct s3c2410_dma_chan *channel, void *data,
> +		       int size, enum s3c2410_dma_buffresult res)
>  {
>  	struct snd_pcm_substream *substream = data;
>  	struct runtime_data *prtd = substream->runtime->private_data;
> @@ -128,10 +104,8 @@ static void audio_buffdone(void *data)
>  			snd_pcm_period_elapsed(substream);
> 
>  		spin_lock(&prtd->lock);
> -		if (!samsung_dma_has_circular()) {
> -			prtd->dma_loaded--;
> -			dma_enqueue(substream);
> -		}
> +		prtd->dma_loaded--;
> +		dma_enqueue(substream);
>  		spin_unlock(&prtd->lock);
>  	}
>  }
> @@ -145,8 +119,7 @@ static int dma_hw_params(struct snd_pcm_substream
> *substream, unsigned long totbytes = params_buffer_bytes(params);
>  	struct s3c_dma_params *dma =
>  		snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
> -	struct samsung_dma_req req;
> -	struct samsung_dma_config config;
> +	enum dma_transfer_direction direction;
> 
>  	pr_debug("Entered %s\n", __func__);
> 
> @@ -164,24 +137,22 @@ static int dma_hw_params(struct snd_pcm_substream
> *substream, pr_debug("params %p, client %p, channel %d\n", prtd->params,
>  			prtd->params->client, prtd->params->channel);
> 
> -		prtd->params->ops = samsung_dma_get_ops();
> -
> -		req.cap = (samsung_dma_has_circular() ?
> -			DMA_CYCLIC : DMA_SLAVE);
> -		req.client = prtd->params->client;
> -		config.direction =
> -			(substream->stream == SNDRV_PCM_STREAM_PLAYBACK
> +		direction = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK
>  			? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM);
> -		config.width = prtd->params->dma_size;
> -		config.fifo = prtd->params->dma_addr;
> -		prtd->params->ch = prtd->params->ops->request(
> -				prtd->params->channel, &req, rtd->cpu_dai->dev,
> -				prtd->params->ch_name);
> -		if (!prtd->params->ch) {
> +
> +		if (s3c2410_dma_request(prtd->params->channel,
> +					(void*)prtd->params->client, NULL) < 0) {
>  			pr_err("Failed to allocate DMA channel\n");
> +			s3c2410_dma_free(prtd->params->channel,
> +					 (void*)prtd->params->client);
>  			return -ENXIO;
>  		}
> -		prtd->params->ops->config(prtd->params->ch, &config);
> +		s3c2410_dma_devconfig(prtd->params->channel, direction,
> +				      prtd->params->dma_addr);
> +		s3c2410_dma_config(prtd->params->channel,
> +				   prtd->params->dma_size);
> +		s3c2410_dma_set_buffdone_fn(prtd->params->channel,
> +					    audio_buffdone);
>  	}
> 
>  	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
> @@ -208,9 +179,9 @@ static int dma_hw_free(struct snd_pcm_substream
> *substream) snd_pcm_set_runtime_buffer(substream, NULL);
> 
>  	if (prtd->params) {
> -		prtd->params->ops->flush(prtd->params->ch);
> -		prtd->params->ops->release(prtd->params->ch,
> -					prtd->params->client);
> +		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_FLUSH);
> +		s3c2410_dma_free(prtd->params->channel,
> +				 (void*)prtd->params->client);
>  		prtd->params = NULL;
>  	}
> 
> @@ -230,7 +201,7 @@ static int dma_prepare(struct snd_pcm_substream
> *substream) return 0;
> 
>  	/* flush the DMA channel */
> -	prtd->params->ops->flush(prtd->params->ch);
> +	s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_FLUSH);
> 
>  	prtd->dma_loaded = 0;
>  	prtd->dma_pos = prtd->dma_start;
> @@ -253,12 +224,12 @@ static int dma_trigger(struct snd_pcm_substream
> *substream, int cmd) switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  		prtd->state |= ST_RUNNING;
> -		prtd->params->ops->trigger(prtd->params->ch);
> +		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_START);
>  		break;
> 
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		prtd->state &= ~ST_RUNNING;
> -		prtd->params->ops->stop(prtd->params->ch);
> +		s3c2410_dma_ctrl(prtd->params->channel, S3C2410_DMAOP_STOP);
>  		break;
> 
>  	default:
> diff --git a/sound/soc/samsung/dma.h b/sound/soc/samsung/dma.h
> index ad7c0f0..c52fa86 100644
> --- a/sound/soc/samsung/dma.h
> +++ b/sound/soc/samsung/dma.h
> @@ -23,8 +23,6 @@ struct s3c_dma_params {
>  	int channel;				/* Channel ID */
>  	dma_addr_t dma_addr;
>  	int dma_size;			/* Size of the DMA transfer */
> -	unsigned ch;
> -	struct samsung_dma_ops *ops;
>  	char *ch_name;
>  	struct snd_dmaengine_dai_dma_data dma_data;
>  };
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown May 2, 2014, 5:24 p.m. UTC | #3
On Fri, May 02, 2014 at 12:35:21AM +0200, Arnd Bergmann wrote:
> On Thursday 01 May 2014 12:11:25 Mark Brown wrote:

> > Why is the fix for this not to ensure that s3c24xx always enables the
> > DMA controller - how likely is it that it would be sane to build a
> > kernel without DMA after all?

> S3C24XX_DMA is only needed for ASoC and for s3cmci. The latter
> uses 'depends on S3C24XX_DMA'. arch/arm/configs/tct_hammer_defconfig
> is an example of a configuration that does not include DMA because
> it uses neither of the two drivers.

I'm having a hard time caring about that, the arch code can always do an
"if whatever".  If that board is what I think it actually does have
audio on it, either the defconfig was never updated or the driver wasn't
merged into mainline.

> How about the patch below?

I guess (modulo the issues below).  Killing the wrapper and making the
existing selection of DMA support actually select DMA support does avoid
the redundancy.

> diff --git a/sound/soc/samsung/ac97.c b/sound/soc/samsung/ac97.c
> index 76b072b..28fe097 100644
> --- a/sound/soc/samsung/ac97.c
> +++ b/sound/soc/samsung/ac97.c
> @@ -253,10 +253,7 @@ static int s3c_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
>  
>  	writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL);
>  
> -	if (!dma_data->ops)
> -		dma_data->ops = samsung_dma_get_ops();
> -
> -	dma_data->ops->started(dma_data->channel);
> +	s3c2410_dma_ctrl(dma_data->channel, S3C2410_DMAOP_STARTED);
>  
>  	return 0;
>  }

This code is broken in general - at least s3c64xx has AC'97.  That's
broken anyway though.
Mark Brown May 2, 2014, 5:26 p.m. UTC | #4
On Fri, May 02, 2014 at 12:25:58PM +0200, Heiko Stübner wrote:

> Mark already removed support for the legacy API from the s3c64xx spi driver 
> (used by s3c2416 and s3c2443), so I guess to way forward would be to "simply" 
> convert asoc and s3cmci to dmaengine and get rid of it altogether.

ASoC is already totally capable of using dmaengine on Samsung platforms,
we're just wating for the architecture to be converted.  I know there's
a dmaengine driver present but it needs to actually be used on the
platforms, there should be no blocker on that from the ASoC side.
diff mbox

Patch

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 99cc196..7b610a8 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -1,7 +1,6 @@ 
 config SND_SOC_SAMSUNG
 	tristate "ASoC support for Samsung"
 	depends on PLAT_SAMSUNG
-	select S3C2410_DMA if ARCH_S3C24XX
 	select S3C64XX_PL080 if ARCH_S3C64XX
 	select SND_S3C_DMA if !ARCH_S3C24XX
 	select SND_S3C_DMA_LEGACY if ARCH_S3C24XX
@@ -15,11 +14,11 @@  config SND_S3C_DMA
 	tristate
 
 config SND_S3C_DMA_LEGACY
+	select S3C24XX_DMA
 	tristate
 
 config SND_S3C24XX_I2S
 	tristate
-	select S3C24XX_DMA
 
 config SND_S3C_I2SV2_SOC
 	tristate
@@ -27,7 +26,6 @@  config SND_S3C_I2SV2_SOC
 config SND_S3C2412_SOC_I2S
 	tristate
 	select SND_S3C_I2SV2_SOC
-	select S3C2410_DMA
 
 config SND_SAMSUNG_PCM
 	tristate
@@ -83,7 +81,6 @@  config SND_SOC_SAMSUNG_SMDK_WM8994
 config SND_SOC_SAMSUNG_SMDK2443_WM9710
 	tristate "SoC AC97 Audio support for SMDK2443 - WM9710"
 	depends on SND_SOC_SAMSUNG && MACH_SMDK2443
-	select S3C2410_DMA
 	select AC97_BUS
 	select SND_SOC_AC97_CODEC
 	select SND_SAMSUNG_AC97
@@ -94,7 +91,6 @@  config SND_SOC_SAMSUNG_SMDK2443_WM9710
 config SND_SOC_SAMSUNG_LN2440SBC_ALC650
 	tristate "SoC AC97 Audio support for LN2440SBC - ALC650"
 	depends on SND_SOC_SAMSUNG && ARCH_S3C24XX
-	select S3C2410_DMA
 	select AC97_BUS
 	select SND_SOC_AC97_CODEC
 	select SND_SAMSUNG_AC97