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

[Jerome Brunet]

On Wed 15 Jan 2025 at 10:56, Jiebing Chen <jiebing.chen@xxxxxxxxxxx> wrote:

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

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