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.
