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

From: Jon Hunter
Date: Mon Jul 30 2018 - 05:46:27 EST



On 27/07/18 13:59, Jorge Sanjuan wrote:
> From: Edward Cragg <edward.cragg@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@xxxxxxxxxxxxxxx>
> ---
> 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