diff mbox

[v2,4/5] ALSA: hda: add component support

Message ID 1418118078-3676-3-git-send-email-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Imre Deak Dec. 9, 2014, 9:41 a.m. UTC
Register a component master to be used to interface with the i915
driver. This is meant to replace the current interface which is based on
module symbol lookups.

Note that currently we keep the existing behavior and pin the i915
module while the hda driver is loaded. Using the component interface
allows us to remove this dependency once support for dynamically
enabling / disabling the HDMI functionality is added to the driver.

v2:
- change roles between the hda and i915 components (Daniel)

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 sound/pci/hda/hda_i915.c  | 137 +++++++++++++++++++++++++++++++++-------------
 sound/pci/hda/hda_intel.c |   3 +-
 sound/pci/hda/hda_priv.h  |   4 ++
 3 files changed, 104 insertions(+), 40 deletions(-)

Comments

Daniel Vetter Dec. 9, 2014, 10:19 a.m. UTC | #1
On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> Register a component master to be used to interface with the i915
> driver. This is meant to replace the current interface which is based on
> module symbol lookups.
> 
> Note that currently we keep the existing behavior and pin the i915
> module while the hda driver is loaded. Using the component interface
> allows us to remove this dependency once support for dynamically
> enabling / disabling the HDMI functionality is added to the driver.
> 
> v2:
> - change roles between the hda and i915 components (Daniel)
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Ok, I think this is a good foundation to build on and later on add more
pieces to fix suspend/resume and similar.

But we also need to discuss how to merge this. My proposal is that I do a
special topic branch with just the patches in this series and then pull
that into drm-intel-next and send a pull request for it to Takashi for
sound-next, too. That way we don't have any depencies between drm and
sound for the 3.20 merge window.

Testing by intel QA is also solved this way since sound-next is already
pulled into drm-intel-nightly. And both gfx and sound QA use that branch.

Takashi/Dave, does this sound good?

Thanks, Daniel
Takashi Iwai Dec. 9, 2014, 5:30 p.m. UTC | #2
At Tue, 9 Dec 2014 11:19:34 +0100,
Daniel Vetter wrote:
> 
> On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> > Register a component master to be used to interface with the i915
> > driver. This is meant to replace the current interface which is based on
> > module symbol lookups.
> > 
> > Note that currently we keep the existing behavior and pin the i915
> > module while the hda driver is loaded. Using the component interface
> > allows us to remove this dependency once support for dynamically
> > enabling / disabling the HDMI functionality is added to the driver.
> > 
> > v2:
> > - change roles between the hda and i915 components (Daniel)
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Ok, I think this is a good foundation to build on and later on add more
> pieces to fix suspend/resume and similar.
> 
> But we also need to discuss how to merge this. My proposal is that I do a
> special topic branch with just the patches in this series and then pull
> that into drm-intel-next and send a pull request for it to Takashi for
> sound-next, too. That way we don't have any depencies between drm and
> sound for the 3.20 merge window.
> 
> Testing by intel QA is also solved this way since sound-next is already
> pulled into drm-intel-nightly. And both gfx and sound QA use that branch.
> 
> Takashi/Dave, does this sound good?

Yes, this sounds good.  Maybe it'd be good to base the branch on
3.19-rc1 so that all changes are covered?


Takashi
Daniel Vetter Dec. 10, 2014, 9:27 a.m. UTC | #3
On Tue, Dec 09, 2014 at 06:30:40PM +0100, Takashi Iwai wrote:
> At Tue, 9 Dec 2014 11:19:34 +0100,
> Daniel Vetter wrote:
> > 
> > On Tue, Dec 09, 2014 at 11:41:18AM +0200, Imre Deak wrote:
> > > Register a component master to be used to interface with the i915
> > > driver. This is meant to replace the current interface which is based on
> > > module symbol lookups.
> > > 
> > > Note that currently we keep the existing behavior and pin the i915
> > > module while the hda driver is loaded. Using the component interface
> > > allows us to remove this dependency once support for dynamically
> > > enabling / disabling the HDMI functionality is added to the driver.
> > > 
> > > v2:
> > > - change roles between the hda and i915 components (Daniel)
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Ok, I think this is a good foundation to build on and later on add more
> > pieces to fix suspend/resume and similar.
> > 
> > But we also need to discuss how to merge this. My proposal is that I do a
> > special topic branch with just the patches in this series and then pull
> > that into drm-intel-next and send a pull request for it to Takashi for
> > sound-next, too. That way we don't have any depencies between drm and
> > sound for the 3.20 merge window.
> > 
> > Testing by intel QA is also solved this way since sound-next is already
> > pulled into drm-intel-nightly. And both gfx and sound QA use that branch.
> > 
> > Takashi/Dave, does this sound good?
> 
> Yes, this sounds good.  Maybe it'd be good to base the branch on
> 3.19-rc1 so that all changes are covered?

Guess that depends when the patches are ready for merging and whether
there's conflicts. I have sound-next in my integration tree so will
noticed and can act accordingly.
-Daniel
diff mbox

Patch

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index 4e4b733..2bf6a1b 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -18,8 +18,10 @@ 
 
 #include <linux/init.h>
 #include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/component.h>
+#include <drm/i915_component.h>
 #include <sound/core.h>
-#include <drm/i915_powerwell.h>
 #include "hda_priv.h"
 #include "hda_i915.h"
 
@@ -31,32 +33,33 @@ 
 #define AZX_REG_EM4			0x100c
 #define AZX_REG_EM5			0x1010
 
-static int (*get_power)(void);
-static int (*put_power)(void);
-static int (*get_cdclk)(void);
-
 int hda_display_power(struct azx *chip, bool enable)
 {
-	if (!get_power || !put_power)
+	struct i915_display_component *dcomp = &chip->display_component;
+
+	if (!dcomp->ops)
 		return -ENODEV;
 
 	pr_debug("HDA display power %s \n",
 			enable ? "Enable" : "Disable");
 	if (enable)
-		return get_power();
+		dcomp->ops->get_power(dcomp->dev);
 	else
-		return put_power();
+		dcomp->ops->put_power(dcomp->dev);
+
+	return 0;
 }
 
 void haswell_set_bclk(struct azx *chip)
 {
 	int cdclk_freq;
 	unsigned int bclk_m, bclk_n;
+	struct i915_display_component *dcomp = &chip->display_component;
 
-	if (!get_cdclk)
+	if (!dcomp->ops)
 		return;
 
-	cdclk_freq = get_cdclk();
+	cdclk_freq = dcomp->ops->get_cdclk_freq(dcomp->dev);
 	switch (cdclk_freq) {
 	case 337500:
 		bclk_m = 16;
@@ -84,47 +87,104 @@  void haswell_set_bclk(struct azx *chip)
 	azx_writew(chip, EM5, bclk_n);
 }
 
+static int hda_component_master_bind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_display_component *dcomp = &chip->display_component;
+	int ret;
+
+	ret = component_bind_all(dev, dcomp);
+	if (ret < 0)
+		return ret;
+
+	if (WARN_ON(!(dcomp->dev && dcomp->ops && dcomp->ops->get_power &&
+		      dcomp->ops->put_power && dcomp->ops->get_cdclk_freq))) {
+		ret = -EINVAL;
+		goto out_unbind;
+	}
+
+	/*
+	 * Atm, we don't support dynamic unbinding initiated by the child
+	 * component, so pin its containing module until we unbind.
+	 */
+	if (!try_module_get(dcomp->ops->owner)) {
+		ret = -ENODEV;
+		goto out_unbind;
+	}
+
+	return 0;
+
+out_unbind:
+	component_unbind_all(dev, dcomp);
+
+	return ret;
+}
+
+static void hda_component_master_unbind(struct device *dev)
+{
+	struct snd_card *card = dev_get_drvdata(dev);
+	struct azx *chip = card->private_data;
+	struct i915_display_component *dcomp = &chip->display_component;
+
+	module_put(dcomp->ops->owner);
+	component_unbind_all(dev, dcomp);
+	WARN_ON(dcomp->ops || dcomp->dev);
+}
+
+static const struct component_master_ops hda_component_master_ops = {
+	.bind = hda_component_master_bind,
+	.unbind = hda_component_master_unbind,
+};
+
+static int hda_component_master_match(struct device *dev, void *data)
+{
+	/* i915 is the only supported component */
+	return !strcmp(dev->driver->name, "i915");
+}
 
 int hda_i915_init(struct azx *chip)
 {
-	int err = 0;
+	struct component_match *match = NULL;
+	struct device *dev = &chip->pci->dev;
+	struct i915_display_component *dcomp = &chip->display_component;
+	int ret;
 
-	get_power = symbol_request(i915_request_power_well);
-	if (!get_power) {
-		pr_warn("hda-i915: get_power symbol get fail\n");
-		return -ENODEV;
-	}
-
-	put_power = symbol_request(i915_release_power_well);
-	if (!put_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-		return -ENODEV;
-	}
-
-	get_cdclk = symbol_request(i915_get_cdclk_freq);
-	if (!get_cdclk)	/* may have abnormal BCLK and audio playback rate */
-		pr_warn("hda-i915: get_cdclk symbol get fail\n");
-
+	component_match_add(dev, &match, hda_component_master_match, chip);
+	ret = component_master_add_with_match(dev, &hda_component_master_ops,
+					      match);
+	if (ret < 0)
+		goto out_err;
+
+	/*
+	 * Atm, we don't support deferring the component binding, so make sure
+	 * i915 is loaded and that the binding successfully completes.
+	 */
+	ret = request_module("i915");
+	if (ret < 0)
+		goto out_master_del;
+
+	if (!dcomp->ops) {
+		ret = -ENODEV;
+		goto out_master_del;
+	}
+
-	pr_debug("HDA driver get symbol successfully from i915 module\n");
+	pr_debug("hda-i915: bound to i915 component\n");
+
+	return 0;
+out_master_del:
+	component_master_del(dev, &hda_component_master_ops);
+out_err:
+	pr_warn("hda-i915: failed to add component master (%d)\n", ret);
 
-	return err;
+	return ret;
 }
 
 int hda_i915_exit(struct azx *chip)
 {
-	if (get_power) {
-		symbol_put(i915_request_power_well);
-		get_power = NULL;
-	}
-	if (put_power) {
-		symbol_put(i915_release_power_well);
-		put_power = NULL;
-	}
-	if (get_cdclk) {
-		symbol_put(i915_get_cdclk_freq);
-		get_cdclk = NULL;
-	}
+	struct device *dev = &chip->pci->dev;
+
+	component_master_del(dev, &hda_component_master_ops);
 
 	return 0;
 }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f3b5dcd..e15b30e 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1912,8 +1912,7 @@  static int azx_probe_continue(struct azx *chip)
 #ifdef CONFIG_SND_HDA_I915
 		err = hda_i915_init(chip);
 		if (err < 0) {
-			dev_err(chip->card->dev,
-				"Error request power-well from i915\n");
+			dev_err(chip->card->dev, "Error binding to i915\n");
 			goto out_free;
 		}
 		err = hda_display_power(chip, true);
diff --git a/sound/pci/hda/hda_priv.h b/sound/pci/hda/hda_priv.h
index aa484fd..af227f3 100644
--- a/sound/pci/hda/hda_priv.h
+++ b/sound/pci/hda/hda_priv.h
@@ -18,6 +18,7 @@ 
 #include <linux/clocksource.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
+#include <drm/i915_component.h>
 
 /*
  * registers
@@ -291,6 +292,9 @@  struct azx {
 	struct pci_dev *pci;
 	int dev_index;
 
+	/* i915 component interface */
+	struct i915_display_component display_component;
+
 	/* chip type specific */
 	int driver_type;
 	unsigned int driver_caps;