| Message ID | 1387277564-4774-2-git-send-email-dev@lynxeye.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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".
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". -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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...
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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;