lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1jr0545v8w.fsf@starbuckisacylon.baylibre.com>
Date: Wed, 15 Jan 2025 09:36:31 +0100
From: Jerome Brunet <jbrunet@...libre.com>
To: Jiebing Chen <jiebing.chen@...ogic.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

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.

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ