[PATCH 0/2] ASoC: tlv320dac33 fixes

[PATCH 0/2] ASoC: tlv320dac33 fixes

[Peter Ujfalusi]

Hello,

Revisiting the start/stop sequence used by the tlv320dac33 codec in
order to avoid race conditions, which can occur, when the codec is in
FIFO modes.
It has been found that OMAP3 McBSP needs the BCLK continuously running
on the serial interface when it is configured to be slave.
DAC33 can be configured in a way, that it keeps the BCLK ticking, and
only enable the FS when it is needed.
Option for the platform data to select between continuous BCLK or non
continuous BCLK.

---
Peter Ujfalusi (2):
  ASoC: tlv320dac33: Start/stop sequence change
  ASoC: tlv320dac33: Add option for keeping the BCLK running

 include/sound/tlv320dac33-plat.h |    1 +
 sound/soc/codecs/tlv320dac33.c   |   38 ++++++++++++++------------------------
 2 files changed, 15 insertions(+), 24 deletions(-)

[PATCH 1/2] ASoC: tlv320dac33: Start/stop sequence change

[Peter Ujfalusi]

To avoid race condition especially in FIFO modes the
sequence for enabling and disabling the codec need to
be changed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/tlv320dac33.c |   25 +++----------------------
 1 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c
index f9f367d..e845c4b 100644
--- a/sound/soc/codecs/tlv320dac33.c
+++ b/sound/soc/codecs/tlv320dac33.c
@@ -310,7 +310,8 @@ static inline void dac33_soft_power(struct snd_soc_codec *codec, int power)
 	if (power)
 		reg |= DAC33_PDNALLB;
 	else
-		reg &= ~DAC33_PDNALLB;
+		reg &= ~(DAC33_PDNALLB | DAC33_OSCPDNB |
+			 DAC33_DACRPDNB | DAC33_DACLPDNB);
 	dac33_write(codec, DAC33_PWR_CTRL, reg);
 }
 
@@ -634,26 +635,6 @@ static irqreturn_t dac33_interrupt_handler(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static void dac33_shutdown(struct snd_pcm_substream *substream,
-			     struct snd_soc_dai *dai)
-{
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_device *socdev = rtd->socdev;
-	struct snd_soc_codec *codec = socdev->card->codec;
-	struct tlv320dac33_priv *dac33 = codec->private_data;
-	unsigned int pwr_ctrl;
-
-	/* Stop pending workqueue */
-	if (dac33->fifo_mode)
-		cancel_work_sync(&dac33->work);
-
-	mutex_lock(&dac33->mutex);
-	pwr_ctrl = dac33_read_reg_cache(codec, DAC33_PWR_CTRL);
-	pwr_ctrl &= ~(DAC33_OSCPDNB | DAC33_DACRPDNB | DAC33_DACLPDNB);
-	dac33_write(codec, DAC33_PWR_CTRL, pwr_ctrl);
-	mutex_unlock(&dac33->mutex);
-}
-
 static void dac33_oscwait(struct snd_soc_codec *codec)
 {
 	int timeout = 20;
@@ -751,6 +732,7 @@ static int dac33_prepare_chip(struct snd_pcm_substream *substream)
 	}
 
 	mutex_lock(&dac33->mutex);
+	dac33_soft_power(codec, 0);
 	dac33_soft_power(codec, 1);
 
 	reg_tmp = dac33_read_reg_cache(codec, DAC33_INT_OSC_CTRL);
@@ -1185,7 +1167,6 @@ EXPORT_SYMBOL_GPL(soc_codec_dev_tlv320dac33);
 #define DAC33_FORMATS	SNDRV_PCM_FMTBIT_S16_LE
 
 static struct snd_soc_dai_ops dac33_dai_ops = {
-	.shutdown	= dac33_shutdown,
 	.hw_params	= dac33_hw_params,
 	.prepare	= dac33_pcm_prepare,
 	.trigger	= dac33_pcm_trigger,
-- 
1.7.0.2

[PATCH 2/2] ASoC: tlv320dac33: Add option for keeping the BCLK running

[Peter Ujfalusi]

Platform data option for the codec to keep the BCLK clock
continuously running in FIFO modes (codec master).

OMAP3 McBSP when in slave mode needs continuous BCLK running
on the serial bus in order to operate correctly.

Since in FIFO mode the DAC33 can also shut down the BCLK clock
and enable it only when it is needed, let the platforms decide
if the CPU side needs the BCLK running or not.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 include/sound/tlv320dac33-plat.h |    1 +
 sound/soc/codecs/tlv320dac33.c   |   13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/sound/tlv320dac33-plat.h b/include/sound/tlv320dac33-plat.h
index ac06652..3f428d5 100644
--- a/include/sound/tlv320dac33-plat.h
+++ b/include/sound/tlv320dac33-plat.h
@@ -15,6 +15,7 @@
 
 struct tlv320dac33_platform_data {
 	int power_gpio;
+	int keep_bclk;	/* Keep the BCLK running in FIFO modes */
 	u8 burst_bclkdiv;
 };
 
diff --git a/sound/soc/codecs/tlv320dac33.c b/sound/soc/codecs/tlv320dac33.c
index e845c4b..a6f1927 100644
--- a/sound/soc/codecs/tlv320dac33.c
+++ b/sound/soc/codecs/tlv320dac33.c
@@ -93,6 +93,8 @@ struct tlv320dac33_priv {
 	unsigned int nsample;		/* burst read amount from host */
 	u8 burst_bclkdiv;		/* BCLK divider value in burst mode */
 
+	int keep_bclk;			/* Keep the BCLK continuously running
+					 * in FIFO modes */
 	enum dac33_state state;
 };
 
@@ -803,7 +805,10 @@ static int dac33_prepare_chip(struct snd_pcm_substream *substream)
 		 */
 		fifoctrl_a &= ~DAC33_FBYPAS;
 		fifoctrl_a &= ~DAC33_FAUTO;
-		aictrl_b &= ~DAC33_BCLKON;
+		if (dac33->keep_bclk)
+			aictrl_b |= DAC33_BCLKON;
+		else
+			aictrl_b &= ~DAC33_BCLKON;
 		break;
 	case DAC33_FIFO_MODE7:
 		/*
@@ -814,7 +819,10 @@ static int dac33_prepare_chip(struct snd_pcm_substream *substream)
 		 */
 		fifoctrl_a &= ~DAC33_FBYPAS;
 		fifoctrl_a |= DAC33_FAUTO;
-		aictrl_b &= ~DAC33_BCLKON;
+		if (dac33->keep_bclk)
+			aictrl_b |= DAC33_BCLKON;
+		else
+			aictrl_b &= ~DAC33_BCLKON;
 		break;
 	default:
 		/*
@@ -1234,6 +1242,7 @@ static int __devinit dac33_i2c_probe(struct i2c_client *client,
 
 	dac33->power_gpio = pdata->power_gpio;
 	dac33->burst_bclkdiv = pdata->burst_bclkdiv;
+	dac33->keep_bclk = pdata->keep_bclk;
 	dac33->irq = client->irq;
 	dac33->nsample = NSAMPLE_MAX;
 	/* Disable FIFO use by default */
-- 
1.7.0.2

Re: [PATCH 0/2] ASoC: tlv320dac33 fixes

[Jarkko Nikula]

On Thu, 11 Mar 2010 16:26:20 +0200
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> It has been found that OMAP3 McBSP needs the BCLK continuously running
> on the serial interface when it is configured to be slave.
> 
Just curious: what would happen if the BCLK is cut while the McBSP is
operating? I'm thinking are there also other similar problems, e.g. if
the rate is not correct.


-- 
Jarkko

Re: [PATCH 0/2] ASoC: tlv320dac33 fixes

[Peter Ujfalusi]

On Thursday 11 March 2010 16:46:28 ext Jarkko Nikula wrote:
> On Thu, 11 Mar 2010 16:26:20 +0200
> 
> Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:
> > It has been found that OMAP3 McBSP needs the BCLK continuously running
> > on the serial interface when it is configured to be slave.
> 
> Just curious: what would happen if the BCLK is cut while the McBSP is
> operating?

The symptom is that we can not access to McBSP register address space causing 
kernel panic, which can only be fixed by rebooting the device.
The burst driven BCLK causes some internal state machine to stuck, leaving the 
given McBSP port dead from the outside. Other ports operate after this event.
Obviously this only bites in McBSP slave mode, and with codec like DAC33 which 
have burst mode, in other cases the BCLK is always running (or McBSP is master).

> I'm thinking are there also other similar problems, e.g. if
> the rate is not correct.

Hmmm, could be possible, but I did not experienced with such a problem.

-- 
Péter

Re: [PATCH 0/2] ASoC: tlv320dac33 fixes

[Jarkko Nikula]

On Thu, 11 Mar 2010 16:55:12 +0200
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> > Just curious: what would happen if the BCLK is cut while the McBSP is
> > operating?
> 
> The symptom is that we can not access to McBSP register address space causing 
> kernel panic, which can only be fixed by rebooting the device.
> The burst driven BCLK causes some internal state machine to stuck, leaving the 
> given McBSP port dead from the outside. Other ports operate after this event.
> Obviously this only bites in McBSP slave mode, and with codec like DAC33 which 
> have burst mode, in other cases the BCLK is always running (or McBSP is master).
> 
> > I'm thinking are there also other similar problems, e.g. if
> > the rate is not correct.
> 
> Hmmm, could be possible, but I did not experienced with such a problem.
> 
Thanks for sharing this info.

Sounds exactly similar problem what I encountered once with the OMAP2420
and EAC. I didn't debug that problem any further then but there also
some register accesses (not all) caused kernel panic if the external
clock was missing or if the rate wasn't correct.


-- 
Jarkko

Re: [PATCH 1/2] ASoC: tlv320dac33: Start/stop sequence change

[Liam Girdwood]

On Thu, 2010-03-11 at 16:26 +0200, Peter Ujfalusi wrote:
> To avoid race condition especially in FIFO modes the
> sequence for enabling and disabling the codec need to
> be changed.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 2/2] ASoC: tlv320dac33: Add option for keeping the BCLK running

[Liam Girdwood]

On Thu, 2010-03-11 at 16:26 +0200, Peter Ujfalusi wrote:
> Platform data option for the codec to keep the BCLK clock
> continuously running in FIFO modes (codec master).
> 
> OMAP3 McBSP when in slave mode needs continuous BCLK running
> on the serial bus in order to operate correctly.
> 
> Since in FIFO mode the DAC33 can also shut down the BCLK clock
> and enable it only when it is needed, let the platforms decide
> if the CPU side needs the BCLK running or not.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 0/2] ASoC: tlv320dac33 fixes

[Mark Brown]

On Thu, Mar 11, 2010 at 04:26:20PM +0200, Peter Ujfalusi wrote:

> Revisiting the start/stop sequence used by the tlv320dac33 codec in
> order to avoid race conditions, which can occur, when the codec is in
> FIFO modes.

Applied, thanks.

> It has been found that OMAP3 McBSP needs the BCLK continuously running
> on the serial interface when it is configured to be slave.
> DAC33 can be configured in a way, that it keeps the BCLK ticking, and
> only enable the FS when it is needed.

I suspect there's some cause and effect going on in the CODEC having
this feature :/