[alsa-devel] [PATCH 0/4] ASoC: Tegra30 TDM support
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
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
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
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
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
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?
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.
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
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.
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
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
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.
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
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
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-supp... 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
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-supp... 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
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
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
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.
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
participants (6)
-
Ben Dooks -
Ben Dooks -
Jon Hunter -
Jorge Sanjuan -
kbuild test robot -
Mark Brown