Re: [RFC][PATCH 21/26] alsa: ASoC: Add JZ4740 ASoC support

[Lars-Peter Clausen <lars@metafoo.de>]

Liam Girdwood wrote:
> On Wed, 2010-06-02 at 21:12 +0200, Lars-Peter Clausen wrote:
>   
>> This patch adds ASoC support for JZ4740 SoCs I2S module.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
>> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
>> Cc: alsa-devel@alsa-project.org
>> ---
>>  sound/soc/Kconfig             |    1 +
>>  sound/soc/Makefile            |    1 +
>>  sound/soc/jz4740/Kconfig      |   13 +
>>  sound/soc/jz4740/Makefile     |    9 +
>>  sound/soc/jz4740/jz4740-i2s.c |  568 +++++++++++++++++++++++++++++++++++++++++
>>  sound/soc/jz4740/jz4740-i2s.h |   18 ++
>>  sound/soc/jz4740/jz4740-pcm.c |  350 +++++++++++++++++++++++++
>>  sound/soc/jz4740/jz4740-pcm.h |   22 ++
>>  8 files changed, 982 insertions(+), 0 deletions(-)
>>  create mode 100644 sound/soc/jz4740/Kconfig
>>  create mode 100644 sound/soc/jz4740/Makefile
>>  create mode 100644 sound/soc/jz4740/jz4740-i2s.c
>>  create mode 100644 sound/soc/jz4740/jz4740-i2s.h
>>  create mode 100644 sound/soc/jz4740/jz4740-pcm.c
>>  create mode 100644 sound/soc/jz4740/jz4740-pcm.h
>>
>> diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
>> index b1749bc..5a7a724 100644
>> --- a/sound/soc/Kconfig
>> +++ b/sound/soc/Kconfig
>> @@ -36,6 +36,7 @@ source "sound/soc/s3c24xx/Kconfig"
>>  source "sound/soc/s6000/Kconfig"
>>  source "sound/soc/sh/Kconfig"
>>  source "sound/soc/txx9/Kconfig"
>> +source "sound/soc/jz4740/Kconfig"
>>  
>>  # Supported codecs
>>  source "sound/soc/codecs/Kconfig"
>> diff --git a/sound/soc/Makefile b/sound/soc/Makefile
>> index 1470141..fdbe74d 100644
>> --- a/sound/soc/Makefile
>> +++ b/sound/soc/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_SND_SOC)	+= s3c24xx/
>>  obj-$(CONFIG_SND_SOC)	+= s6000/
>>  obj-$(CONFIG_SND_SOC)	+= sh/
>>  obj-$(CONFIG_SND_SOC)	+= txx9/
>> +obj-$(CONFIG_SND_SOC)	+= jz4740/
>> diff --git a/sound/soc/jz4740/Kconfig b/sound/soc/jz4740/Kconfig
>> new file mode 100644
>> index 0000000..39df949
>> --- /dev/null
>> +++ b/sound/soc/jz4740/Kconfig
>> @@ -0,0 +1,13 @@
>> +config SND_JZ4740_SOC
>> +	tristate "SoC Audio for Ingenic JZ4740 SoC"
>> +	depends on SOC_JZ4740 && SND_SOC
>> +	help
>> +	  Say Y or M if you want to add support for codecs attached to
>> +	  the Jz4740 AC97, I2S or SSP interface. You will also need
>>     
>
> Do you have an AC97 or SSP interface ?
>
>   
Whoops. Copy-paste leftover...
>> +	[....]
>> +
>> +
>> +static int jz4740_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>> +	struct snd_soc_dai *dai)
>> +{
>> +	struct jz4740_i2s *i2s = jz4740_dai_to_i2s(dai);
>> +	bool playback = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
>> +
>> +	uint32_t ctrl;
>> +	uint32_t mask;
>> +
>> +	if (playback)
>>     
>
> It's best to use (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) here.
>
>   
hm, ok
>> +	[...]
>> --- /dev/null
>> +++ b/sound/soc/jz4740/jz4740-pcm.c
>> @@ -0,0 +1,350 @@
>> + [...] 
>> +static int jz4740_pcm_hw_params(struct snd_pcm_substream *substream,
>> +	struct snd_pcm_hw_params *params)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct jz4740_runtime_data *prtd = runtime->private_data;
>> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +	struct jz4740_pcm_config *config;
>> +
>> +	config = snd_soc_dai_get_dma_data(rtd->dai->cpu_dai, substream);
>> +	if (!prtd->dma) {
>> +		const char *dma_channel_name;
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			dma_channel_name = "PCM Playback";
>> +		else
>> +			dma_channel_name = "PCM Capture";
>> +
>> +		prtd->dma = jz4740_dma_request(substream, dma_channel_name);
>>     
>
> dma_channel_name variable is not required here. Just use the const char
> * directly.
>
>   
I actually had it like that before, but I think it is much more readable
in its current form. Considering that stream value will either be 0 or 1
it might work to put the channel names into a static array.
>> +	[...]
>> +
>> +static snd_pcm_uframes_t jz4740_pcm_pointer(struct snd_pcm_substream *substream)
>> +{
>> +	struct snd_pcm_runtime *runtime = substream->runtime;
>> +	struct jz4740_runtime_data *prtd = runtime->private_data;
>> +	unsigned long count, pos;
>> +	snd_pcm_uframes_t offset;
>> +	struct jz4740_dma_chan *dma = prtd->dma;
>> +
>> +	count = jz4740_dma_get_residue(dma);
>> +	if (prtd->dma_pos == prtd->dma_start)
>> +		pos = prtd->dma_end - prtd->dma_start - count;
>> +	else
>> +		pos = prtd->dma_pos - prtd->dma_start - count;
>> +
>> +	offset = bytes_to_frames(runtime, pos);
>> +	if (offset >= runtime->buffer_size)
>> +		offset = 0;
>> +
>>     
>
> Could you comment your calculation a little more.
>   
Will do.
>
> Thanks
>
> Liam
>   
Thanks for the review
- Lars