[PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Olaya, Margarita]

From: Misael Lopez Cruz <x0052729@ti.com>

When the codec is powered-up through external AUDPWRON line it starts
its power-up sequence. The completion of the sequence is signaled
through the audio interrupt, and then codec is operational.

If NAUDINT irq is provided, CODEC driver starts a wait_for_completion
just after AUDPWRON line transitions from low to high. It's signaled as
complete when servicing READYINT interrupt.

If AUDPWRON gpio line is provided but NAUDINT irq is not, then CODEC
driver enables READYINT and polls on INTID register. If none of them are
provided, then CODEC uses manual power sequences and disables all audio
interrupts.

Signed-off-by: Misael Lopez Cruz <x0052729@ti.com>
Signed-off-by: Jorge Eduardo Candelaria <jorge.candelaria@ti.com>
Signed-off-by: Margarita Olaya Cabrera <magi.olaya@ti.com>
---
 sound/soc/codecs/twl6030.c |   61 +++++++++++++++++++++++++++++++++++++------
 sound/soc/codecs/twl6030.h |    1 +
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/twl6030.c b/sound/soc/codecs/twl6030.c
index b8dd5ae..2847f1b 100644
--- a/sound/soc/codecs/twl6030.c
+++ b/sound/soc/codecs/twl6030.c
@@ -52,6 +52,7 @@ struct twl6030_data {
 	int non_lp;
 	unsigned int sysclk;
 	struct snd_pcm_hw_constraint_list *sysclk_constraints;
+	struct completion ready;
 };
 
 /*
@@ -372,6 +373,7 @@ static int twl6030_power_mode_event(struct snd_soc_dapm_widget *w,
 static irqreturn_t twl6030_naudint_handler(int irq, void *data)
 {
 	struct snd_soc_codec *codec = data;
+	struct twl6030_data *priv = codec->private_data;
 	u8 intid;
 
 	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid, TWL6030_REG_INTID);
@@ -391,7 +393,7 @@ static irqreturn_t twl6030_naudint_handler(int irq, void *data)
 		dev_alert(codec->dev, "vib drivers over current detection\n");
 		break;
 	case TWL6030_READYINT:
-		dev_alert(codec->dev, "codec is ready\n");
+		complete(&priv->ready);
 		break;
 	default:
 		dev_err(codec->dev, "unknown audio interrupt %d\n", intid);
@@ -626,11 +628,45 @@ static int twl6030_add_widgets(struct snd_soc_codec *codec)
 	return 0;
 }
 
+static int twl6030_power_up_completion(struct snd_soc_codec *codec,
+					int naudint)
+{
+	struct twl6030_data *priv = codec->private_data;
+	int time_left;
+	u8 intid;
+
+	if (naudint) {
+		/* wait for ready interrupt with 48 ms timeout */
+		time_left = wait_for_completion_timeout(&priv->ready,
+					msecs_to_jiffies(48));
+	} else {
+		/* retry 3 times only */
+		for (time_left = 3; time_left > 0; time_left--) {
+			mdelay(16);
+			twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
+					TWL6030_REG_INTID);
+			if (intid & TWL6030_READYINT)
+				break;
+		}
+	}
+
+	if (!time_left) {
+		dev_err(codec->dev, "timeout waiting for READYINT\n");
+		return -ETIMEDOUT;
+	}
+
+	priv->codec_powered = 1;
+
+	return 0;
+}
+
 static int twl6030_set_bias_level(struct snd_soc_codec *codec,
 				enum snd_soc_bias_level level)
 {
 	struct twl6030_data *priv = codec->private_data;
 	int audpwron = priv->audpwron;
+	int naudint = priv->naudint;
+	int ret;
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
@@ -643,8 +679,10 @@ static int twl6030_set_bias_level(struct snd_soc_codec *codec,
 			/* use AUDPWRON line */
 			gpio_set_value(audpwron, 1);
 
-			/* power-up sequence latency */
-			mdelay(16);
+			/* wait for power-up completion */
+			ret = twl6030_power_up_completion(codec, naudint);
+			if (ret)
+				return ret;
 
 			/* sync registers updated during power-up sequence */
 			twl6030_read(codec, TWL6030_REG_NCPCTL);
@@ -653,12 +691,11 @@ static int twl6030_set_bias_level(struct snd_soc_codec *codec,
 		} else {
 			/* use manual power-up sequence */
 			twl6030_power_up(codec);
+			priv->codec_powered = 1;
 		}
 
 		/* initialize vdd/vss registers with reg_cache */
 		twl6030_init_vdd_regs(codec);
-
-		priv->codec_powered = 1;
 		break;
 	case SND_SOC_BIAS_OFF:
 		if (!priv->codec_powered)
@@ -1067,6 +1104,7 @@ static int __devinit twl6030_codec_probe(struct platform_device *pdev)
 	mutex_init(&codec->mutex);
 	INIT_LIST_HEAD(&codec->dapm_widgets);
 	INIT_LIST_HEAD(&codec->dapm_paths);
+	init_completion(&priv->ready);
 
 	if (gpio_is_valid(audpwron)) {
 		ret = gpio_request(audpwron, "audpwron");
@@ -1089,10 +1127,15 @@ static int __devinit twl6030_codec_probe(struct platform_device *pdev)
 		if (ret)
 			goto gpio2_err;
 	} else {
-		dev_warn(codec->dev,
-			"no naudint irq, audio interrupts disabled\n");
-		twl6030_write_reg_cache(codec, TWL6030_REG_INTMR,
-					TWL6030_ALLINT_MSK);
+		if (gpio_is_valid(audpwron)) {
+			/* enable only codec ready interrupt */
+			twl6030_write_reg_cache(codec, TWL6030_REG_INTMR,
+					~TWL6030_READYMSK & TWL6030_ALLINT_MSK);
+		} else {
+			/* no interrupts at all */
+			twl6030_write_reg_cache(codec, TWL6030_REG_INTMR,
+						TWL6030_ALLINT_MSK);
+		}
 	}
 
 	/* init vio registers */
diff --git a/sound/soc/codecs/twl6030.h b/sound/soc/codecs/twl6030.h
index 90b8a44..d591be1 100644
--- a/sound/soc/codecs/twl6030.h
+++ b/sound/soc/codecs/twl6030.h
@@ -79,6 +79,7 @@
 
 /* INTMR (0x04) fields */
 
+#define TWL6030_READYMSK		0x40
 #define TWL6030_ALLINT_MSK		0x7B
 
 /* NCPCTL (0x05) fields */
-- 
1.6.1.3

Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Mark Brown]

On Tue, Feb 23, 2010 at 06:10:54PM -0600, Olaya, Margarita wrote:

> +	if (naudint) {
> +		/* wait for ready interrupt with 48 ms timeout */
> +		time_left = wait_for_completion_timeout(&priv->ready,
> +					msecs_to_jiffies(48));
> +	} else {
> +		/* retry 3 times only */
> +		for (time_left = 3; time_left > 0; time_left--) {
> +			mdelay(16);
> +			twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
> +					TWL6030_REG_INTID);
> +			if (intid & TWL6030_READYINT)
> +				break;
> +		}
> +	}

It strikes me that you could combine these two cases - the
wait_for_completion_timeout() will function just as well as a delay.
I'd also expect to see an error reported if the device doesn't report as
ready one way or another.

RE: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Olaya, Margarita]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Wednesday, February 24, 2010 7:59 AM
> To: Olaya, Margarita
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org; lrg@slimlogic.co.uk
> Subject: Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion
> 
> On Tue, Feb 23, 2010 at 06:10:54PM -0600, Olaya, Margarita wrote:
> 
> > +	if (naudint) {
> > +		/* wait for ready interrupt with 48 ms timeout */
> > +		time_left = wait_for_completion_timeout(&priv->ready,
> > +					msecs_to_jiffies(48));
> > +	} else {
> > +		/* retry 3 times only */
> > +		for (time_left = 3; time_left > 0; time_left--) {
> > +			mdelay(16);
> > +			twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
> > +					TWL6030_REG_INTID);
> > +			if (intid & TWL6030_READYINT)
> > +				break;
> > +		}
> > +	}
> 
> It strikes me that you could combine these two cases - the
> wait_for_completion_timeout() will function just as well as a delay.
> I'd also expect to see an error reported if the device doesn't report as
> ready one way or another.

It is split to prevent the case of none valid irq line connected, in such case, wait_for_completion won't work.

Regards,
Margarita 

Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Mark Brown]

On 25 Feb 2010, at 00:24, "Olaya, Margarita" <magi.olaya@ti.com> wrote:

>
>
>> -----Original Message-----
>> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
>> Sent: Wednesday, February 24, 2010 7:59 AM
>> To: Olaya, Margarita
>> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org; lrg@slimlogic.co.uk
>> Subject: Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence  
>> completion
>>
>> On Tue, Feb 23, 2010 at 06:10:54PM -0600, Olaya, Margarita wrote:
>>
>>> +    if (naudint) {
>>> +        /* wait for ready interrupt with 48 ms timeout */
>>> +        time_left = wait_for_completion_timeout(&priv->ready,
>>> +                    msecs_to_jiffies(48));
>>> +    } else {
>>> +        /* retry 3 times only */
>>> +        for (time_left = 3; time_left > 0; time_left--) {
>>> +            mdelay(16);
>>> +            twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
>>> +                    TWL6030_REG_INTID);
>>> +            if (intid & TWL6030_READYINT)
>>> +                break;
>>> +        }
>>> +    }
>>
>> It strikes me that you could combine these two cases - the
>> wait_for_completion_timeout() will function just as well as a delay.
>> I'd also expect to see an error reported if the device doesn't  
>> report as
>> ready one way or another.
>
> It is split to prevent the case of none valid irq line connected, in  
> such case, wait_for_completion won't work

It will - you can specify a timeout so if the interrupt doesn't happen  
all that happens is that you delay for the specified timeout.

>

Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Olaya, Margarita]

On Thursday, February 25, 2010 2:42 AM  Mark Brown wrote: 

>>> On Tue, Feb 23, 2010 at 06:10:54PM -0600, Olaya, Margarita wrote:
>>> 
>>>> +    if (naudint) {
>>>> +        /* wait for ready interrupt with 48 ms timeout */
>>>> +        time_left = wait_for_completion_timeout(&priv->ready,
>>>> +                    msecs_to_jiffies(48));

Phoenix manages automatic and manual power on sequences. READYINT
indicates the completion of power up sequence, Phoenix audio drives
the NAUDINT line low when an interrupt is internally detected, when
automatic power on sequence is used, the state of READYINIT is verified
through the interrupt handler using wait_for_completion.

>>>> +    } else {
>>>> +        /* retry 3 times only */
>>>> +        for (time_left = 3; time_left > 0; time_left--) { +    
>>>> mdelay(16); +           
>>>> twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid, +           
>>>> TWL6030_REG_INTID); +            if (intid & TWL6030_READYINT)
>>>> +                break;
>>>> +        }

When manual power on sequence is used the driver verifies the status of 
READYINIT by polling.

In both cases if READYINIT is not set before the timeout runs out it means
the codec is not powering on and the driver reports an error.

- Margarita

>>>> +    }
>>> 



>>> It strikes me that you could combine these two cases - the
>>> wait_for_completion_timeout() will function just as well as a
>>> delay. I'd also expect to see an error reported if the device
>>> doesn't report as ready one way or another.
>> 
>> It is split to prevent the case of none valid irq line connected,
>> in such case, wait_for_completion won't work
> 
> It will - you can specify a timeout so if the interrupt doesn't
> happen all that happens is that you delay for the specified timeout.

Re: [alsa-devel] [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Mark Brown]

On Fri, Feb 26, 2010 at 03:04:22PM -0600, Olaya, Margarita wrote:

> In both cases if READYINIT is not set before the timeout runs out it means
> the codec is not powering on and the driver reports an error.

That's kind of my point - because you're checking for the same status
there shouldn't be any need to special case the situation where there's
no interrupt, I'd expect to be able to use wait_for_timeout_interruptible()
for the delay and have the timeout completion interrupt cause that to be
signalled.  No *terribly* important but it makes it clearer what's going
on.

RE: [alsa-devel] [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Olaya, Margarita]

On Friday, February 26, 2010 3:27 PM  Mark Brown wrote: 

> On Fri, Feb 26, 2010 at 03:04:22PM -0600, Olaya, Margarita wrote:
> 
>> In both cases if READYINIT is not set before the timeout runs out
>> it means the codec is not powering on and the driver reports an
>> error. 
> 
> That's kind of my point - because you're checking for the same
> status there shouldn't be any need to special case the situation
> where there's
> no interrupt, I'd expect to be able to use
> wait_for_timeout_interruptible()
> for the delay and have the timeout completion interrupt cause
> that to be
> signalled.  No *terribly* important but it makes it clearer
> what's going
> on.

Do you mean something like this?
time_left = wait_for_completion_timeout(&priv->ready,
                                        msecs_to_jiffies(48));
if(!time_left) {
	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
                                        TWL6040_REG_INTID);
      if (!(intid & TWL6040_READYINT))
		goto error;
}

	return 0;

error:
	dev_err(codec->dev, "timeout waiting for READYINT\n");
	return -ETIMEDOUT;

but in this case will it not take unnecessarily 48ms when the
interruption line is not valid?

Regards,
Margarita

Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Mark Brown]

On Fri, Feb 26, 2010 at 06:22:34PM -0600, Olaya, Margarita wrote:

> Do you mean something like this?
> time_left = wait_for_completion_timeout(&priv->ready,
>                                         msecs_to_jiffies(48));
> if(!time_left) {
> 	twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE, &intid,
>                                         TWL6040_REG_INTID);
>       if (!(intid & TWL6040_READYINT))
> 		goto error;
> }
> 
> 	return 0;
> 
> error:
> 	dev_err(codec->dev, "timeout waiting for READYINT\n");
> 	return -ETIMEDOUT;

Yes, or wrapped in a for loop with shorter timeouts on the individual
waits.

> but in this case will it not take unnecessarily 48ms when the
> interruption line is not valid?

You're always going to get some additional delay when polling unless you
busy wait for completion, which obviously has its own problems.

Re: [PATCHv4 7/7] ASoC: TWL6030: Detect power-up sequence completion

[Olaya, Margarita]

On Monday, March 01, 2010 6:14 AM  Mark Brown wrote: 

> On Fri, Feb 26, 2010 at 06:22:34PM -0600, Olaya, Margarita wrote:
> 
>> Do you mean something like this?
>> time_left = wait_for_completion_timeout(&priv->ready,
>>                                         msecs_to_jiffies(48));
>> 	if(!time_left) { twl_i2c_read_u8(TWL4030_MODULE_AUDIO_VOICE,
>>                                         &intid, TWL6040_REG_INTID);
>>       if (!(intid & TWL6040_READYINT))
>> 		goto error;
>> }
>> 
>> 	return 0;
>> 
>> error:
>> 	dev_err(codec->dev, "timeout waiting for READYINT\n");
>> 	return -ETIMEDOUT;
> 
> Yes, or wrapped in a for loop with shorter timeouts on the
> individual waits. 
> 
>> but in this case will it not take unnecessarily 48ms when the
>> interruption line is not valid?
> 
> You're always going to get some additional delay when polling
> unless you
> busy wait for completion, which obviously has its own problems.

Ok, thanks for the comment I'll re-write the loop for next version of
patches