[PATCH bisect 0/2] ASoC: fsl_sai: Overwrite trigger()

[PATCH bisect 0/2] ASoC: fsl_sai: Overwrite trigger()

[Nicolin Chen]

This series of patches are bisected from the preivous version so as to
apply the PATCH-1 onto asoc-v3.15-4.

* The patches are generated by using '-U2' because the default '-U3'
  would conflict the baseline without fsl_sai_isr patches.

Nicolin Chen (2):
  ASoC: fsl_sai: Fix buggy configurations in trigger()
  ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

 sound/soc/fsl/fsl_sai.c | 43 +++++++++++++++++++++++--------------------
 sound/soc/fsl/fsl_sai.h | 11 +++++++++++
 2 files changed, 34 insertions(+), 20 deletions(-)

-- 
1.8.4

[PATCH bisect 1/2] ASoC: fsl_sai: Fix buggy configurations in trigger()

[Nicolin Chen]

The current trigger() has two crucial problems:
1) The DMA request enabling operations (FSL_SAI_CSR_FRDE) for Tx and Rx are
   now totally exclusive: It would fail to run simultaneous Tx-Rx cases.
2) The TERE disabling operation depends on an incorrect condition -- active
   reference count that only gets increased in snd_pcm_open() and decreased
   in snd_pcm_close(): The TERE would never get cleared.

So this patch overwrites the trigger function by following these rules:
A) We continue to support tx-async-while-rx-sync-to-tx case alone, which's
   originally limited by this fsl_sai driver, but we make the code easy to
   modify for the further support of the opposite case.
B) We enable both TE and RE for PLAYBACK stream or CAPTURE stream but only
   enabling the DMA request bit (FSL_SAI_CSR_FRDE) of the current direction
   due to the requirement of SAI -- For tx-async-while-rx-sync-to-tx case,
   the receiver is enabled only when both the transmitter and receiver are
   enabled.

Tested cases:
a) aplay test.wav -d5
b) arecord -r44100 -c2 -fS16_LE test.wav -d5
c) arecord -r44100 -c2 -fS16_LE -d5 | aplay
d) (aplay test2.wav &); sleep 1; arecord -r44100 -c2 -fS16_LE test.wav -d1
e) (arecord -r44100 -c2 -fS16_LE test.wav -d5 &); sleep 1; aplay test.wav -d1

Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 35 +++++++++++++++++------------------
 sound/soc/fsl/fsl_sai.h | 10 ++++++++++
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index f088545..bdfd497 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -366,4 +366,5 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 {
 	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
 	u32 tcsr, rcsr;
 
@@ -380,12 +381,4 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 	regmap_read(sai->regmap, FSL_SAI_RCSR, &rcsr);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		tcsr |= FSL_SAI_CSR_FRDE;
-		rcsr &= ~FSL_SAI_CSR_FRDE;
-	} else {
-		rcsr |= FSL_SAI_CSR_FRDE;
-		tcsr &= ~FSL_SAI_CSR_FRDE;
-	}
-
 	/*
 	 * It is recommended that the transmitter is the last enabled
@@ -396,20 +389,26 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		tcsr |= FSL_SAI_CSR_TERE;
-		rcsr |= FSL_SAI_CSR_TERE;
+		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
+			regmap_update_bits(sai->regmap, FSL_SAI_RCSR,
+					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
+			regmap_update_bits(sai->regmap, FSL_SAI_TCSR,
+					   FSL_SAI_CSR_TERE, FSL_SAI_CSR_TERE);
+		}
 
-		regmap_write(sai->regmap, FSL_SAI_RCSR, rcsr);
-		regmap_write(sai->regmap, FSL_SAI_TCSR, tcsr);
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
+				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (!(cpu_dai->playback_active || cpu_dai->capture_active)) {
-			tcsr &= ~FSL_SAI_CSR_TERE;
-			rcsr &= ~FSL_SAI_CSR_TERE;
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
+				   FSL_SAI_CSR_FRDE, 0);
+
+		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
+			regmap_update_bits(sai->regmap, FSL_SAI_TCSR,
+					   FSL_SAI_CSR_TERE, 0);
+			regmap_update_bits(sai->regmap, FSL_SAI_RCSR,
+					   FSL_SAI_CSR_TERE, 0);
 		}
-
-		regmap_write(sai->regmap, FSL_SAI_TCSR, tcsr);
-		regmap_write(sai->regmap, FSL_SAI_RCSR, rcsr);
 		break;
 	default:
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index a264185..64b6fe7 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -36,4 +36,14 @@
 #define FSL_SAI_RMR	0xe0 /* SAI Receive Mask */
 
+#define FSL_SAI_xCSR(tx)	(tx ? FSL_SAI_TCSR : FSL_SAI_RCSR)
+#define FSL_SAI_xCR1(tx)	(tx ? FSL_SAI_TCR1 : FSL_SAI_RCR1)
+#define FSL_SAI_xCR2(tx)	(tx ? FSL_SAI_TCR2 : FSL_SAI_RCR2)
+#define FSL_SAI_xCR3(tx)	(tx ? FSL_SAI_TCR3 : FSL_SAI_RCR3)
+#define FSL_SAI_xCR4(tx)	(tx ? FSL_SAI_TCR4 : FSL_SAI_RCR4)
+#define FSL_SAI_xCR5(tx)	(tx ? FSL_SAI_TCR5 : FSL_SAI_RCR5)
+#define FSL_SAI_xDR(tx)		(tx ? FSL_SAI_TDR : FSL_SAI_RDR)
+#define FSL_SAI_xFR(tx)		(tx ? FSL_SAI_TFR : FSL_SAI_RFR)
+#define FSL_SAI_xMR(tx)		(tx ? FSL_SAI_TMR : FSL_SAI_RMR)
+
 /* SAI Transmit/Recieve Control Register */
 #define FSL_SAI_CSR_TERE	BIT(31)
-- 
1.8.4

Re: [PATCH bisect 1/2] ASoC: fsl_sai: Fix buggy configurations in trigger()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Tue, Apr 01, 2014 at 11:17:06AM +0800, Nicolin Chen wrote:
> The current trigger() has two crucial problems:
> 1) The DMA request enabling operations (FSL_SAI_CSR_FRDE) for Tx and Rx are
>    now totally exclusive: It would fail to run simultaneous Tx-Rx cases.
> 2) The TERE disabling operation depends on an incorrect condition -- active
>    reference count that only gets increased in snd_pcm_open() and decreased
>    in snd_pcm_close(): The TERE would never get cleared.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Nicolin Chen]

We only enable one side interrupt for each stream since over/underrun
on the opposite stream would be resulted from what we previously did,
enabling TERE but remaining FRDE disabled, even though the xrun on the
opposite direction will not break the current stream.

Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 8 ++++++--
 sound/soc/fsl/fsl_sai.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index bdfd497..d64c33f 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -397,4 +397,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
+				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
 				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
 		break;
@@ -404,4 +406,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
 				   FSL_SAI_CSR_FRDE, 0);
+		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
+				   FSL_SAI_CSR_xIE_MASK, 0);
 
 		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
@@ -464,6 +468,6 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai)
 	struct fsl_sai *sai = dev_get_drvdata(cpu_dai->dev);
 
-	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, FSL_SAI_FLAGS);
-	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, FSL_SAI_FLAGS);
+	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
+	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
 	regmap_update_bits(sai->regmap, FSL_SAI_TCR1, FSL_SAI_CR1_RFW_MASK,
 			   FSL_SAI_MAXBURST_TX * 2);
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 64b6fe7..be26d46 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -59,4 +59,5 @@
 #define FSL_SAI_CSR_FRF		BIT(16)
 #define FSL_SAI_CSR_xIE_SHIFT	8
+#define FSL_SAI_CSR_xIE_MASK	(0x1f << FSL_SAI_CSR_xIE_SHIFT)
 #define FSL_SAI_CSR_WSIE	BIT(12)
 #define FSL_SAI_CSR_SEIE	BIT(11)
-- 
1.8.4

RE: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Dongsheng.Wang]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTGludXhwcGMtZGV2IFtt
YWlsdG86bGludXhwcGMtZGV2LQ0KPiBib3VuY2VzK2I0MDUzND1mcmVlc2NhbGUuY29tQGxpc3Rz
Lm96bGFicy5vcmddIE9uIEJlaGFsZiBPZiBOaWNvbGluIENoZW4NCj4gU2VudDogVHVlc2RheSwg
QXByaWwgMDEsIDIwMTQgMTE6MTcgQU0NCj4gVG86IGJyb29uaWVAa2VybmVsLm9yZw0KPiBDYzog
YWxzYS1kZXZlbEBhbHNhLXByb2plY3Qub3JnOyBYaXVibyBMaS1CNDcwNTM7IGxpbnV4cHBjLWRl
dkBsaXN0cy5vemxhYnMub3JnOw0KPiBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyB0aW11
ckB0YWJpLm9yZw0KPiBTdWJqZWN0OiBbUEFUQ0ggYmlzZWN0IDIvMl0gQVNvQzogZnNsX3NhaTog
U2VwYXJhdGVseSBlbmFibGUgaW50ZXJydXB0cyBmb3IgVHgNCj4gYW5kIFJ4IHN0cmVhbXMNCj4g
DQo+IFdlIG9ubHkgZW5hYmxlIG9uZSBzaWRlIGludGVycnVwdCBmb3IgZWFjaCBzdHJlYW0gc2lu
Y2Ugb3Zlci91bmRlcnJ1bg0KPiBvbiB0aGUgb3Bwb3NpdGUgc3RyZWFtIHdvdWxkIGJlIHJlc3Vs
dGVkIGZyb20gd2hhdCB3ZSBwcmV2aW91c2x5IGRpZCwNCj4gZW5hYmxpbmcgVEVSRSBidXQgcmVt
YWluaW5nIEZSREUgZGlzYWJsZWQsIGV2ZW4gdGhvdWdoIHRoZSB4cnVuIG9uIHRoZQ0KPiBvcHBv
c2l0ZSBkaXJlY3Rpb24gd2lsbCBub3QgYnJlYWsgdGhlIGN1cnJlbnQgc3RyZWFtLg0KPiANCj4g
U2lnbmVkLW9mZi1ieTogTmljb2xpbiBDaGVuIDxHdWFuZ3l1LkNoZW5AZnJlZXNjYWxlLmNvbT4N
Cj4gQWNrZWQtYnk6IFhpdWJvIExpIDxMaS5YaXVib0BmcmVlc2NhbGUuY29tPg0KPiAtLS0NCj4g
IHNvdW5kL3NvYy9mc2wvZnNsX3NhaS5jIHwgOCArKysrKystLQ0KPiAgc291bmQvc29jL2ZzbC9m
c2xfc2FpLmggfCAxICsNCj4gIDIgZmlsZXMgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAyIGRl
bGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBhL3NvdW5kL3NvYy9mc2wvZnNsX3NhaS5jIGIv
c291bmQvc29jL2ZzbC9mc2xfc2FpLmMNCj4gaW5kZXggYmRmZDQ5Ny4uZDY0YzMzZiAxMDA2NDQN
Cj4gLS0tIGEvc291bmQvc29jL2ZzbC9mc2xfc2FpLmMNCj4gKysrIGIvc291bmQvc29jL2ZzbC9m
c2xfc2FpLmMNCj4gQEAgLTM5Nyw0ICszOTcsNiBAQCBzdGF0aWMgaW50IGZzbF9zYWlfdHJpZ2dl
cihzdHJ1Y3Qgc25kX3BjbV9zdWJzdHJlYW0NCj4gKnN1YnN0cmVhbSwgaW50IGNtZCwNCj4gDQo+
ICAJCXJlZ21hcF91cGRhdGVfYml0cyhzYWktPnJlZ21hcCwgRlNMX1NBSV94Q1NSKHR4KSwNCj4g
KwkJCQkgICBGU0xfU0FJX0NTUl94SUVfTUFTSywgRlNMX1NBSV9GTEFHUyk7DQo+ICsJCXJlZ21h
cF91cGRhdGVfYml0cyhzYWktPnJlZ21hcCwgRlNMX1NBSV94Q1NSKHR4KSwNCj4gIAkJCQkgICBG
U0xfU0FJX0NTUl9GUkRFLCBGU0xfU0FJX0NTUl9GUkRFKTsNCj4gIAkJYnJlYWs7DQo+IEBAIC00
MDQsNCArNDA2LDYgQEAgc3RhdGljIGludCBmc2xfc2FpX3RyaWdnZXIoc3RydWN0IHNuZF9wY21f
c3Vic3RyZWFtDQo+ICpzdWJzdHJlYW0sIGludCBjbWQsDQo+ICAJCXJlZ21hcF91cGRhdGVfYml0
cyhzYWktPnJlZ21hcCwgRlNMX1NBSV94Q1NSKHR4KSwNCj4gIAkJCQkgICBGU0xfU0FJX0NTUl9G
UkRFLCAwKTsNCj4gKwkJcmVnbWFwX3VwZGF0ZV9iaXRzKHNhaS0+cmVnbWFwLCBGU0xfU0FJX3hD
U1IodHgpLA0KPiArCQkJCSAgIEZTTF9TQUlfQ1NSX3hJRV9NQVNLLCAwKTsNCj4gDQo+ICAJCWlm
ICghKHRjc3IgJiBGU0xfU0FJX0NTUl9GUkRFIHx8IHJjc3IgJiBGU0xfU0FJX0NTUl9GUkRFKSkg
ew0KPiBAQCAtNDY0LDYgKzQ2OCw2IEBAIHN0YXRpYyBpbnQgZnNsX3NhaV9kYWlfcHJvYmUoc3Ry
dWN0IHNuZF9zb2NfZGFpICpjcHVfZGFpKQ0KPiAgCXN0cnVjdCBmc2xfc2FpICpzYWkgPSBkZXZf
Z2V0X2RydmRhdGEoY3B1X2RhaS0+ZGV2KTsNCj4gDQo+IC0JcmVnbWFwX3VwZGF0ZV9iaXRzKHNh
aS0+cmVnbWFwLCBGU0xfU0FJX1RDU1IsIDB4ZmZmZmZmZmYsIEZTTF9TQUlfRkxBR1MpOw0KPiAt
CXJlZ21hcF91cGRhdGVfYml0cyhzYWktPnJlZ21hcCwgRlNMX1NBSV9SQ1NSLCAweGZmZmZmZmZm
LCBGU0xfU0FJX0ZMQUdTKTsNCj4gKwlyZWdtYXBfdXBkYXRlX2JpdHMoc2FpLT5yZWdtYXAsIEZT
TF9TQUlfVENTUiwgMHhmZmZmZmZmZiwgMHgwKTsNCj4gKwlyZWdtYXBfdXBkYXRlX2JpdHMoc2Fp
LT5yZWdtYXAsIEZTTF9TQUlfUkNTUiwgMHhmZmZmZmZmZiwgMHgwKTsNCg0KV2h5IGFyZSB5b3Ug
cmVtb3ZlIHRoaXMgbWFjcm8/IERvbid0IHVzZSBtYWdpYyBudW1iZXIuDQoNClJlZ2FyZHMsDQot
RG9uZ3NoZW5nDQo=

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Nicolin Chen]

On Tue, Apr 01, 2014 at 11:25:02AM +0800, Wang Dongsheng-B40534 wrote:
> > Subject: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx
> > and Rx streams
> > 
> > We only enable one side interrupt for each stream since over/underrun
> > on the opposite stream would be resulted from what we previously did,
> > enabling TERE but remaining FRDE disabled, even though the xrun on the
> > opposite direction will not break the current stream.
> > 
> > Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
> > Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
> > ---
> >  sound/soc/fsl/fsl_sai.c | 8 ++++++--
> >  sound/soc/fsl/fsl_sai.h | 1 +
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index bdfd497..d64c33f 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -397,4 +397,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> > *substream, int cmd,
> > 
> >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > +				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
> > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> >  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
> >  		break;
> > @@ -404,4 +406,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> > *substream, int cmd,
> >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> >  				   FSL_SAI_CSR_FRDE, 0);
> > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > +				   FSL_SAI_CSR_xIE_MASK, 0);
> > 
> >  		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
> > @@ -464,6 +468,6 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai)
> >  	struct fsl_sai *sai = dev_get_drvdata(cpu_dai->dev);
> > 
> > -	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, FSL_SAI_FLAGS);
> > -	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, FSL_SAI_FLAGS);
> > +	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
> > +	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
> 
> Why are you remove this macro? Don't use magic number.

It's pretty clear that the so-called magic number is to clear the settings
in the registers for driver init as what this driver did at the first place
-- no offense but I don't think you would ask this if you check the git-log
of the driver.

Thank you,
Nicolin Chen

RE: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Dongsheng.Wang]

> -----Original Message-----
> From: Nicolin Chen [mailto:Guangyu.Chen@freescale.com]
> Sent: Tuesday, April 01, 2014 11:14 AM
> To: Wang Dongsheng-B40534
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Xiubo Li-B47053; lin=
uxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; timur@tabi.org
> Subject: Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrup=
ts for
> Tx and Rx streams
>=20
> On Tue, Apr 01, 2014 at 11:25:02AM +0800, Wang Dongsheng-B40534 wrote:
> > > Subject: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrup=
ts for
> Tx
> > > and Rx streams
> > >
> > > We only enable one side interrupt for each stream since over/underrun
> > > on the opposite stream would be resulted from what we previously did,
> > > enabling TERE but remaining FRDE disabled, even though the xrun on th=
e
> > > opposite direction will not break the current stream.
> > >
> > > Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
> > > Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
> > > ---
> > >  sound/soc/fsl/fsl_sai.c | 8 ++++++--
> > >  sound/soc/fsl/fsl_sai.h | 1 +
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > index bdfd497..d64c33f 100644
> > > --- a/sound/soc/fsl/fsl_sai.c
> > > +++ b/sound/soc/fsl/fsl_sai.c
> > > @@ -397,4 +397,6 @@ static int fsl_sai_trigger(struct snd_pcm_substre=
am
> > > *substream, int cmd,
> > >
> > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > +				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
> > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > >  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
> > >  		break;
> > > @@ -404,4 +406,6 @@ static int fsl_sai_trigger(struct snd_pcm_substre=
am
> > > *substream, int cmd,
> > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > >  				   FSL_SAI_CSR_FRDE, 0);
> > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > +				   FSL_SAI_CSR_xIE_MASK, 0);
> > >
> > >  		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
> > > @@ -464,6 +468,6 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *=
cpu_dai)
> > >  	struct fsl_sai *sai =3D dev_get_drvdata(cpu_dai->dev);
> > >
> > > -	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, FSL_SAI_F=
LAGS);
> > > -	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, FSL_SAI_F=
LAGS);
> > > +	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
> > > +	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
> >
> > Why are you remove this macro? Don't use magic number.
>=20
> It's pretty clear that the so-called magic number is to clear the setting=
s
> in the registers for driver init as what this driver did at the first pla=
ce
> -- no offense but I don't think you would ask this if you check the git-l=
og
> of the driver.
>=20
~FSL_SAI_MASK is better than 0x0. And you also replace 0xffffffff.

Regards,
-Dongsheng

> Thank you,
> Nicolin Chen

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Nicolin Chen]

On Tue, Apr 01, 2014 at 11:48:16AM +0800, Wang Dongsheng-B40534 wrote:
> 
> 
> > -----Original Message-----
> > From: Nicolin Chen [mailto:Guangyu.Chen@freescale.com]
> > Sent: Tuesday, April 01, 2014 11:14 AM
> > To: Wang Dongsheng-B40534
> > Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Xiubo Li-B47053; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; timur@tabi.org
> > Subject: Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for
> > Tx and Rx streams
> > 
> > On Tue, Apr 01, 2014 at 11:25:02AM +0800, Wang Dongsheng-B40534 wrote:
> > > > Subject: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for
> > Tx
> > > > and Rx streams
> > > >
> > > > We only enable one side interrupt for each stream since over/underrun
> > > > on the opposite stream would be resulted from what we previously did,
> > > > enabling TERE but remaining FRDE disabled, even though the xrun on the
> > > > opposite direction will not break the current stream.
> > > >
> > > > Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
> > > > Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
> > > > ---
> > > >  sound/soc/fsl/fsl_sai.c | 8 ++++++--
> > > >  sound/soc/fsl/fsl_sai.h | 1 +
> > > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > > index bdfd497..d64c33f 100644
> > > > --- a/sound/soc/fsl/fsl_sai.c
> > > > +++ b/sound/soc/fsl/fsl_sai.c
> > > > @@ -397,4 +397,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> > > > *substream, int cmd,
> > > >
> > > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > +				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
> > > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > >  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
> > > >  		break;
> > > > @@ -404,4 +406,6 @@ static int fsl_sai_trigger(struct snd_pcm_substream
> > > > *substream, int cmd,
> > > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > >  				   FSL_SAI_CSR_FRDE, 0);
> > > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > +				   FSL_SAI_CSR_xIE_MASK, 0);
> > > >
> > > >  		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
> > > > @@ -464,6 +468,6 @@ static int fsl_sai_dai_probe(struct snd_soc_dai *cpu_dai)
> > > >  	struct fsl_sai *sai = dev_get_drvdata(cpu_dai->dev);
> > > >
> > > > -	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, FSL_SAI_FLAGS);
> > > > -	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, FSL_SAI_FLAGS);
> > > > +	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
> > > > +	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
> > >
> > > Why are you remove this macro? Don't use magic number.
> > 
> > It's pretty clear that the so-called magic number is to clear the settings
> > in the registers for driver init as what this driver did at the first place
> > -- no offense but I don't think you would ask this if you check the git-log
> > of the driver.
> > 
> ~FSL_SAI_MASK is better than 0x0. And you also replace 0xffffffff.

I would later send a patch to reset SAI for a true init instead of these lines
but not within this patch as it's focusing on the interrupts enabling.

So please don't grasp the mask here. Just let me continue.

Thank you,
Nicolin Chen

RE: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Dongsheng.Wang]

> -----Original Message-----
> From: Nicolin Chen [mailto:Guangyu.Chen@freescale.com]
> Sent: Tuesday, April 01, 2014 11:38 AM
> To: Wang Dongsheng-B40534
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Xiubo Li-B47053; lin=
uxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; timur@tabi.org
> Subject: Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrup=
ts for
> Tx and Rx streams
>=20
> On Tue, Apr 01, 2014 at 11:48:16AM +0800, Wang Dongsheng-B40534 wrote:
> >
> >
> > > -----Original Message-----
> > > From: Nicolin Chen [mailto:Guangyu.Chen@freescale.com]
> > > Sent: Tuesday, April 01, 2014 11:14 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Xiubo Li-B47053;
> linuxppc-
> > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; timur@tabi.org
> > > Subject: Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable inte=
rrupts
> for
> > > Tx and Rx streams
> > >
> > > On Tue, Apr 01, 2014 at 11:25:02AM +0800, Wang Dongsheng-B40534 wrote=
:
> > > > > Subject: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable inte=
rrupts
> for
> > > Tx
> > > > > and Rx streams
> > > > >
> > > > > We only enable one side interrupt for each stream since over/unde=
rrun
> > > > > on the opposite stream would be resulted from what we previously =
did,
> > > > > enabling TERE but remaining FRDE disabled, even though the xrun o=
n the
> > > > > opposite direction will not break the current stream.
> > > > >
> > > > > Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
> > > > > Acked-by: Xiubo Li <Li.Xiubo@freescale.com>
> > > > > ---
> > > > >  sound/soc/fsl/fsl_sai.c | 8 ++++++--
> > > > >  sound/soc/fsl/fsl_sai.h | 1 +
> > > > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > > > > index bdfd497..d64c33f 100644
> > > > > --- a/sound/soc/fsl/fsl_sai.c
> > > > > +++ b/sound/soc/fsl/fsl_sai.c
> > > > > @@ -397,4 +397,6 @@ static int fsl_sai_trigger(struct snd_pcm_sub=
stream
> > > > > *substream, int cmd,
> > > > >
> > > > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > > +				   FSL_SAI_CSR_xIE_MASK, FSL_SAI_FLAGS);
> > > > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > >  				   FSL_SAI_CSR_FRDE, FSL_SAI_CSR_FRDE);
> > > > >  		break;
> > > > > @@ -404,4 +406,6 @@ static int fsl_sai_trigger(struct snd_pcm_sub=
stream
> > > > > *substream, int cmd,
> > > > >  		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > >  				   FSL_SAI_CSR_FRDE, 0);
> > > > > +		regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx),
> > > > > +				   FSL_SAI_CSR_xIE_MASK, 0);
> > > > >
> > > > >  		if (!(tcsr & FSL_SAI_CSR_FRDE || rcsr & FSL_SAI_CSR_FRDE)) {
> > > > > @@ -464,6 +468,6 @@ static int fsl_sai_dai_probe(struct snd_soc_d=
ai
> *cpu_dai)
> > > > >  	struct fsl_sai *sai =3D dev_get_drvdata(cpu_dai->dev);
> > > > >
> > > > > -	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff,
> FSL_SAI_FLAGS);
> > > > > -	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff,
> FSL_SAI_FLAGS);
> > > > > +	regmap_update_bits(sai->regmap, FSL_SAI_TCSR, 0xffffffff, 0x0);
> > > > > +	regmap_update_bits(sai->regmap, FSL_SAI_RCSR, 0xffffffff, 0x0);
> > > >
> > > > Why are you remove this macro? Don't use magic number.
> > >
> > > It's pretty clear that the so-called magic number is to clear the set=
tings
> > > in the registers for driver init as what this driver did at the first=
 place
> > > -- no offense but I don't think you would ask this if you check the g=
it-log
> > > of the driver.
> > >
> > ~FSL_SAI_MASK is better than 0x0. And you also replace 0xffffffff.
>=20
> I would later send a patch to reset SAI for a true init instead of these =
lines
> but not within this patch as it's focusing on the interrupts enabling.
>=20
> So please don't grasp the mask here. Just let me continue.
>=20

:), fine.

Regards,
-Dongsheng

> Thank you,
> Nicolin Chen

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Tue, Apr 01, 2014 at 11:17:07AM +0800, Nicolin Chen wrote:
> We only enable one side interrupt for each stream since over/underrun
> on the opposite stream would be resulted from what we previously did,
> enabling TERE but remaining FRDE disabled, even though the xrun on the
> opposite direction will not break the current stream.

This still doesn't apply against fsl-sai (nor for-next).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Nicolin Chen]

On Tue, Apr 01, 2014 at 01:07:15PM +0100, Mark Brown wrote:
> On Tue, Apr 01, 2014 at 11:17:07AM +0800, Nicolin Chen wrote:
> > We only enable one side interrupt for each stream since over/underrun
> > on the opposite stream would be resulted from what we previously did,
> > enabling TERE but remaining FRDE disabled, even though the xrun on the
> > opposite direction will not break the current stream.
> 
> This still doesn't apply against fsl-sai (nor for-next).

Sir, I just rebased my for-next branch again and found that it's missing
two applied patches: "ASoC: fsl_sai: Add isr to deal with error flag" and
"ASoC: fsl_sai: Improve fsl_sai_isr()", so that's why this PATCH-2 could
not be applied against it as it needs the macro that's included in the
patch "ASoC: fsl_sai: Add isr to deal with error flag".

What should I do now?

Thank you,
Nicolin

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Tue, Apr 01, 2014 at 09:21:57PM +0800, Nicolin Chen wrote:

> Sir, I just rebased my for-next branch again and found that it's missing
> two applied patches: "ASoC: fsl_sai: Add isr to deal with error flag" and
> "ASoC: fsl_sai: Improve fsl_sai_isr()", so that's why this PATCH-2 could
> not be applied against it as it needs the macro that's included in the
> patch "ASoC: fsl_sai: Add isr to deal with error flag".

Ah, those dropped out of -next due to the merge window.  I've applied
your new patch now.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH bisect 2/2] ASoC: fsl_sai: Separately enable interrupts for Tx and Rx streams

[Nicolin Chen]

On Tue, Apr 01, 2014 at 05:42:54PM +0100, Mark Brown wrote:
> On Tue, Apr 01, 2014 at 09:21:57PM +0800, Nicolin Chen wrote:
> 
> > Sir, I just rebased my for-next branch again and found that it's missing
> > two applied patches: "ASoC: fsl_sai: Add isr to deal with error flag" and
> > "ASoC: fsl_sai: Improve fsl_sai_isr()", so that's why this PATCH-2 could
> > not be applied against it as it needs the macro that's included in the
> > patch "ASoC: fsl_sai: Add isr to deal with error flag".
> 
> Ah, those dropped out of -next due to the merge window.  I've applied
> your new patch now.

I can find them on the fsl-sai topic now. Thank you.

Re: [PATCH bisect 0/2] ASoC: fsl_sai: Overwrite trigger()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]

On Tue, Apr 01, 2014 at 11:17:05AM +0800, Nicolin Chen wrote:

> * The patches are generated by using '-U2' because the default '-U3'
>   would conflict the baseline without fsl_sai_isr patches.

What are these "fsi_sai_isr patches"?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH bisect 0/2] ASoC: fsl_sai: Overwrite trigger()

[Nicolin Chen]

On Tue, Apr 01, 2014 at 12:04:12PM +0100, Mark Brown wrote:
> On Tue, Apr 01, 2014 at 11:17:05AM +0800, Nicolin Chen wrote:
> 
> > * The patches are generated by using '-U2' because the default '-U3'
> >   would conflict the baseline without fsl_sai_isr patches.
> 
> What are these "fsi_sai_isr patches"?

Ah..I should have mentioned the full patch name:
e2681a1 ASoC: fsl_sai: Add isr to deal with error flag

The patch conflicts against that asoc-3.15-4 tag at the fsl_sai.h because
this isr patch added some new macros to it.

Thank you,
Nicolin Chen