Patchwork [1/3,Raring,SRU] ALSA: hda - Add power state filtering

login
register
mail settings
Submitter Tim Gardner
Date Aug. 4, 2013, 9:59 a.m.
Message ID <1375610369-16225-1-git-send-email-tim.gardner@canonical.com>
Download mbox | patch
Permalink /patch/264507/
State New
Headers show

Comments

Tim Gardner - Aug. 4, 2013, 9:59 a.m.
From: Takashi Iwai <tiwai@suse.de>

BugLink: http://bugs.launchpad.net/bugs/1183125

Add a hook to struct hda_codec for filtering the target power state of
each widget when powering up/down.  The current hackish EAPD check is
implemented as the default hook pointer, too.

This allows codec drivers to implement own power filter.  In the
upcoming changes, the generic parser will have the better power filter
based on the active paths.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
(cherry picked from commit 9419ab6b72325e20789a61004cf68dc9e909a009)

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 sound/pci/hda/hda_codec.c      |   39 +++++++++++++++++++++++++--------------
 sound/pci/hda/hda_codec.h      |    7 +++++--
 sound/pci/hda/patch_conexant.c |    2 +-
 sound/pci/hda/patch_sigmatel.c |    2 +-
 4 files changed, 32 insertions(+), 18 deletions(-)
Seth Forshee - Aug. 5, 2013, 8:03 a.m.
On Sun, Aug 04, 2013 at 03:59:27AM -0600, Tim Gardner wrote:
> From: Takashi Iwai <tiwai@suse.de>
> 
> BugLink: http://bugs.launchpad.net/bugs/1183125
> 
> Add a hook to struct hda_codec for filtering the target power state of
> each widget when powering up/down.  The current hackish EAPD check is
> implemented as the default hook pointer, too.
> 
> This allows codec drivers to implement own power filter.  In the
> upcoming changes, the generic parser will have the better power filter
> based on the active paths.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> (cherry picked from commit 9419ab6b72325e20789a61004cf68dc9e909a009)
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

This patch looks like it should have no functional change, and I don't
think either of the subsequent patches depend on it. It looks safe
enough, but it does contain a fair amount of churn. Are you sure this
one is needed?

Seth
Tim Gardner - Aug. 5, 2013, 8:37 a.m.
On 08/05/2013 09:03 AM, Seth Forshee wrote:
> On Sun, Aug 04, 2013 at 03:59:27AM -0600, Tim Gardner wrote:
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> BugLink: http://bugs.launchpad.net/bugs/1183125
>>
>> Add a hook to struct hda_codec for filtering the target power state of
>> each widget when powering up/down.  The current hackish EAPD check is
>> implemented as the default hook pointer, too.
>>
>> This allows codec drivers to implement own power filter.  In the
>> upcoming changes, the generic parser will have the better power filter
>> based on the active paths.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> (cherry picked from commit 9419ab6b72325e20789a61004cf68dc9e909a009)
>>
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>
> This patch looks like it should have no functional change, and I don't
> think either of the subsequent patches depend on it. It looks safe
> enough, but it does contain a fair amount of churn. Are you sure this
> one is needed?
>
> Seth
>

I was basing the first commit on the info in 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1183125/comments/5 
- so perhaps the simplest fix is just patch 2 & 3.

rtg
Jason DeRose - Aug. 5, 2013, 9:29 p.m.
So the 3rd commit (17df3f55652f7ea8fb1197b5c32e227b3da9f215) is written against
a version of snd_hda_codec_set_power_to_all() that takes 3 arguments, whereas
the current version of snd_hda_codec_set_power_to_all() in raring takes 4 args.

The 1st commit (9419ab6b72325e20789a61004cf68dc9e909a009) is what changed the
snd_hda_codec_set_power_to_all() function signature.  So although you can
cleanly cherry-pick just the 2nd and 3rd, you'll get a build failure.

----- Original Message -----
From: "Tim Gardner" <tim.gardner@canonical.com>
To: "Seth Forshee" <seth.forshee@canonical.com>
Cc: "Takashi Iwai" <tiwai@suse.de>, kernel-team@lists.ubuntu.com
Sent: Monday, August 5, 2013 2:37:56 AM
Subject: Re: [PATCH 1/3 Raring SRU] ALSA: hda - Add power state filtering

On 08/05/2013 09:03 AM, Seth Forshee wrote:
> On Sun, Aug 04, 2013 at 03:59:27AM -0600, Tim Gardner wrote:
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> BugLink: http://bugs.launchpad.net/bugs/1183125
>>
>> Add a hook to struct hda_codec for filtering the target power state of
>> each widget when powering up/down.  The current hackish EAPD check is
>> implemented as the default hook pointer, too.
>>
>> This allows codec drivers to implement own power filter.  In the
>> upcoming changes, the generic parser will have the better power filter
>> based on the active paths.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> (cherry picked from commit 9419ab6b72325e20789a61004cf68dc9e909a009)
>>
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>
> This patch looks like it should have no functional change, and I don't
> think either of the subsequent patches depend on it. It looks safe
> enough, but it does contain a fair amount of churn. Are you sure this
> one is needed?
>
> Seth
>

I was basing the first commit on the info in 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1183125/comments/5 
- so perhaps the simplest fix is just patch 2 & 3.

rtg
Tim Gardner - Aug. 6, 2013, 5:38 a.m.
On 08/05/2013 10:29 PM, Jason DeRose wrote:
> So the 3rd commit (17df3f55652f7ea8fb1197b5c32e227b3da9f215) is written against
> a version of snd_hda_codec_set_power_to_all() that takes 3 arguments, whereas
> the current version of snd_hda_codec_set_power_to_all() in raring takes 4 args.
>
> The 1st commit (9419ab6b72325e20789a61004cf68dc9e909a009) is what changed the
> snd_hda_codec_set_power_to_all() function signature.  So although you can
> cleanly cherry-pick just the 2nd and 3rd, you'll get a build failure.
>

Good point.
Tim Gardner - Aug. 6, 2013, 6:50 a.m.

Patch

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e330933..303c54b 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1206,6 +1206,8 @@  static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec,
 
 static unsigned int hda_set_power_state(struct hda_codec *codec,
 				unsigned int power_state);
+static unsigned int default_power_filter(struct hda_codec *codec, hda_nid_t nid,
+					 unsigned int power_state);
 
 /**
  * snd_hda_codec_new - create a HDA codec
@@ -1324,6 +1326,7 @@  int snd_hda_codec_new(struct hda_bus *bus,
 #endif
 	codec->epss = snd_hda_codec_get_supported_ps(codec, fg,
 					AC_PWRST_EPSS);
+	codec->power_filter = default_power_filter;
 
 	/* power-up all before initialization */
 	hda_set_power_state(codec, AC_PWRST_D0);
@@ -3521,29 +3524,23 @@  EXPORT_SYMBOL_HDA(snd_hda_sequence_write_cache);
 #endif /* CONFIG_PM */
 
 void snd_hda_codec_set_power_to_all(struct hda_codec *codec, hda_nid_t fg,
-				    unsigned int power_state,
-				    bool eapd_workaround)
+				    unsigned int power_state)
 {
 	hda_nid_t nid = codec->start_nid;
 	int i;
 
 	for (i = 0; i < codec->num_nodes; i++, nid++) {
 		unsigned int wcaps = get_wcaps(codec, nid);
+		unsigned int state = power_state;
 		if (!(wcaps & AC_WCAP_POWER))
 			continue;
-		/* don't power down the widget if it controls eapd and
-		 * EAPD_BTLENABLE is set.
-		 */
-		if (eapd_workaround && power_state == AC_PWRST_D3 &&
-		    get_wcaps_type(wcaps) == AC_WID_PIN &&
-		    (snd_hda_query_pin_caps(codec, nid) & AC_PINCAP_EAPD)) {
-			int eapd = snd_hda_codec_read(codec, nid, 0,
-						AC_VERB_GET_EAPD_BTLENABLE, 0);
-			if (eapd & 0x02)
+		if (codec->power_filter) {
+			state = codec->power_filter(codec, nid, power_state);
+			if (state != power_state && power_state == AC_PWRST_D3)
 				continue;
 		}
 		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE,
-				    power_state);
+				    state);
 	}
 }
 EXPORT_SYMBOL_HDA(snd_hda_codec_set_power_to_all);
@@ -3590,6 +3587,21 @@  static unsigned int hda_sync_power_state(struct hda_codec *codec,
 	return state;
 }
 
+/* don't power down the widget if it controls eapd and EAPD_BTLENABLE is set */
+static unsigned int default_power_filter(struct hda_codec *codec, hda_nid_t nid,
+					 unsigned int power_state)
+{
+	if (power_state == AC_PWRST_D3 &&
+	    get_wcaps_type(get_wcaps(codec, nid)) == AC_WID_PIN &&
+	    (snd_hda_query_pin_caps(codec, nid) & AC_PINCAP_EAPD)) {
+		int eapd = snd_hda_codec_read(codec, nid, 0,
+					      AC_VERB_GET_EAPD_BTLENABLE, 0);
+		if (eapd & 0x02)
+			return AC_PWRST_D0;
+	}
+	return power_state;
+}
+
 /*
  * set power state of the codec, and return the power state
  */
@@ -3615,8 +3627,7 @@  static unsigned int hda_set_power_state(struct hda_codec *codec,
 			snd_hda_codec_read(codec, fg, 0,
 					   AC_VERB_SET_POWER_STATE,
 					   power_state);
-			snd_hda_codec_set_power_to_all(codec, fg, power_state,
-						       true);
+			snd_hda_codec_set_power_to_all(codec, fg, power_state);
 		}
 		state = hda_sync_power_state(codec, fg, power_state);
 		if (!(state & AC_PWRST_ERROR))
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index af6259c..fa95d41 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -882,6 +882,10 @@  struct hda_codec {
 	spinlock_t power_lock;
 #endif
 
+	/* filter the requested power state per nid */
+	unsigned int (*power_filter)(struct hda_codec *codec, hda_nid_t nid,
+				     unsigned int power_state);
+
 	/* codec-specific additional proc output */
 	void (*proc_widget_hook)(struct snd_info_buffer *buffer,
 				 struct hda_codec *codec, hda_nid_t nid);
@@ -1038,8 +1042,7 @@  extern const struct snd_pcm_chmap_elem snd_pcm_2_1_chmaps[];
 void snd_hda_get_codec_name(struct hda_codec *codec, char *name, int namelen);
 void snd_hda_bus_reboot_notify(struct hda_bus *bus);
 void snd_hda_codec_set_power_to_all(struct hda_codec *codec, hda_nid_t fg,
-				    unsigned int power_state,
-				    bool eapd_workaround);
+				    unsigned int power_state);
 
 int snd_hda_lock_devices(struct hda_bus *bus);
 void snd_hda_unlock_devices(struct hda_bus *bus);
diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index 09fae16..0eea727 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -435,7 +435,7 @@  static void conexant_set_power(struct hda_codec *codec, hda_nid_t fg,
 	/* partial workaround for "azx_get_response timeout" */
 	if (power_state == AC_PWRST_D0)
 		msleep(10);
-	snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
+	snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }
 
 static int conexant_init(struct hda_codec *codec)
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c
index a86547c..4b6cffe 100644
--- a/sound/pci/hda/patch_sigmatel.c
+++ b/sound/pci/hda/patch_sigmatel.c
@@ -5110,7 +5110,7 @@  static void stac92xx_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 	}
 	snd_hda_codec_read(codec, fg, 0, AC_VERB_SET_POWER_STATE,
 			afg_power_state);
-	snd_hda_codec_set_power_to_all(codec, fg, power_state, true);
+	snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }
 #else
 #define stac92xx_suspend	NULL