Re: [PATCH 2/3] ASoC: meson: s4:support for the on-chip audio

[Jiebing Chen <jiebing.chen@amlogic.com>]

在 2025/1/15 19:47, Jiebing Chen 写道:
>
> 在 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@amlogic.com> 
>>> wrote:
>>>
>>>> 在 2025/1/14 22:05, Jerome Brunet 写道:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>> On Tue 14 Jan 2025 at 19:20, Jiebing Chen 
>>>>> <jiebing.chen@amlogic.com> wrote:
>>>>>
>>>>>>>>> +
>>>>>>>>> +MODULE_DESCRIPTION("Amlogic to codec driver");
>>>>>>>>> +MODULE_AUTHOR("jiebing.chen@amlogic.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.

At first I thought you said 24bit is s24_3le, becasue of the fddr can't 
get it,

so  spend a lot of time trying to explain it

I think it may be because of this difference that we are not focused on 
the same problem

yes, we support s24_le,  the internal codec also process it 24 bit of 32 
bit,

if we send valid 32bit data, it only process 24bit, discard 8 bit data, 
if this behavior should be prohibit

we will follow this rule

>>>> 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