[PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

This patch is to fix interrupt fails when resume back.
In solution, we add IRQ re-initiation,
and reconstruct suspend and resume with set_bias_level function.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 504c969..2e2e11a 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -1092,6 +1092,36 @@ static int nau8825_set_sysclk(struct snd_soc_codec *codec, int clk_id,
 	return nau8825_configure_sysclk(nau8825, clk_id, freq);
 }
 
+static int nau8825_resume_setup(struct nau8825 *nau8825)
+{
+	struct regmap *regmap = nau8825->regmap;
+
+	/* IRQ Output Enable */
+	regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
+		NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
+
+	/* Enable internal VCO needed for interruptions */
+	nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
+
+	/* Enable DDACR needed for interrupts */
+	regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
+		NAU8825_ENABLE_DACR, NAU8825_ENABLE_DACR);
+
+	/* Chip needs one FSCLK cycle in order to generate interrupts,
+	 * as we cannot guarantee one will be provided by the system. Turning
+	 * master mode on then off enables us to generate that FSCLK cycle
+	 * with a minimum of contention on the clock bus.
+	 */
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
+
+	nau8825_restart_jack_detection(regmap);
+
+	return 0;
+}
+
 static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 				   enum snd_soc_bias_level level)
 {
@@ -1121,6 +1151,8 @@ static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 					"Failed to sync cache: %d\n", ret);
 				return ret;
 			}
+			if (nau8825->irq)
+				nau8825_resume_setup(nau8825);
 		}
 
 		break;
@@ -1330,24 +1362,22 @@ static int nau8825_i2c_remove(struct i2c_client *client)
 #ifdef CONFIG_PM_SLEEP
 static int nau8825_suspend(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(nau8825->dapm);
 
-	disable_irq(client->irq);
+	disable_irq(nau8825->irq);
+	snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
 	regcache_cache_only(nau8825->regmap, true);
-	regcache_mark_dirty(nau8825->regmap);
 
 	return 0;
 }
 
 static int nau8825_resume(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
 
 	regcache_cache_only(nau8825->regmap, false);
-	regcache_sync(nau8825->regmap);
-	enable_irq(client->irq);
+	enable_irq(nau8825->irq);
 
 	return 0;
 }
-- 
2.6.4

[PATCH v2] ASoC: nau8825: reduce stanby power consumption

[John Hsu]

There is side effect at last patch.
The playback has no sound with MCLK if internal clock frequency decreases.
Thus, we need to recover frequency setting when MCLK case.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 51 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 2e2e11a..b9bcb10 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -239,6 +239,29 @@ static int nau8825_pump_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int nau8825_dacr_event(struct snd_soc_dapm_widget *w,
+		struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct nau8825 *nau8825 = snd_soc_codec_get_drvdata(codec);
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMU:
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_ENA_CTRL,
+			NAU8825_ENABLE_DACR, NAU8825_ENABLE_DACR);
+		break;
+	case SND_SOC_DAPM_POST_PMD:
+		if (!nau8825->irq)
+			regmap_update_bits(nau8825->regmap,
+				NAU8825_REG_ENA_CTRL, NAU8825_ENABLE_DACR, 0);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const char * const nau8825_adc_decimation[] = {
 	"32", "64", "128", "256"
 };
@@ -313,16 +336,17 @@ static const struct snd_soc_dapm_widget nau8825_dapm_widgets[] = {
 		0),
 
 	/* ADC for button press detection */
-	SND_SOC_DAPM_ADC("SAR", NULL, NAU8825_REG_SAR_CTRL,
-		NAU8825_SAR_ADC_EN_SFT, 0),
+	SND_SOC_DAPM_SUPPLY("SAR", NAU8825_REG_SAR_CTRL,
+		NAU8825_SAR_ADC_EN_SFT, 0, NULL, 0),
 
 	SND_SOC_DAPM_DAC("ADACL", NULL, NAU8825_REG_RDAC, 12, 0),
 	SND_SOC_DAPM_DAC("ADACR", NULL, NAU8825_REG_RDAC, 13, 0),
 	SND_SOC_DAPM_SUPPLY("ADACL Clock", NAU8825_REG_RDAC, 8, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("ADACR Clock", NAU8825_REG_RDAC, 9, 0, NULL, 0),
 
-	SND_SOC_DAPM_DAC("DDACR", NULL, NAU8825_REG_ENA_CTRL,
-		NAU8825_ENABLE_DACR_SFT, 0),
+	SND_SOC_DAPM_DAC_E("DDACR", NULL, SND_SOC_NOPM, 0, 0,
+		nau8825_dacr_event, SND_SOC_DAPM_POST_PMU |
+		SND_SOC_DAPM_POST_PMD),
 	SND_SOC_DAPM_DAC("DDACL", NULL, NAU8825_REG_ENA_CTRL,
 		NAU8825_ENABLE_DACL_SFT, 0),
 	SND_SOC_DAPM_SUPPLY("DDAC Clock", NAU8825_REG_ENA_CTRL, 6, 0, NULL, 0),
@@ -845,13 +869,6 @@ static int nau8825_codec_probe(struct snd_soc_codec *codec)
 
 	nau8825->dapm = dapm;
 
-	/* The interrupt clock is gated by x1[10:8],
-	 * one of them needs to be enabled all the time for
-	 * interrupts to happen.
-	 */
-	snd_soc_dapm_force_enable_pin(dapm, "DDACR");
-	snd_soc_dapm_sync(dapm);
-
 	/* Unmask interruptions. Handler uses dapm object so we can enable
 	 * interruptions only after dapm is fully initialized.
 	 */
@@ -1032,6 +1049,8 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
 			NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
 		regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
+		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
+			NAU8825_CLK_MCLK_SRC_MASK, 0);
 		ret = nau8825_mclk_prepare(nau8825, freq);
 		if (ret)
 			return ret;
@@ -1042,6 +1061,12 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 			NAU8825_DCO_EN);
 		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
 			NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_VCO);
+		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
+			NAU8825_CLK_MCLK_SRC_MASK, 0xf);
+		regmap_update_bits(regmap, NAU8825_REG_FLL1,
+			NAU8825_FLL_RATIO_MASK, 0x10);
+		regmap_update_bits(regmap, NAU8825_REG_FLL6,
+			NAU8825_SDM_EN, NAU8825_SDM_EN);
 		if (nau8825->mclk_freq) {
 			clk_disable_unprepare(nau8825->mclk);
 			nau8825->mclk_freq = 0;
@@ -1276,9 +1301,7 @@ static int nau8825_setup_irq(struct nau8825 *nau8825)
 	/* Enable internal VCO needed for interruptions */
 	nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
 
-	/* Enable DDACR needed for interrupts
-	 * It is the same as force_enable_pin("DDACR") we do later
-	 */
+	/* Enable DDACR needed for interrupts */
 	regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
 		NAU8825_ENABLE_DACR, NAU8825_ENABLE_DACR);
 
-- 
2.6.4

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[Mark Brown]

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

On Mon, Feb 29, 2016 at 03:01:45AM +0800, John Hsu wrote:

> +static int nau8825_resume_setup(struct nau8825 *nau8825)
> +{

I'd expect to see this shared with initial power on?

>  	regcache_cache_only(nau8825->regmap, false);
> -	regcache_sync(nau8825->regmap);
> -	enable_irq(client->irq);
> +	enable_irq(nau8825->irq);

We're removing the register cache sync here but I don't see us adding it
anywhere else.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH v2] ASoC: nau8825: reduce stanby power consumption

[Mark Brown]

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

On Mon, Feb 29, 2016 at 03:47:07AM +0800, John Hsu wrote:
> There is side effect at last patch.
> The playback has no sound with MCLK if internal clock frequency decreases.
> Thus, we need to recover frequency setting when MCLK case.

This doesn't apply against current code, please check and resend also...

>  	/* ADC for button press detection */
> -	SND_SOC_DAPM_ADC("SAR", NULL, NAU8825_REG_SAR_CTRL,
> -		NAU8825_SAR_ADC_EN_SFT, 0),
> +	SND_SOC_DAPM_SUPPLY("SAR", NAU8825_REG_SAR_CTRL,
> +		NAU8825_SAR_ADC_EN_SFT, 0, NULL, 0),

This looks like an unrelated change so should be in a separate patch.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

Hi,

On 3/1/2016 11:26 AM, Mark Brown wrote:
> On Mon, Feb 29, 2016 at 03:01:45AM +0800, John Hsu wrote:
>
>   
>> +static int nau8825_resume_setup(struct nau8825 *nau8825)
>> +{
>>     
>
> I'd expect to see this shared with initial power on?
>
>   

There is a little things different between resume and initiation.
But I think the function could be reused.

>>  	regcache_cache_only(nau8825->regmap, false);
>> -	regcache_sync(nau8825->regmap);
>> -	enable_irq(client->irq);
>> +	enable_irq(nau8825->irq);
>>     
>
> We're removing the register cache sync here but I don't see us adding it
> anywhere else.
>   
A part of suspend and resume action moves to set bias function, 
nau8825_set_bias_level.
We make register cache dirty in bias off; and make register cache sync 
after resume in bias standby.

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

Hi,

On 3/10/2016 2:12 PM, John Hsu wrote:
>>>      regcache_cache_only(nau8825->regmap, false);
>>> -    regcache_sync(nau8825->regmap);
>>> -    enable_irq(client->irq);
>>> +    enable_irq(nau8825->irq);
>>>     
>>
>> We're removing the register cache sync here but I don't see us adding it
>> anywhere else.
>>   
> A part of suspend and resume action moves to set bias function, 
> nau8825_set_bias_level.
> We make register cache dirty in bias off; and make register cache sync 
> after resume in bias standby.
>
Just confirm it. Could you accept the explain about the register cache sync?
We need to change it or not? Very appreciate.

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[Mark Brown]

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

On Thu, Mar 10, 2016 at 02:12:46PM +0800, John Hsu wrote:
> On 3/1/2016 11:26 AM, Mark Brown wrote:
> >On Mon, Feb 29, 2016 at 03:01:45AM +0800, John Hsu wrote:

> >>+static int nau8825_resume_setup(struct nau8825 *nau8825)
> >>+{

> >I'd expect to see this shared with initial power on?

> There is a little things different between resume and initiation.
> But I think the function could be reused.

Really?  Bear in mind the device might have lost power over suspend...

> >> 	regcache_cache_only(nau8825->regmap, false);
> >>-	regcache_sync(nau8825->regmap);
> >>-	enable_irq(client->irq);
> >>+	enable_irq(nau8825->irq);

> >We're removing the register cache sync here but I don't see us adding it
> >anywhere else.

> A part of suspend and resume action moves to set bias function,
> nau8825_set_bias_level.
> We make register cache dirty in bias off; and make register cache sync after
> resume in bias standby.

OK.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[Mark Brown]

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

On Tue, Mar 15, 2016 at 03:53:03PM +0800, John Hsu wrote:
> On 3/10/2016 2:12 PM, John Hsu wrote:

> >A part of suspend and resume action moves to set bias function,
> >nau8825_set_bias_level.
> >We make register cache dirty in bias off; and make register cache sync
> >after resume in bias standby.

> Just confirm it. Could you accept the explain about the register cache sync?
> We need to change it or not? Very appreciate.

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  Sending content
free pings just adds to the mail volume (if they are seen at all) and if 
something has gone wrong you'll have to resend the patches anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

Hi,

On 3/15/2016 5:27 PM, Mark Brown wrote:
> On Thu, Mar 10, 2016 at 02:12:46PM +0800, John Hsu wrote:
>   
>> On 3/1/2016 11:26 AM, Mark Brown wrote:
>>     
>>> On Mon, Feb 29, 2016 at 03:01:45AM +0800, John Hsu wrote:
>>>       
>
>   
>>>> +static int nau8825_resume_setup(struct nau8825 *nau8825)
>>>> +{
>>>>         
>
>   
>>> I'd expect to see this shared with initial power on?
>>>       
>
>   
>> There is a little things different between resume and initiation.
>> But I think the function could be reused.
>>     
>
> Really?  Bear in mind the device might have lost power over suspend...
>   

Thanks for your remind. I'll design this function based on the concept.

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

Hi,

On 3/26/2016 8:08 AM, Ben Zhang wrote:
> On Mon, Mar 21, 2016 at 8:57 PM, John Hsu <KCHSU0@nuvoton.com> wrote:
>   
>> Modify power management function to sync behavior with set bias function.
>> And codec needs to config interrupt setting again to recover interrupt.
>>
>> Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
>> ---
>>  sound/soc/codecs/nau8825.c | 70 ++++++++++++++++++++++++++++------------------
>>  1 file changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
>> index 106c391..b8af46b 100644
>> --- a/sound/soc/codecs/nau8825.c
>> +++ b/sound/soc/codecs/nau8825.c
>> @@ -1204,6 +1204,42 @@ static int nau8825_set_sysclk(struct snd_soc_codec *codec, int clk_id,
>>         return nau8825_configure_sysclk(nau8825, clk_id, freq);
>>  }
>>
>> +static void nau8825_configure_interrupt(struct nau8825 *nau8825)
>> +{
>> +       struct regmap *regmap = nau8825->regmap;
>> +
>> +       /* IRQ Output Enable */
>> +       regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
>> +               NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
>> +
>> +       /* Enable internal VCO needed for interruptions */
>> +       nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
>>     
>
> It seems more flexible to let ASoC machine drivers manage
> the sysclk with snd_soc_dai_set_sysclk, instead of force
> switching to the internal VCO clock at resume. Some platforms
> may have external clock available all the time. If a platform
> doesn't supply clock to the codec when there is no active
> playback/capture, but wants jack detection interrupt, the
> machine driver should explicitly request the internal VCO clock.
>
>   
When system bootup or resume, there is usually no clock in system.
It is better to provide the internal clock if codec needs it for the 
interrupt.
I agree that will be flexible if machine driver can control it.
>> +
>> +       /* Enable ADC needed for interrupts
>> +        * It is the same as force_enable_pin("ADC") we do later
>> +        */
>> +       regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
>> +               NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);
>>     
>
> The register values for the above irq/clock/adc configurations
> are saved and restored by regcache, so we don't need to set
> them explicitly at every resume.
>
>   
>> +
>> +       /* Chip needs one FSCLK cycle in order to generate interrupts,
>> +        * as we cannot guarantee one will be provided by the system. Turning
>> +        * master mode on then off enables us to generate that FSCLK cycle
>> +        * with a minimum of contention on the clock bus.
>> +        */
>> +       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
>> +               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
>> +       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
>> +               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
>> +}
>> +
>> +static void nau8825_resume_setup(struct nau8825 *nau8825)
>> +{
>> +       struct regmap *regmap = nau8825->regmap;
>> +
>> +       nau8825_configure_interrupt(nau8825);
>> +       nau8825_restart_jack_detection(regmap);
>> +}
>> +
>>  static int nau8825_set_bias_level(struct snd_soc_codec *codec,
>>                                    enum snd_soc_bias_level level)
>>  {
>> @@ -1233,6 +1269,8 @@ static int nau8825_set_bias_level(struct snd_soc_codec *codec,
>>                                         "Failed to sync cache: %d\n", ret);
>>                                 return ret;
>>                         }
>> +                       if (nau8825->irq)
>> +                               nau8825_resume_setup(nau8825);
>>                 }
>>
>>                 break;
>> @@ -1346,29 +1384,9 @@ static int nau8825_read_device_properties(struct device *dev,
>>
>>  static int nau8825_setup_irq(struct nau8825 *nau8825)
>>  {
>> -       struct regmap *regmap = nau8825->regmap;
>>         int ret;
>>
>> -       /* IRQ Output Enable */
>> -       regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
>> -               NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
>> -
>> -       /* Enable internal VCO needed for interruptions */
>> -       nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
>> -
>> -       /* Enable ADC needed for interrupts */
>> -       regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
>> -               NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);
>> -
>> -       /* Chip needs one FSCLK cycle in order to generate interrupts,
>> -        * as we cannot guarantee one will be provided by the system. Turning
>> -        * master mode on then off enables us to generate that FSCLK cycle
>> -        * with a minimum of contention on the clock bus.
>> -        */
>> -       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
>> -               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
>> -       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
>> -               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
>> +       nau8825_configure_interrupt(nau8825);
>>
>>         ret = devm_request_threaded_irq(nau8825->dev, nau8825->irq, NULL,
>>                 nau8825_interrupt, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> @@ -1440,24 +1458,22 @@ static int nau8825_i2c_remove(struct i2c_client *client)
>>  #ifdef CONFIG_PM_SLEEP
>>  static int nau8825_suspend(struct device *dev)
>>  {
>> -       struct i2c_client *client = to_i2c_client(dev);
>>         struct nau8825 *nau8825 = dev_get_drvdata(dev);
>> +       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(nau8825->dapm);
>>
>> -       disable_irq(client->irq);
>> +       disable_irq(nau8825->irq);
>> +       snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
>>     
>
> Shouldn't have to force bias off. When there is no force
> enabled ADC/DACs, dapm_power_widgets will power
> the codec off at suspend:
>
> "SAR" can be changed to a dapm supply or micbias widget.
> "DDACR" will be fixed by
> "ASoC: nau8825: change output power for interrupt"
>
>   
The SAR is force enabled when mic detected. It shouldn't be powered down
when suspend. But I think change to supply is a good idea because that
will make codec bias level work normally not always keep bias on.

I think it is necessary to force to set bias off because we need do
something in  bias off case in set bias function. The function
shouldn't affect codec power because power decision is made
by widget connect status or some condition.
>>         regcache_cache_only(nau8825->regmap, true);
>> -       regcache_mark_dirty(nau8825->regmap);
>>
>>         return 0;
>>  }
>>
>>  static int nau8825_resume(struct device *dev)
>>  {
>> -       struct i2c_client *client = to_i2c_client(dev);
>>         struct nau8825 *nau8825 = dev_get_drvdata(dev);
>>
>>         regcache_cache_only(nau8825->regmap, false);
>> -       regcache_sync(nau8825->regmap);
>> -       enable_irq(client->irq);
>> +       enable_irq(nau8825->irq);
>>     
>
> The dev_pm_ops.resume races against nau8825_set_bias_level
> and nau8825_resume_setup. It may be better to use the
> snd_soc_codec_driver.resume so that ASoC core can sync
> everything correctly.
>
>   
Yes, it should be.
>>         return 0;
>>  }
>> --
>> 2.6.4
>>
>>     
>
> Hi John,
>
> I've uploaded "ASoC: nau8825: Fix jack detection across suspend"
> which fixes jack detection and the issues mentioned above.
>
> Thanks,
> Ben
> .
>
>   
I see. Thank you for remind.

Re: [PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[Ben Zhang]

On Mon, Mar 21, 2016 at 8:57 PM, John Hsu <KCHSU0@nuvoton.com> wrote:
> Modify power management function to sync behavior with set bias function.
> And codec needs to config interrupt setting again to recover interrupt.
>
> Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
> ---
>  sound/soc/codecs/nau8825.c | 70 ++++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
> index 106c391..b8af46b 100644
> --- a/sound/soc/codecs/nau8825.c
> +++ b/sound/soc/codecs/nau8825.c
> @@ -1204,6 +1204,42 @@ static int nau8825_set_sysclk(struct snd_soc_codec *codec, int clk_id,
>         return nau8825_configure_sysclk(nau8825, clk_id, freq);
>  }
>
> +static void nau8825_configure_interrupt(struct nau8825 *nau8825)
> +{
> +       struct regmap *regmap = nau8825->regmap;
> +
> +       /* IRQ Output Enable */
> +       regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
> +               NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
> +
> +       /* Enable internal VCO needed for interruptions */
> +       nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);

It seems more flexible to let ASoC machine drivers manage
the sysclk with snd_soc_dai_set_sysclk, instead of force
switching to the internal VCO clock at resume. Some platforms
may have external clock available all the time. If a platform
doesn't supply clock to the codec when there is no active
playback/capture, but wants jack detection interrupt, the
machine driver should explicitly request the internal VCO clock.

> +
> +       /* Enable ADC needed for interrupts
> +        * It is the same as force_enable_pin("ADC") we do later
> +        */
> +       regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
> +               NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);

The register values for the above irq/clock/adc configurations
are saved and restored by regcache, so we don't need to set
them explicitly at every resume.

> +
> +       /* Chip needs one FSCLK cycle in order to generate interrupts,
> +        * as we cannot guarantee one will be provided by the system. Turning
> +        * master mode on then off enables us to generate that FSCLK cycle
> +        * with a minimum of contention on the clock bus.
> +        */
> +       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
> +               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
> +       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
> +               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
> +}
> +
> +static void nau8825_resume_setup(struct nau8825 *nau8825)
> +{
> +       struct regmap *regmap = nau8825->regmap;
> +
> +       nau8825_configure_interrupt(nau8825);
> +       nau8825_restart_jack_detection(regmap);
> +}
> +
>  static int nau8825_set_bias_level(struct snd_soc_codec *codec,
>                                    enum snd_soc_bias_level level)
>  {
> @@ -1233,6 +1269,8 @@ static int nau8825_set_bias_level(struct snd_soc_codec *codec,
>                                         "Failed to sync cache: %d\n", ret);
>                                 return ret;
>                         }
> +                       if (nau8825->irq)
> +                               nau8825_resume_setup(nau8825);
>                 }
>
>                 break;
> @@ -1346,29 +1384,9 @@ static int nau8825_read_device_properties(struct device *dev,
>
>  static int nau8825_setup_irq(struct nau8825 *nau8825)
>  {
> -       struct regmap *regmap = nau8825->regmap;
>         int ret;
>
> -       /* IRQ Output Enable */
> -       regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
> -               NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
> -
> -       /* Enable internal VCO needed for interruptions */
> -       nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
> -
> -       /* Enable ADC needed for interrupts */
> -       regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
> -               NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);
> -
> -       /* Chip needs one FSCLK cycle in order to generate interrupts,
> -        * as we cannot guarantee one will be provided by the system. Turning
> -        * master mode on then off enables us to generate that FSCLK cycle
> -        * with a minimum of contention on the clock bus.
> -        */
> -       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
> -               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
> -       regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
> -               NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
> +       nau8825_configure_interrupt(nau8825);
>
>         ret = devm_request_threaded_irq(nau8825->dev, nau8825->irq, NULL,
>                 nau8825_interrupt, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> @@ -1440,24 +1458,22 @@ static int nau8825_i2c_remove(struct i2c_client *client)
>  #ifdef CONFIG_PM_SLEEP
>  static int nau8825_suspend(struct device *dev)
>  {
> -       struct i2c_client *client = to_i2c_client(dev);
>         struct nau8825 *nau8825 = dev_get_drvdata(dev);
> +       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(nau8825->dapm);
>
> -       disable_irq(client->irq);
> +       disable_irq(nau8825->irq);
> +       snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);

Shouldn't have to force bias off. When there is no force
enabled ADC/DACs, dapm_power_widgets will power
the codec off at suspend:

"SAR" can be changed to a dapm supply or micbias widget.
"DDACR" will be fixed by
"ASoC: nau8825: change output power for interrupt"

>         regcache_cache_only(nau8825->regmap, true);
> -       regcache_mark_dirty(nau8825->regmap);
>
>         return 0;
>  }
>
>  static int nau8825_resume(struct device *dev)
>  {
> -       struct i2c_client *client = to_i2c_client(dev);
>         struct nau8825 *nau8825 = dev_get_drvdata(dev);
>
>         regcache_cache_only(nau8825->regmap, false);
> -       regcache_sync(nau8825->regmap);
> -       enable_irq(client->irq);
> +       enable_irq(nau8825->irq);

The dev_pm_ops.resume races against nau8825_set_bias_level
and nau8825_resume_setup. It may be better to use the
snd_soc_codec_driver.resume so that ASoC core can sync
everything correctly.

>
>         return 0;
>  }
> --
> 2.6.4
>

Hi John,

I've uploaded "ASoC: nau8825: Fix jack detection across suspend"
which fixes jack detection and the issues mentioned above.

Thanks,
Ben

[PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

Modify power management function to sync behavior with set bias function.
And codec needs to config interrupt setting again to recover interrupt.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 70 ++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 106c391..b8af46b 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -1204,6 +1204,42 @@ static int nau8825_set_sysclk(struct snd_soc_codec *codec, int clk_id,
 	return nau8825_configure_sysclk(nau8825, clk_id, freq);
 }
 
+static void nau8825_configure_interrupt(struct nau8825 *nau8825)
+{
+	struct regmap *regmap = nau8825->regmap;
+
+	/* IRQ Output Enable */
+	regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
+		NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
+
+	/* Enable internal VCO needed for interruptions */
+	nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
+
+	/* Enable ADC needed for interrupts
+	 * It is the same as force_enable_pin("ADC") we do later
+	 */
+	regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
+		NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);
+
+	/* Chip needs one FSCLK cycle in order to generate interrupts,
+	 * as we cannot guarantee one will be provided by the system. Turning
+	 * master mode on then off enables us to generate that FSCLK cycle
+	 * with a minimum of contention on the clock bus.
+	 */
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
+}
+
+static void nau8825_resume_setup(struct nau8825 *nau8825)
+{
+	struct regmap *regmap = nau8825->regmap;
+
+	nau8825_configure_interrupt(nau8825);
+	nau8825_restart_jack_detection(regmap);
+}
+
 static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 				   enum snd_soc_bias_level level)
 {
@@ -1233,6 +1269,8 @@ static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 					"Failed to sync cache: %d\n", ret);
 				return ret;
 			}
+			if (nau8825->irq)
+				nau8825_resume_setup(nau8825);
 		}
 
 		break;
@@ -1346,29 +1384,9 @@ static int nau8825_read_device_properties(struct device *dev,
 
 static int nau8825_setup_irq(struct nau8825 *nau8825)
 {
-	struct regmap *regmap = nau8825->regmap;
 	int ret;
 
-	/* IRQ Output Enable */
-	regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
-		NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
-
-	/* Enable internal VCO needed for interruptions */
-	nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
-
-	/* Enable ADC needed for interrupts */
-	regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
-		NAU8825_ENABLE_ADC, NAU8825_ENABLE_ADC);
-
-	/* Chip needs one FSCLK cycle in order to generate interrupts,
-	 * as we cannot guarantee one will be provided by the system. Turning
-	 * master mode on then off enables us to generate that FSCLK cycle
-	 * with a minimum of contention on the clock bus.
-	 */
-	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
-		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
-	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
-		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
+	nau8825_configure_interrupt(nau8825);
 
 	ret = devm_request_threaded_irq(nau8825->dev, nau8825->irq, NULL,
 		nau8825_interrupt, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
@@ -1440,24 +1458,22 @@ static int nau8825_i2c_remove(struct i2c_client *client)
 #ifdef CONFIG_PM_SLEEP
 static int nau8825_suspend(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(nau8825->dapm);
 
-	disable_irq(client->irq);
+	disable_irq(nau8825->irq);
+	snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
 	regcache_cache_only(nau8825->regmap, true);
-	regcache_mark_dirty(nau8825->regmap);
 
 	return 0;
 }
 
 static int nau8825_resume(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
 
 	regcache_cache_only(nau8825->regmap, false);
-	regcache_sync(nau8825->regmap);
-	enable_irq(client->irq);
+	enable_irq(nau8825->irq);
 
 	return 0;
 }
-- 
2.6.4

[PATCH] ASoC: nau8825: fix interrupt fails and unstable after resume

[John Hsu]

This patch is to fix interrupt fails when resume back.
In solution, we add IRQ re-initiation,
and reconstruct suspend and resume with set_bias_level function.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 504c969..2e2e11a 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -1092,6 +1092,36 @@ static int nau8825_set_sysclk(struct snd_soc_codec *codec, int clk_id,
 	return nau8825_configure_sysclk(nau8825, clk_id, freq);
 }
 
+static int nau8825_resume_setup(struct nau8825 *nau8825)
+{
+	struct regmap *regmap = nau8825->regmap;
+
+	/* IRQ Output Enable */
+	regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
+		NAU8825_IRQ_OUTPUT_EN, NAU8825_IRQ_OUTPUT_EN);
+
+	/* Enable internal VCO needed for interruptions */
+	nau8825_configure_sysclk(nau8825, NAU8825_CLK_INTERNAL, 0);
+
+	/* Enable DDACR needed for interrupts */
+	regmap_update_bits(regmap, NAU8825_REG_ENA_CTRL,
+		NAU8825_ENABLE_DACR, NAU8825_ENABLE_DACR);
+
+	/* Chip needs one FSCLK cycle in order to generate interrupts,
+	 * as we cannot guarantee one will be provided by the system. Turning
+	 * master mode on then off enables us to generate that FSCLK cycle
+	 * with a minimum of contention on the clock bus.
+	 */
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_MASTER);
+	regmap_update_bits(regmap, NAU8825_REG_I2S_PCM_CTRL2,
+		NAU8825_I2S_MS_MASK, NAU8825_I2S_MS_SLAVE);
+
+	nau8825_restart_jack_detection(regmap);
+
+	return 0;
+}
+
 static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 				   enum snd_soc_bias_level level)
 {
@@ -1121,6 +1151,8 @@ static int nau8825_set_bias_level(struct snd_soc_codec *codec,
 					"Failed to sync cache: %d\n", ret);
 				return ret;
 			}
+			if (nau8825->irq)
+				nau8825_resume_setup(nau8825);
 		}
 
 		break;
@@ -1330,24 +1362,22 @@ static int nau8825_i2c_remove(struct i2c_client *client)
 #ifdef CONFIG_PM_SLEEP
 static int nau8825_suspend(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(nau8825->dapm);
 
-	disable_irq(client->irq);
+	disable_irq(nau8825->irq);
+	snd_soc_codec_force_bias_level(codec, SND_SOC_BIAS_OFF);
 	regcache_cache_only(nau8825->regmap, true);
-	regcache_mark_dirty(nau8825->regmap);
 
 	return 0;
 }
 
 static int nau8825_resume(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
 	struct nau8825 *nau8825 = dev_get_drvdata(dev);
 
 	regcache_cache_only(nau8825->regmap, false);
-	regcache_sync(nau8825->regmap);
-	enable_irq(client->irq);
+	enable_irq(nau8825->irq);
 
 	return 0;
 }
-- 
2.6.4