[PATCH 1/3] ALSA: hda - jack poll once if jackpoll_interval==0

[PATCH 1/3] ALSA: hda - jack poll once if jackpoll_interval==0

[Wang Xingchao]

jackpoll_interval used to poll jack event periodically, if it's 0,
give the caller one chance to probe jack status.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_codec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8a005f0..74c11bc 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1216,11 +1216,12 @@ static void hda_jackpoll_work(struct work_struct *work)
 {
 	struct hda_codec *codec =
 		container_of(work, struct hda_codec, jackpoll_work.work);
-	if (!codec->jackpoll_interval)
-		return;
 
 	snd_hda_jack_set_dirty_all(codec);
 	snd_hda_jack_poll_all(codec);
+	if (!codec->jackpoll_interval)
+		return;
+
 	queue_delayed_work(codec->bus->workq, &codec->jackpoll_work,
 			   codec->jackpoll_interval);
 }
-- 
1.8.3.2

[PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang Xingchao]

With runtime power save feature enabled, Headphone hotplug
event will not be detected while controller/codec in D3. HDA has
feature WAKEEN to let codec wake up system if controller is in D3 or
system in S3.(HDA Spec 4.5.9.2/3). Codec can send out INT or wake up
controller depending on whether CIE or GIE enabled.(Figure 4, Interupt
structure).

The controller must be in RESET mode after enter runtime-suspend, otherwise
it will not be waken up even if codec send out wake-up event. And STATESTS
will be cleared after controller brought out of RESET mode.

This patch only enable WAKEEN for runtime-suspend(Controller D3) mode,
not for system S3 mode. with tool "evtest", Headphone hotplug events
could be cought and reported successfully.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 8860dd5..a7ac7fd 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip = card->private_data;
+	int status;
+
+	/* enable controller wake up event */
+	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
+		  STATESTS_INT_MASK);
 
 	azx_stop_chip(chip);
 	azx_enter_link_reset(chip);
@@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip = card->private_data;
+	struct hda_bus *bus;
+	struct hda_codec *codec;
+	int status;
 
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		hda_display_power(true);
+
+	/* Read STATESTS before controller reset */
+	status = azx_readw(chip, STATESTS);
+
 	azx_init_pci(chip);
 	azx_init_chip(chip, 1);
+
+	bus = chip->bus;
+	if (status && bus) {
+		list_for_each_entry(codec, &bus->codec_list, list)
+			if (status & (1 << codec->addr))
+				queue_delayed_work(codec->bus->workq,
+						   &codec->jackpoll_work, codec->jackpoll_interval);
+	}
+
+	/* disable controller Wake Up event*/
+	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
+			~STATESTS_INT_MASK);
+
 	return 0;
 }
 
-- 
1.8.3.2

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[David Henningsson]

On 07/22/2013 09:19 AM, Wang Xingchao wrote:
> With runtime power save feature enabled, Headphone hotplug
> event will not be detected while controller/codec in D3. HDA has
> feature WAKEEN to let codec wake up system if controller is in D3 or
> system in S3.(HDA Spec 4.5.9.2/3). Codec can send out INT or wake up
> controller depending on whether CIE or GIE enabled.(Figure 4, Interupt
> structure).

Oh, so you actually got it working? Nice! :-)

> The controller must be in RESET mode after enter runtime-suspend, otherwise
> it will not be waken up even if codec send out wake-up event. And STATESTS
> will be cleared after controller brought out of RESET mode.

There seems to be nothing in this patch that sets the controller in 
RESET mode, is this something done in a later patch, or is that code 
already present today, or...?

> This patch only enable WAKEEN for runtime-suspend(Controller D3) mode,
> not for system S3 mode. with tool "evtest", Headphone hotplug events
> could be cought and reported successfully.

>
> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> ---
>   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 8860dd5..a7ac7fd 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device *dev)
>   {
>   	struct snd_card *card = dev_get_drvdata(dev);
>   	struct azx *chip = card->private_data;
> +	int status;
> +
> +	/* enable controller wake up event */
> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> +		  STATESTS_INT_MASK);
>
>   	azx_stop_chip(chip);
>   	azx_enter_link_reset(chip);
> @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device *dev)
>   {
>   	struct snd_card *card = dev_get_drvdata(dev);
>   	struct azx *chip = card->private_data;
> +	struct hda_bus *bus;
> +	struct hda_codec *codec;
> +	int status;
>
>   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
>   		hda_display_power(true);
> +
> +	/* Read STATESTS before controller reset */
> +	status = azx_readw(chip, STATESTS);
> +
>   	azx_init_pci(chip);
>   	azx_init_chip(chip, 1);
> +
> +	bus = chip->bus;
> +	if (status && bus) {
> +		list_for_each_entry(codec, &bus->codec_list, list)
> +			if (status & (1 << codec->addr))
> +				queue_delayed_work(codec->bus->workq,
> +						   &codec->jackpoll_work, codec->jackpoll_interval);

Is there a reason you want to move the jack detection to a delayed work, 
i e, why can't we just call:

  	snd_hda_jack_set_dirty_all(codec);
  	snd_hda_jack_poll_all(codec);

...from here?

> +	}
> +
> +	/* disable controller Wake Up event*/
> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> +			~STATESTS_INT_MASK);
> +
>   	return 0;
>   }
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang, Xingchao]

[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]



> -----Original Message-----
> From: David Henningsson [mailto:david.henningsson@canonical.com]
> Sent: Tuesday, July 23, 2013 3:37 PM
> To: Wang Xingchao
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de; Girdwood, Liam R; Wang,
> Xingchao
> Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> 
> On 07/22/2013 09:19 AM, Wang Xingchao wrote:
> > With runtime power save feature enabled, Headphone hotplug event will
> > not be detected while controller/codec in D3. HDA has feature WAKEEN
> > to let codec wake up system if controller is in D3 or system in
> > S3.(HDA Spec 4.5.9.2/3). Codec can send out INT or wake up controller
> > depending on whether CIE or GIE enabled.(Figure 4, Interupt
> > structure).
> 
> Oh, so you actually got it working? Nice! :-)
> 
> > The controller must be in RESET mode after enter runtime-suspend,
> > otherwise it will not be waken up even if codec send out wake-up
> > event. And STATESTS will be cleared after controller brought out of RESET
> mode.
> 
> There seems to be nothing in this patch that sets the controller in RESET mode,
> is this something done in a later patch, or is that code already present today,
> or...?

Yeah, an interesting story on this. You can see the original patch in attached file, it did add
reset before runtime-suspend. But I find Mengdong did same thing for another bug fix already.

commit 3af3f356e16041c3353210214da601782e2cd8b4
Author: Mengdong Lin <mengdong.lin@intel.com>
Date:   Mon Jun 24 10:18:54 2013 -0400

    ALSA: hda - reset hda link during system/runtime suspend

    If all the codecs report ClkStopOK (OK to stop bus clock) after being put to
    D3, this patch will reset the HDA link before the controller is put to D3.

    So the link will be in reset during system or runtime suspend, the bus clock
    stops and the codecs are in D3(ClkStop) state.

    This may help to reduce power consumption by dozens of mW on some peripheral
    hda codecs.

    Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
    Signed-off-by: Takashi Iwai <tiwai@suse.de>

Thanks
--xingchao
> 
> > This patch only enable WAKEEN for runtime-suspend(Controller D3) mode,
> > not for system S3 mode. with tool "evtest", Headphone hotplug events
> > could be cought and reported successfully.
> 
> >
> > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > ---
> >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 8860dd5..a7ac7fd 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
> *dev)
> >   {
> >   	struct snd_card *card = dev_get_drvdata(dev);
> >   	struct azx *chip = card->private_data;
> > +	int status;
> > +
> > +	/* enable controller wake up event */
> > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > +		  STATESTS_INT_MASK);
> >
> >   	azx_stop_chip(chip);
> >   	azx_enter_link_reset(chip);
> > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
> *dev)
> >   {
> >   	struct snd_card *card = dev_get_drvdata(dev);
> >   	struct azx *chip = card->private_data;
> > +	struct hda_bus *bus;
> > +	struct hda_codec *codec;
> > +	int status;
> >
> >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> >   		hda_display_power(true);
> > +
> > +	/* Read STATESTS before controller reset */
> > +	status = azx_readw(chip, STATESTS);
> > +
> >   	azx_init_pci(chip);
> >   	azx_init_chip(chip, 1);
> > +
> > +	bus = chip->bus;
> > +	if (status && bus) {
> > +		list_for_each_entry(codec, &bus->codec_list, list)
> > +			if (status & (1 << codec->addr))
> > +				queue_delayed_work(codec->bus->workq,
> > +						   &codec->jackpoll_work,
> codec->jackpoll_interval);
> 
> Is there a reason you want to move the jack detection to a delayed work, i e,
> why can't we just call:
> 
>   	snd_hda_jack_set_dirty_all(codec);
>   	snd_hda_jack_poll_all(codec);
> 
> ...from here?
> 
> > +	}
> > +
> > +	/* disable controller Wake Up event*/
> > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> > +			~STATESTS_INT_MASK);
> > +
> >   	return 0;
> >   }
> >
> >
> 
> 
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

[-- Attachment #2: 0001-ALSA-hda-WAKEEN-feature-enabling-for-runtime-pm.patch --]
[-- Type: application/octet-stream, Size: 2990 bytes --]

From a480d6bc8e9e86720b8a9767011a9439e8685373 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Mon, 22 Jul 2013 02:48:44 -0400
Subject: [PATCH] ALSA: hda - WAKEEN feature enabling for runtime pm

With runtime power save feature enabled, Headphone hotplug
event will not be detected while controller/codec in D3. HDA has
feature WAKEEN to let codec wake up system if controller is in D3 or
system in S3.(HDA Spec 4.5.9.2/3). Codec can send out INT or wake up
controller depending on whether CIE or GIE enabled.(Figure 4, Interupt
structure).

The controller must be in RESET mode after enter runtime-suspend, otherwise
it will not be waken up even if codec send out wake-up event. And STATESTS
will be cleared after controller brought out of RESET mode.

This patch only enable WAKEEN for runtime-suspend(Controller D3) mode,
not for system S3 mode. with tool "evtest", Headphone hotplug events
could be cought and reported successfully.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_intel.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index cc548e00..cbb0a5f 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2953,10 +2953,27 @@ static int azx_runtime_suspend(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip = card->private_data;
+	int event, status;
+	unsigned long timeout;
 
 	snd_printk("Power-well: Azx runtime suspend called\n");
+
+	/* enable controller wake up event */
+	azx_writew(chip, WAKEEN, azx_readb(chip, WAKEEN) |
+		  STATESTS_INT_MASK);
+
 	azx_stop_chip(chip);
 	azx_clear_irq_pending(chip);
+
+
+	/* reset controller */
+	azx_writel(chip, GCTL, azx_readl(chip, GCTL) & ~ICH6_GCTL_RESET);
+
+	timeout = jiffies + msecs_to_jiffies(100);
+	while (azx_readb(chip, GCTL) &&
+			time_before(jiffies, timeout))
+		usleep_range(500, 1000);
+
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		hda_display_power(false);
 	return 0;
@@ -2966,12 +2983,32 @@ static int azx_runtime_resume(struct device *dev)
 {
 	struct snd_card *card = dev_get_drvdata(dev);
 	struct azx *chip = card->private_data;
+	struct hda_bus *bus;
+	struct hda_codec *codec;
+	int status;
 
 	snd_printk("Power-well: Azx runtime resume called\n");
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		hda_display_power(true);
+	
+	/* Read STATESTS before controller reset*/
+	status = azx_readw(chip, STATESTS);
+	bus = chip->bus;
+
 	azx_init_pci(chip);
 	azx_init_chip(chip, 1);
+
+	if (status && bus) {
+		list_for_each_entry(codec, &bus->codec_list, list)
+			if (status & (1 << codec->addr))
+				queue_delayed_work(codec->bus->workq,
+						   &codec->jackpoll_work, codec->jackpoll_interval);
+	}
+
+	/* disable controller Wake Up event*/
+	azx_writew(chip, WAKEEN, azx_readb(chip, WAKEEN) & 
+		  ~STATESTS_INT_MASK);
+
 	return 0;
 }
 
-- 
1.8.3.2


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

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang, Xingchao]

> >
> > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > ---
> >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 8860dd5..a7ac7fd 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
> *dev)
> >   {
> >   	struct snd_card *card = dev_get_drvdata(dev);
> >   	struct azx *chip = card->private_data;
> > +	int status;
> > +
> > +	/* enable controller wake up event */
> > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > +		  STATESTS_INT_MASK);
> >
> >   	azx_stop_chip(chip);
> >   	azx_enter_link_reset(chip);
> > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
> *dev)
> >   {
> >   	struct snd_card *card = dev_get_drvdata(dev);
> >   	struct azx *chip = card->private_data;
> > +	struct hda_bus *bus;
> > +	struct hda_codec *codec;
> > +	int status;
> >
> >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> >   		hda_display_power(true);
> > +
> > +	/* Read STATESTS before controller reset */
> > +	status = azx_readw(chip, STATESTS);
> > +
> >   	azx_init_pci(chip);
> >   	azx_init_chip(chip, 1);
> > +
> > +	bus = chip->bus;
> > +	if (status && bus) {
> > +		list_for_each_entry(codec, &bus->codec_list, list)
> > +			if (status & (1 << codec->addr))
> > +				queue_delayed_work(codec->bus->workq,
> > +						   &codec->jackpoll_work,
> codec->jackpoll_interval);
> 
> Is there a reason you want to move the jack detection to a delayed work, i e,
> why can't we just call:
> 
>   	snd_hda_jack_set_dirty_all(codec);
>   	snd_hda_jack_poll_all(codec);
> 
> ...from here?

In fact it doesnot work for me, It will again call runtime_resume() and caused dead loop.

--xingchao
> 
> > +	}
> > +
> > +	/* disable controller Wake Up event*/
> > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> > +			~STATESTS_INT_MASK);
> > +
> >   	return 0;
> >   }
> >
> >
> 
> 
> 
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Takashi Iwai]

At Tue, 23 Jul 2013 08:09:02 +0000,
Wang, Xingchao wrote:
> 
> > >
> > > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > > ---
> > >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > >   1 file changed, 25 insertions(+)
> > >
> > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > index 8860dd5..a7ac7fd 100644
> > > --- a/sound/pci/hda/hda_intel.c
> > > +++ b/sound/pci/hda/hda_intel.c
> > > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
> > *dev)
> > >   {
> > >   	struct snd_card *card = dev_get_drvdata(dev);
> > >   	struct azx *chip = card->private_data;
> > > +	int status;
> > > +
> > > +	/* enable controller wake up event */
> > > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > +		  STATESTS_INT_MASK);
> > >
> > >   	azx_stop_chip(chip);
> > >   	azx_enter_link_reset(chip);
> > > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
> > *dev)
> > >   {
> > >   	struct snd_card *card = dev_get_drvdata(dev);
> > >   	struct azx *chip = card->private_data;
> > > +	struct hda_bus *bus;
> > > +	struct hda_codec *codec;
> > > +	int status;
> > >
> > >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > >   		hda_display_power(true);
> > > +
> > > +	/* Read STATESTS before controller reset */
> > > +	status = azx_readw(chip, STATESTS);
> > > +
> > >   	azx_init_pci(chip);
> > >   	azx_init_chip(chip, 1);
> > > +
> > > +	bus = chip->bus;
> > > +	if (status && bus) {
> > > +		list_for_each_entry(codec, &bus->codec_list, list)
> > > +			if (status & (1 << codec->addr))
> > > +				queue_delayed_work(codec->bus->workq,
> > > +						   &codec->jackpoll_work,
> > codec->jackpoll_interval);
> > 
> > Is there a reason you want to move the jack detection to a delayed work, i e,
> > why can't we just call:
> > 
> >   	snd_hda_jack_set_dirty_all(codec);
> >   	snd_hda_jack_poll_all(codec);
> > 
> > ...from here?
> 
> In fact it doesnot work for me, It will again call runtime_resume() and caused dead loop.

It means that the power refcount is messed up by this way.

When a verb is executed, the codec goes to a full resume mode, which
turns up the controller, eventually calling azx_power_notify().
In azx_power_notify(), pm_runtime_{get|put}_sync() is called
accordingly, which again goes to runtime resume.  A quick fix would be
to add a flag or something to avoid reentrace.

But, calling the jackpoll in the resume is basically superfluous.
As already mentioned, issuing a verb itself triggers the full resume,
and this already includes the update of the whole jack status.
Thus, executing jackpoll at that place means to perform the jack
polling twice.

In other words, what's we need to achieve is just to call
snd_hda_resume() appropriately in the runtime resume case.
Of course, this will need more fixes for avoiding reentrance, etc.

But, it leads to another question: do we need a full resume just for
jack detection and user-space notification?  Just reading the pin
detect state should be able to run even in D3 (for chips that are
capable of it), and the notification itself doesn't need any audio
hardware action.


Takashi

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[David Henningsson]

On 07/24/2013 12:48 PM, Takashi Iwai wrote:
> At Tue, 23 Jul 2013 08:09:02 +0000,
> Wang, Xingchao wrote:
>>
>>>>
>>>> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
>>>> ---
>>>>    sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>>> index 8860dd5..a7ac7fd 100644
>>>> --- a/sound/pci/hda/hda_intel.c
>>>> +++ b/sound/pci/hda/hda_intel.c
>>>> @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
>>> *dev)
>>>>    {
>>>>    	struct snd_card *card = dev_get_drvdata(dev);
>>>>    	struct azx *chip = card->private_data;
>>>> +	int status;
>>>> +
>>>> +	/* enable controller wake up event */
>>>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
>>>> +		  STATESTS_INT_MASK);
>>>>
>>>>    	azx_stop_chip(chip);
>>>>    	azx_enter_link_reset(chip);
>>>> @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
>>> *dev)
>>>>    {
>>>>    	struct snd_card *card = dev_get_drvdata(dev);
>>>>    	struct azx *chip = card->private_data;
>>>> +	struct hda_bus *bus;
>>>> +	struct hda_codec *codec;
>>>> +	int status;
>>>>
>>>>    	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
>>>>    		hda_display_power(true);
>>>> +
>>>> +	/* Read STATESTS before controller reset */
>>>> +	status = azx_readw(chip, STATESTS);
>>>> +
>>>>    	azx_init_pci(chip);
>>>>    	azx_init_chip(chip, 1);
>>>> +
>>>> +	bus = chip->bus;
>>>> +	if (status && bus) {
>>>> +		list_for_each_entry(codec, &bus->codec_list, list)
>>>> +			if (status & (1 << codec->addr))
>>>> +				queue_delayed_work(codec->bus->workq,
>>>> +						   &codec->jackpoll_work,
>>> codec->jackpoll_interval);
>>>
>>> Is there a reason you want to move the jack detection to a delayed work, i e,
>>> why can't we just call:
>>>
>>>    	snd_hda_jack_set_dirty_all(codec);
>>>    	snd_hda_jack_poll_all(codec);
>>>
>>> ...from here?
>>
>> In fact it doesnot work for me, It will again call runtime_resume() and caused dead loop.
>
> It means that the power refcount is messed up by this way.
>
> When a verb is executed, the codec goes to a full resume mode, which
> turns up the controller, eventually calling azx_power_notify().
> In azx_power_notify(), pm_runtime_{get|put}_sync() is called
> accordingly, which again goes to runtime resume.  A quick fix would be
> to add a flag or something to avoid reentrace.
>
> But, calling the jackpoll in the resume is basically superfluous.
> As already mentioned, issuing a verb itself triggers the full resume,
> and this already includes the update of the whole jack status.
> Thus, executing jackpoll at that place means to perform the jack
> polling twice.
>
> In other words, what's we need to achieve is just to call
> snd_hda_resume() appropriately in the runtime resume case.
> Of course, this will need more fixes for avoiding reentrance, etc.
>
> But, it leads to another question: do we need a full resume just for
> jack detection and user-space notification?  Just reading the pin
> detect state should be able to run even in D3 (for chips that are
> capable of it), and the notification itself doesn't need any audio
> hardware action.

The STATESTS register only indicates which codec requested wakeup, not 
which pin. So you need to issue the get_pin_sense verb for all pins on 
the codec, which means that the codec - controller link must be powered up.

So that's half of the resume procedure already. Are you proposing we 
introduce some kind of "half-resumed" mode that we would be in when we 
wait for the response from get_pin_sense? It sounds like additional 
complexity for very little gain in power.

Or am I missing something here?


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Takashi Iwai]

At Wed, 24 Jul 2013 13:36:32 +0200,
David Henningsson wrote:
> 
> On 07/24/2013 12:48 PM, Takashi Iwai wrote:
> > At Tue, 23 Jul 2013 08:09:02 +0000,
> > Wang, Xingchao wrote:
> >>
> >>>>
> >>>> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> >>>> ---
> >>>>    sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> >>>>    1 file changed, 25 insertions(+)
> >>>>
> >>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >>>> index 8860dd5..a7ac7fd 100644
> >>>> --- a/sound/pci/hda/hda_intel.c
> >>>> +++ b/sound/pci/hda/hda_intel.c
> >>>> @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct device
> >>> *dev)
> >>>>    {
> >>>>    	struct snd_card *card = dev_get_drvdata(dev);
> >>>>    	struct azx *chip = card->private_data;
> >>>> +	int status;
> >>>> +
> >>>> +	/* enable controller wake up event */
> >>>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> >>>> +		  STATESTS_INT_MASK);
> >>>>
> >>>>    	azx_stop_chip(chip);
> >>>>    	azx_enter_link_reset(chip);
> >>>> @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct device
> >>> *dev)
> >>>>    {
> >>>>    	struct snd_card *card = dev_get_drvdata(dev);
> >>>>    	struct azx *chip = card->private_data;
> >>>> +	struct hda_bus *bus;
> >>>> +	struct hda_codec *codec;
> >>>> +	int status;
> >>>>
> >>>>    	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> >>>>    		hda_display_power(true);
> >>>> +
> >>>> +	/* Read STATESTS before controller reset */
> >>>> +	status = azx_readw(chip, STATESTS);
> >>>> +
> >>>>    	azx_init_pci(chip);
> >>>>    	azx_init_chip(chip, 1);
> >>>> +
> >>>> +	bus = chip->bus;
> >>>> +	if (status && bus) {
> >>>> +		list_for_each_entry(codec, &bus->codec_list, list)
> >>>> +			if (status & (1 << codec->addr))
> >>>> +				queue_delayed_work(codec->bus->workq,
> >>>> +						   &codec->jackpoll_work,
> >>> codec->jackpoll_interval);
> >>>
> >>> Is there a reason you want to move the jack detection to a delayed work, i e,
> >>> why can't we just call:
> >>>
> >>>    	snd_hda_jack_set_dirty_all(codec);
> >>>    	snd_hda_jack_poll_all(codec);
> >>>
> >>> ...from here?
> >>
> >> In fact it doesnot work for me, It will again call runtime_resume() and caused dead loop.
> >
> > It means that the power refcount is messed up by this way.
> >
> > When a verb is executed, the codec goes to a full resume mode, which
> > turns up the controller, eventually calling azx_power_notify().
> > In azx_power_notify(), pm_runtime_{get|put}_sync() is called
> > accordingly, which again goes to runtime resume.  A quick fix would be
> > to add a flag or something to avoid reentrace.
> >
> > But, calling the jackpoll in the resume is basically superfluous.
> > As already mentioned, issuing a verb itself triggers the full resume,
> > and this already includes the update of the whole jack status.
> > Thus, executing jackpoll at that place means to perform the jack
> > polling twice.
> >
> > In other words, what's we need to achieve is just to call
> > snd_hda_resume() appropriately in the runtime resume case.
> > Of course, this will need more fixes for avoiding reentrance, etc.
> >
> > But, it leads to another question: do we need a full resume just for
> > jack detection and user-space notification?  Just reading the pin
> > detect state should be able to run even in D3 (for chips that are
> > capable of it), and the notification itself doesn't need any audio
> > hardware action.
> 
> The STATESTS register only indicates which codec requested wakeup, not 
> which pin. So you need to issue the get_pin_sense verb for all pins on 
> the codec, which means that the codec - controller link must be powered up.
> 
> So that's half of the resume procedure already.

The rest half is rather a longer run; it executes many verbs to
restore the whole codec widget states.  And, it's done just to run a
few GET_PIN_SENSE verbs.  For reading GET_PIN_SENSE, we even don't
have to power up the codec.

> Are you proposing we 
> introduce some kind of "half-resumed" mode that we would be in when we 
> wait for the response from get_pin_sense? It sounds like additional 
> complexity for very little gain in power.

Yeah, the complexity question is still valid.

However, in this runtime PM suspend case, the system doesn't react to
change the configuration for auto-mute, etc, at this point.  It just
needs to send a notification to the user-space.  So, it sends a few
GET_PIN_SENSE verbs, then updates the kctl / input-jack stuff --
that's all it needs.

For example, suppose that we have a function to execute the verb
without the power up/down sequence, e.g. snd_hda_codec_read_no_pm().
Then change snd_hda_codec_read() call with this one if codec->epss is
true.

Of course, it won't suffice to assure that it's executed in the proper
power state, so the end result might become too complex.  Needs more
investigations.


Takashi

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang, Xingchao]

[-- Attachment #1: Type: text/plain, Size: 6098 bytes --]



> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 8:24 PM
> To: David Henningsson
> Cc: Wang, Xingchao; Wang Xingchao; alsa-devel@alsa-project.org; Girdwood,
> Liam R
> Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> 
> At Wed, 24 Jul 2013 13:36:32 +0200,
> David Henningsson wrote:
> >
> > On 07/24/2013 12:48 PM, Takashi Iwai wrote:
> > > At Tue, 23 Jul 2013 08:09:02 +0000,
> > > Wang, Xingchao wrote:
> > >>
> > >>>>
> > >>>> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > >>>> ---
> > >>>>    sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > >>>>    1 file changed, 25 insertions(+)
> > >>>>
> > >>>> diff --git a/sound/pci/hda/hda_intel.c
> > >>>> b/sound/pci/hda/hda_intel.c index 8860dd5..a7ac7fd 100644
> > >>>> --- a/sound/pci/hda/hda_intel.c
> > >>>> +++ b/sound/pci/hda/hda_intel.c
> > >>>> @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct
> > >>>> device
> > >>> *dev)
> > >>>>    {
> > >>>>    	struct snd_card *card = dev_get_drvdata(dev);
> > >>>>    	struct azx *chip = card->private_data;
> > >>>> +	int status;
> > >>>> +
> > >>>> +	/* enable controller wake up event */
> > >>>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > >>>> +		  STATESTS_INT_MASK);
> > >>>>
> > >>>>    	azx_stop_chip(chip);
> > >>>>    	azx_enter_link_reset(chip);
> > >>>> @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct
> > >>>> device
> > >>> *dev)
> > >>>>    {
> > >>>>    	struct snd_card *card = dev_get_drvdata(dev);
> > >>>>    	struct azx *chip = card->private_data;
> > >>>> +	struct hda_bus *bus;
> > >>>> +	struct hda_codec *codec;
> > >>>> +	int status;
> > >>>>
> > >>>>    	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > >>>>    		hda_display_power(true);
> > >>>> +
> > >>>> +	/* Read STATESTS before controller reset */
> > >>>> +	status = azx_readw(chip, STATESTS);
> > >>>> +
> > >>>>    	azx_init_pci(chip);
> > >>>>    	azx_init_chip(chip, 1);
> > >>>> +
> > >>>> +	bus = chip->bus;
> > >>>> +	if (status && bus) {
> > >>>> +		list_for_each_entry(codec, &bus->codec_list, list)
> > >>>> +			if (status & (1 << codec->addr))
> > >>>> +				queue_delayed_work(codec->bus->workq,
> > >>>> +						   &codec->jackpoll_work,
> > >>> codec->jackpoll_interval);
> > >>>
> > >>> Is there a reason you want to move the jack detection to a delayed
> > >>> work, i e, why can't we just call:
> > >>>
> > >>>    	snd_hda_jack_set_dirty_all(codec);
> > >>>    	snd_hda_jack_poll_all(codec);
> > >>>
> > >>> ...from here?
> > >>
> > >> In fact it doesnot work for me, It will again call runtime_resume() and
> caused dead loop.
> > >
> > > It means that the power refcount is messed up by this way.
> > >
> > > When a verb is executed, the codec goes to a full resume mode, which
> > > turns up the controller, eventually calling azx_power_notify().
> > > In azx_power_notify(), pm_runtime_{get|put}_sync() is called
> > > accordingly, which again goes to runtime resume.  A quick fix would
> > > be to add a flag or something to avoid reentrace.
> > >
> > > But, calling the jackpoll in the resume is basically superfluous.
> > > As already mentioned, issuing a verb itself triggers the full
> > > resume, and this already includes the update of the whole jack status.
> > > Thus, executing jackpoll at that place means to perform the jack
> > > polling twice.
> > >
> > > In other words, what's we need to achieve is just to call
> > > snd_hda_resume() appropriately in the runtime resume case.
> > > Of course, this will need more fixes for avoiding reentrance, etc.
> > >
> > > But, it leads to another question: do we need a full resume just for
> > > jack detection and user-space notification?  Just reading the pin
> > > detect state should be able to run even in D3 (for chips that are
> > > capable of it), and the notification itself doesn't need any audio
> > > hardware action.

Takashi, I donot find description about reading pin state in D3 in HD-Audio spec.
Am I missing something?

> >
> > The STATESTS register only indicates which codec requested wakeup, not
> > which pin. So you need to issue the get_pin_sense verb for all pins on
> > the codec, which means that the codec - controller link must be powered up.
> >

Yes, as Takashi mentioned above, if each pin's jack state could be read in D3, it's not a problem anymore.
I work out a new patch to remember old pin_sense and report to user-space when receive wake-up event.
This could avoid polling jack state and send command, I thought it could avoid waking up audio controller/codec, but not...
(for detail changes please find in attached patch file)

> > So that's half of the resume procedure already.
> 
> The rest half is rather a longer run; it executes many verbs to restore the whole
> codec widget states.  And, it's done just to run a few GET_PIN_SENSE verbs.
> For reading GET_PIN_SENSE, we even don't have to power up the codec.
> 
> > Are you proposing we
> > introduce some kind of "half-resumed" mode that we would be in when we
> > wait for the response from get_pin_sense? It sounds like additional
> > complexity for very little gain in power.
> 
> Yeah, the complexity question is still valid.
> 
> However, in this runtime PM suspend case, the system doesn't react to change
> the configuration for auto-mute, etc, at this point.  It just needs to send a
> notification to the user-space.  So, it sends a few GET_PIN_SENSE verbs, then
> updates the kctl / input-jack stuff -- that's all it needs.
> 
> For example, suppose that we have a function to execute the verb without the
> power up/down sequence, e.g. snd_hda_codec_read_no_pm().
> Then change snd_hda_codec_read() call with this one if codec->epss is true.
> 
> Of course, it won't suffice to assure that it's executed in the proper power
> state, so the end result might become too complex.  Needs more
> investigations.
> 
> 
> Takashi

[-- Attachment #2: 0001-ALSA-hda-Jack-hotplug-support-in-D3.patch --]
[-- Type: application/octet-stream, Size: 5837 bytes --]

From 4da3a97e04773e7d26ec595b8326c437a39083d7 Mon Sep 17 00:00:00 2001
From: Wang Xingchao <xingchao.wang@linux.intel.com>
Date: Thu, 25 Jul 2013 09:24:49 -0400
Subject: [PATCH] ALSA: hda - Jack hotplug support in D3

This patch intended to avoid waking up the controller/codec, but FAILED.
Even no touch hardware register in runtime_resum/suspend callback when
detected jack hotplug(leave codec/controller in D3), i still see the
controller was waken up, pm_notify() was called.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_intel.c | 16 ++++++++++----
 sound/pci/hda/hda_jack.c  | 55 ++++++++++++++++++++++++++++++++++++++++-------
 sound/pci/hda/hda_jack.h  |  1 +
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 708e988..38eac3a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -63,6 +63,7 @@
 #include <linux/firmware.h>
 #include "hda_codec.h"
 #include "hda_i915.h"
+#include "hda_jack.h"
 
 
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
@@ -529,6 +530,7 @@ struct azx {
 	unsigned int snoop:1;
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
+	unsigned int jack_resume:1;
 
 	/* VGA-switcheroo setup */
 	unsigned int use_vga_switcheroo:1;
@@ -1378,6 +1380,7 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 	u8 sd_status;
 	int i, ok;
 
+
 #ifdef CONFIG_PM_RUNTIME
 	if (chip->pci->dev.power.runtime_status != RPM_ACTIVE)
 		return IRQ_NONE;
@@ -2823,6 +2826,7 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
 static void azx_power_notify(struct hda_bus *bus, bool power_up)
 {
 	struct azx *chip = bus->private_data;
+	snd_printk("jack: check calling here\n");
 
 	if (!(chip->driver_caps & AZX_DCAPS_PM_RUNTIME))
 		return;
@@ -2956,12 +2960,13 @@ static int azx_runtime_suspend(struct device *dev)
 	int event, status;
 	unsigned long timeout;
 
-	snd_printk("Power-well: Azx runtime suspend called\n");
-
 	/* enable controller wake up event */
 	azx_writew(chip, WAKEEN, azx_readb(chip, WAKEEN) |
 		  STATESTS_INT_MASK);
 
+	if (chip->jack_resume)
+		goto out;
+
 	azx_stop_chip(chip);
 	azx_clear_irq_pending(chip);
 
@@ -2974,6 +2979,7 @@ static int azx_runtime_suspend(struct device *dev)
 			time_before(jiffies, timeout))
 		usleep_range(500, 1000);
 
+out:
 	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		hda_display_power(false);
 	return 0;
@@ -2998,11 +3004,13 @@ static int azx_runtime_resume(struct device *dev)
 	azx_init_pci(chip);
 	azx_init_chip(chip, 1);
 
+	chip->jack_resume = !!status;
 	if (status && bus) {
 		list_for_each_entry(codec, &bus->codec_list, list)
 			if (status & (1 << codec->addr))
-				queue_delayed_work(codec->bus->workq,
-						   &codec->jackpoll_work, codec->jackpoll_interval);
+				snd_hda_jack_report_no_wakeup(codec);
+	
+		return 0;
 	}
 
 	/* disable controller Wake Up event*/
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index 9e0a952..8b0f619 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -260,23 +260,23 @@ int snd_hda_jack_set_gating_jack(struct hda_codec *codec, hda_nid_t gated_nid,
 }
 EXPORT_SYMBOL_HDA(snd_hda_jack_set_gating_jack);
 
-/**
- * snd_hda_jack_report_sync - sync the states of all jacks and report if changed
- */
-void snd_hda_jack_report_sync(struct hda_codec *codec)
+static void update_jacks(struct hda_codec *codec)
 {
 	struct hda_jack_tbl *jack;
-	int i, state;
+	int i;
 
 	/* update all jacks at first */
 	jack = codec->jacktbl.list;
 	for (i = 0; i < codec->jacktbl.used; i++, jack++)
 		if (jack->nid)
 			jack_detect_update(codec, jack);
+}
+
+static void report_jacks(struct hda_codec *codec)
+{
+	struct hda_jack_tbl *jack;
+	int i, state;
 
-	/* report the updated jacks; it's done after updating all jacks
-	 * to make sure that all gating jacks properly have been set
-	 */
 	jack = codec->jacktbl.list;
 	for (i = 0; i < codec->jacktbl.used; i++, jack++)
 		if (jack->nid) {
@@ -290,9 +290,48 @@ void snd_hda_jack_report_sync(struct hda_codec *codec)
 						state ? jack->type : 0);
 #endif
 		}
+
+}
+
+/**
+ * snd_hda_jack_report_sync - sync the states of all jacks and report if changed
+ */
+void snd_hda_jack_report_sync(struct hda_codec *codec)
+{
+	struct hda_jack_tbl *jack;
+	int i, state;
+
+	update_jacks(codec);
+	/* report the updated jacks; it's done after updating all jacks
+	 * to make sure that all gating jacks properly have been set
+	 */
+	report_jacks(codec);
 }
 EXPORT_SYMBOL_HDA(snd_hda_jack_report_sync);
 
+void snd_hda_jack_report_no_wakeup(struct hda_codec *codec)
+{
+	struct hda_jack_tbl *jack;
+	int i, state;
+
+	/* revert all jack state */
+	jack = codec->jacktbl.list;
+	for (i = 0; i < codec->jacktbl.used; i++, jack++)
+		if (jack->nid) {
+			state = get_jack_plug_state(jack->pin_sense);
+			if (!jack->pin_sense)
+				jack->pin_sense = AC_PINSENSE_PRESENCE;
+			else
+				jack->pin_sense = 0;
+		}
+
+	/* report the updated jacks; it's done after updating all jacks
+	 * to make sure that all gating jacks properly have been set
+	 */
+	report_jacks(codec);
+}
+EXPORT_SYMBOL_HDA(snd_hda_jack_report_no_wakeup);
+
 #ifdef CONFIG_SND_HDA_INPUT_JACK
 /* guess the jack type from the pin-config */
 static int get_input_jack_type(struct hda_codec *codec, hda_nid_t nid)
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index ec12abd..b14cf24 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -85,6 +85,7 @@ int snd_hda_jack_add_kctls(struct hda_codec *codec,
 			   const struct auto_pin_cfg *cfg);
 
 void snd_hda_jack_report_sync(struct hda_codec *codec);
+void snd_hda_jack_report_no_wakeup(struct hda_codec *codec);
 
 void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res);
 
-- 
1.8.3.2


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

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 6:49 PM
> To: Wang, Xingchao
> Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> Girdwood, Liam R
> Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> 
> At Tue, 23 Jul 2013 08:09:02 +0000,
> Wang, Xingchao wrote:
> >
> > > >
> > > > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > > > ---
> > > >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > > >   1 file changed, 25 insertions(+)
> > > >
> > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > index 8860dd5..a7ac7fd 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct
> > > > device
> > > *dev)
> > > >   {
> > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > >   	struct azx *chip = card->private_data;
> > > > +	int status;
> > > > +
> > > > +	/* enable controller wake up event */
> > > > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > > +		  STATESTS_INT_MASK);
> > > >
> > > >   	azx_stop_chip(chip);
> > > >   	azx_enter_link_reset(chip);
> > > > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct
> > > > device
> > > *dev)
> > > >   {
> > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > >   	struct azx *chip = card->private_data;
> > > > +	struct hda_bus *bus;
> > > > +	struct hda_codec *codec;
> > > > +	int status;
> > > >
> > > >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > > >   		hda_display_power(true);
> > > > +
> > > > +	/* Read STATESTS before controller reset */
> > > > +	status = azx_readw(chip, STATESTS);
> > > > +
> > > >   	azx_init_pci(chip);
> > > >   	azx_init_chip(chip, 1);
> > > > +
> > > > +	bus = chip->bus;
> > > > +	if (status && bus) {
> > > > +		list_for_each_entry(codec, &bus->codec_list, list)
> > > > +			if (status & (1 << codec->addr))
> > > > +				queue_delayed_work(codec->bus->workq,
> > > > +						   &codec->jackpoll_work,
> > > codec->jackpoll_interval);
> > >
> > > Is there a reason you want to move the jack detection to a delayed
> > > work, i e, why can't we just call:
> > >
> > >   	snd_hda_jack_set_dirty_all(codec);
> > >   	snd_hda_jack_poll_all(codec);
> > >
> > > ...from here?
> >
> > In fact it doesnot work for me, It will again call runtime_resume() and caused
> dead loop.
> 
> It means that the power refcount is messed up by this way.
> 
> When a verb is executed, the codec goes to a full resume mode, which turns up
> the controller, eventually calling azx_power_notify().
> In azx_power_notify(), pm_runtime_{get|put}_sync() is called accordingly,
> which again goes to runtime resume.  A quick fix would be to add a flag or
> something to avoid reentrace.
> 
> But, calling the jackpoll in the resume is basically superfluous.
> As already mentioned, issuing a verb itself triggers the full resume, and this
> already includes the update of the whole jack status.
> Thus, executing jackpoll at that place means to perform the jack polling twice.
> 
> In other words, what's we need to achieve is just to call
> snd_hda_resume() appropriately in the runtime resume case.
> Of course, this will need more fixes for avoiding reentrance, etc.

Thanks for clarification on the internal logic, Takashi.
I will modify the patch and do some test tormorrow when I'm at office.

> 
> But, it leads to another question: do we need a full resume just for jack
> detection and user-space notification?  Just reading the pin detect state
> should be able to run even in D3 (for chips that are capable of it), and the
> notification itself doesn't need any audio hardware action.

It will not harm the basic audio playback function. In case audio controller is in runtime suspend mode,
Jack detection may not be detected, but when user start to play audio, It will wake up system and
Jack event will be updated then. So user will not hear sound from speaker in such case.

The exception is user space did not get real-time jack event, it will confuse user.

Thanks
--xingchao
> 
> 
> Takashi

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Takashi Iwai]

At Wed, 24 Jul 2013 13:40:15 +0000,
Wang, Xingchao wrote:
> 
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, July 24, 2013 6:49 PM
> > To: Wang, Xingchao
> > Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> > Girdwood, Liam R
> > Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> > 
> > At Tue, 23 Jul 2013 08:09:02 +0000,
> > Wang, Xingchao wrote:
> > >
> > > > >
> > > > > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > > > > ---
> > > > >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > > > >   1 file changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > > index 8860dd5..a7ac7fd 100644
> > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct
> > > > > device
> > > > *dev)
> > > > >   {
> > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > >   	struct azx *chip = card->private_data;
> > > > > +	int status;
> > > > > +
> > > > > +	/* enable controller wake up event */
> > > > > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > > > +		  STATESTS_INT_MASK);
> > > > >
> > > > >   	azx_stop_chip(chip);
> > > > >   	azx_enter_link_reset(chip);
> > > > > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct
> > > > > device
> > > > *dev)
> > > > >   {
> > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > >   	struct azx *chip = card->private_data;
> > > > > +	struct hda_bus *bus;
> > > > > +	struct hda_codec *codec;
> > > > > +	int status;
> > > > >
> > > > >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > > > >   		hda_display_power(true);
> > > > > +
> > > > > +	/* Read STATESTS before controller reset */
> > > > > +	status = azx_readw(chip, STATESTS);
> > > > > +
> > > > >   	azx_init_pci(chip);
> > > > >   	azx_init_chip(chip, 1);
> > > > > +
> > > > > +	bus = chip->bus;
> > > > > +	if (status && bus) {
> > > > > +		list_for_each_entry(codec, &bus->codec_list, list)
> > > > > +			if (status & (1 << codec->addr))
> > > > > +				queue_delayed_work(codec->bus->workq,
> > > > > +						   &codec->jackpoll_work,
> > > > codec->jackpoll_interval);
> > > >
> > > > Is there a reason you want to move the jack detection to a delayed
> > > > work, i e, why can't we just call:
> > > >
> > > >   	snd_hda_jack_set_dirty_all(codec);
> > > >   	snd_hda_jack_poll_all(codec);
> > > >
> > > > ...from here?
> > >
> > > In fact it doesnot work for me, It will again call runtime_resume() and caused
> > dead loop.
> > 
> > It means that the power refcount is messed up by this way.
> > 
> > When a verb is executed, the codec goes to a full resume mode, which turns up
> > the controller, eventually calling azx_power_notify().
> > In azx_power_notify(), pm_runtime_{get|put}_sync() is called accordingly,
> > which again goes to runtime resume.  A quick fix would be to add a flag or
> > something to avoid reentrace.
> > 
> > But, calling the jackpoll in the resume is basically superfluous.
> > As already mentioned, issuing a verb itself triggers the full resume, and this
> > already includes the update of the whole jack status.
> > Thus, executing jackpoll at that place means to perform the jack polling twice.
> > 
> > In other words, what's we need to achieve is just to call
> > snd_hda_resume() appropriately in the runtime resume case.
> > Of course, this will need more fixes for avoiding reentrance, etc.
> 
> Thanks for clarification on the internal logic, Takashi.
> I will modify the patch and do some test tormorrow when I'm at office.

Well, it might be that reusing the jackpoll would be simpler in the
end.  But, even if it's so, give a better description in the first
patch.  It doesn't clarify _why_ it's needed.  And need to clarify
that it doesn't give any ill-effect, too.

(And actually there will be an impact by that change; patch_via.c
 changes the jackpoll_interval dynamically, and the work will be still
 pending with jackpoll_interval=0.  This code should be modified
 before your patch, i.e. clearing the jackpoll_interval before
 cancel_work_sync().)


> > But, it leads to another question: do we need a full resume just for jack
> > detection and user-space notification?  Just reading the pin detect state
> > should be able to run even in D3 (for chips that are capable of it), and the
> > notification itself doesn't need any audio hardware action.
> 
> It will not harm the basic audio playback function. In case audio controller is in runtime suspend mode,
> Jack detection may not be detected, but when user start to play audio, It will wake up system and
> Jack event will be updated then. So user will not hear sound from speaker in such case.
> 
> The exception is user space did not get real-time jack event, it will confuse user.

Yes, and doing the full resume takes time and power.  So, if it's
avoidable, it'd be better to avoid it.  But, again, it depends on how
complex it'd become.


Takashi

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Takashi Iwai]

At Wed, 24 Jul 2013 15:52:16 +0200,
Takashi Iwai wrote:
> 
> At Wed, 24 Jul 2013 13:40:15 +0000,
> Wang, Xingchao wrote:
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Wednesday, July 24, 2013 6:49 PM
> > > To: Wang, Xingchao
> > > Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> > > Girdwood, Liam R
> > > Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> > > 
> > > At Tue, 23 Jul 2013 08:09:02 +0000,
> > > Wang, Xingchao wrote:
> > > >
> > > > > >
> > > > > > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > > > > > ---
> > > > > >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > > > > >   1 file changed, 25 insertions(+)
> > > > > >
> > > > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > > > index 8860dd5..a7ac7fd 100644
> > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct
> > > > > > device
> > > > > *dev)
> > > > > >   {
> > > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > > >   	struct azx *chip = card->private_data;
> > > > > > +	int status;
> > > > > > +
> > > > > > +	/* enable controller wake up event */
> > > > > > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > > > > +		  STATESTS_INT_MASK);
> > > > > >
> > > > > >   	azx_stop_chip(chip);
> > > > > >   	azx_enter_link_reset(chip);
> > > > > > @@ -2983,11 +2988,31 @@ static int azx_runtime_resume(struct
> > > > > > device
> > > > > *dev)
> > > > > >   {
> > > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > > >   	struct azx *chip = card->private_data;
> > > > > > +	struct hda_bus *bus;
> > > > > > +	struct hda_codec *codec;
> > > > > > +	int status;
> > > > > >
> > > > > >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > > > > >   		hda_display_power(true);
> > > > > > +
> > > > > > +	/* Read STATESTS before controller reset */
> > > > > > +	status = azx_readw(chip, STATESTS);
> > > > > > +
> > > > > >   	azx_init_pci(chip);
> > > > > >   	azx_init_chip(chip, 1);
> > > > > > +
> > > > > > +	bus = chip->bus;
> > > > > > +	if (status && bus) {
> > > > > > +		list_for_each_entry(codec, &bus->codec_list, list)
> > > > > > +			if (status & (1 << codec->addr))
> > > > > > +				queue_delayed_work(codec->bus->workq,
> > > > > > +						   &codec->jackpoll_work,
> > > > > codec->jackpoll_interval);
> > > > >
> > > > > Is there a reason you want to move the jack detection to a delayed
> > > > > work, i e, why can't we just call:
> > > > >
> > > > >   	snd_hda_jack_set_dirty_all(codec);
> > > > >   	snd_hda_jack_poll_all(codec);
> > > > >
> > > > > ...from here?
> > > >
> > > > In fact it doesnot work for me, It will again call runtime_resume() and caused
> > > dead loop.
> > > 
> > > It means that the power refcount is messed up by this way.
> > > 
> > > When a verb is executed, the codec goes to a full resume mode, which turns up
> > > the controller, eventually calling azx_power_notify().
> > > In azx_power_notify(), pm_runtime_{get|put}_sync() is called accordingly,
> > > which again goes to runtime resume.  A quick fix would be to add a flag or
> > > something to avoid reentrace.
> > > 
> > > But, calling the jackpoll in the resume is basically superfluous.
> > > As already mentioned, issuing a verb itself triggers the full resume, and this
> > > already includes the update of the whole jack status.
> > > Thus, executing jackpoll at that place means to perform the jack polling twice.
> > > 
> > > In other words, what's we need to achieve is just to call
> > > snd_hda_resume() appropriately in the runtime resume case.
> > > Of course, this will need more fixes for avoiding reentrance, etc.
> > 
> > Thanks for clarification on the internal logic, Takashi.
> > I will modify the patch and do some test tormorrow when I'm at office.
> 
> Well, it might be that reusing the jackpoll would be simpler in the
> end.  But, even if it's so, give a better description in the first
> patch.  It doesn't clarify _why_ it's needed.  And need to clarify
> that it doesn't give any ill-effect, too.
> 
> (And actually there will be an impact by that change; patch_via.c
>  changes the jackpoll_interval dynamically, and the work will be still
>  pending with jackpoll_interval=0.  This code should be modified
>  before your patch, i.e. clearing the jackpoll_interval before
>  cancel_work_sync().)

BTW, one thing I'm not sure after reading the patch is how the runtime
power refcount is managed after this wakeup is handled.  That is, the
device goes down to the runtime suspend properly again after the pin
detection and notification is done?


Takashi

Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm

[Wang, Xingchao]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 24, 2013 10:02 PM
> To: Wang, Xingchao
> Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> Girdwood, Liam R
> Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for runtime pm
> 
> At Wed, 24 Jul 2013 15:52:16 +0200,
> Takashi Iwai wrote:
> >
> > At Wed, 24 Jul 2013 13:40:15 +0000,
> > Wang, Xingchao wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > > Sent: Wednesday, July 24, 2013 6:49 PM
> > > > To: Wang, Xingchao
> > > > Cc: David Henningsson; Wang Xingchao; alsa-devel@alsa-project.org;
> > > > Girdwood, Liam R
> > > > Subject: Re: [PATCH 2/3] ALSA: hda - WAKEEN feature enabling for
> > > > runtime pm
> > > >
> > > > At Tue, 23 Jul 2013 08:09:02 +0000, Wang, Xingchao wrote:
> > > > >
> > > > > > >
> > > > > > > Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> > > > > > > ---
> > > > > > >   sound/pci/hda/hda_intel.c | 25 +++++++++++++++++++++++++
> > > > > > >   1 file changed, 25 insertions(+)
> > > > > > >
> > > > > > > diff --git a/sound/pci/hda/hda_intel.c
> > > > > > > b/sound/pci/hda/hda_intel.c index 8860dd5..a7ac7fd 100644
> > > > > > > --- a/sound/pci/hda/hda_intel.c
> > > > > > > +++ b/sound/pci/hda/hda_intel.c
> > > > > > > @@ -2970,6 +2970,11 @@ static int azx_runtime_suspend(struct
> > > > > > > device
> > > > > > *dev)
> > > > > > >   {
> > > > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > > > >   	struct azx *chip = card->private_data;
> > > > > > > +	int status;
> > > > > > > +
> > > > > > > +	/* enable controller wake up event */
> > > > > > > +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) |
> > > > > > > +		  STATESTS_INT_MASK);
> > > > > > >
> > > > > > >   	azx_stop_chip(chip);
> > > > > > >   	azx_enter_link_reset(chip); @@ -2983,11 +2988,31 @@
> > > > > > > static int azx_runtime_resume(struct device
> > > > > > *dev)
> > > > > > >   {
> > > > > > >   	struct snd_card *card = dev_get_drvdata(dev);
> > > > > > >   	struct azx *chip = card->private_data;
> > > > > > > +	struct hda_bus *bus;
> > > > > > > +	struct hda_codec *codec;
> > > > > > > +	int status;
> > > > > > >
> > > > > > >   	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
> > > > > > >   		hda_display_power(true);
> > > > > > > +
> > > > > > > +	/* Read STATESTS before controller reset */
> > > > > > > +	status = azx_readw(chip, STATESTS);
> > > > > > > +
> > > > > > >   	azx_init_pci(chip);
> > > > > > >   	azx_init_chip(chip, 1);
> > > > > > > +
> > > > > > > +	bus = chip->bus;
> > > > > > > +	if (status && bus) {
> > > > > > > +		list_for_each_entry(codec, &bus->codec_list, list)
> > > > > > > +			if (status & (1 << codec->addr))
> > > > > > > +				queue_delayed_work(codec->bus->workq,
> > > > > > > +						   &codec->jackpoll_work,
> > > > > > codec->jackpoll_interval);
> > > > > >
> > > > > > Is there a reason you want to move the jack detection to a
> > > > > > delayed work, i e, why can't we just call:
> > > > > >
> > > > > >   	snd_hda_jack_set_dirty_all(codec);
> > > > > >   	snd_hda_jack_poll_all(codec);
> > > > > >
> > > > > > ...from here?
> > > > >
> > > > > In fact it doesnot work for me, It will again call
> > > > > runtime_resume() and caused
> > > > dead loop.
> > > >
> > > > It means that the power refcount is messed up by this way.
> > > >
> > > > When a verb is executed, the codec goes to a full resume mode,
> > > > which turns up the controller, eventually calling azx_power_notify().
> > > > In azx_power_notify(), pm_runtime_{get|put}_sync() is called
> > > > accordingly, which again goes to runtime resume.  A quick fix
> > > > would be to add a flag or something to avoid reentrace.
> > > >
> > > > But, calling the jackpoll in the resume is basically superfluous.
> > > > As already mentioned, issuing a verb itself triggers the full
> > > > resume, and this already includes the update of the whole jack status.
> > > > Thus, executing jackpoll at that place means to perform the jack polling
> twice.
> > > >
> > > > In other words, what's we need to achieve is just to call
> > > > snd_hda_resume() appropriately in the runtime resume case.
> > > > Of course, this will need more fixes for avoiding reentrance, etc.
> > >
> > > Thanks for clarification on the internal logic, Takashi.
> > > I will modify the patch and do some test tormorrow when I'm at office.
> >
> > Well, it might be that reusing the jackpoll would be simpler in the
> > end.  But, even if it's so, give a better description in the first
> > patch.  It doesn't clarify _why_ it's needed.  And need to clarify
> > that it doesn't give any ill-effect, too.
> >
> > (And actually there will be an impact by that change; patch_via.c
> > changes the jackpoll_interval dynamically, and the work will be still
> > pending with jackpoll_interval=0.  This code should be modified
> > before your patch, i.e. clearing the jackpoll_interval before
> >  cancel_work_sync().)
> 
> BTW, one thing I'm not sure after reading the patch is how the runtime power
> refcount is managed after this wakeup is handled.  That is, the device goes
> down to the runtime suspend properly again after the pin detection and
> notification is done?
> 
Yes, once Jack event reported to userspace, audio driver enter runtime_suspend quickly.
But I did not track the power refcount changing in the test.

thanks
--xingchao
> 
> Takashi

[PATCH 3/3] ALSA: hda - use azx_writew() for 16-bit length register

[Wang Xingchao]

Register STATESTS is 16-bit length, use correct API for read/write.

Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
---
 sound/pci/hda/hda_intel.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index a7ac7fd..c6f7c19 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1160,7 +1160,7 @@ static int azx_reset(struct azx *chip, int full_reset)
 		goto __skip;
 
 	/* clear STATESTS */
-	azx_writeb(chip, STATESTS, STATESTS_INT_MASK);
+	azx_writew(chip, STATESTS, STATESTS_INT_MASK);
 
 	/* reset controller */
 	azx_enter_link_reset(chip);
@@ -1242,7 +1242,7 @@ static void azx_int_clear(struct azx *chip)
 	}
 
 	/* clear STATESTS */
-	azx_writeb(chip, STATESTS, STATESTS_INT_MASK);
+	azx_writew(chip, STATESTS, STATESTS_INT_MASK);
 
 	/* clear rirb status */
 	azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
@@ -1451,8 +1451,8 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
 
 #if 0
 	/* clear state status int */
-	if (azx_readb(chip, STATESTS) & 0x04)
-		azx_writeb(chip, STATESTS, 0x04);
+	if (azx_readw(chip, STATESTS) & 0x04)
+		azx_writew(chip, STATESTS, 0x04);
 #endif
 	spin_unlock(&chip->reg_lock);
 	
-- 
1.8.3.2

Re: [PATCH 3/3] ALSA: hda - use azx_writew() for 16-bit length register

[Takashi Iwai]

At Mon, 22 Jul 2013 03:19:18 -0400,
Wang Xingchao wrote:
> 
> Register STATESTS is 16-bit length, use correct API for read/write.
> 
> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>

Applied this patch now, as this is a proper fix.


thanks,

Takashi

> ---
>  sound/pci/hda/hda_intel.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index a7ac7fd..c6f7c19 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1160,7 +1160,7 @@ static int azx_reset(struct azx *chip, int full_reset)
>  		goto __skip;
>  
>  	/* clear STATESTS */
> -	azx_writeb(chip, STATESTS, STATESTS_INT_MASK);
> +	azx_writew(chip, STATESTS, STATESTS_INT_MASK);
>  
>  	/* reset controller */
>  	azx_enter_link_reset(chip);
> @@ -1242,7 +1242,7 @@ static void azx_int_clear(struct azx *chip)
>  	}
>  
>  	/* clear STATESTS */
> -	azx_writeb(chip, STATESTS, STATESTS_INT_MASK);
> +	azx_writew(chip, STATESTS, STATESTS_INT_MASK);
>  
>  	/* clear rirb status */
>  	azx_writeb(chip, RIRBSTS, RIRB_INT_MASK);
> @@ -1451,8 +1451,8 @@ static irqreturn_t azx_interrupt(int irq, void *dev_id)
>  
>  #if 0
>  	/* clear state status int */
> -	if (azx_readb(chip, STATESTS) & 0x04)
> -		azx_writeb(chip, STATESTS, 0x04);
> +	if (azx_readw(chip, STATESTS) & 0x04)
> +		azx_writew(chip, STATESTS, 0x04);
>  #endif
>  	spin_unlock(&chip->reg_lock);
>  	
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/3] ALSA: hda - jack poll once if jackpoll_interval==0

[Takashi Iwai]

At Mon, 22 Jul 2013 03:19:16 -0400,
Wang Xingchao wrote:
> 
> jackpoll_interval used to poll jack event periodically, if it's 0,
> give the caller one chance to probe jack status.

Why?  I miss the background...

jackpoll_interval=0 means that the polling is disabled.  So, there
should be no polling at all.


Takashi

> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
> ---
>  sound/pci/hda/hda_codec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 8a005f0..74c11bc 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -1216,11 +1216,12 @@ static void hda_jackpoll_work(struct work_struct *work)
>  {
>  	struct hda_codec *codec =
>  		container_of(work, struct hda_codec, jackpoll_work.work);
> -	if (!codec->jackpoll_interval)
> -		return;
>  
>  	snd_hda_jack_set_dirty_all(codec);
>  	snd_hda_jack_poll_all(codec);
> +	if (!codec->jackpoll_interval)
> +		return;
> +
>  	queue_delayed_work(codec->bus->workq, &codec->jackpoll_work,
>  			   codec->jackpoll_interval);
>  }
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/3] ALSA: hda - jack poll once if jackpoll_interval==0

[David Henningsson]

On 07/24/2013 12:08 PM, Takashi Iwai wrote:
> At Mon, 22 Jul 2013 03:19:16 -0400,
> Wang Xingchao wrote:
>>
>> jackpoll_interval used to poll jack event periodically, if it's 0,
>> give the caller one chance to probe jack status.
>
> Why?  I miss the background...
>
> jackpoll_interval=0 means that the polling is disabled.  So, there
> should be no polling at all.

I wondered that too, but figured it's to make the next patch work. I 
asked why one couldn't just call snd_hda_jack_set_dirty/poll_all from 
runtime_resume, but xingchao said it lead to a loop of some sort.

Reference: 
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-July/064348.html

>
>
> Takashi
>
>> Signed-off-by: Wang Xingchao <xingchao.wang@linux.intel.com>
>> ---
>>   sound/pci/hda/hda_codec.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index 8a005f0..74c11bc 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -1216,11 +1216,12 @@ static void hda_jackpoll_work(struct work_struct *work)
>>   {
>>   	struct hda_codec *codec =
>>   		container_of(work, struct hda_codec, jackpoll_work.work);
>> -	if (!codec->jackpoll_interval)
>> -		return;
>>
>>   	snd_hda_jack_set_dirty_all(codec);
>>   	snd_hda_jack_poll_all(codec);
>> +	if (!codec->jackpoll_interval)
>> +		return;
>> +
>>   	queue_delayed_work(codec->bus->workq, &codec->jackpoll_work,
>>   			   codec->jackpoll_interval);
>>   }
>> --
>> 1.8.3.2
>>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic