[PATCH v2 RESEND 1/2] ASOC: tegra: fix matching of AC97 components

[PATCH v2 RESEND 1/2] ASOC: tegra: fix matching of AC97 components

[Lucas Stach]

Matching works completely based on the cpu of_node.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: no changes

CC: <alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org>
CC: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
 sound/soc/tegra/tegra_wm9712.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c
index 5e11963..45b5789 100644
--- a/sound/soc/tegra/tegra_wm9712.c
+++ b/sound/soc/tegra/tegra_wm9712.c
@@ -55,7 +55,6 @@ static int tegra_wm9712_init(struct snd_soc_pcm_runtime *rtd)
 static struct snd_soc_dai_link tegra_wm9712_dai = {
 	.name = "AC97 HiFi",
 	.stream_name = "AC97 HiFi",
-	.cpu_dai_name = "tegra20-ac97",
 	.codec_dai_name = "wm9712-hifi",
 	.codec_name = "wm9712-codec",
 	.init = tegra_wm9712_init,
-- 
1.8.3.1

[PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Lucas Stach]

This corrects the Tegra AC97 clock handling to be in line with
the devicetree. Audio PLL is controlled by the machine driver, only
AC97 host controller clock remains to be independent.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
--
v2:
- remove redundant clock rate setting
- defer removal of err_clk_put label to avoid conflict with Stephens
  dma channel cleanup

CC: Mark Brown <broonie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
CC: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org

This is basically a v2 of "ASOC: tegra: move AC97 clock defines to the
controller node" incorporating the feedback I got from both Stephen and Mark.
---
 sound/soc/tegra/tegra20_ac97.c | 19 +++----------------
 sound/soc/tegra/tegra20_ac97.h |  1 -
 sound/soc/tegra/tegra_wm9712.c | 17 ++++++++++++++++-
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index ae27bcd..297fb17 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -37,7 +37,6 @@
 #include <sound/soc.h>
 #include <sound/dmaengine_pcm.h>
 
-#include "tegra_asoc_utils.h"
 #include "tegra20_ac97.h"
 
 #define DRV_NAME "tegra20-ac97"
@@ -387,24 +386,16 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	ac97->playback_dma_data.maxburst = 4;
 	ac97->playback_dma_data.slave_id = of_dma[1];
 
-	ret = tegra_asoc_utils_init(&ac97->util_data, &pdev->dev);
-	if (ret)
-		goto err_clk_put;
-
-	ret = tegra_asoc_utils_set_ac97_rate(&ac97->util_data);
-	if (ret)
-		goto err_asoc_utils_fini;
-
 	ret = clk_prepare_enable(ac97->clk_ac97);
 	if (ret) {
 		dev_err(&pdev->dev, "clk_enable failed: %d\n", ret);
-		goto err_asoc_utils_fini;
+		goto err;
 	}
 
 	ret = snd_soc_set_ac97_ops(&tegra20_ac97_ops);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to set AC'97 ops: %d\n", ret);
-		goto err_asoc_utils_fini;
+		goto err;
 	}
 
 	ret = snd_soc_register_component(&pdev->dev, &tegra20_ac97_component,
@@ -412,7 +403,7 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
 		ret = -ENOMEM;
-		goto err_asoc_utils_fini;
+		goto err;
 	}
 
 	ret = tegra_pcm_platform_register(&pdev->dev);
@@ -428,8 +419,6 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 
 err_unregister_component:
 	snd_soc_unregister_component(&pdev->dev);
-err_asoc_utils_fini:
-	tegra_asoc_utils_fini(&ac97->util_data);
 err_clk_put:
 err:
 	snd_soc_set_ac97_ops(NULL);
@@ -443,8 +432,6 @@ static int tegra20_ac97_platform_remove(struct platform_device *pdev)
 	tegra_pcm_platform_unregister(&pdev->dev);
 	snd_soc_unregister_component(&pdev->dev);
 
-	tegra_asoc_utils_fini(&ac97->util_data);
-
 	clk_disable_unprepare(ac97->clk_ac97);
 
 	snd_soc_set_ac97_ops(NULL);
diff --git a/sound/soc/tegra/tegra20_ac97.h b/sound/soc/tegra/tegra20_ac97.h
index 4acb3aa..0a39d82 100644
--- a/sound/soc/tegra/tegra20_ac97.h
+++ b/sound/soc/tegra/tegra20_ac97.h
@@ -90,6 +90,5 @@ struct tegra20_ac97 {
 	struct regmap *regmap;
 	int reset_gpio;
 	int sync_gpio;
-	struct tegra_asoc_utils_data util_data;
 };
 #endif /* __TEGRA20_AC97_H__ */
diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c
index 45b5789..25a7f82 100644
--- a/sound/soc/tegra/tegra_wm9712.c
+++ b/sound/soc/tegra/tegra_wm9712.c
@@ -29,10 +29,13 @@
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
 
+#include "tegra_asoc_utils.h"
+
 #define DRV_NAME "tegra-snd-wm9712"
 
 struct tegra_wm9712 {
 	struct platform_device *codec;
+	struct tegra_asoc_utils_data util_data;
 };
 
 static const struct snd_soc_dapm_widget tegra_wm9712_dapm_widgets[] = {
@@ -118,15 +121,25 @@ static int tegra_wm9712_driver_probe(struct platform_device *pdev)
 
 	tegra_wm9712_dai.platform_of_node = tegra_wm9712_dai.cpu_of_node;
 
+	ret = tegra_asoc_utils_init(&machine->util_data, &pdev->dev);
+	if (ret)
+		goto codec_unregister;
+
+	ret = tegra_asoc_utils_set_ac97_rate(&machine->util_data);
+	if (ret)
+		goto asoc_utils_fini;
+
 	ret = snd_soc_register_card(card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n",
 			ret);
-		goto codec_unregister;
+		goto asoc_utils_fini;
 	}
 
 	return 0;
 
+asoc_utils_fini:
+	tegra_asoc_utils_fini(&machine->util_data);
 codec_unregister:
 	platform_device_del(machine->codec);
 codec_put:
@@ -141,6 +154,8 @@ static int tegra_wm9712_driver_remove(struct platform_device *pdev)
 
 	snd_soc_unregister_card(card);
 
+	tegra_asoc_utils_fini(&machine->util_data);
+
 	platform_device_unregister(machine->codec);
 
 	return 0;
-- 
1.8.3.1

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Mark Brown]

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

On Tue, Dec 17, 2013 at 11:52:44AM +0100, Lucas Stach wrote:
> This corrects the Tegra AC97 clock handling to be in line with
> the devicetree. Audio PLL is controlled by the machine driver, only
> AC97 host controller clock remains to be independent.

I don't understand this commit message at all I'm afraid.  What does it
mean to "be in line with the devicetree" and what is the purpose of this
change?

I should also have mentioned on the previous patch: please use subject
lines matching the subsystem.  You've got "ASOC" not "ASoC".

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Lucas Stach]

Am Dienstag, den 17.12.2013, 14:00 +0000 schrieb Mark Brown:
> On Tue, Dec 17, 2013 at 11:52:44AM +0100, Lucas Stach wrote:
> > This corrects the Tegra AC97 clock handling to be in line with
> > the devicetree. Audio PLL is controlled by the machine driver, only
> > AC97 host controller clock remains to be independent.
> 
> I don't understand this commit message at all I'm afraid.  What does it
> mean to "be in line with the devicetree" and what is the purpose of this
> change?
> 
Clocks were added to the DT after the Tegra AC97 driver was initially
pushed upstream and the layout changed a bit in the process. Stephen
Warren convinced me that it's the right thing to let the Audio PLL be
controlled by the ASoC machine driver, rather than the AC97 host
controller driver.

So currently the host controller driver asks for clocks that aren't
there in the DT. This change corrects the clock handling so that the
host controller driver only ask for it's own clock, but the Audio PLL
handling is pushed into the machine driver.

Regards,
Lucas

> I should also have mentioned on the previous patch: please use subject
> lines matching the subsystem.  You've got "ASOC" not "ASoC".

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Mark Brown]

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

On Tue, Dec 17, 2013 at 03:49:53PM +0100, Lucas Stach wrote:
> Am Dienstag, den 17.12.2013, 14:00 +0000 schrieb Mark Brown:

> > I don't understand this commit message at all I'm afraid.  What does it
> > mean to "be in line with the devicetree" and what is the purpose of this
> > change?

> Clocks were added to the DT after the Tegra AC97 driver was initially
> pushed upstream and the layout changed a bit in the process. Stephen
> Warren convinced me that it's the right thing to let the Audio PLL be
> controlled by the ASoC machine driver, rather than the AC97 host
> controller driver.

> So currently the host controller driver asks for clocks that aren't
> there in the DT. This change corrects the clock handling so that the
> host controller driver only ask for it's own clock, but the Audio PLL
> handling is pushed into the machine driver.

Are we sure this is the best thing to do here?  There is standard
clocking behaviour specified in AC'97 so if the machine driver clocking
control includes that it's not clear that we want to vary it.  In any
case a clearer changelog explaining that the driver never worked due to
these clocking changes would be good.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Stephen Warren]

On 12/17/2013 02:43 PM, Mark Brown wrote:
> On Tue, Dec 17, 2013 at 03:49:53PM +0100, Lucas Stach wrote:
>> Am Dienstag, den 17.12.2013, 14:00 +0000 schrieb Mark Brown:
> 
>>> I don't understand this commit message at all I'm afraid.  What does it
>>> mean to "be in line with the devicetree" and what is the purpose of this
>>> change?
> 
>> Clocks were added to the DT after the Tegra AC97 driver was initially
>> pushed upstream and the layout changed a bit in the process. Stephen
>> Warren convinced me that it's the right thing to let the Audio PLL be
>> controlled by the ASoC machine driver, rather than the AC97 host
>> controller driver.
> 
>> So currently the host controller driver asks for clocks that aren't
>> there in the DT. This change corrects the clock handling so that the
>> host controller driver only ask for it's own clock, but the Audio PLL
>> handling is pushed into the machine driver.
> 
> Are we sure this is the best thing to do here?  There is standard
> clocking behaviour specified in AC'97 so if the machine driver clocking
> control includes that it's not clear that we want to vary it.  In any
> case a clearer changelog explaining that the driver never worked due to
> these clocking changes would be good.

The Tegra clocking architecture has a shared audio PLL that provides
clocks to the various IO controllers (I2S, AC'97, S/PDIF). In order to
allow multiple IO controllers to be in use at once, a single SW entity
has to manage the clocks, so that it can configure the audio PLL, rather
than having each individual IO controller attempt to assert ownership on
the shared resource. The centralized PLL management needs to switch the
PLL rate between 2 different values for 48-/44.1KHz-based audio for
example, and deny requests to switch if already-active audio is running
at the other rate.

So yes, I think doing this all in the machine driver is the best thing.

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Mark Brown]

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

On Tue, Dec 17, 2013 at 03:20:35PM -0700, Stephen Warren wrote:

> The Tegra clocking architecture has a shared audio PLL that provides
> clocks to the various IO controllers (I2S, AC'97, S/PDIF). In order to
> allow multiple IO controllers to be in use at once, a single SW entity
> has to manage the clocks, so that it can configure the audio PLL, rather
> than having each individual IO controller attempt to assert ownership on
> the shared resource. The centralized PLL management needs to switch the
> PLL rate between 2 different values for 48-/44.1KHz-based audio for
> example, and deny requests to switch if already-active audio is running
> at the other rate.

> So yes, I think doing this all in the machine driver is the best thing.

How does doing this in the machine driver help here?  The machine driver
isn't going to be any more coordinated with other machine drivers than
the controller is.

Note also that AC'97 will essentially force a constant always on clock
if you want to stay in spec which I imagine any system using AC'97 with
Tegra would want to do given that it's not exactly cutting edge...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Stephen Warren]

On 12/17/2013 03:28 PM, Mark Brown wrote:
> On Tue, Dec 17, 2013 at 03:20:35PM -0700, Stephen Warren wrote:
> 
>> The Tegra clocking architecture has a shared audio PLL that provides
>> clocks to the various IO controllers (I2S, AC'97, S/PDIF). In order to
>> allow multiple IO controllers to be in use at once, a single SW entity
>> has to manage the clocks, so that it can configure the audio PLL, rather
>> than having each individual IO controller attempt to assert ownership on
>> the shared resource. The centralized PLL management needs to switch the
>> PLL rate between 2 different values for 48-/44.1KHz-based audio for
>> example, and deny requests to switch if already-active audio is running
>> at the other rate.
> 
>> So yes, I think doing this all in the machine driver is the best thing.
> 
> How does doing this in the machine driver help here?  The machine driver
> isn't going to be any more coordinated with other machine drivers than
> the controller is.

There would only be one machine driver loaded at a time. It should
provide top-level control over all the audio paths in the Tegra audio
subsystem.

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Mark Brown]

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

On Tue, Dec 17, 2013 at 03:33:54PM -0700, Stephen Warren wrote:
> On 12/17/2013 03:28 PM, Mark Brown wrote:

> > How does doing this in the machine driver help here?  The machine driver
> > isn't going to be any more coordinated with other machine drivers than
> > the controller is.

> There would only be one machine driver loaded at a time. It should
> provide top-level control over all the audio paths in the Tegra audio
> subsystem.

OK, so that's not how everyone else has done these isolated
controllers...  to be honest I'm not convinced that this is something
worth worrying about for AC'97 - the clocking is fixed in the spec
unless you're being fancy and the chances of anyone being fancy are
minimal these days.  It's probably more likely to make life miserable
for people trying to get AC'97 working than anything else.

But like I say the big thing is a comprehensible changelog.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH v2 RESEND 2/2] ASOC: tegra: fix AC97 clock handling

[Lucas Stach]

Am Dienstag, den 17.12.2013, 22:39 +0000 schrieb Mark Brown:
> On Tue, Dec 17, 2013 at 03:33:54PM -0700, Stephen Warren wrote:
> > On 12/17/2013 03:28 PM, Mark Brown wrote:
> 
> > > How does doing this in the machine driver help here?  The machine driver
> > > isn't going to be any more coordinated with other machine drivers than
> > > the controller is.
> 
> > There would only be one machine driver loaded at a time. It should
> > provide top-level control over all the audio paths in the Tegra audio
> > subsystem.
> 
> OK, so that's not how everyone else has done these isolated
> controllers...  to be honest I'm not convinced that this is something
> worth worrying about for AC'97 - the clocking is fixed in the spec
> unless you're being fancy and the chances of anyone being fancy are
> minimal these days.  It's probably more likely to make life miserable
> for people trying to get AC'97 working than anything else.
> 
Essentially the only Tegra machine driver using AC97 is providing a
fixed clock. It's just a convention for all Tegra sound drivers to do
the PLL handling in the machine driver, I'd say it just more confusing
to do it another way just for this one corner case.

> But like I say the big thing is a comprehensible changelog.

Ok, I'll resend with a better changelog.

Regards,
Lucas

Re: [PATCH v2 RESEND 1/2] ASOC: tegra: fix matching of AC97 components

[Mark Brown]

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

On Tue, Dec 17, 2013 at 11:52:43AM +0100, Lucas Stach wrote:
> Matching works completely based on the cpu of_node.

Applied, though this isn't a fix at all so I updated the subject line.

> CC: Mark Brown <broonie@linaro.org>

Why are you adding my work address?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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