[PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Lucas Stach]

Different from other Tegra sound controllers drivers, the AC97
controller driver uses the tegra asoc utils directly to request the
needed clocks, as they are needed at AC97 init time. Move the DT clock
defines to the right place.

Signed-off-by: Lucas Stach <dev-8ppwABl0HbeELgA04lAiVw@public.gmane.org>
---
CC: <alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org>
CC: <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-colibri-512.dtsi | 5 -----
 arch/arm/boot/dts/tegra20.dtsi             | 6 +++++-
 sound/soc/tegra/tegra20_ac97.c             | 2 +-
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
index 2fcb3f2..fbb52e0 100644
--- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
@@ -491,11 +491,6 @@
 			"Mic", "MIC1";
 
 		nvidia,ac97-controller = <&ac97>;
-
-		clocks = <&tegra_car TEGRA20_CLK_PLL_A>,
-			 <&tegra_car TEGRA20_CLK_PLL_A_OUT0>,
-			 <&tegra_car TEGRA20_CLK_CDEV1>;
-		clock-names = "pll_a", "pll_a_out0", "mclk";
 	};
 
 	regulators {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 9653fd8..ad13f57 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -223,7 +223,11 @@
 		reg = <0x70002000 0x200>;
 		interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 12>;
-		clocks = <&tegra_car TEGRA20_CLK_AC97>;
+		clocks = <&tegra_car TEGRA20_CLK_PLL_A>,
+			 <&tegra_car TEGRA20_CLK_PLL_A_OUT0>,
+			 <&tegra_car TEGRA20_CLK_CDEV1>,
+			 <&tegra_car TEGRA20_CLK_AC97>;
+		clock-names = "pll_a", "pll_a_out0", "mclk", "ac97";
 		status = "disabled";
 	};
 
diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index 6c48662..6bbffd1 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -326,7 +326,7 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	}
 	dev_set_drvdata(&pdev->dev, ac97);
 
-	ac97->clk_ac97 = devm_clk_get(&pdev->dev, NULL);
+	ac97->clk_ac97 = devm_clk_get(&pdev->dev, "ac97");
 	if (IS_ERR(ac97->clk_ac97)) {
 		dev_err(&pdev->dev, "Can't retrieve ac97 clock\n");
 		ret = PTR_ERR(ac97->clk_ac97);
-- 
1.8.3.1

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Mark Brown]

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

On Sun, Jul 21, 2013 at 11:28:49PM +0200, Lucas Stach wrote:
> Different from other Tegra sound controllers drivers, the AC97
> controller driver uses the tegra asoc utils directly to request the
> needed clocks, as they are needed at AC97 init time. Move the DT clock
> defines to the right place.

I'm sorry but I just don't understand what this change is supposed to do
- what is the current place, what is wrong with it and what is the
correct place?

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

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

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Lucas Stach]

Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:
> On Sun, Jul 21, 2013 at 11:28:49PM +0200, Lucas Stach wrote:
> > Different from other Tegra sound controllers drivers, the AC97
> > controller driver uses the tegra asoc utils directly to request the
> > needed clocks, as they are needed at AC97 init time. Move the DT clock
> > defines to the right place.
> 
> I'm sorry but I just don't understand what this change is supposed to do
> - what is the current place, what is wrong with it and what is the
> correct place?

The clocks used by the Tegra ASoC utils were defined in the machine
driver DT node for all boards, as this is were they get requested by the
I2C and SPDIF Tegra audio drivers. Differently from those two the AC97
driver has to request those clocks in the controller drivers, as they
are needed at this point for proper initialisation.
So the patch moves the clocks from the machine driver node to the AC97
controller DT node, so they can be requested in the right driver.

Regards,
Lucas

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Mark Brown]

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

On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:
> Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:

> > I'm sorry but I just don't understand what this change is supposed to do
> > - what is the current place, what is wrong with it and what is the
> > correct place?

> The clocks used by the Tegra ASoC utils were defined in the machine
> driver DT node for all boards, as this is were they get requested by the
> I2C and SPDIF Tegra audio drivers. Differently from those two the AC97
> driver has to request those clocks in the controller drivers, as they
> are needed at this point for proper initialisation.
> So the patch moves the clocks from the machine driver node to the AC97
> controller DT node, so they can be requested in the right driver.

Why is the way the other devices are doing this sensible?

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

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Lucas Stach]

Am Montag, den 22.07.2013, 10:46 +0100 schrieb Mark Brown:
> On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:
> > Am Montag, den 22.07.2013, 00:36 +0100 schrieb Mark Brown:
> 
> > > I'm sorry but I just don't understand what this change is supposed to do
> > > - what is the current place, what is wrong with it and what is the
> > > correct place?
> 
> > The clocks used by the Tegra ASoC utils were defined in the machine
> > driver DT node for all boards, as this is were they get requested by the
> > I2C and SPDIF Tegra audio drivers. Differently from those two the AC97
> > driver has to request those clocks in the controller drivers, as they
> > are needed at this point for proper initialisation.
> > So the patch moves the clocks from the machine driver node to the AC97
> > controller DT node, so they can be requested in the right driver.
> 
> Why is the way the other devices are doing this sensible?

Because they only need those clocks when actually playing audio, so
requesting them late in the process when loading the machine driver is
perfectly fine for them. The AC97 controller however already needs those
clocks when initializing the controller and codec, so it has to request
them earlier.

Regards,
Lucas

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Mark Brown]

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

On Mon, Jul 22, 2013 at 06:26:20PM +0200, Lucas Stach wrote:
> Am Montag, den 22.07.2013, 10:46 +0100 schrieb Mark Brown:
> > On Mon, Jul 22, 2013 at 09:08:47AM +0200, Lucas Stach wrote:

> > > The clocks used by the Tegra ASoC utils were defined in the machine
> > > driver DT node for all boards, as this is were they get requested by the
> > > I2C and SPDIF Tegra audio drivers. Differently from those two the AC97

> > Why is the way the other devices are doing this sensible?

> Because they only need those clocks when actually playing audio, so
> requesting them late in the process when loading the machine driver is
> perfectly fine for them. The AC97 controller however already needs those
> clocks when initializing the controller and codec, so it has to request
> them earlier.

But why does it make sense to do that?  The clocks are directly
connected to the IP blocks in hardware after all..

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

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

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Stephen Warren]

On 07/21/2013 02:28 PM, Lucas Stach wrote:
> Different from other Tegra sound controllers drivers, the AC97
> controller driver uses the tegra asoc utils directly to request the
> needed clocks, as they are needed at AC97 init time. Move the DT clock
> defines to the right place.

I'm not convinced this is the correct approach.

The machine driver needs to manage these clocks, so that it can
co-ordinate between different audio paths. For example, consider a
system that supports two different I2S paths. In HW, if both are active
at once, these need to both run at a derivative of 48KHz or both run at
a derivative of 44.1KHz. The machine driver is the central place that
enforces that, and should eventually automatically place constraints on
one stream when another is configured for a specific sample rate. By the
same argument, AC'97 can't be a special case here, in case there's some
system with both AC'97 and I2S hooked up.

Re: [PATCH 1/4] ASOC: tegra: move AC97 clock defines to the controller node

[Lucas Stach]

Am Dienstag, den 23.07.2013, 09:47 -0700 schrieb Stephen Warren:
> On 07/21/2013 02:28 PM, Lucas Stach wrote:
> > Different from other Tegra sound controllers drivers, the AC97
> > controller driver uses the tegra asoc utils directly to request the
> > needed clocks, as they are needed at AC97 init time. Move the DT clock
> > defines to the right place.
> 
> I'm not convinced this is the correct approach.
> 
> The machine driver needs to manage these clocks, so that it can
> co-ordinate between different audio paths. For example, consider a
> system that supports two different I2S paths. In HW, if both are active
> at once, these need to both run at a derivative of 48KHz or both run at
> a derivative of 44.1KHz. The machine driver is the central place that
> enforces that, and should eventually automatically place constraints on
> one stream when another is configured for a specific sample rate. By the
> same argument, AC'97 can't be a special case here, in case there's some
> system with both AC'97 and I2S hooked up.

I find it highly unlikely to find any Tegra 20 board with such a
configuration. But as your argument is technically correct, I'll try to
fix things up the right way and see how big the impact is.

Regards,
Lucas

[PATCH 2/4] 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>
---
This got broken some time ago. No cc stable as it's not really a
regression, as I can't remember a single point in time when everything
AC97 related would have been in place and working.

CC: <alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org>
CC: <broonie-DgEjT+Ai2ygdnm+yROfE0A@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

Re: [PATCH 2/4] ASOC: tegra: fix matching of AC97 components

[Stephen Warren]

On 07/21/2013 02:28 PM, Lucas Stach wrote:
> Matching works completely based on the cpu of_node.

> diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c

>  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",

Shouldn't codec_of_node be used in preference to codec_name too?