Re: [PATCH 01/38] ASoC: soc-component: Add comment for the endianness flag

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: Kirill Marinushkin <kmarinushkin@birdec.com>
Cc: oder_chiou@realtek.com, steven.eckhoff.opensource@gmail.com,
	alexandre.belloni@bootlin.com, lars@metafoo.de,
	kuninori.morimoto.gx@renesas.com,
	chrome-platform@lists.linux.dev, patches@opensource.cirrus.com,
	lgirdwood@gmail.com, jiaxin.yu@mediatek.com, tzungbi@google.com,
	broonie@kernel.org, srinivas.kandagatla@linaro.org,
	matthias.bgg@gmail.com, linux-mediatek@lists.infradead.org,
	codrin.ciubotariu@microchip.com, alsa-devel@alsa-project.org,
	bleung@chromium.org, cychiang@chromium.org
Subject: Re: [PATCH 01/38] ASoC: soc-component: Add comment for the endianness flag
Date: Tue, 10 May 2022 08:53:23 +0000	[thread overview]
Message-ID: <20220510085323.GA38351@ediswmail.ad.cirrus.com> (raw)
In-Reply-To: <901cb995-4a82-741e-00ea-a1c0b22ae749@birdec.com>

On Mon, May 09, 2022 at 09:22:42PM +0200, Kirill Marinushkin wrote:
> On 5/9/22 10:37 AM, Charles Keepax wrote:
> >On Sun, May 08, 2022 at 10:34:12PM +0200, Kirill Marinushkin wrote:
> >This sounds like an error on the CPU side of the DAI link rather
> >than the CODEC side of the DAI link. The formats that will be
> >supported on the link are the union of the CPU and CODEC supported
> >formats, ie. a format must be supported on both for the DAI to
> >support it.
> 
> Yes, agree, both sides of the DAI link should provide only
> endianness they support.
> 
> This works currently, but, from my understending, it will break,
> after we set `endianness = 1`.

It will break if the CPU side claims it supports endians which it
doesn't which I believe is what you have in your system.

> As soon as we start setting `endianness = 1`, the function
> `convert_endianness_formats()` will extend LE to (LE | BE), right?

Correct.

> If so, setting `endianness = 1` is the source of an error, right?

No, potentially it exposes an error but it is not the source of the
error. Endian should have no meaning once you cross an I2S bus.

> >If this is not the case then
> >the host I2S controller is claiming to support an endian it does
> >not, and the problem should be fixed on that side by removing the
> >supported endian.
> 
> I think we have a misundersanding of my example.
> 
> In my example, i don't mean, that my CPU part of the DAI link is broken.

Why are you sure the CPU part of the DAI link isn't broken? I2S
defines the order bits should be sent out on the bus, this means if
the CPU side recieves big endian or little endian data it doesn't
matter, the actual bus signals should be identical in both cases.
If only one endian works the actual signals on the bus must have
been different depending on which endian you played in which case
the host side doesn't match the I2S specification.

My point is that what you are seeing is exactly what you get if
your CPU DAI claimed to support both big and little endian but
just puts that data directly out on the bus, rather than adapting
to the endian of data it receives.

And to be fair this is exactly what I am trying to tackle with this
series. The fact we have so many CODECs that are incorrectly not
specifying the endianness flag means that it is very easy for the
host side guys to do testing and not realise that the CODEC is the
one limiting the supported endians on the DAI link and thus miss
they have claimed to support endians they don't.

I believe you are right that there is a reasonable chance will see
some breakage after these patches, but that breakage should be
exposing host side bugs which we need to fix anyway.

> What i tried to demonstrate - is that if i set the unsupported
> endianness, i wouldn't expect that "the CODEC probably can care
> about the endian", as the message in [PATCH 00/38] suggests.

That comment in my original post was in respect to CODECs that
recieve/transmit audio directly over a SPI bus rather than a
normal audio bus. The is no standard governing how audio data is
transmitted over the SPI bus, thus endian could be important,
although I need to do some more work to analyse how people are
using these links. Often they are implemented effectively as a
CPU side DAI anyway.

Thanks,
Charles