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

[Ricard Wanderlof]

On Wed, 28 Aug 2019, Sasha Levin wrote:

> On Tue, Aug 27, 2019 at 12:00:14PM +0100, Mark Brown wrote:
> > On Sun, Aug 25, 2019 at 09:35:15PM -0400, Sasha Levin wrote:
> > > On Wed, Aug 14, 2019 at 10:22:13AM +0100, Mark Brown wrote:
> > 
> > > > > If the DAI format setup fails, there is no valid communication format
> > > > > between CPU and CODEC, so fail card instantiation, rather than
> > > continue
> > > > > with a card that will most likely not function properly.
> > 
> > > > This is another one where if nobody noticed a problem already and things
> > > > just happened to be working this might break things, it's vanishingly
> > > > unlikely to fix anything that was broken.
> > 
> > > Same as the other patch: this patch suggests it fixes a real bug, and if
> > > this patch is broken let's fix it.
> > 
> > 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
> 
> 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.
> 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?

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.

/Ricard
-- 
Ricard Wolf Wanderlof                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30