[PATCH 0/4] ASoC: davinci-mcasp: Fix master mode capture (w/o playback)

[PATCH 0/4] ASoC: davinci-mcasp: Fix master mode capture (w/o playback)

[Peter Ujfalusi]

Hi,

When McASP is master the TX clocks are used for capture clocking as well. Other
parts of the code already does the configuration for both tx and rx - for
different bits in xxFMT/xxFMCT registers we should do the same in other places.

Along with this fix I have added three cleanup patch which came along when I was
debugging the issue.

Regards,
Peter
---
Peter Ujfalusi (4):
  ASoC: davinci-mcasp: Harmonize the sub hw_params function names
  ASoC: davinci-mcasp: Configure xxTDM, xxFMT and xxFMCT registers
    synchronously
  ASoC: davinci-mcasp: Return value handling cleanup for
    mcasp_common_hw_param()
  ASoC: davinci-mcasp: Code cleanup in davinci_mcasp_hw_params()

 sound/soc/davinci/davinci-mcasp.c | 81 +++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 42 deletions(-)

-- 
1.8.5.3

[PATCH 1/4] ASoC: davinci-mcasp: Harmonize the sub hw_params function names

[Peter Ujfalusi]

Instead of
davinci_hw_common_param - for common, I2S/DIT mode settings
davinci_hw_dit_param - for DIT protocol configuration
davinci_hw_param - for I2S (and compatible protocols)

Use the following names:
mcasp_common_hw_param, mcasp_dit_hw_param and mcasp_i2s_hw_param.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index b7858bfa0295..9ec456aaf80b 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -448,7 +448,7 @@ static int davinci_config_channel_size(struct davinci_mcasp *mcasp,
 	return 0;
 }
 
-static int davinci_hw_common_param(struct davinci_mcasp *mcasp, int stream,
+static int mcasp_common_hw_param(struct davinci_mcasp *mcasp, int stream,
 				    int channels)
 {
 	int i;
@@ -524,7 +524,7 @@ static int davinci_hw_common_param(struct davinci_mcasp *mcasp, int stream,
 	return 0;
 }
 
-static void davinci_hw_param(struct davinci_mcasp *mcasp, int stream)
+static void mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 {
 	int i, active_slots;
 	u32 mask = 0;
@@ -567,7 +567,7 @@ static void davinci_hw_param(struct davinci_mcasp *mcasp, int stream)
 }
 
 /* S/PDIF */
-static void davinci_hw_dit_param(struct davinci_mcasp *mcasp)
+static void mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 {
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
@@ -611,7 +611,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 
 	active_serializers = (channels + slots - 1) / slots;
 
-	if (davinci_hw_common_param(mcasp, substream->stream, channels) == -EINVAL)
+	if (mcasp_common_hw_param(mcasp, substream->stream, channels) == -EINVAL)
 		return -EINVAL;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		fifo_level = mcasp->txnumevt * active_serializers;
@@ -619,9 +619,9 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		fifo_level = mcasp->rxnumevt * active_serializers;
 
 	if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE)
-		davinci_hw_dit_param(mcasp);
+		mcasp_dit_hw_param(mcasp);
 	else
-		davinci_hw_param(mcasp, substream->stream);
+		mcasp_i2s_hw_param(mcasp, substream->stream);
 
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_U8:
-- 
1.8.5.3

[PATCH 2/4] ASoC: davinci-mcasp: Configure xxTDM, xxFMT and xxFMCT registers synchronously

[Peter Ujfalusi]

These registers can be configured synchronously for playback and capture.
Furthermore when McASP is in master and sync mode the capture operation
needs the TX path to be configured in order to be able to provide the needed
clocks for the bus.
xxFMT and xxFMCT registers has been already configured for both TX and RX
other places in the driver.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 56 +++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 9ec456aaf80b..ae3e40a63e5e 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -524,12 +524,18 @@ static int mcasp_common_hw_param(struct davinci_mcasp *mcasp, int stream,
 	return 0;
 }
 
-static void mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
+static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 {
 	int i, active_slots;
 	u32 mask = 0;
 	u32 busel = 0;
 
+	if ((mcasp->tdm_slots < 2) || (mcasp->tdm_slots > 32)) {
+		dev_err(mcasp->dev, "tdm slot %d not supported\n",
+			mcasp->tdm_slots);
+		return -EINVAL;
+	}
+
 	active_slots = (mcasp->tdm_slots > 31) ? 32 : mcasp->tdm_slots;
 	for (i = 0; i < active_slots; i++)
 		mask |= (1 << i);
@@ -539,35 +545,21 @@ static void mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 	if (!mcasp->dat_port)
 		busel = TXSEL;
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		/* bit stream is MSB first  with no delay */
-		/* DSP_B mode */
-		mcasp_set_reg(mcasp, DAVINCI_MCASP_TXTDM_REG, mask);
-		mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, busel | TXORD);
-
-		if ((mcasp->tdm_slots >= 2) && (mcasp->tdm_slots <= 32))
-			mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG,
-				       FSXMOD(mcasp->tdm_slots), FSXMOD(0x1FF));
-		else
-			printk(KERN_ERR "playback tdm slot %d not supported\n",
-				mcasp->tdm_slots);
-	} else {
-		/* bit stream is MSB first with no delay */
-		/* DSP_B mode */
-		mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, busel | RXORD);
-		mcasp_set_reg(mcasp, DAVINCI_MCASP_RXTDM_REG, mask);
-
-		if ((mcasp->tdm_slots >= 2) && (mcasp->tdm_slots <= 32))
-			mcasp_mod_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG,
-				       FSRMOD(mcasp->tdm_slots), FSRMOD(0x1FF));
-		else
-			printk(KERN_ERR "capture tdm slot %d not supported\n",
-				mcasp->tdm_slots);
-	}
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_TXTDM_REG, mask);
+	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, busel | TXORD);
+	mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG,
+		       FSXMOD(mcasp->tdm_slots), FSXMOD(0x1FF));
+
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_RXTDM_REG, mask);
+	mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, busel | RXORD);
+	mcasp_mod_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG,
+		       FSRMOD(mcasp->tdm_slots), FSRMOD(0x1FF));
+
+	return 0;
 }
 
 /* S/PDIF */
-static void mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
+static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 {
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
@@ -589,6 +581,8 @@ static void mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 
 	/* Enable the DIT */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
+
+	return 0;
 }
 
 static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
@@ -605,6 +599,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 	u8 slots = mcasp->tdm_slots;
 	u8 active_serializers;
 	int channels;
+	int ret;
 	struct snd_interval *pcm_channels = hw_param_interval(params,
 					SNDRV_PCM_HW_PARAM_CHANNELS);
 	channels = pcm_channels->min;
@@ -619,9 +614,12 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		fifo_level = mcasp->rxnumevt * active_serializers;
 
 	if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE)
-		mcasp_dit_hw_param(mcasp);
+		ret = mcasp_dit_hw_param(mcasp);
 	else
-		mcasp_i2s_hw_param(mcasp, substream->stream);
+		ret = mcasp_i2s_hw_param(mcasp, substream->stream);
+
+	if (ret)
+		return ret;
 
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_U8:
-- 
1.8.5.3

[PATCH 3/4] ASoC: davinci-mcasp: Return value handling cleanup for mcasp_common_hw_param()

[Peter Ujfalusi]

Take the return value from mcasp_common_hw_param() and use that in case of
error.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index ae3e40a63e5e..c1c1ac8945d4 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -606,8 +606,10 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 
 	active_serializers = (channels + slots - 1) / slots;
 
-	if (mcasp_common_hw_param(mcasp, substream->stream, channels) == -EINVAL)
-		return -EINVAL;
+	ret = mcasp_common_hw_param(mcasp, substream->stream, channels);
+	if (ret)
+		return ret;
+
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		fifo_level = mcasp->txnumevt * active_serializers;
 	else
-- 
1.8.5.3

[PATCH 4/4] ASoC: davinci-mcasp: Code cleanup in davinci_mcasp_hw_params()

[Peter Ujfalusi]

Rearrange the code in the function for readability.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index c1c1ac8945d4..48079dbad767 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -598,23 +598,13 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 	u8 fifo_level;
 	u8 slots = mcasp->tdm_slots;
 	u8 active_serializers;
-	int channels;
+	int channels = params_channels(params);
 	int ret;
-	struct snd_interval *pcm_channels = hw_param_interval(params,
-					SNDRV_PCM_HW_PARAM_CHANNELS);
-	channels = pcm_channels->min;
-
-	active_serializers = (channels + slots - 1) / slots;
 
 	ret = mcasp_common_hw_param(mcasp, substream->stream, channels);
 	if (ret)
 		return ret;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		fifo_level = mcasp->txnumevt * active_serializers;
-	else
-		fifo_level = mcasp->rxnumevt * active_serializers;
-
 	if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE)
 		ret = mcasp_dit_hw_param(mcasp);
 	else
@@ -655,6 +645,13 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	/* Calculate FIFO level */
+	active_serializers = (channels + slots - 1) / slots;
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		fifo_level = mcasp->txnumevt * active_serializers;
+	else
+		fifo_level = mcasp->rxnumevt * active_serializers;
+
 	if (mcasp->version == MCASP_VERSION_2 && !fifo_level)
 		dma_params->acnt = 4;
 	else
-- 
1.8.5.3

Re: [PATCH 1/4] ASoC: davinci-mcasp: Harmonize the sub hw_params function names

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 401 bytes --]

On Thu, Jan 30, 2014 at 03:15:22PM +0200, Peter Ujfalusi wrote:
> Instead of
> davinci_hw_common_param - for common, I2S/DIT mode settings
> davinci_hw_dit_param - for DIT protocol configuration
> davinci_hw_param - for I2S (and compatible protocols)

I've applied this but please don't put stylistic cleanups like this
before bugfixes in patch serieses - it creates a barrier to applying the
bugfix.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/4] ASoC: davinci-mcasp: Configure xxTDM, xxFMT and xxFMCT registers synchronously

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

On Thu, Jan 30, 2014 at 03:15:23PM +0200, Peter Ujfalusi wrote:
> These registers can be configured synchronously for playback and capture.
> Furthermore when McASP is in master and sync mode the capture operation
> needs the TX path to be configured in order to be able to provide the needed
> clocks for the bus.
> xxFMT and xxFMCT registers has been already configured for both TX and RX
> other places in the driver.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/4] ASoC: davinci-mcasp: Return value handling cleanup for mcasp_common_hw_param()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 168 bytes --]

On Thu, Jan 30, 2014 at 03:15:24PM +0200, Peter Ujfalusi wrote:
> Take the return value from mcasp_common_hw_param() and use that in case of
> error.

Applied, thanks.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 4/4] ASoC: davinci-mcasp: Code cleanup in davinci_mcasp_hw_params()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 273 bytes --]

On Thu, Jan 30, 2014 at 03:15:25PM +0200, Peter Ujfalusi wrote:
> Rearrange the code in the function for readability.

This doesn't apply cleanly on my topic/davinci, presumably because of
Jiri's recent work though I didn't look too hard.  Can you please check
and resend?

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 4/4] ASoC: davinci-mcasp: Code cleanup in davinci_mcasp_hw_params()

[Peter Ujfalusi]

On 01/31/2014 06:11 PM, Mark Brown wrote:
> On Thu, Jan 30, 2014 at 03:15:25PM +0200, Peter Ujfalusi wrote:
>> Rearrange the code in the function for readability.
> 
> This doesn't apply cleanly on my topic/davinci, presumably because of
> Jiri's recent work though I didn't look too hard.  Can you please check
> and resend?

Yes, I will. While I'm at this I will add two more cleanup patches.

-- 
Péter