Message-ID: <af4fde1c-281b-4826-a52c-a4e14727eb67@amlogic.com>
Date: Wed, 15 Jan 2025 19:47:32 +0800
From: Jiebing Chen <jiebing.chen@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: jiebing chen via B4 Relay <devnull+jiebing.chen.amlogic.com@...nel.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org
Subject: Re: [PATCH 2/3] ASoC: meson: s4:support for the on-chip audio


在 2025/1/15 18:36, Jiebing Chen 写道:
>
> 在 2025/1/15 16:36, Jerome Brunet 写道:
>> [ EXTERNAL EMAIL ]
>>
>> On Wed 15 Jan 2025 at 10:56, Jiebing Chen <jiebing.chen@...ogic.com> 
>> wrote:
>>
>>> 在 2025/1/14 22:05, Jerome Brunet 写道:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Tue 14 Jan 2025 at 19:20, Jiebing Chen 
>>>> <jiebing.chen@...ogic.com> wrote:
>>>>
>>>>>>>> +
>>>>>>>> +MODULE_DESCRIPTION("Amlogic to codec driver");
>>>>>>>> +MODULE_AUTHOR("jiebing.chen@...ogic.com");
>>>>>>>> +MODULE_LICENSE("GPL");
>>>>>>>> diff --git a/sound/soc/meson/t9015.c b/sound/soc/meson/t9015.c
>>>>>>>> index
>>>>>>>> 571f65788c592050abdca264f5656d4d1a9d99f6..2db1cd18cf2cea507f3d7282054e03d953586648 
>>>>>>>>
>>>>>>>> 100644
>>>>>>>> --- a/sound/soc/meson/t9015.c
>>>>>>>> +++ b/sound/soc/meson/t9015.c
>>>>>>>> @@ -89,10 +89,7 @@ static struct snd_soc_dai_driver t9015_dai = {
>>>>>>>>                  .channels_min = 1,
>>>>>>>>                  .channels_max = 2,
>>>>>>>>                  .rates = SNDRV_PCM_RATE_8000_96000,
>>>>>>>> -             .formats = (SNDRV_PCM_FMTBIT_S8 |
>>>>>>>> -                         SNDRV_PCM_FMTBIT_S16_LE |
>>>>>>>> -                         SNDRV_PCM_FMTBIT_S20_LE |
>>>>>>>> -                         SNDRV_PCM_FMTBIT_S24_LE),
>>>>>>>> +             .formats = (SNDRV_PCM_FMTBIT_S16_LE |
>>>>>>>> SNDRV_PCM_FMTBIT_S32_LE),
>>>>>>> Again, mixed up changes with zero justification.
>>>>>>>
>>>>>>> This drops S8 and S16 format support for the existing SoCs (such 
>>>>>>> as GXL)
>>>>>>> which is known to work and add S32 support on an HW documented 
>>>>>>> as 24bits
>>>>>>> only. Can you explain ?
>>>>> for g12a, sm1 etc, it is use new audio ip, GXL is old ip,
>>>> If there are chips difference we did not know about, then you should
>>>> introduce those difference, without breaking existing support -
>>>> including for GXL, which is what you did IIUC.
>>>>
>>>>> the new ip not support 24 bit,
>>>> Are sure about that ? that code has been there for a while.
>>>>
>>>> If sm1 does not support SNDRV_PCM_FMTBIT_S24_LE, you should a fix 
>>>> up patch for
>>>> that, with the proper "Fixes:" tag, how to reproduce the problem and
>>>> explaining the fix.
>>> maybe there are some gap , we support SNDRV_PCM_FMTBIT_S24, not 
>>> support the
>>>
>>> SNDRV_PCM_FMTBIT_S24_3LE,  for SNDRV_PCM_FMTBIT_S24
>>>
>>> it is  Signed, 24-bit (32-bit in memory), little endian , the audio dma
>>> busrt is 64bit
>> It makes absolutely no sense to discuss memory layout for the codec.
>>
>>> it can get the full data. we send the 32 bit data  mclk = 32bit* 48k 
>>> *4,
>>> use the clk to send
>>>
>>> the  SNDRV_PCM_FMTBIT_S24,   the hadware always send the 32bit data
>> No it does not. It send 24 bits of data over a 32 bits physical word 
>> with
>> 8 bits ignored.
>
> The original intention, we play 32bit data from tdmout, it play error, 
> so we add the s32_le
>
> for tdmouta ,it can bind Multiple codec, one codec is the internal codec,
>
> other is external codec, tdmout can send the data to internal codec 
> and external codec from the output pad
>
> for example, tdmout send 4 ch, 2 ch is send the internal codec, 2 ch 
> send the data pad
>
> it aplay error, due to the internal codec fomat parameter limiting 
> condition
>
> There is a contradiction here,  Considering our internal codec can 
> process this it, drop 8bit, 24bit valid
>
> therefore software can set s24_le/s32_le, still  work ok for hardware, 
> so Multiple ch can support for internal codec and external codec
>
>
add the s32_le format for the t9015.c,  it can resovle Multiple codec 
s32_le format case,  although hardware only process 24bit of 32bit, but 
it can't affect the acodec hardware work, it still work fine

usually we add the s32_le format support, if not allow to do it, think 
of it as a limitation, we can  remove 32bit test for it

>>
>>> so, i think we only add the SNDRV_PCM_FMTBIT_S32 base on it
>> That's wrong if the codec does not actually use the full 32bits ... and
>> I have clear indication that's what the codec is doing, on GXL at least.
>>
>>> we think the 24 bit is the SNDRV_PCM_FMTBIT_S24_3LE, it is 24bit in 
>>> memroy,
>>>
>>> due to the dma busrt 64 bit limit, it can't align the sample bit, if 
>>> it is
>>> 24 bit
>> Again, memory layout makes no sense here.
>>
>>> so the clock configure can't 24bit clock,
>> I disagree and this has been tested. If you have a test case showing
>> otherwise please share it.
>>
>>> by the way, We discuss internally for gxl,
>>>
>>> it also support the SNDRV_PCM_FMTBIT_S32
>>>
>> Does it really ? If it is just to ignore the 8bits LSB, that not a 
>> support.
>>
>>>>> usually support 16/32 bit for new audio ip , for 
>>>>> SNDRV_PCM_FMTBIT_S24_LE,
>>>>> it width =24, phy =32
>>>> Yes physical of SNDRV_PCM_FMTBIT_S24_LE, so most chip supporting 32 
>>>> bits
>>>> width would support this S24_LE, unless there is something odd.
>>>>
>>>>> it was  treated as 32 bit to send for tdm, so we can only add the 
>>>>> S32LE
>>>>> base on it , right ?
>>>> You are asking me ? How am I suppose to know ?
>>>>
>>>>> but if the gxl not support the 32bit
>>>> I don't see a problem with a DAC taking input on 32bits physical
>>>> interface and ignoring some bit on processing.
>>>>
>>>> If that's not the case, please send a proper fix change with some 
>>>> explanation
>>>>
>>>>> we need add new snd_soc_dai_driver t9015_dai_s4 ?
>>>>>
>>>> If I understood correctly format depends on the chip and needs to
>>>> adjusted including for sm1.
>>>>
>>>>>>>>          },
>>>>>>>>          .ops = &t9015_dai_ops,
>>>>>>>>     };
>>>>>>> -- Jerome
>>>> -- 
>>>> Jerome
>> -- 
>> Jerome

Powered by blists - more mailing lists