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.