Re: [PATCH AUTOSEL 5.2 040/123] ASoC: Fail card instantiation if DAI format setup fails

[Mark Brown]

On Wed, Aug 28, 2019 at 09:07:17AM +0200, Ricard Wanderlof wrote:
> On Wed, 28 Aug 2019, Sasha Levin wrote:
> > On Tue, Aug 27, 2019 at 12:00:14PM +0100, Mark Brown wrote:

> > > If anyone ran into this on the older kernel and fixed or worked
> > > around it locally there's a reasonable chance this will then
> > > break what they're doing.  The patch itself is perfectly fine but

I should also have added that there's also the potential that things are
just working as they are and that detecting errors will cause new
failures.

> > But there's not much we can do here. We can't hold off on fixing
> > breakage such as this because existing users have workarounds for this.

If people are running into problems here then they just don't have
working audio; if they're shipping with that presumably they either
don't mind about it or have done something to fix it.  Either way this
patch won't by itself give anyone working audio that didn't have it
before.

> > Are we breaking kernel ABI with this patch then?

> > And what about new users? We'll let them get hit by the issue and
> > develop their own workarounds?

Hopefully we don't have too many new users adopting the older stables
you want to backport to...  in any case this patch just makes the error
reporting more forceful, it won't actually fix the issue it's reporting.

> My $0.02 here: In my specific case, we noticed the problem because there 
> was an unexpected left shift in the captured audio data, since the codec 
> and CPU DAIs were using different formats when the DAI format was not 
> explicitly set. The fix for that was to add

> simple-audio-card,format= "i2s";

> to the devicetree audio card section which of course should have been 
> there all the time. The fact that the kernel failed halt the 
> initialization of the audio card lengthened the debug time, but did not 
> provoke me to attempt a workaround, since the information (the error 
> printout from the ALSA framework when an invalid daifmt setting was made) 
> was actually right there in the kernel log.

> Possibly there might be other usecases, but in our case, if the kernel had 
> stopped the audio initialization it would then have been more obvious 
> where to start looking.

Right, returning the error makes things more obvious for system
integrators so it's a good change to have.  On the other hand there's
potential breakage if for example the hardware happens to default to the
correct format or an error is detected after enough configuration has
been done to the hardware to make it function well enough.  In that case
we'll start returning an error, failing to initialize the card and the
system will loose audio.

Basically this patch won't make anything work that didn't work before.

Attachment: signature.asc
Description: PGP signature