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

From: Kirill Marinushkin <kmarinushkin@birdec.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>,
	Mark Brown <broonie@kernel.org>
Cc: lgirdwood@gmail.com, codrin.ciubotariu@microchip.com,
	lars@metafoo.de, cychiang@chromium.org, tzungbi@google.com,
	bleung@chromium.org, matthias.bgg@gmail.com,
	oder_chiou@realtek.com, steven.eckhoff.opensource@gmail.com,
	srinivas.kandagatla@linaro.org, alexandre.belloni@bootlin.com,
	kuninori.morimoto.gx@renesas.com, jiaxin.yu@mediatek.com,
	alsa-devel@alsa-project.org, chrome-platform@lists.linux.dev,
	linux-mediatek@lists.infradead.org,
	patches@opensource.cirrus.com
Subject: Re: [PATCH 01/38] ASoC: soc-component: Add comment for the endianness flag
Date: Mon, 9 May 2022 22:11:03 +0200	[thread overview]
Message-ID: <8db9f890-1513-d9ed-58e3-5b402468288e@birdec.com> (raw)
In-Reply-To: <Ynlryv8fgKiHYXUt@sirena.org.uk>

Hello Charles, Mark,

Thank you for the clarification!

Without such a deep understanding of ASoC, as you have, I see a risk in 
a bulk enable of `endianness = 1`, the way we do in this patch set.

Here, we enable an extra feature. Worst case, if some codec doesn't 
support the feature, we will have a system, which thinks that it's 
supported, but in reality, it doesn't work. And we will not even have a 
error message, because the driver advertises the feature as supported.

Maybe my carefulness is not applicable here. I see that i don't have 
enough expertise in `endianness = 1`, to participate in making the 
decision here. But at least i want to ensure, that we all understand the 
risk.

Best Regards,

Kirill

On 5/9/22 9:30 PM, Mark Brown wrote:
> On Mon, May 09, 2022 at 09:22:42PM +0200, Kirill Marinushkin wrote:
>> On 5/9/22 10:37 AM, Charles Keepax 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`.
>> As soon as we start setting `endianness = 1`, the function
>> `convert_endianness_formats()` will extend LE to (LE | BE), right?
>> If so, setting `endianness = 1` is the source of an error, right?
> If doing this for the CODEC side of the link causes an issue it's just
> exposing an existing bug on the CPU side of the link which may already
> be affecting other systems - like Charles says the CODEC is expecting a
> fixed bit order regardless of the memory layout of the data.
>
>>> The CPU I2S hardware should be sending out the bits in the same
>>> order regardless of if the data you feed it is BE or LE, as I2S
>>> specifies an ordering for the bits.
>> What does the endianness in the driver configure, then?
> On the CODEC driver side it is meaningless.  On the CPU side it controls
> the in memory layout of the data.
>
>>> 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.
>> 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. I would expect, that i will have no
>> sound.
> If the CPU side of the link is fine then there should be no problem, we
> simply start supporting both endian settings all the way through the
> chain, if userspace chooses something that wasn't supported before then
> the CPU side driver will look at what's being configured and set up the
> hardware appropriately.