[PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Nicolin Chen]

This patch enables Tx-related SIER_FLAGS only when direction is PLAYBACK
and does same thing for CAPTURE. Also, after TRIGGER_STOP/PAUSE, it will
disable SIER_xFLAGS for symmetric.

[ Passed compile-test with mpc85xx_defconfig ]

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/fsl/fsl_ssi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 35e2773..3797bf0 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,11 +107,14 @@ static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set)
 #endif
 
 /* SIER bitflag of interrupts to enable */
-#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
+#define SIER_TFLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
 		    CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TUE0_EN | \
-		    CCSR_SSI_SIER_TUE1_EN | CCSR_SSI_SIER_RFRC_EN | \
-		    CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \
-		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
+		    CCSR_SSI_SIER_TUE1_EN)
+#define SIER_RFLAGS (CCSR_SSI_SIER_RFRC_EN | CCSR_SSI_SIER_RDMAE | \
+		    CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_ROE0_EN | \
+		    CCSR_SSI_SIER_ROE1_EN)
+
+#define SIER_FLAGS (SIER_TFLAGS | SIER_RFLAGS)
 
 /**
  * fsl_ssi_private: per-SSI private data
@@ -560,12 +563,12 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		if (ssi_private->use_dma)
-			sier_bits = SIER_FLAGS;
+			sier_bits = SIER_TFLAGS;
 		else
 			sier_bits = CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN;
 	} else {
 		if (ssi_private->use_dma)
-			sier_bits = SIER_FLAGS;
+			sier_bits = SIER_RFLAGS;
 		else
 			sier_bits = CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN;
 	}
@@ -579,6 +582,8 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 		else
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE);
+
+		write_ssi_mask(&ssi->sier, 0, sier_bits);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
@@ -591,14 +596,14 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 		if (!ssi_private->imx_ac97 && (read_ssi(&ssi->scr) &
 					(CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE)) == 0)
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_SSIEN, 0);
+
+		write_ssi_mask(&ssi->sier, sier_bits, 0);
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	write_ssi(sier_bits, &ssi->sier);
-
 	return 0;
 }
 
-- 
1.8.4

[PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Nicolin Chen]

This patch enables Tx-related SIER_FLAGS only when direction is PLAYBACK
and does same thing for CAPTURE. Also, after TRIGGER_STOP/PAUSE, it will
disable SIER_xFLAGS for symmetric.

[ Passed compile-test with mpc85xx_defconfig ]

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/fsl/fsl_ssi.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 35e2773..3797bf0 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -107,11 +107,14 @@ static inline void write_ssi_mask(u32 __iomem *addr, u32 clear, u32 set)
 #endif
 
 /* SIER bitflag of interrupts to enable */
-#define SIER_FLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
+#define SIER_TFLAGS (CCSR_SSI_SIER_TFRC_EN | CCSR_SSI_SIER_TDMAE | \
 		    CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TUE0_EN | \
-		    CCSR_SSI_SIER_TUE1_EN | CCSR_SSI_SIER_RFRC_EN | \
-		    CCSR_SSI_SIER_RDMAE | CCSR_SSI_SIER_RIE | \
-		    CCSR_SSI_SIER_ROE0_EN | CCSR_SSI_SIER_ROE1_EN)
+		    CCSR_SSI_SIER_TUE1_EN)
+#define SIER_RFLAGS (CCSR_SSI_SIER_RFRC_EN | CCSR_SSI_SIER_RDMAE | \
+		    CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_ROE0_EN | \
+		    CCSR_SSI_SIER_ROE1_EN)
+
+#define SIER_FLAGS (SIER_TFLAGS | SIER_RFLAGS)
 
 /**
  * fsl_ssi_private: per-SSI private data
@@ -560,12 +563,12 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		if (ssi_private->use_dma)
-			sier_bits = SIER_FLAGS;
+			sier_bits = SIER_TFLAGS;
 		else
 			sier_bits = CCSR_SSI_SIER_TIE | CCSR_SSI_SIER_TFE0_EN;
 	} else {
 		if (ssi_private->use_dma)
-			sier_bits = SIER_FLAGS;
+			sier_bits = SIER_RFLAGS;
 		else
 			sier_bits = CCSR_SSI_SIER_RIE | CCSR_SSI_SIER_RFF0_EN;
 	}
@@ -579,6 +582,8 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 		else
 			write_ssi_mask(&ssi->scr, 0,
 				CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE);
+
+		write_ssi_mask(&ssi->sier, 0, sier_bits);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
@@ -591,14 +596,14 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 		if (!ssi_private->imx_ac97 && (read_ssi(&ssi->scr) &
 					(CCSR_SSI_SCR_TE | CCSR_SSI_SCR_RE)) == 0)
 			write_ssi_mask(&ssi->scr, CCSR_SSI_SCR_SSIEN, 0);
+
+		write_ssi_mask(&ssi->sier, sier_bits, 0);
 		break;
 
 	default:
 		return -EINVAL;
 	}
 
-	write_ssi(sier_bits, &ssi->sier);
-
 	return 0;
 }
 
-- 
1.8.4

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Nicolin Chen]

On Tue, Oct 29, 2013 at 06:59:44AM -0500, Timur Tabi wrote:
> Nicolin Chen wrote:
> >This patch enables Tx-related SIER_FLAGS only when direction is PLAYBACK
> >and does same thing for CAPTURE. Also, after TRIGGER_STOP/PAUSE, it will
> >disable SIER_xFLAGS for symmetric.
> 
> I'm okay with this patch in principle, but why bother?  The sysfs
> entry is going to display all interrupts anyway, and so the result
> will be the same.
>

Well, actually I just wanted to clear T/RDMAE to disable DMA request,
but it seems to be much easier to do it like this based on current
code and disabling unused interrupts should be better right? :)

Thank you,
Nicolin Chen

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Timur Tabi]

Nicolin Chen wrote:
> This patch enables Tx-related SIER_FLAGS only when direction is PLAYBACK
> and does same thing for CAPTURE. Also, after TRIGGER_STOP/PAUSE, it will
> disable SIER_xFLAGS for symmetric.

I'm okay with this patch in principle, but why bother?  The sysfs entry 
is going to display all interrupts anyway, and so the result will be the 
same.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Nicolin Chen]

On Tue, Oct 29, 2013 at 07:18:21AM -0500, Timur Tabi wrote:
> Nicolin Chen wrote:
> >Well, actually I just wanted to clear T/RDMAE to disable DMA request,
> >but it seems to be much easier to do it like this based on current
> >code and disabling unused interrupts should be better right?:)
> 
> It's not better if it complicates the code and has no real impact.
> The code has been running fine for years the way it is.  Unless you
> can show me that it actually makes a difference, I will have to NACK
> this patch.
>

The DMA request might be remaining high if not disabling it. This would
cause SDMA re-check this request, while it has no BD existing. For the
other interrupts, if you don't like it, I can drop it. Just modification
to the driver might not be less complicated.

Thank you,
Nicolin Chen

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Timur Tabi]

Nicolin Chen wrote:
> Well, actually I just wanted to clear T/RDMAE to disable DMA request,
> but it seems to be much easier to do it like this based on current
> code and disabling unused interrupts should be better right?:)

It's not better if it complicates the code and has no real impact.  The 
code has been running fine for years the way it is.  Unless you can show 
me that it actually makes a difference, I will have to NACK this patch.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Timur Tabi]

Nicolin Chen wrote:
> The DMA request might be remaining high if not disabling it.

Might?  Are you just guessing?

> This would
> cause SDMA re-check this request, while it has no BD existing. For the
> other interrupts, if you don't like it, I can drop it. Just modification
> to the driver might not be less complicated.

I'm only talking about this particular patch.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Chen Guangyu-B42378]

I mean there is a possibility.

 I'm sorry if my patch is kinda annoying and it really bother you, sir. I also want to make things better.

If you really don't like it, we can drop it. It's all your call.

And thank you for reviewing.

Sent by Android device.

Timur Tabi <timur@tabi.org> wrote:


Nicolin Chen wrote:
> The DMA request might be remaining high if not disabling it.

Might?  Are you just guessing?

> This would
> cause SDMA re-check this request, while it has no BD existing. For the
> other interrupts, if you don't like it, I can drop it. Just modification
> to the driver might not be less complicated.

I'm only talking about this particular patch.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Chen Guangyu-B42378]

I mean there is a possibility.

 I'm sorry if my patch is kinda annoying and it really bother you, sir. I a=
lso want to make things better.

If you really don't like it, we can drop it. It's all your call.

And thank you for reviewing.

Sent by Android device.

Timur Tabi <timur@tabi.org> wrote:


Nicolin Chen wrote:
> The DMA request might be remaining high if not disabling it.

Might?  Are you just guessing?

> This would
> cause SDMA re-check this request, while it has no BD existing. For the
> other interrupts, if you don't like it, I can drop it. Just modification
> to the driver might not be less complicated.

I'm only talking about this particular patch.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Timur Tabi]

Chen Guangyu-B42378 wrote:
> I mean there is a possibility.
>
>   I'm sorry if my patch is kinda annoying and it really bother you, sir. I also want to make things better.

It does not bother me.  I'm glad people are working on my driver.  I 
just want to make sure that my driver does not get bloated.

>
> If you really don't like it, we can drop it. It's all your call.

I think this patch should be dropped, because I don't see any real 
reason for it.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Chen Guangyu-B42378]

As you wish, sir.

Sent by Android device.

Timur Tabi <timur@tabi.org> wrote:


Chen Guangyu-B42378 wrote:
> I mean there is a possibility.
>
>   I'm sorry if my patch is kinda annoying and it really bother you, sir. I also want to make things better.

It does not bother me.  I'm glad people are working on my driver.  I
just want to make sure that my driver does not get bloated.

>
> If you really don't like it, we can drop it. It's all your call.

I think this patch should be dropped, because I don't see any real
reason for it.

Re: [PATCH] ASoC: fsl_ssi: separately enable and disable TIE/RIE in trigger()

[Chen Guangyu-B42378]

As you wish, sir.

Sent by Android device.

Timur Tabi <timur@tabi.org> wrote:


Chen Guangyu-B42378 wrote:
> I mean there is a possibility.
>
>   I'm sorry if my patch is kinda annoying and it really bother you, sir. =
I also want to make things better.

It does not bother me.  I'm glad people are working on my driver.  I
just want to make sure that my driver does not get bloated.

>
> If you really don't like it, we can drop it. It's all your call.

I think this patch should be dropped, because I don't see any real
reason for it.