ASoc / PCM recording-related regression between v5.4 and v5.5

[Hin-Tak Leung]

On Friday, 23 April 2021, 12:22:11 BST, Hin-Tak Leung <htl10 at users.sourceforge.net> wrote:

> On Thursday, 22 April 2021, 15:01:02 BST, Péter Ujfalusi <peter.ujfalusi at gmail.com> wrote:

> > Can you point me to the out of tree driver and the related dts files?
> > Can it be reproduced with rpbi w/o any hat? I have one hifiberry but
> > that is playback only and if I'm not mistaken the 3.5 on the board also?

> For your purpose you can see my fork (which includes v5.4 -> v5.10 migration change, unlike the vendor's), 
> https://github.com/HinTak/seeed-voicecard . 

> The dts concerned is: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-8mic-voicecard-overlay.dts

> The card trigger code is in: https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-voicecard.c

> while the codec trigger code is in
> https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac108.c
> and also https://github.com/HinTak/seeed-voicecard/blob/v5.9/ac101.c ! 

> Note that the routine ac101_trigger() is not used, although the ac101 codec itself is involved.
> There is a spinlock in ac108_trigger(), which I have long suspected to be incorrect, and has been known to cause the > pi to kernel panic if built with PREEMPT ; Now I think it is there to make it work under the previous wrong sequence.

> There is another device of the same family - which records okay regardless of the trigger sequence 
> https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-4mic-voicecard-overlay.dts .

> The problematic device has 8 mics, using two ac108 for 2 x 4-ADC and the ac101 for DAC loopback (and as clock
> master?). The 4-mics record-okay device has just one ac108 (and without a ac101) to record 4-channels.

I have gone ahead and inserted a number of pr_info()'s to all the triggers (and compare the order of calls in 5.4 and 5.10) ; there was a mistake in my comment above: ac101_trigger() is used! it is just not set up as a call-back in the component struct.

If I understand it correctly, the 8-mics device was designed to do record and playback simultaneously; the master clock is really on the DAC ac101 codec; and ac101_trigger() needed to be called before the card trigger code seeed_voice_card_trigger() calling _set_clock().

In 5.4, we had ac108_trigger() calling ac101_trigger() , then seeed_voice_card_trigger() calling _set_clock(), which works. 
in 5.10 (and 5.5+), seeed_voice_card_trigger() calling _set_clock(), then ac108_trigger() calling ac101_trigger() , which does not.

So I made this rather ugly change to the driver code, and it gives me the right behavior:

<diff-begin>
diff --git a/Makefile b/Makefile
index b9de7f4..1aa0949 100644
--- a/Makefile
+++ b/Makefile
@@ -14,7 +14,7 @@ ifneq ($(KERNELRELEASE),)
 
 snd-soc-wm8960-objs := wm8960.o
 snd-soc-ac108-objs := ac108.o ac101.o
-snd-soc-seeed-voicecard-objs := seeed-voicecard.o
+snd-soc-seeed-voicecard-objs := seeed-voicecard.o ac101.o
 
 
 obj-m += snd-soc-wm8960.o
diff --git a/ac108.c b/ac108.c
index 5fe5176..c755741 100644
--- a/ac108.c
+++ b/ac108.c
@@ -1060,7 +1060,7 @@ static int ac108_trigger(struct snd_pcm_substream *substream, int cmd,
 
 spin_lock_irqsave(&ac10x->lock, flags);
 
- if (ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
+ if (!cmd && ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
 ac101_trigger(substream, cmd, dai);
 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 goto __ret;
diff --git a/seeed-voicecard.c b/seeed-voicecard.c
index 23dfb15..244dc6e 100644
--- a/seeed-voicecard.c
+++ b/seeed-voicecard.c
@@ -197,6 +197,7 @@ static int seeed_voice_card_trigger(struct snd_pcm_substream *substream, int cmd
 {
 struct snd_soc_pcm_runtime *rtd = substream->private_data;
 struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
+ struct ac10x_priv *ac10x = snd_soc_dai_get_drvdata(dai);
 struct seeed_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
 #if CONFIG_AC10X_TRIG_LOCK
 unsigned long flags;
@@ -216,6 +217,9 @@ static int seeed_voice_card_trigger(struct snd_pcm_substream *substream, int cmd
 /* I know it will degrades performance, but I have no choice */
 spin_lock_irqsave(&priv->lock, flags);
 #endif
+ if (cmd && ac10x->i2c101 && _MASTER_MULTI_CODEC == _MASTER_AC101) {
+ ac101_trigger(substream, cmd, dai);
+ }
 if (_set_clock[SNDRV_PCM_STREAM_CAPTURE]) _set_clock[SNDRV_PCM_STREAM_CAPTURE](1);
 if (_set_clock[SNDRV_PCM_STREAM_PLAYBACK]) _set_clock[SNDRV_PCM_STREAM_PLAYBACK](1);
 #if CONFIG_AC10X_TRIG_LOCK
<diff-ends>

It is certainly not ideal for the card driver to be linked directly and calling codec routines... Is there a better way of doing this, perhaps by re-writing the dts in https://github.com/HinTak/seeed-voicecard/blob/v5.9/seeed-8mic-voicecard-overlay.dts ? 

While we are at it, the spinlock in ac108.c (just above the changed line) seems wrong; can somebody have a look and advise on what better way to do it?

Regards,
Hin-Tak