Re: [PATCH 10/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats

From: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz,
	tiwai@suse.com, linux-mips@vger.kernel.org,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats
Date: Thu, 07 Jul 2022 15:25:06 +0100	[thread overview]
Message-ID: <jp7AjR5Hus1i7prrsjVT3n5vI5jaszTk@localhost> (raw)
In-Reply-To: <H4ANER.XJSSJIHNXTEA1@crapouillou.net>

Paul Cercueil <paul@crapouillou.net> writes:

> Le mer., juil. 6 2022 at 22:13:29 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a écrit :
>> The audio controller on JZ47xx SoCs supports 20- and 24-bit
>> samples coming from memory. Allow those formats to be used
>> with the I2S driver.
>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
>> ---
>>  sound/soc/jz4740/jz4740-i2s.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
>> index 80b355d715ce..ee99c5e781ec 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -222,9 +222,15 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream
>> *substream,
>>  	case SNDRV_PCM_FORMAT_S8:
>>  		sample_size = 0;
>>  		break;
>> -	case SNDRV_PCM_FORMAT_S16:
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>>  		sample_size = 1;
>>  		break;
>> +	case SNDRV_PCM_FORMAT_S20_LE:
>> +		sample_size = 3;
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +		sample_size = 4;
>> +		break;
>
> Did you test these? It is unclear to me, looking at the JZ4740 PM, if the
> 18-bit, 20-bit and 24-bit samples are in 4 bytes or 3 bytes.
>
> Cheers,
> -Paul
>

I was only able to test 24-bit -- in practice 20-bit seems pretty rare
and I'm finding it difficult to convert anything into that format with
standard tools like ffmpeg, sox, etc. so I can't really test it.

From what I understand, this is configuring the number of significant
bits in the FIFO and has nothing to do with the in-memory format. So
my commit message is a bit inaccurate.

DMA can only do 4-byte transfers so that's why only 4-byte formats are
usable. I suppose I could handle the 3-byte variants here, but there's
probably no point to doing that if DMA can't transfer them.

>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -362,7 +368,9 @@ static const struct snd_soc_dai_ops jz4740_i2s_dai_ops =
>> {
>>  };
>>  #define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
>> -		SNDRV_PCM_FMTBIT_S16_LE)
>> +			 SNDRV_PCM_FMTBIT_S16_LE | \
>> +			 SNDRV_PCM_FMTBIT_S20_LE | \
>> +			 SNDRV_PCM_FMTBIT_S24_LE)
>>  static struct snd_soc_dai_driver jz4740_i2s_dai = {
>>  	.probe = jz4740_i2s_dai_probe,
>> --
>> 2.35.1
>> 