Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20220706211330.120198-1-aidanmacdonald.0x0@gmail.com>
 <20220706211330.120198-11-aidanmacdonald.0x0@gmail.com>
 <H4ANER.XJSSJIHNXTEA1@crapouillou.net>
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
In-reply-to: <H4ANER.XJSSJIHNXTEA1@crapouillou.net>
Message-ID: <jp7AjR5Hus1i7prrsjVT3n5vI5jaszTk@localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Paul Cercueil <paul@crapouillou.net> writes:

> Le mer., juil. 6 2022 at 22:13:29 +0100, Aidan MacDonald
> <aidanmacdonald.0x0@gmail.com> a =C3=A9crit :
>> 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_subs=
tream
>> *substream,
>>  	case SNDRV_PCM_FORMAT_S8:
>>  		sample_size =3D 0;
>>  		break;
>> -	case SNDRV_PCM_FORMAT_S16:
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>>  		sample_size =3D 1;
>>  		break;
>> +	case SNDRV_PCM_FORMAT_S20_LE:
>> +		sample_size =3D 3;
>> +		break;
>> +	case SNDRV_PCM_FORMAT_S24_LE:
>> +		sample_size =3D 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_o=
ps =3D
>> {
>>  };
>>  #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 =3D {
>>  	.probe =3D jz4740_i2s_dai_probe,
>> --
>> 2.35.1
>>=20