[4/4] 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(-)

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

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.