[PATCH 0/4] ASoC: Tegra30 TDM support

[PATCH 0/4] ASoC: Tegra30 TDM support

[Jorge Sanjuan]

This patchset adds support for TDM audio on Tegra30 hardware.
It adds the DAI's `set_tdm_slot` callback and enables a tegra
pcm to have up to 8 channels.

It also includes support for other audio formats supported by
the Tegra30 HW and fixes a broken macro needed for setting the
TDM on the registers.

Based on Linux 4.18-rc3 tag.

Edward Cragg (4):
  ASoC: tegra: i2s: Fix typo/broken macro
  ASoC: tegra: Add a TDM configuration callback
  ASoC: tegra: Allow 32-bit and 24-bit samples
  ASoC: tegra: i2s: Add support for more than 2 channels

 sound/soc/tegra/tegra30_i2s.c | 72 ++++++++++++++++++++++++++++++++++++-------
 sound/soc/tegra/tegra30_i2s.h |  2 +-
 2 files changed, 62 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH 1/4] ASoC: tegra: i2s: Fix typo/broken macro

[Jorge Sanjuan]

From: Edward Cragg <edward.cragg@codethink.co.uk>

Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.

Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/soc/tegra/tegra30_i2s.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
index 774fc6ad2026..2e561e946de2 100644
--- a/sound/soc/tegra/tegra30_i2s.h
+++ b/sound/soc/tegra/tegra30_i2s.h
@@ -173,7 +173,7 @@
 /* Number of slots in frame, minus 1 */
 #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT		16
 #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US	7
-#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
+#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
 
 /* TDM mode slot enable bitmask */
 #define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT	8
-- 
2.11.0

[PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Jorge Sanjuan]

From: Edward Cragg <edward.cragg@codethink.co.uk>

Add a callback to configure TDM settings for the Tegra30
I2S ASoC 'platform' driver.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
[jorge.sanjuan@codethink.co.uk: Style fixes]
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index 0b176ea24914..ff1996f215ed 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	return 0;
 }
 
+static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
+			       unsigned int tx_mask, unsigned int rx_mask,
+			       int slots, int slot_width)
+{
+	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+	unsigned int mask = 0, val = 0;
+
+	dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x"
+		"slots: 0x%08x width: %d\n",
+		__func__, tx_mask, rx_mask, slots, slot_width);
+
+	/* Set up slots and tx/rx masks */
+	mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
+	       TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |
+	       TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;
+
+	val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
+	      (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |
+	      ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
+
+	pm_runtime_get_sync(dai->dev);
+	regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
+
+	/* Set FSYNC width */
+	mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK;
+	val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT;
+
+	regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val);
+	pm_runtime_put(dai->dev);
+
+	return 0;
+}
+
 static int tegra30_i2s_probe(struct snd_soc_dai *dai)
 {
 	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -279,6 +312,7 @@ static const struct snd_soc_dai_ops tegra30_i2s_dai_ops = {
 	.set_fmt	= tegra30_i2s_set_fmt,
 	.hw_params	= tegra30_i2s_hw_params,
 	.trigger	= tegra30_i2s_trigger,
+	.set_tdm_slot	= tegra30_i2s_set_tdm,
 };
 
 static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
-- 
2.11.0

[PATCH 3/4] ASoC: tegra: Allow 32-bit and 24-bit samples

[Jorge Sanjuan]

From: Edward Cragg <edward.cragg@codethink.co.uk>

The tegra3 audio can support 32 and 24 bit sample sizes so add
the option to the tegra30_i2s_hw_params to configure the S32_LE/S24_LE
format when requested.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
[jorge.sanjuan@codethink.co.uk: Squashed multiple patches]
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/soc/tegra/tegra30_i2s.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index ff1996f215ed..e26c19ef7439 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -150,6 +150,15 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
 		sample_size = 16;
 		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_24;
+		sample_size = 24;
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
+		sample_size = 32;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -322,14 +331,18 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_96000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE |
+			   SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S16_LE,
 	},
 	.capture = {
 		.stream_name = "Capture",
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_96000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SNDRV_PCM_FMTBIT_S32_LE |
+			   SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S16_LE,
 	},
 	.ops = &tegra30_i2s_dai_ops,
 	.symmetric_rates = 1,
-- 
2.11.0

[PATCH 4/4] ASoC: tegra: i2s: Add support for more than 2 channels

[Jorge Sanjuan]

From: Edward Cragg <edward.cragg@codethink.co.uk>

The CIF configuration and clock setting is currently hard coded for 2
channels. Since the hardware is capable of supporting 1-8 channels add
support for reading the channel count from the supplied parameters to
allow for better TDM support. It seems the original implementation of this
driver was fixed at 2 channels for simplicity, and not implementing TDM.

Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
---
 sound/soc/tegra/tegra30_i2s.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index e26c19ef7439..0f240d7989d0 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -138,16 +138,17 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct device *dev = dai->dev;
 	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 	unsigned int mask, val, reg;
-	int ret, sample_size, srate, i2sclock, bitcnt;
+	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits, channels;
 	struct tegra30_ahub_cif_conf cif_conf;
 
-	if (params_channels(params) != 2)
+	if (params_channels(params) > 8)
 		return -EINVAL;
 
 	mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
 		sample_size = 16;
 		break;
 	case SNDRV_PCM_FORMAT_S24_LE:
@@ -157,6 +158,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 		break;
 	case SNDRV_PCM_FORMAT_S32_LE:
 		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
+		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
 		sample_size = 32;
 		break;
 	default:
@@ -166,9 +168,10 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val);
 
 	srate = params_rate(params);
+	channels = params_channels(params);
 
 	/* Final "* 2" required by Tegra hardware */
-	i2sclock = srate * params_channels(params) * sample_size * 2;
+	i2sclock = srate * channels * sample_size * 2;
 
 	bitcnt = (i2sclock / (2 * srate)) - 1;
 	if (bitcnt < 0 || bitcnt > TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_MASK_US)
@@ -188,10 +191,10 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	regmap_write(i2s->regmap, TEGRA30_I2S_TIMING, val);
 
 	cif_conf.threshold = 0;
-	cif_conf.audio_channels = 2;
-	cif_conf.client_channels = 2;
-	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
-	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
+	cif_conf.audio_channels = channels;
+	cif_conf.client_channels = channels;
+	cif_conf.audio_bits = audio_bits;
+	cif_conf.client_bits = audio_bits;
 	cif_conf.expand = 0;
 	cif_conf.stereo_conv = 0;
 	cif_conf.replicate = 0;
@@ -329,7 +332,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
 	.playback = {
 		.stream_name = "Playback",
 		.channels_min = 2,
-		.channels_max = 2,
+		.channels_max = 8,
 		.rates = SNDRV_PCM_RATE_8000_96000,
 		.formats = SNDRV_PCM_FMTBIT_S32_LE |
 			   SNDRV_PCM_FMTBIT_S24_LE |
@@ -338,7 +341,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
 	.capture = {
 		.stream_name = "Capture",
 		.channels_min = 2,
-		.channels_max = 2,
+		.channels_max = 8,
 		.rates = SNDRV_PCM_RATE_8000_96000,
 		.formats = SNDRV_PCM_FMTBIT_S32_LE |
 			   SNDRV_PCM_FMTBIT_S24_LE |
-- 
2.11.0

Re: [PATCH 3/4] ASoC: tegra: Allow 32-bit and 24-bit samples

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4517 bytes --]

Hi Edward,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

Note: the linux-review/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720 HEAD 14bbc96df0fa027f7bc057eb2da8181baff4e22c builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   sound/soc/tegra/tegra30_i2s.c: In function 'tegra30_i2s_hw_params':
>> sound/soc/tegra/tegra30_i2s.c:155:3: error: 'audio_bits' undeclared (first use in this function); did you mean 'audit_names'?
      audio_bits = TEGRA30_AUDIOCIF_BITS_24;
      ^~~~~~~~~~
      audit_names
   sound/soc/tegra/tegra30_i2s.c:155:3: note: each undeclared identifier is reported only once for each function it appears in

vim +155 sound/soc/tegra/tegra30_i2s.c

   133	
   134	static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
   135					 struct snd_pcm_hw_params *params,
   136					 struct snd_soc_dai *dai)
   137	{
   138		struct device *dev = dai->dev;
   139		struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
   140		unsigned int mask, val, reg;
   141		int ret, sample_size, srate, i2sclock, bitcnt;
   142		struct tegra30_ahub_cif_conf cif_conf;
   143	
   144		if (params_channels(params) != 2)
   145			return -EINVAL;
   146	
   147		mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
   148		switch (params_format(params)) {
   149		case SNDRV_PCM_FORMAT_S16_LE:
   150			val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
   151			sample_size = 16;
   152			break;
   153		case SNDRV_PCM_FORMAT_S24_LE:
   154			val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
 > 155			audio_bits = TEGRA30_AUDIOCIF_BITS_24;
   156			sample_size = 24;
   157			break;
   158		case SNDRV_PCM_FORMAT_S32_LE:
   159			val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
   160			sample_size = 32;
   161			break;
   162		default:
   163			return -EINVAL;
   164		}
   165	
   166		regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val);
   167	
   168		srate = params_rate(params);
   169	
   170		/* Final "* 2" required by Tegra hardware */
   171		i2sclock = srate * params_channels(params) * sample_size * 2;
   172	
   173		bitcnt = (i2sclock / (2 * srate)) - 1;
   174		if (bitcnt < 0 || bitcnt > TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_MASK_US)
   175			return -EINVAL;
   176	
   177		ret = clk_set_rate(i2s->clk_i2s, i2sclock);
   178		if (ret) {
   179			dev_err(dev, "Can't set I2S clock rate: %d\n", ret);
   180			return ret;
   181		}
   182	
   183		val = bitcnt << TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_SHIFT;
   184	
   185		if (i2sclock % (2 * srate))
   186			val |= TEGRA30_I2S_TIMING_NON_SYM_ENABLE;
   187	
   188		regmap_write(i2s->regmap, TEGRA30_I2S_TIMING, val);
   189	
   190		cif_conf.threshold = 0;
   191		cif_conf.audio_channels = 2;
   192		cif_conf.client_channels = 2;
   193		cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
   194		cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
   195		cif_conf.expand = 0;
   196		cif_conf.stereo_conv = 0;
   197		cif_conf.replicate = 0;
   198		cif_conf.truncate = 0;
   199		cif_conf.mono_conv = 0;
   200	
   201		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
   202			cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX;
   203			reg = TEGRA30_I2S_CIF_RX_CTRL;
   204		} else {
   205			cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_TX;
   206			reg = TEGRA30_I2S_CIF_TX_CTRL;
   207		}
   208	
   209		i2s->soc_data->set_audio_cif(i2s->regmap, reg, &cif_conf);
   210	
   211		val = (1 << TEGRA30_I2S_OFFSET_RX_DATA_OFFSET_SHIFT) |
   212		      (1 << TEGRA30_I2S_OFFSET_TX_DATA_OFFSET_SHIFT);
   213		regmap_write(i2s->regmap, TEGRA30_I2S_OFFSET, val);
   214	
   215		return 0;
   216	}
   217	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43928 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] ASoC: tegra: Allow 32-bit and 24-bit samples

[Ben Dooks]

On 2018-07-28 23:28, kbuild test robot wrote:
> Hi Edward,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on tegra/for-next]
> [also build test ERROR on v4.18-rc6 next-20180727]
> [if your patch is applied to the wrong git tree, please drop us a note
> to help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git 
> for-next
> config: arm-multi_v7_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=arm
> 
> Note: the
> linux-review/Jorge-Sanjuan/ASoC-Tegra30-TDM-support/20180728-163720
> HEAD 14bbc96df0fa027f7bc057eb2da8181baff4e22c builds fine.
>       It only hurts bisectibility.
> 
> All errors (new ones prefixed by >>):
> 
>    sound/soc/tegra/tegra30_i2s.c: In function 'tegra30_i2s_hw_params':
>>> sound/soc/tegra/tegra30_i2s.c:155:3: error: 'audio_bits' undeclared 
>>> (first use in this function); did you mean 'audit_names'?
>       audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>       ^~~~~~~~~~
>       audit_names
>    sound/soc/tegra/tegra30_i2s.c:155:3: note: each undeclared
> identifier is reported only once for each function it appears in
> 
> vim +155 sound/soc/tegra/tegra30_i2s.c
> 
>    133
>    134	static int tegra30_i2s_hw_params(struct snd_pcm_substream 
> *substream,
>    135					 struct snd_pcm_hw_params *params,
>    136					 struct snd_soc_dai *dai)
>    137	{
>    138		struct device *dev = dai->dev;
>    139		struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>    140		unsigned int mask, val, reg;
>    141		int ret, sample_size, srate, i2sclock, bitcnt;
>    142		struct tegra30_ahub_cif_conf cif_conf;
>    143
>    144		if (params_channels(params) != 2)
>    145			return -EINVAL;
>    146
>    147		mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
>    148		switch (params_format(params)) {
>    149		case SNDRV_PCM_FORMAT_S16_LE:
>    150			val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
>    151			sample_size = 16;
>    152			break;
>    153		case SNDRV_PCM_FORMAT_S24_LE:
>    154			val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>  > 155			audio_bits = TEGRA30_AUDIOCIF_BITS_24;
>    156			sample_size = 24;
>    157			break;
>    158		case SNDRV_PCM_FORMAT_S32_LE:
>    159			val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
>    160			sample_size = 32;
>    161			break;
>    162		default:
>    163			return -EINVAL;
>    164		}
> 

looks like we failed to merge in a fix from later in the internal
series we have.

jorge: can we get the channel fix from here into this patch and 
resubmit?

commit dd439f5f0b748eba43da7f18cabec8850dcd18b1
Author: Edward Cragg <edward.cragg@codethink.co.uk>
Date:   Thu Sep 15 17:01:49 2016 +0100

     ASoC: tegra: i2s: Add support for more than 2 channels

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 746 bytes --]

On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan wrote:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> Add a callback to configure TDM settings for the Tegra30
> I2S ASoC 'platform' driver.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>

This says it was britten by Edward but there's a signoff from Ben before
his?

> +	dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x"
> +		"slots: 0x%08x width: %d\n",
> +		__func__, tx_mask, rx_mask, slots, slot_width);

Please don't split log messages over lines, it makes it harder to grep
for them.  Just use a long line.

I'm also not seeing any validation of the parameters?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/4] ASoC: tegra: i2s: Fix typo/broken macro

[Jon Hunter]

On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.
> 
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  sound/soc/tegra/tegra30_i2s.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
> index 774fc6ad2026..2e561e946de2 100644
> --- a/sound/soc/tegra/tegra30_i2s.h
> +++ b/sound/soc/tegra/tegra30_i2s.h
> @@ -173,7 +173,7 @@
>  /* Number of slots in frame, minus 1 */
>  #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT		16
>  #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US	7
> -#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
> +#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
>  
>  /* TDM mode slot enable bitmask */
>  #define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT	8

Thanks for fixing.

Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Ben Dooks]

On Mon, Jul 30, 2018 at 09:49:08AM +0100, Mark Brown wrote:
> On Fri, Jul 27, 2018 at 01:59:29PM +0100, Jorge Sanjuan wrote:
> > From: Edward Cragg <edward.cragg@codethink.co.uk>
> > 
> > Add a callback to configure TDM settings for the Tegra30
> > I2S ASoC 'platform' driver.
> > 
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> This says it was britten by Edward but there's a signoff from Ben before
> his?

Editing accdient, I was originally going to submit this series.
 
> > +	dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x"
> > +		"slots: 0x%08x width: %d\n",
> > +		__func__, tx_mask, rx_mask, slots, slot_width);
> 
> Please don't split log messages over lines, it makes it harder to grep
> for them.  Just use a long line.
> 
> I'm also not seeing any validation of the parameters?



> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Jon Hunter]

On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> Add a callback to configure TDM settings for the Tegra30
> I2S ASoC 'platform' driver.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> [jorge.sanjuan@codethink.co.uk: Style fixes]
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  sound/soc/tegra/tegra30_i2s.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index 0b176ea24914..ff1996f215ed 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -265,6 +265,39 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
>  	return 0;
>  }
>  
> +static int tegra30_i2s_set_tdm(struct snd_soc_dai *dai,
> +			       unsigned int tx_mask, unsigned int rx_mask,
> +			       int slots, int slot_width)
> +{
> +	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +	unsigned int mask = 0, val = 0;
> +
> +	dev_dbg(dai->dev, "%s: setting TDM: tx_mask: 0x%08x rx_mask: 0x%08x"
> +		"slots: 0x%08x width: %d\n",
> +		__func__, tx_mask, rx_mask, slots, slot_width);
> +
> +	/* Set up slots and tx/rx masks */
> +	mask = TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK |
> +	       TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_MASK |
> +	       TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_MASK;
> +
> +	val = (tx_mask << TEGRA30_I2S_SLOT_CTRL_TX_SLOT_ENABLES_SHIFT) |
> +	      (rx_mask << TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT) |
> +	      ((slots - 1) << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT);
> +
> +	pm_runtime_get_sync(dai->dev);
> +	regmap_update_bits(i2s->regmap, TEGRA30_I2S_SLOT_CTRL, mask, val);
> +
> +	/* Set FSYNC width */
> +	mask = TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_MASK;
> +	val = (slot_width - 1) << TEGRA30_I2S_CH_CTRL_FSYNC_WIDTH_SHIFT;
> +
> +	regmap_update_bits(i2s->regmap, TEGRA30_I2S_CH_CTRL, mask, val);
> +	pm_runtime_put(dai->dev);
> +
> +	return 0;
> +}
> +

Looking at the TRM for Tegra30 and Tegra124, the I2S_SLOT_CTRL register is different
where for Tegra30 the 'TOTAL_SLOTS' bit are in position 18:16, but for Tegra124 they
are 3:0. This driver supports both Tegra30 and Tegra124, and so this function will
need to handle both.

It can be quite common for the fsync-width for DSP modes to be a single clock and so 
I am not sure that is makes sense to set this here always to the slot width. It maybe
worth considering add a DT property for specifying the fsync width.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 4/4] ASoC: tegra: i2s: Add support for more than 2 channels

[Jon Hunter]

On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <edward.cragg@codethink.co.uk>
> 
> The CIF configuration and clock setting is currently hard coded for 2
> channels. Since the hardware is capable of supporting 1-8 channels add
> support for reading the channel count from the supplied parameters to
> allow for better TDM support. It seems the original implementation of this
> driver was fixed at 2 channels for simplicity, and not implementing TDM.
> 
> Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
> ---
>  sound/soc/tegra/tegra30_i2s.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index e26c19ef7439..0f240d7989d0 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -138,16 +138,17 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	struct device *dev = dai->dev;
>  	struct tegra30_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>  	unsigned int mask, val, reg;
> -	int ret, sample_size, srate, i2sclock, bitcnt;
> +	int ret, sample_size, srate, i2sclock, bitcnt, audio_bits, channels;
>  	struct tegra30_ahub_cif_conf cif_conf;
>  
> -	if (params_channels(params) != 2)
> +	if (params_channels(params) > 8)
>  		return -EINVAL;

For normal I2S mode, channels should always be 2 and so it could be worth checking
if we are using TDM mode here or not.

>  
>  	mask = TEGRA30_I2S_CTRL_BIT_SIZE_MASK;
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:
>  		val = TEGRA30_I2S_CTRL_BIT_SIZE_16;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_16;
>  		sample_size = 16;
>  		break;
>  	case SNDRV_PCM_FORMAT_S24_LE:
> @@ -157,6 +158,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  		break;
>  	case SNDRV_PCM_FORMAT_S32_LE:
>  		val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> +		audio_bits = TEGRA30_AUDIOCIF_BITS_32;
>  		sample_size = 32;
>  		break;
>  	default:
> @@ -166,9 +168,10 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	regmap_update_bits(i2s->regmap, TEGRA30_I2S_CTRL, mask, val);
>  
>  	srate = params_rate(params);
> +	channels = params_channels(params);
>  
>  	/* Final "* 2" required by Tegra hardware */
> -	i2sclock = srate * params_channels(params) * sample_size * 2;
> +	i2sclock = srate * channels * sample_size * 2;
>  
>  	bitcnt = (i2sclock / (2 * srate)) - 1;
>  	if (bitcnt < 0 || bitcnt > TEGRA30_I2S_TIMING_CHANNEL_BIT_COUNT_MASK_US)
> @@ -188,10 +191,10 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
>  	regmap_write(i2s->regmap, TEGRA30_I2S_TIMING, val);
>  
>  	cif_conf.threshold = 0;
> -	cif_conf.audio_channels = 2;
> -	cif_conf.client_channels = 2;
> -	cif_conf.audio_bits = TEGRA30_AUDIOCIF_BITS_16;
> -	cif_conf.client_bits = TEGRA30_AUDIOCIF_BITS_16;
> +	cif_conf.audio_channels = channels;
> +	cif_conf.client_channels = channels;
> +	cif_conf.audio_bits = audio_bits;
> +	cif_conf.client_bits = audio_bits;
>  	cif_conf.expand = 0;
>  	cif_conf.stereo_conv = 0;
>  	cif_conf.replicate = 0;
> @@ -329,7 +332,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
>  	.playback = {
>  		.stream_name = "Playback",
>  		.channels_min = 2,
> -		.channels_max = 2,
> +		.channels_max = 8,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
>  		.formats = SNDRV_PCM_FMTBIT_S32_LE |
>  			   SNDRV_PCM_FMTBIT_S24_LE |
> @@ -338,7 +341,7 @@ static const struct snd_soc_dai_driver tegra30_i2s_dai_template = {
>  	.capture = {
>  		.stream_name = "Capture",
>  		.channels_min = 2,
> -		.channels_max = 2,
> +		.channels_max = 8,
>  		.rates = SNDRV_PCM_RATE_8000_96000,
>  		.formats = SNDRV_PCM_FMTBIT_S32_LE |
>  			   SNDRV_PCM_FMTBIT_S24_LE |
> 

Otherwise, assuming that you fix patch 3/4 and rebase this one, looks good to me.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 442 bytes --]

On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote:

> It can be quite common for the fsync-width for DSP modes to be a single clock and so 
> I am not sure that is makes sense to set this here always to the slot width. It maybe
> worth considering add a DT property for specifying the fsync width.

DSP modes only care about the rising edge of the LRCLK, the pulse can be
any width without causing interoperability problems.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 4/4] ASoC: tegra: i2s: Add support for more than 2 channels

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Mon, Jul 30, 2018 at 10:46:14AM +0100, Jon Hunter wrote:
> On 27/07/18 13:59, Jorge Sanjuan wrote:

> > -	if (params_channels(params) != 2)
> > +	if (params_channels(params) > 8)
> >  		return -EINVAL;

> For normal I2S mode, channels should always be 2 and so it could be worth checking
> if we are using TDM mode here or not.

Yes, there's some question if a multi-channel I2S setup is going to be
all the left channels then all the right channels, have multiple data
lines in parallel (this especially common for high end applications) or
something else.  Usually it's safer to use a DSP mode for those.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Applied "ASoC: tegra: i2s: Fix typo/broken macro" to the asoc tree

[Mark Brown]

The patch

   ASoC: tegra: i2s: Fix typo/broken macro

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 279fef50b607f9cee94f10bae84f6730e97ccd7c Mon Sep 17 00:00:00 2001
From: Edward Cragg <edward.cragg@codethink.co.uk>
Date: Fri, 27 Jul 2018 13:59:28 +0100
Subject: [PATCH] ASoC: tegra: i2s: Fix typo/broken macro

Fix typo in macro TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK.

Signed-off-by: Edward Cragg <edward.cragg@codethink.co.uk>
Signed-off-by: Jorge Sanjuan <jorge.sanjuan@codethink.co.uk>
Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/tegra/tegra30_i2s.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra30_i2s.h b/sound/soc/tegra/tegra30_i2s.h
index 774fc6ad2026..2e561e946de2 100644
--- a/sound/soc/tegra/tegra30_i2s.h
+++ b/sound/soc/tegra/tegra30_i2s.h
@@ -173,7 +173,7 @@
 /* Number of slots in frame, minus 1 */
 #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT		16
 #define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US	7
-#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOT_SHIFT)
+#define TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK		(TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_MASK_US << TEGRA30_I2S_SLOT_CTRL_TOTAL_SLOTS_SHIFT)
 
 /* TDM mode slot enable bitmask */
 #define TEGRA30_I2S_SLOT_CTRL_RX_SLOT_ENABLES_SHIFT	8
-- 
2.18.0

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Jon Hunter]

On 30/07/18 11:18, Mark Brown wrote:
> On Mon, Jul 30, 2018 at 10:31:16AM +0100, Jon Hunter wrote:
> 
>> It can be quite common for the fsync-width for DSP modes to be a single clock and so 
>> I am not sure that is makes sense to set this here always to the slot width. It maybe
>> worth considering add a DT property for specifying the fsync width.
> 
> DSP modes only care about the rising edge of the LRCLK, the pulse can be
> any width without causing interoperability problems.

OK, thanks I was not able to find a spec that defines this, but I saw a
lot of codecs use a single bit clock width. So then equally making the
default '1' should also be fine.

I still do not like configuring the fsync width in this function. The
fsync width needs to be configured for both DSP modes and normal I2S
modes and so it seems it would be more appropriate to do this in the
hw_params function for this driver.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Jon Hunter]

On 30/07/18 15:04, Jon Hunter wrote:
> I still do not like configuring the fsync width in this function. The
> fsync width needs to be configured for both DSP modes and normal I2S
> modes and so it seems it would be more appropriate to do this in the
> hw_params function for this driver.

That said, it does not look like this current driver ever programs the
fsync width for normal I2S mode (which I thought was necessary as we do
in other Tegra I2S drivers). So I will check on this and confirm.

Jon

-- 
nvpublic

Re: [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]

On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote:
> On 30/07/18 11:18, Mark Brown wrote:

> > DSP modes only care about the rising edge of the LRCLK, the pulse can be
> > any width without causing interoperability problems.

> OK, thanks I was not able to find a spec that defines this, but I saw a
> lot of codecs use a single bit clock width. So then equally making the
> default '1' should also be fine.

There's not really a spec for this, it's just what tends to be
implemented.

> I still do not like configuring the fsync width in this function. The
> fsync width needs to be configured for both DSP modes and normal I2S
> modes and so it seems it would be more appropriate to do this in the
> hw_params function for this driver.

You *could* just always use the I2S width, it's going to look odd when
people use a scope but it will work most of the time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/4] ASoC: Tegra30 TDM support

[Ben Dooks]

On 2018-07-27 13:59, Jorge Sanjuan wrote:
> This patchset adds support for TDM audio on Tegra30 hardware.
> It adds the DAI's `set_tdm_slot` callback and enables a tegra
> pcm to have up to 8 channels.
> 
> It also includes support for other audio formats supported by
> the Tegra30 HW and fixes a broken macro needed for setting the
> TDM on the registers.
> 
> Based on Linux 4.18-rc3 tag.
> 
> Edward Cragg (4):
>   ASoC: tegra: i2s: Fix typo/broken macro
>   ASoC: tegra: Add a TDM configuration callback
>   ASoC: tegra: Allow 32-bit and 24-bit samples
>   ASoC: tegra: i2s: Add support for more than 2 channels
> 
>  sound/soc/tegra/tegra30_i2s.c | 72 
> ++++++++++++++++++++++++++++++++++++-------
>  sound/soc/tegra/tegra30_i2s.h |  2 +-
>  2 files changed, 62 insertions(+), 12 deletions(-)

Just as a note, Ed Cragg has moved on from Codethink and I've not
got a new address for him. We've been cleaning some of the work Ed,
Jorge, Terry and myself have been doing.

-- 
Ben

Re: [Linux-kernel] [PATCH 2/4] ASoC: tegra: Add a TDM configuration callback

[Ben Dooks]

On 2018-07-30 16:07, Mark Brown wrote:
> On Mon, Jul 30, 2018 at 03:04:46PM +0100, Jon Hunter wrote:
>> On 30/07/18 11:18, Mark Brown wrote:
> 
>> > DSP modes only care about the rising edge of the LRCLK, the pulse can be
>> > any width without causing interoperability problems.
> 
>> OK, thanks I was not able to find a spec that defines this, but I saw 
>> a
>> lot of codecs use a single bit clock width. So then equally making the
>> default '1' should also be fine.
> 
> There's not really a spec for this, it's just what tends to be
> implemented.
> 
>> I still do not like configuring the fsync width in this function. The
>> fsync width needs to be configured for both DSP modes and normal I2S
>> modes and so it seems it would be more appropriate to do this in the
>> hw_params function for this driver.
> 
> You *could* just always use the I2S width, it's going to look odd when
> people use a scope but it will work most of the time.

We did this as we were dealing with a legacy system in which we didn't
know if this was important setting or not, so we tried to make the
settings as close as possible to the original nvidia supplied source.

-- 
Ben