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

[John Hsu <KCHSU0@nuvoton.com>]

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.