On Friday, 23 April 2021, 12:22:11 BST, Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx> wrote: > On Thursday, 22 April 2021, 15:01:02 BST, Péter Ujfalusi <peter.ujfalusi@xxxxxxxxx> 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
