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

[Jerome Brunet <jbrunet@baylibre.com>]

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.

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