* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-16 3:11 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-16 3:11 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Maxime Ripard, Chen-Yu Tsai, Marcus Cooper, Yong Deng,
alsa-devel, linux-arm-kernel
Cc: Vasily Khoruzhick
startup() and shutdown() hooks are called for both substreams,
so stopping either substream when another is running breaks the
latter.
E.g. playback breaks if capture is stopped when playback is running.
Move code from startup() and shutdown() to resume() and suspend()
hooks respectively to fix this issue
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: - rename from "ASoC: sun4i-i2s: don't try to start up
or shut down DAI if it's active"
- move code from startup/shutdown hooks into pm_runtime
sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
1 file changed, 25 insertions(+), 36 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index a4aa931ebfae..daa6c08cffbc 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
return 0;
}
-static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
- /* Enable the whole hardware block */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
-
- /* Enable the first output line */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_SDO_EN_MASK,
- SUN4I_I2S_CTRL_SDO_EN(0));
-
-
- return clk_prepare_enable(i2s->mod_clk);
-}
-
-static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
- clk_disable_unprepare(i2s->mod_clk);
-
- /* Disable our output lines */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
-
- /* Disable the whole hardware block */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_GL_EN, 0);
-}
-
static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int freq, int dir)
{
@@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
.hw_params = sun4i_i2s_hw_params,
.set_fmt = sun4i_i2s_set_fmt,
.set_sysclk = sun4i_i2s_set_sysclk,
- .shutdown = sun4i_i2s_shutdown,
- .startup = sun4i_i2s_startup,
.trigger = sun4i_i2s_trigger,
};
@@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
goto err_disable_clk;
}
+ /* Enable the whole hardware block */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
+
+ /* Enable the first output line */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_SDO_EN_MASK,
+ SUN4I_I2S_CTRL_SDO_EN(0));
+
+ ret = clk_prepare_enable(i2s->mod_clk);
+ if (ret) {
+ dev_err(dev, "Failed to enable module clock\n");
+ goto err_disable_clk;
+ }
+
return 0;
err_disable_clk:
@@ -880,6 +859,16 @@ static int sun4i_i2s_runtime_suspend(struct device *dev)
{
struct sun4i_i2s *i2s = dev_get_drvdata(dev);
+ clk_disable_unprepare(i2s->mod_clk);
+
+ /* Disable our output lines */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
+
+ /* Disable the whole hardware block */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_GL_EN, 0);
+
regcache_cache_only(i2s->regmap, true);
clk_disable_unprepare(i2s->bus_clk);
--
2.19.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-16 3:11 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-16 3:11 UTC (permalink / raw)
To: linux-arm-kernel
startup() and shutdown() hooks are called for both substreams,
so stopping either substream when another is running breaks the
latter.
E.g. playback breaks if capture is stopped when playback is running.
Move code from startup() and shutdown() to resume() and suspend()
hooks respectively to fix this issue
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: - rename from "ASoC: sun4i-i2s: don't try to start up
or shut down DAI if it's active"
- move code from startup/shutdown hooks into pm_runtime
sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
1 file changed, 25 insertions(+), 36 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index a4aa931ebfae..daa6c08cffbc 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
return 0;
}
-static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
- /* Enable the whole hardware block */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
-
- /* Enable the first output line */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_SDO_EN_MASK,
- SUN4I_I2S_CTRL_SDO_EN(0));
-
-
- return clk_prepare_enable(i2s->mod_clk);
-}
-
-static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
- struct snd_soc_dai *dai)
-{
- struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
-
- clk_disable_unprepare(i2s->mod_clk);
-
- /* Disable our output lines */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
-
- /* Disable the whole hardware block */
- regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
- SUN4I_I2S_CTRL_GL_EN, 0);
-}
-
static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int freq, int dir)
{
@@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
.hw_params = sun4i_i2s_hw_params,
.set_fmt = sun4i_i2s_set_fmt,
.set_sysclk = sun4i_i2s_set_sysclk,
- .shutdown = sun4i_i2s_shutdown,
- .startup = sun4i_i2s_startup,
.trigger = sun4i_i2s_trigger,
};
@@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
goto err_disable_clk;
}
+ /* Enable the whole hardware block */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
+
+ /* Enable the first output line */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_SDO_EN_MASK,
+ SUN4I_I2S_CTRL_SDO_EN(0));
+
+ ret = clk_prepare_enable(i2s->mod_clk);
+ if (ret) {
+ dev_err(dev, "Failed to enable module clock\n");
+ goto err_disable_clk;
+ }
+
return 0;
err_disable_clk:
@@ -880,6 +859,16 @@ static int sun4i_i2s_runtime_suspend(struct device *dev)
{
struct sun4i_i2s *i2s = dev_get_drvdata(dev);
+ clk_disable_unprepare(i2s->mod_clk);
+
+ /* Disable our output lines */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
+
+ /* Disable the whole hardware block */
+ regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+ SUN4I_I2S_CTRL_GL_EN, 0);
+
regcache_cache_only(i2s->regmap, true);
clk_disable_unprepare(i2s->bus_clk);
--
2.19.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-16 3:11 ` Vasily Khoruzhick
@ 2018-10-16 7:16 ` Maxime Ripard
-1 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-16 7:16 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, linux-arm-kernel
On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> startup() and shutdown() hooks are called for both substreams,
> so stopping either substream when another is running breaks the
> latter.
>
> E.g. playback breaks if capture is stopped when playback is running.
>
> Move code from startup() and shutdown() to resume() and suspend()
> hooks respectively to fix this issue
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> or shut down DAI if it's active"
> - move code from startup/shutdown hooks into pm_runtime
>
> sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index a4aa931ebfae..daa6c08cffbc 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> return 0;
> }
>
> -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> - /* Enable the whole hardware block */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> -
> - /* Enable the first output line */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_SDO_EN_MASK,
> - SUN4I_I2S_CTRL_SDO_EN(0));
> -
> -
> - return clk_prepare_enable(i2s->mod_clk);
> -}
> -
> -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> - clk_disable_unprepare(i2s->mod_clk);
> -
> - /* Disable our output lines */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> -
> - /* Disable the whole hardware block */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_GL_EN, 0);
> -}
> -
> static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq, int dir)
> {
> @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> .hw_params = sun4i_i2s_hw_params,
> .set_fmt = sun4i_i2s_set_fmt,
> .set_sysclk = sun4i_i2s_set_sysclk,
> - .shutdown = sun4i_i2s_shutdown,
> - .startup = sun4i_i2s_startup,
> .trigger = sun4i_i2s_trigger,
> };
>
> @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> goto err_disable_clk;
> }
>
> + /* Enable the whole hardware block */
> + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> +
So this is definitely needed
> + /* Enable the first output line */
> + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> + SUN4I_I2S_CTRL_SDO_EN_MASK,
> + SUN4I_I2S_CTRL_SDO_EN(0));
> +
> + ret = clk_prepare_enable(i2s->mod_clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable module clock\n");
> + goto err_disable_clk;
> + }
But we really don't want to enable the module clock before the
playback / capture actually starts. And I'm guessing it would make
more sense to keep the output enable configuration there as well,
since it's likely that we will have support for it in the future, and
we'll need additional information from ASoC when we'll do.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-16 7:16 ` Maxime Ripard
0 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-16 7:16 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> startup() and shutdown() hooks are called for both substreams,
> so stopping either substream when another is running breaks the
> latter.
>
> E.g. playback breaks if capture is stopped when playback is running.
>
> Move code from startup() and shutdown() to resume() and suspend()
> hooks respectively to fix this issue
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> or shut down DAI if it's active"
> - move code from startup/shutdown hooks into pm_runtime
>
> sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> 1 file changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index a4aa931ebfae..daa6c08cffbc 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> return 0;
> }
>
> -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> - /* Enable the whole hardware block */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> -
> - /* Enable the first output line */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_SDO_EN_MASK,
> - SUN4I_I2S_CTRL_SDO_EN(0));
> -
> -
> - return clk_prepare_enable(i2s->mod_clk);
> -}
> -
> -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> -
> - clk_disable_unprepare(i2s->mod_clk);
> -
> - /* Disable our output lines */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> -
> - /* Disable the whole hardware block */
> - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> - SUN4I_I2S_CTRL_GL_EN, 0);
> -}
> -
> static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq, int dir)
> {
> @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> .hw_params = sun4i_i2s_hw_params,
> .set_fmt = sun4i_i2s_set_fmt,
> .set_sysclk = sun4i_i2s_set_sysclk,
> - .shutdown = sun4i_i2s_shutdown,
> - .startup = sun4i_i2s_startup,
> .trigger = sun4i_i2s_trigger,
> };
>
> @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> goto err_disable_clk;
> }
>
> + /* Enable the whole hardware block */
> + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> +
So this is definitely needed
> + /* Enable the first output line */
> + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> + SUN4I_I2S_CTRL_SDO_EN_MASK,
> + SUN4I_I2S_CTRL_SDO_EN(0));
> +
> + ret = clk_prepare_enable(i2s->mod_clk);
> + if (ret) {
> + dev_err(dev, "Failed to enable module clock\n");
> + goto err_disable_clk;
> + }
But we really don't want to enable the module clock before the
playback / capture actually starts. And I'm guessing it would make
more sense to keep the output enable configuration there as well,
since it's likely that we will have support for it in the future, and
we'll need additional information from ASoC when we'll do.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-16 7:16 ` Maxime Ripard
@ 2018-10-17 3:08 ` Vasily Khoruzhick
-1 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 3:08 UTC (permalink / raw)
To: Maxime Ripard
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > startup() and shutdown() hooks are called for both substreams,
> > so stopping either substream when another is running breaks the
> > latter.
> >
> > E.g. playback breaks if capture is stopped when playback is running.
> >
> > Move code from startup() and shutdown() to resume() and suspend()
> > hooks respectively to fix this issue
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > or shut down DAI if it's active"
> > - move code from startup/shutdown hooks into pm_runtime
> >
> > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > 1 file changed, 25 insertions(+), 36 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index a4aa931ebfae..daa6c08cffbc 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > return 0;
> > }
> >
> > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > - struct snd_soc_dai *dai)
> > -{
> > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > -
> > - /* Enable the whole hardware block */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > -
> > - /* Enable the first output line */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > - SUN4I_I2S_CTRL_SDO_EN(0));
> > -
> > -
> > - return clk_prepare_enable(i2s->mod_clk);
> > -}
> > -
> > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > - struct snd_soc_dai *dai)
> > -{
> > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > -
> > - clk_disable_unprepare(i2s->mod_clk);
> > -
> > - /* Disable our output lines */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > -
> > - /* Disable the whole hardware block */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_GL_EN, 0);
> > -}
> > -
> > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > unsigned int freq, int dir)
> > {
> > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > .hw_params = sun4i_i2s_hw_params,
> > .set_fmt = sun4i_i2s_set_fmt,
> > .set_sysclk = sun4i_i2s_set_sysclk,
> > - .shutdown = sun4i_i2s_shutdown,
> > - .startup = sun4i_i2s_startup,
> > .trigger = sun4i_i2s_trigger,
> > };
> >
> > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > goto err_disable_clk;
> > }
> >
> > + /* Enable the whole hardware block */
> > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > +
>
> So this is definitely needed
>
> > + /* Enable the first output line */
> > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > + SUN4I_I2S_CTRL_SDO_EN(0));
> > +
> > + ret = clk_prepare_enable(i2s->mod_clk);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable module clock\n");
> > + goto err_disable_clk;
> > + }
>
> But we really don't want to enable the module clock before the
> playback / capture actually starts. And I'm guessing it would make
> more sense to keep the output enable configuration there as well,
> since it's likely that we will have support for it in the future, and
> we'll need additional information from ASoC when we'll do.
Should we go with v1 version of my patch then? I see no reason to
shuffle code around if we keep startup() and shutdown() hooks.
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 3:08 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 3:08 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > startup() and shutdown() hooks are called for both substreams,
> > so stopping either substream when another is running breaks the
> > latter.
> >
> > E.g. playback breaks if capture is stopped when playback is running.
> >
> > Move code from startup() and shutdown() to resume() and suspend()
> > hooks respectively to fix this issue
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > or shut down DAI if it's active"
> > - move code from startup/shutdown hooks into pm_runtime
> >
> > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > 1 file changed, 25 insertions(+), 36 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index a4aa931ebfae..daa6c08cffbc 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > return 0;
> > }
> >
> > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > - struct snd_soc_dai *dai)
> > -{
> > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > -
> > - /* Enable the whole hardware block */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > -
> > - /* Enable the first output line */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > - SUN4I_I2S_CTRL_SDO_EN(0));
> > -
> > -
> > - return clk_prepare_enable(i2s->mod_clk);
> > -}
> > -
> > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > - struct snd_soc_dai *dai)
> > -{
> > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > -
> > - clk_disable_unprepare(i2s->mod_clk);
> > -
> > - /* Disable our output lines */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > -
> > - /* Disable the whole hardware block */
> > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > - SUN4I_I2S_CTRL_GL_EN, 0);
> > -}
> > -
> > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > unsigned int freq, int dir)
> > {
> > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > .hw_params = sun4i_i2s_hw_params,
> > .set_fmt = sun4i_i2s_set_fmt,
> > .set_sysclk = sun4i_i2s_set_sysclk,
> > - .shutdown = sun4i_i2s_shutdown,
> > - .startup = sun4i_i2s_startup,
> > .trigger = sun4i_i2s_trigger,
> > };
> >
> > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > goto err_disable_clk;
> > }
> >
> > + /* Enable the whole hardware block */
> > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > +
>
> So this is definitely needed
>
> > + /* Enable the first output line */
> > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > + SUN4I_I2S_CTRL_SDO_EN(0));
> > +
> > + ret = clk_prepare_enable(i2s->mod_clk);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable module clock\n");
> > + goto err_disable_clk;
> > + }
>
> But we really don't want to enable the module clock before the
> playback / capture actually starts. And I'm guessing it would make
> more sense to keep the output enable configuration there as well,
> since it's likely that we will have support for it in the future, and
> we'll need additional information from ASoC when we'll do.
Should we go with v1 version of my patch then? I see no reason to
shuffle code around if we keep startup() and shutdown() hooks.
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 3:08 ` Vasily Khoruzhick
@ 2018-10-17 7:45 ` Maxime Ripard
-1 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 7:45 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
[-- Attachment #1.1: Type: text/plain, Size: 5005 bytes --]
On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > startup() and shutdown() hooks are called for both substreams,
> > > so stopping either substream when another is running breaks the
> > > latter.
> > >
> > > E.g. playback breaks if capture is stopped when playback is running.
> > >
> > > Move code from startup() and shutdown() to resume() and suspend()
> > > hooks respectively to fix this issue
> > >
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > or shut down DAI if it's active"
> > > - move code from startup/shutdown hooks into pm_runtime
> > >
> > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > index a4aa931ebfae..daa6c08cffbc 100644
> > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > return 0;
> > > }
> > >
> > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > - struct snd_soc_dai *dai)
> > > -{
> > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > -
> > > - /* Enable the whole hardware block */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > -
> > > - /* Enable the first output line */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > -
> > > -
> > > - return clk_prepare_enable(i2s->mod_clk);
> > > -}
> > > -
> > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > - struct snd_soc_dai *dai)
> > > -{
> > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > -
> > > - clk_disable_unprepare(i2s->mod_clk);
> > > -
> > > - /* Disable our output lines */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > -
> > > - /* Disable the whole hardware block */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > -}
> > > -
> > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > unsigned int freq, int dir)
> > > {
> > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > .hw_params = sun4i_i2s_hw_params,
> > > .set_fmt = sun4i_i2s_set_fmt,
> > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > - .shutdown = sun4i_i2s_shutdown,
> > > - .startup = sun4i_i2s_startup,
> > > .trigger = sun4i_i2s_trigger,
> > > };
> > >
> > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > goto err_disable_clk;
> > > }
> > >
> > > + /* Enable the whole hardware block */
> > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > +
> >
> > So this is definitely needed
> >
> > > + /* Enable the first output line */
> > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > +
> > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable module clock\n");
> > > + goto err_disable_clk;
> > > + }
> >
> > But we really don't want to enable the module clock before the
> > playback / capture actually starts. And I'm guessing it would make
> > more sense to keep the output enable configuration there as well,
> > since it's likely that we will have support for it in the future, and
> > we'll need additional information from ASoC when we'll do.
>
> Should we go with v1 version of my patch then? I see no reason to
> shuffle code around if we keep startup() and shutdown() hooks.
I'd go for something in between. runtime_pm already provides whether
the device is active or not, so you can just move the
SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
functions, and that should fix your issue.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 7:45 ` Maxime Ripard
0 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 7:45 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > startup() and shutdown() hooks are called for both substreams,
> > > so stopping either substream when another is running breaks the
> > > latter.
> > >
> > > E.g. playback breaks if capture is stopped when playback is running.
> > >
> > > Move code from startup() and shutdown() to resume() and suspend()
> > > hooks respectively to fix this issue
> > >
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > or shut down DAI if it's active"
> > > - move code from startup/shutdown hooks into pm_runtime
> > >
> > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > index a4aa931ebfae..daa6c08cffbc 100644
> > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > return 0;
> > > }
> > >
> > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > - struct snd_soc_dai *dai)
> > > -{
> > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > -
> > > - /* Enable the whole hardware block */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > -
> > > - /* Enable the first output line */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > -
> > > -
> > > - return clk_prepare_enable(i2s->mod_clk);
> > > -}
> > > -
> > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > - struct snd_soc_dai *dai)
> > > -{
> > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > -
> > > - clk_disable_unprepare(i2s->mod_clk);
> > > -
> > > - /* Disable our output lines */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > -
> > > - /* Disable the whole hardware block */
> > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > -}
> > > -
> > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > unsigned int freq, int dir)
> > > {
> > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > .hw_params = sun4i_i2s_hw_params,
> > > .set_fmt = sun4i_i2s_set_fmt,
> > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > - .shutdown = sun4i_i2s_shutdown,
> > > - .startup = sun4i_i2s_startup,
> > > .trigger = sun4i_i2s_trigger,
> > > };
> > >
> > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > goto err_disable_clk;
> > > }
> > >
> > > + /* Enable the whole hardware block */
> > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > +
> >
> > So this is definitely needed
> >
> > > + /* Enable the first output line */
> > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > +
> > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to enable module clock\n");
> > > + goto err_disable_clk;
> > > + }
> >
> > But we really don't want to enable the module clock before the
> > playback / capture actually starts. And I'm guessing it would make
> > more sense to keep the output enable configuration there as well,
> > since it's likely that we will have support for it in the future, and
> > we'll need additional information from ASoC when we'll do.
>
> Should we go with v1 version of my patch then? I see no reason to
> shuffle code around if we keep startup() and shutdown() hooks.
I'd go for something in between. runtime_pm already provides whether
the device is active or not, so you can just move the
SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
functions, and that should fix your issue.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181017/65c9e55c/attachment.sig>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 7:45 ` Maxime Ripard
@ 2018-10-17 14:41 ` Vasily Khoruzhick
-1 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 14:41 UTC (permalink / raw)
To: Maxime Ripard
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > startup() and shutdown() hooks are called for both substreams,
> > > > so stopping either substream when another is running breaks the
> > > > latter.
> > > >
> > > > E.g. playback breaks if capture is stopped when playback is running.
> > > >
> > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > hooks respectively to fix this issue
> > > >
> > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > ---
> > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > or shut down DAI if it's active"
> > > > - move code from startup/shutdown hooks into pm_runtime
> > > >
> > > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > > return 0;
> > > > }
> > > >
> > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > - struct snd_soc_dai *dai)
> > > > -{
> > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > -
> > > > - /* Enable the whole hardware block */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > -
> > > > - /* Enable the first output line */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > -
> > > > -
> > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > -}
> > > > -
> > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > > - struct snd_soc_dai *dai)
> > > > -{
> > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > -
> > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > -
> > > > - /* Disable our output lines */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > -
> > > > - /* Disable the whole hardware block */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > -}
> > > > -
> > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > > unsigned int freq, int dir)
> > > > {
> > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > > .hw_params = sun4i_i2s_hw_params,
> > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > - .shutdown = sun4i_i2s_shutdown,
> > > > - .startup = sun4i_i2s_startup,
> > > > .trigger = sun4i_i2s_trigger,
> > > > };
> > > >
> > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > > goto err_disable_clk;
> > > > }
> > > >
> > > > + /* Enable the whole hardware block */
> > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > +
> > >
> > > So this is definitely needed
> > >
> > > > + /* Enable the first output line */
> > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > +
> > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > + goto err_disable_clk;
> > > > + }
> > >
> > > But we really don't want to enable the module clock before the
> > > playback / capture actually starts. And I'm guessing it would make
> > > more sense to keep the output enable configuration there as well,
> > > since it's likely that we will have support for it in the future, and
> > > we'll need additional information from ASoC when we'll do.
> >
> > Should we go with v1 version of my patch then? I see no reason to
> > shuffle code around if we keep startup() and shutdown() hooks.
>
> I'd go for something in between. runtime_pm already provides whether
> the device is active or not, so you can just move the
> SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> functions, and that should fix your issue.
I still need to guard enabling output line with "if (dai->active)
return", so it doesn't fix the issue unfortunately.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 14:41 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 14:41 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > startup() and shutdown() hooks are called for both substreams,
> > > > so stopping either substream when another is running breaks the
> > > > latter.
> > > >
> > > > E.g. playback breaks if capture is stopped when playback is running.
> > > >
> > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > hooks respectively to fix this issue
> > > >
> > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > ---
> > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > or shut down DAI if it's active"
> > > > - move code from startup/shutdown hooks into pm_runtime
> > > >
> > > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > > return 0;
> > > > }
> > > >
> > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > - struct snd_soc_dai *dai)
> > > > -{
> > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > -
> > > > - /* Enable the whole hardware block */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > -
> > > > - /* Enable the first output line */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > -
> > > > -
> > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > -}
> > > > -
> > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > > - struct snd_soc_dai *dai)
> > > > -{
> > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > -
> > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > -
> > > > - /* Disable our output lines */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > -
> > > > - /* Disable the whole hardware block */
> > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > -}
> > > > -
> > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > > unsigned int freq, int dir)
> > > > {
> > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > > .hw_params = sun4i_i2s_hw_params,
> > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > - .shutdown = sun4i_i2s_shutdown,
> > > > - .startup = sun4i_i2s_startup,
> > > > .trigger = sun4i_i2s_trigger,
> > > > };
> > > >
> > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > > goto err_disable_clk;
> > > > }
> > > >
> > > > + /* Enable the whole hardware block */
> > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > +
> > >
> > > So this is definitely needed
> > >
> > > > + /* Enable the first output line */
> > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > +
> > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > + goto err_disable_clk;
> > > > + }
> > >
> > > But we really don't want to enable the module clock before the
> > > playback / capture actually starts. And I'm guessing it would make
> > > more sense to keep the output enable configuration there as well,
> > > since it's likely that we will have support for it in the future, and
> > > we'll need additional information from ASoC when we'll do.
> >
> > Should we go with v1 version of my patch then? I see no reason to
> > shuffle code around if we keep startup() and shutdown() hooks.
>
> I'd go for something in between. runtime_pm already provides whether
> the device is active or not, so you can just move the
> SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> functions, and that should fix your issue.
I still need to guard enabling output line with "if (dai->active)
return", so it doesn't fix the issue unfortunately.
>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 14:41 ` Vasily Khoruzhick
@ 2018-10-17 15:25 ` Maxime Ripard
-1 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 15:25 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > so stopping either substream when another is running breaks the
> > > > > latter.
> > > > >
> > > > > E.g. playback breaks if capture is stopped when playback is running.
> > > > >
> > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > hooks respectively to fix this issue
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > ---
> > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > or shut down DAI if it's active"
> > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > >
> > > > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > - struct snd_soc_dai *dai)
> > > > > -{
> > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > -
> > > > > - /* Enable the whole hardware block */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > > -
> > > > > - /* Enable the first output line */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > -
> > > > > -
> > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > -}
> > > > > -
> > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > > > - struct snd_soc_dai *dai)
> > > > > -{
> > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > -
> > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > -
> > > > > - /* Disable our output lines */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > -
> > > > > - /* Disable the whole hardware block */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > -}
> > > > > -
> > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > > > unsigned int freq, int dir)
> > > > > {
> > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > - .startup = sun4i_i2s_startup,
> > > > > .trigger = sun4i_i2s_trigger,
> > > > > };
> > > > >
> > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > > > goto err_disable_clk;
> > > > > }
> > > > >
> > > > > + /* Enable the whole hardware block */
> > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > > +
> > > >
> > > > So this is definitely needed
> > > >
> > > > > + /* Enable the first output line */
> > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > +
> > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > + goto err_disable_clk;
> > > > > + }
> > > >
> > > > But we really don't want to enable the module clock before the
> > > > playback / capture actually starts. And I'm guessing it would make
> > > > more sense to keep the output enable configuration there as well,
> > > > since it's likely that we will have support for it in the future, and
> > > > we'll need additional information from ASoC when we'll do.
> > >
> > > Should we go with v1 version of my patch then? I see no reason to
> > > shuffle code around if we keep startup() and shutdown() hooks.
> >
> > I'd go for something in between. runtime_pm already provides whether
> > the device is active or not, so you can just move the
> > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > functions, and that should fix your issue.
>
> I still need to guard enabling output line with "if (dai->active)
> return", so it doesn't fix the issue unfortunately.
Why? The configuration is static, so it probably doesn't change much?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 15:25 ` Maxime Ripard
0 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 15:25 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> >
> > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > so stopping either substream when another is running breaks the
> > > > > latter.
> > > > >
> > > > > E.g. playback breaks if capture is stopped when playback is running.
> > > > >
> > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > hooks respectively to fix this issue
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > ---
> > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > or shut down DAI if it's active"
> > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > >
> > > > > sound/soc/sunxi/sun4i-i2s.c | 61 +++++++++++++++----------------------
> > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > - struct snd_soc_dai *dai)
> > > > > -{
> > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > -
> > > > > - /* Enable the whole hardware block */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > > -
> > > > > - /* Enable the first output line */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > -
> > > > > -
> > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > -}
> > > > > -
> > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
> > > > > - struct snd_soc_dai *dai)
> > > > > -{
> > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > -
> > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > -
> > > > > - /* Disable our output lines */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > -
> > > > > - /* Disable the whole hardware block */
> > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > -}
> > > > > -
> > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > > > > unsigned int freq, int dir)
> > > > > {
> > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > - .startup = sun4i_i2s_startup,
> > > > > .trigger = sun4i_i2s_trigger,
> > > > > };
> > > > >
> > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct device *dev)
> > > > > goto err_disable_clk;
> > > > > }
> > > > >
> > > > > + /* Enable the whole hardware block */
> > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > + SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
> > > > > +
> > > >
> > > > So this is definitely needed
> > > >
> > > > > + /* Enable the first output line */
> > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > +
> > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > + if (ret) {
> > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > + goto err_disable_clk;
> > > > > + }
> > > >
> > > > But we really don't want to enable the module clock before the
> > > > playback / capture actually starts. And I'm guessing it would make
> > > > more sense to keep the output enable configuration there as well,
> > > > since it's likely that we will have support for it in the future, and
> > > > we'll need additional information from ASoC when we'll do.
> > >
> > > Should we go with v1 version of my patch then? I see no reason to
> > > shuffle code around if we keep startup() and shutdown() hooks.
> >
> > I'd go for something in between. runtime_pm already provides whether
> > the device is active or not, so you can just move the
> > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > functions, and that should fix your issue.
>
> I still need to guard enabling output line with "if (dai->active)
> return", so it doesn't fix the issue unfortunately.
Why? The configuration is static, so it probably doesn't change much?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 15:25 ` Maxime Ripard
@ 2018-10-17 15:36 ` Vasily Khoruzhick
-1 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 15:36 UTC (permalink / raw)
To: Maxime Ripard
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> >
> > <maxime.ripard@bootlin.com> wrote:
> > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > >
> > > > <maxime.ripard@bootlin.com> wrote:
> > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > so stopping either substream when another is running breaks the
> > > > > > latter.
> > > > > >
> > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > running.
> > > > > >
> > > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > > hooks respectively to fix this issue
> > > > > >
> > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > ---
> > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > >
> > > > > > or shut down DAI if it's active"
> > > > > >
> > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > >
> > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > +++++++++++++++----------------------
> > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > >
> > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > return 0;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > > - struct snd_soc_dai *dai)
> > > > > > -{
> > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > -
> > > > > > - /* Enable the whole hardware block */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > -
> > > > > > - /* Enable the first output line */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > -
> > > > > > -
> > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > -}
> > > > > > -
> > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > *substream,
> > > > > > - struct snd_soc_dai *dai)
> > > > > > -{
> > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > -
> > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > -
> > > > > > - /* Disable our output lines */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > -
> > > > > > - /* Disable the whole hardware block */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > -}
> > > > > > -
> > > > > >
> > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > clk_id,
> > > > > >
> > > > > > unsigned int freq, int dir)
> > > > > >
> > > > > > {
> > > > > >
> > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > >
> > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > - .startup = sun4i_i2s_startup,
> > > > > >
> > > > > > .trigger = sun4i_i2s_trigger,
> > > > > >
> > > > > > };
> > > > > >
> > > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct
> > > > > > device *dev)> > > > >
> > > > > > goto err_disable_clk;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > + /* Enable the whole hardware block */
> > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > +
> > > > >
> > > > > So this is definitely needed
> > > > >
> > > > > > + /* Enable the first output line */
> > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > +
> > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > + goto err_disable_clk;
> > > > > > + }
> > > > >
> > > > > But we really don't want to enable the module clock before the
> > > > > playback / capture actually starts. And I'm guessing it would make
> > > > > more sense to keep the output enable configuration there as well,
> > > > > since it's likely that we will have support for it in the future,
> > > > > and
> > > > > we'll need additional information from ASoC when we'll do.
> > > >
> > > > Should we go with v1 version of my patch then? I see no reason to
> > > > shuffle code around if we keep startup() and shutdown() hooks.
> > >
> > > I'd go for something in between. runtime_pm already provides whether
> > > the device is active or not, so you can just move the
> > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > functions, and that should fix your issue.
> >
> > I still need to guard enabling output line with "if (dai->active)
> > return", so it doesn't fix the issue unfortunately.
>
> Why? The configuration is static, so it probably doesn't change much?
startup() and shutdown() are called once for each substream, i.e. for capture
and playback.
If we start playback and capture and the stop either we end up with calling
startup() twice and then shutdown() once. Since it disables I2S output it
breaks substream that is still working.
>
> Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 15:36 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-17 15:36 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> >
> > <maxime.ripard@bootlin.com> wrote:
> > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > >
> > > > <maxime.ripard@bootlin.com> wrote:
> > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > so stopping either substream when another is running breaks the
> > > > > > latter.
> > > > > >
> > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > running.
> > > > > >
> > > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > > hooks respectively to fix this issue
> > > > > >
> > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > ---
> > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > >
> > > > > > or shut down DAI if it's active"
> > > > > >
> > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > >
> > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > +++++++++++++++----------------------
> > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > >
> > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > return 0;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > > - struct snd_soc_dai *dai)
> > > > > > -{
> > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > -
> > > > > > - /* Enable the whole hardware block */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > -
> > > > > > - /* Enable the first output line */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > -
> > > > > > -
> > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > -}
> > > > > > -
> > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > *substream,
> > > > > > - struct snd_soc_dai *dai)
> > > > > > -{
> > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > -
> > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > -
> > > > > > - /* Disable our output lines */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > -
> > > > > > - /* Disable the whole hardware block */
> > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > -}
> > > > > > -
> > > > > >
> > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > clk_id,
> > > > > >
> > > > > > unsigned int freq, int dir)
> > > > > >
> > > > > > {
> > > > > >
> > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > >
> > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > - .startup = sun4i_i2s_startup,
> > > > > >
> > > > > > .trigger = sun4i_i2s_trigger,
> > > > > >
> > > > > > };
> > > > > >
> > > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct
> > > > > > device *dev)> > > > >
> > > > > > goto err_disable_clk;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > + /* Enable the whole hardware block */
> > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > +
> > > > >
> > > > > So this is definitely needed
> > > > >
> > > > > > + /* Enable the first output line */
> > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > +
> > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > + goto err_disable_clk;
> > > > > > + }
> > > > >
> > > > > But we really don't want to enable the module clock before the
> > > > > playback / capture actually starts. And I'm guessing it would make
> > > > > more sense to keep the output enable configuration there as well,
> > > > > since it's likely that we will have support for it in the future,
> > > > > and
> > > > > we'll need additional information from ASoC when we'll do.
> > > >
> > > > Should we go with v1 version of my patch then? I see no reason to
> > > > shuffle code around if we keep startup() and shutdown() hooks.
> > >
> > > I'd go for something in between. runtime_pm already provides whether
> > > the device is active or not, so you can just move the
> > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > functions, and that should fix your issue.
> >
> > I still need to guard enabling output line with "if (dai->active)
> > return", so it doesn't fix the issue unfortunately.
>
> Why? The configuration is static, so it probably doesn't change much?
startup() and shutdown() are called once for each substream, i.e. for capture
and playback.
If we start playback and capture and the stop either we end up with calling
startup() twice and then shutdown() once. Since it disables I2S output it
breaks substream that is still working.
>
> Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 15:36 ` Vasily Khoruzhick
@ 2018-10-17 21:01 ` Maxime Ripard
-1 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 21:01 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> > >
> > > <maxime.ripard@bootlin.com> wrote:
> > > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > > >
> > > > > <maxime.ripard@bootlin.com> wrote:
> > > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > > so stopping either substream when another is running breaks the
> > > > > > > latter.
> > > > > > >
> > > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > > running.
> > > > > > >
> > > > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > > > hooks respectively to fix this issue
> > > > > > >
> > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > ---
> > > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > > >
> > > > > > > or shut down DAI if it's active"
> > > > > > >
> > > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > > >
> > > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > > +++++++++++++++----------------------
> > > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > > >
> > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > > return 0;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > > > - struct snd_soc_dai *dai)
> > > > > > > -{
> > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > -
> > > > > > > - /* Enable the whole hardware block */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > -
> > > > > > > - /* Enable the first output line */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > -
> > > > > > > -
> > > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > > -}
> > > > > > > -
> > > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > > *substream,
> > > > > > > - struct snd_soc_dai *dai)
> > > > > > > -{
> > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > -
> > > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > > -
> > > > > > > - /* Disable our output lines */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > > -
> > > > > > > - /* Disable the whole hardware block */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > > -}
> > > > > > > -
> > > > > > >
> > > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > > clk_id,
> > > > > > >
> > > > > > > unsigned int freq, int dir)
> > > > > > >
> > > > > > > {
> > > > > > >
> > > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > > >
> > > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > > - .startup = sun4i_i2s_startup,
> > > > > > >
> > > > > > > .trigger = sun4i_i2s_trigger,
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct
> > > > > > > device *dev)> > > > >
> > > > > > > goto err_disable_clk;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > + /* Enable the whole hardware block */
> > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > +
> > > > > >
> > > > > > So this is definitely needed
> > > > > >
> > > > > > > + /* Enable the first output line */
> > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > +
> > > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > > + if (ret) {
> > > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > > + goto err_disable_clk;
> > > > > > > + }
> > > > > >
> > > > > > But we really don't want to enable the module clock before the
> > > > > > playback / capture actually starts. And I'm guessing it would make
> > > > > > more sense to keep the output enable configuration there as well,
> > > > > > since it's likely that we will have support for it in the future,
> > > > > > and
> > > > > > we'll need additional information from ASoC when we'll do.
> > > > >
> > > > > Should we go with v1 version of my patch then? I see no reason to
> > > > > shuffle code around if we keep startup() and shutdown() hooks.
> > > >
> > > > I'd go for something in between. runtime_pm already provides whether
> > > > the device is active or not, so you can just move the
> > > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > > functions, and that should fix your issue.
> > >
> > > I still need to guard enabling output line with "if (dai->active)
> > > return", so it doesn't fix the issue unfortunately.
> >
> > Why? The configuration is static, so it probably doesn't change much?
>
> startup() and shutdown() are called once for each substream, i.e. for capture
> and playback.
>
> If we start playback and capture and the stop either we end up with calling
> startup() twice and then shutdown() once. Since it disables I2S output it
> breaks substream that is still working.
Ah, right, I forgot that case. In this case, yeah, your patch makes sense
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-17 21:01 ` Maxime Ripard
0 siblings, 0 replies; 20+ messages in thread
From: Maxime Ripard @ 2018-10-17 21:01 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> > >
> > > <maxime.ripard@bootlin.com> wrote:
> > > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > > >
> > > > > <maxime.ripard@bootlin.com> wrote:
> > > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > > so stopping either substream when another is running breaks the
> > > > > > > latter.
> > > > > > >
> > > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > > running.
> > > > > > >
> > > > > > > Move code from startup() and shutdown() to resume() and suspend()
> > > > > > > hooks respectively to fix this issue
> > > > > > >
> > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > ---
> > > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > > >
> > > > > > > or shut down DAI if it's active"
> > > > > > >
> > > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > > >
> > > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > > +++++++++++++++----------------------
> > > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > > >
> > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > > return 0;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
> > > > > > > - struct snd_soc_dai *dai)
> > > > > > > -{
> > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > -
> > > > > > > - /* Enable the whole hardware block */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > -
> > > > > > > - /* Enable the first output line */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > -
> > > > > > > -
> > > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > > -}
> > > > > > > -
> > > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > > *substream,
> > > > > > > - struct snd_soc_dai *dai)
> > > > > > > -{
> > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > -
> > > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > > -
> > > > > > > - /* Disable our output lines */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > > -
> > > > > > > - /* Disable the whole hardware block */
> > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > > -}
> > > > > > > -
> > > > > > >
> > > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > > clk_id,
> > > > > > >
> > > > > > > unsigned int freq, int dir)
> > > > > > >
> > > > > > > {
> > > > > > >
> > > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > > >
> > > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > > - .startup = sun4i_i2s_startup,
> > > > > > >
> > > > > > > .trigger = sun4i_i2s_trigger,
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > @@ -869,6 +833,21 @@ static int sun4i_i2s_runtime_resume(struct
> > > > > > > device *dev)> > > > >
> > > > > > > goto err_disable_clk;
> > > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > + /* Enable the whole hardware block */
> > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > +
> > > > > >
> > > > > > So this is definitely needed
> > > > > >
> > > > > > > + /* Enable the first output line */
> > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > +
> > > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > > + if (ret) {
> > > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > > + goto err_disable_clk;
> > > > > > > + }
> > > > > >
> > > > > > But we really don't want to enable the module clock before the
> > > > > > playback / capture actually starts. And I'm guessing it would make
> > > > > > more sense to keep the output enable configuration there as well,
> > > > > > since it's likely that we will have support for it in the future,
> > > > > > and
> > > > > > we'll need additional information from ASoC when we'll do.
> > > > >
> > > > > Should we go with v1 version of my patch then? I see no reason to
> > > > > shuffle code around if we keep startup() and shutdown() hooks.
> > > >
> > > > I'd go for something in between. runtime_pm already provides whether
> > > > the device is active or not, so you can just move the
> > > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > > functions, and that should fix your issue.
> > >
> > > I still need to guard enabling output line with "if (dai->active)
> > > return", so it doesn't fix the issue unfortunately.
> >
> > Why? The configuration is static, so it probably doesn't change much?
>
> startup() and shutdown() are called once for each substream, i.e. for capture
> and playback.
>
> If we start playback and capture and the stop either we end up with calling
> startup() twice and then shutdown() once. Since it disables I2S output it
> breaks substream that is still working.
Ah, right, I forgot that case. In this case, yeah, your patch makes sense
Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-17 21:01 ` Maxime Ripard
@ 2018-10-19 16:18 ` Vasily Khoruzhick
-1 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-19 16:18 UTC (permalink / raw)
To: Maxime Ripard
Cc: Linux-ALSA, Takashi Iwai, Liam Girdwood, Marcus Cooper,
Chen-Yu Tsai, Mark Brown, Yong Deng, arm-linux
On Wednesday, October 17, 2018 2:01:59 PM PDT Maxime Ripard wrote:
> On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> > On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> > > >
> > > > <maxime.ripard@bootlin.com> wrote:
> > > > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > > > >
> > > > > > <maxime.ripard@bootlin.com> wrote:
> > > > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick
wrote:
> > > > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > > > so stopping either substream when another is running breaks
> > > > > > > > the
> > > > > > > > latter.
> > > > > > > >
> > > > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > > > running.
> > > > > > > >
> > > > > > > > Move code from startup() and shutdown() to resume() and
> > > > > > > > suspend()
> > > > > > > > hooks respectively to fix this issue
> > > > > > > >
> > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > ---
> > > > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > > > >
> > > > > > > > or shut down DAI if it's active"
> > > > > > > >
> > > > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > > > >
> > > > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > > > +++++++++++++++----------------------
> > > > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream
> > > > > > > > *substream,
> > > > > > > > - struct snd_soc_dai *dai)
> > > > > > > > -{
> > > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > > -
> > > > > > > > - /* Enable the whole hardware block */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > > -
> > > > > > > > - /* Enable the first output line */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > > -
> > > > > > > > -
> > > > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > > > *substream,
> > > > > > > > - struct snd_soc_dai *dai)
> > > > > > > > -{
> > > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > > -
> > > > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > > > -
> > > > > > > > - /* Disable our output lines */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > > > -
> > > > > > > > - /* Disable the whole hardware block */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >
> > > > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > > > clk_id,
> > > > > > > >
> > > > > > > > unsigned int freq, int dir)
> > > > > > > >
> > > > > > > > {
> > > > > > > >
> > > > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > > >
> > > > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > > > >
> > > > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > > > - .startup = sun4i_i2s_startup,
> > > > > > > >
> > > > > > > > .trigger = sun4i_i2s_trigger,
> > > > > > > >
> > > > > > > > };
> > > > > > > >
> > > > > > > > @@ -869,6 +833,21 @@ static int
> > > > > > > > sun4i_i2s_runtime_resume(struct
> > > > > > > > device *dev)> > > > >
> > > > > > > >
> > > > > > > > goto err_disable_clk;
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > + /* Enable the whole hardware block */
> > > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > > +
> > > > > > >
> > > > > > > So this is definitely needed
> > > > > > >
> > > > > > > > + /* Enable the first output line */
> > > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > > +
> > > > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > > > + if (ret) {
> > > > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > > > + goto err_disable_clk;
> > > > > > > > + }
> > > > > > >
> > > > > > > But we really don't want to enable the module clock before the
> > > > > > > playback / capture actually starts. And I'm guessing it would
> > > > > > > make
> > > > > > > more sense to keep the output enable configuration there as
> > > > > > > well,
> > > > > > > since it's likely that we will have support for it in the
> > > > > > > future,
> > > > > > > and
> > > > > > > we'll need additional information from ASoC when we'll do.
> > > > > >
> > > > > > Should we go with v1 version of my patch then? I see no reason to
> > > > > > shuffle code around if we keep startup() and shutdown() hooks.
> > > > >
> > > > > I'd go for something in between. runtime_pm already provides whether
> > > > > the device is active or not, so you can just move the
> > > > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > > > functions, and that should fix your issue.
> > > >
> > > > I still need to guard enabling output line with "if (dai->active)
> > > > return", so it doesn't fix the issue unfortunately.
> > >
> > > Why? The configuration is static, so it probably doesn't change much?
> >
> > startup() and shutdown() are called once for each substream, i.e. for
> > capture and playback.
> >
> > If we start playback and capture and the stop either we end up with
> > calling
> > startup() twice and then shutdown() once. Since it disables I2S output it
> > breaks substream that is still working.
>
> Ah, right, I forgot that case. In this case, yeah, your patch makes sense
>
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Mark, can you pick up this one please? Thanks!
Regards,
Vasily
>
> Thanks!
> Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-19 16:18 ` Vasily Khoruzhick
0 siblings, 0 replies; 20+ messages in thread
From: Vasily Khoruzhick @ 2018-10-19 16:18 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday, October 17, 2018 2:01:59 PM PDT Maxime Ripard wrote:
> On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> > On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
> > > >
> > > > <maxime.ripard@bootlin.com> wrote:
> > > > > On Tue, Oct 16, 2018 at 08:08:41PM -0700, Vasily Khoruzhick wrote:
> > > > > > On Tue, Oct 16, 2018 at 12:16 AM Maxime Ripard
> > > > > >
> > > > > > <maxime.ripard@bootlin.com> wrote:
> > > > > > > On Mon, Oct 15, 2018 at 08:11:41PM -0700, Vasily Khoruzhick
wrote:
> > > > > > > > startup() and shutdown() hooks are called for both substreams,
> > > > > > > > so stopping either substream when another is running breaks
> > > > > > > > the
> > > > > > > > latter.
> > > > > > > >
> > > > > > > > E.g. playback breaks if capture is stopped when playback is
> > > > > > > > running.
> > > > > > > >
> > > > > > > > Move code from startup() and shutdown() to resume() and
> > > > > > > > suspend()
> > > > > > > > hooks respectively to fix this issue
> > > > > > > >
> > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > ---
> > > > > > > > v2: - rename from "ASoC: sun4i-i2s: don't try to start up
> > > > > > > >
> > > > > > > > or shut down DAI if it's active"
> > > > > > > >
> > > > > > > > - move code from startup/shutdown hooks into pm_runtime
> > > > > > > >
> > > > > > > > sound/soc/sunxi/sun4i-i2s.c | 61
> > > > > > > > +++++++++++++++----------------------
> > > > > > > > 1 file changed, 25 insertions(+), 36 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > index a4aa931ebfae..daa6c08cffbc 100644
> > > > > > > > --- a/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > > > > > > > @@ -644,40 +644,6 @@ static int sun4i_i2s_trigger(struct
> > > > > > > > snd_pcm_substream *substream, int cmd,> > > > >
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static int sun4i_i2s_startup(struct snd_pcm_substream
> > > > > > > > *substream,
> > > > > > > > - struct snd_soc_dai *dai)
> > > > > > > > -{
> > > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > > -
> > > > > > > > - /* Enable the whole hardware block */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_GL_EN,
> > > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > > -
> > > > > > > > - /* Enable the first output line */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > > -
> > > > > > > > -
> > > > > > > > - return clk_prepare_enable(i2s->mod_clk);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > -static void sun4i_i2s_shutdown(struct snd_pcm_substream
> > > > > > > > *substream,
> > > > > > > > - struct snd_soc_dai *dai)
> > > > > > > > -{
> > > > > > > > - struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > > > > > > > -
> > > > > > > > - clk_disable_unprepare(i2s->mod_clk);
> > > > > > > > -
> > > > > > > > - /* Disable our output lines */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_SDO_EN_MASK, 0);
> > > > > > > > -
> > > > > > > > - /* Disable the whole hardware block */
> > > > > > > > - regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > - SUN4I_I2S_CTRL_GL_EN, 0);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >
> > > > > > > > static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int
> > > > > > > > clk_id,
> > > > > > > >
> > > > > > > > unsigned int freq, int dir)
> > > > > > > >
> > > > > > > > {
> > > > > > > >
> > > > > > > > @@ -695,8 +661,6 @@ static const struct snd_soc_dai_ops
> > > > > > > > sun4i_i2s_dai_ops = {> > > > >
> > > > > > > >
> > > > > > > > .hw_params = sun4i_i2s_hw_params,
> > > > > > > > .set_fmt = sun4i_i2s_set_fmt,
> > > > > > > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > > > > > >
> > > > > > > > - .shutdown = sun4i_i2s_shutdown,
> > > > > > > > - .startup = sun4i_i2s_startup,
> > > > > > > >
> > > > > > > > .trigger = sun4i_i2s_trigger,
> > > > > > > >
> > > > > > > > };
> > > > > > > >
> > > > > > > > @@ -869,6 +833,21 @@ static int
> > > > > > > > sun4i_i2s_runtime_resume(struct
> > > > > > > > device *dev)> > > > >
> > > > > > > >
> > > > > > > > goto err_disable_clk;
> > > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > + /* Enable the whole hardware block */
> > > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > + SUN4I_I2S_CTRL_GL_EN,
> > > > > > > > SUN4I_I2S_CTRL_GL_EN);
> > > > > > > > +
> > > > > > >
> > > > > > > So this is definitely needed
> > > > > > >
> > > > > > > > + /* Enable the first output line */
> > > > > > > > + regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
> > > > > > > > + SUN4I_I2S_CTRL_SDO_EN_MASK,
> > > > > > > > + SUN4I_I2S_CTRL_SDO_EN(0));
> > > > > > > > +
> > > > > > > > + ret = clk_prepare_enable(i2s->mod_clk);
> > > > > > > > + if (ret) {
> > > > > > > > + dev_err(dev, "Failed to enable module clock\n");
> > > > > > > > + goto err_disable_clk;
> > > > > > > > + }
> > > > > > >
> > > > > > > But we really don't want to enable the module clock before the
> > > > > > > playback / capture actually starts. And I'm guessing it would
> > > > > > > make
> > > > > > > more sense to keep the output enable configuration there as
> > > > > > > well,
> > > > > > > since it's likely that we will have support for it in the
> > > > > > > future,
> > > > > > > and
> > > > > > > we'll need additional information from ASoC when we'll do.
> > > > > >
> > > > > > Should we go with v1 version of my patch then? I see no reason to
> > > > > > shuffle code around if we keep startup() and shutdown() hooks.
> > > > >
> > > > > I'd go for something in between. runtime_pm already provides whether
> > > > > the device is active or not, so you can just move the
> > > > > SUN4I_I2S_CTRL_GL_EN write to the runtime_resume and runtime_suspend
> > > > > functions, and that should fix your issue.
> > > >
> > > > I still need to guard enabling output line with "if (dai->active)
> > > > return", so it doesn't fix the issue unfortunately.
> > >
> > > Why? The configuration is static, so it probably doesn't change much?
> >
> > startup() and shutdown() are called once for each substream, i.e. for
> > capture and playback.
> >
> > If we start playback and capture and the stop either we end up with
> > calling
> > startup() twice and then shutdown() once. Since it disables I2S output it
> > breaks substream that is still working.
>
> Ah, right, I forgot that case. In this case, yeah, your patch makes sense
>
> Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>
Mark, can you pick up this one please? Thanks!
Regards,
Vasily
>
> Thanks!
> Maxime
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
2018-10-19 16:18 ` Vasily Khoruzhick
@ 2018-10-21 14:02 ` Mark Brown
-1 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2018-10-21 14:02 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Linux-ALSA, Maxime Ripard, Takashi Iwai, Liam Girdwood,
Marcus Cooper, Chen-Yu Tsai, Yong Deng, arm-linux
[-- Attachment #1.1: Type: text/plain, Size: 758 bytes --]
On Fri, Oct 19, 2018 at 09:18:18AM -0700, Vasily Khoruzhick wrote:
> On Wednesday, October 17, 2018 2:01:59 PM PDT Maxime Ripard wrote:
> > On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> > > On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > > > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> Mark, can you pick up this one please? Thanks!
Please resend, I dropped it due to the (initial) negative review
comments.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks
@ 2018-10-21 14:02 ` Mark Brown
0 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2018-10-21 14:02 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 19, 2018 at 09:18:18AM -0700, Vasily Khoruzhick wrote:
> On Wednesday, October 17, 2018 2:01:59 PM PDT Maxime Ripard wrote:
> > On Wed, Oct 17, 2018 at 08:36:26AM -0700, Vasily Khoruzhick wrote:
> > > On Wednesday, October 17, 2018 8:25:23 AM PDT Maxime Ripard wrote:
> > > > On Wed, Oct 17, 2018 at 07:41:55AM -0700, Vasily Khoruzhick wrote:
> > > > > On Wed, Oct 17, 2018 at 12:45 AM Maxime Ripard
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> Mark, can you pick up this one please? Thanks!
Please resend, I dropped it due to the (initial) negative review
comments.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181021/b7ba89f4/attachment-0001.sig>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-10-21 14:02 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16 3:11 [PATCH v2] ASoC: sun4i-i2s: move code from startup/shutdown hooks into pm_runtime hooks Vasily Khoruzhick
2018-10-16 3:11 ` Vasily Khoruzhick
2018-10-16 7:16 ` Maxime Ripard
2018-10-16 7:16 ` Maxime Ripard
2018-10-17 3:08 ` Vasily Khoruzhick
2018-10-17 3:08 ` Vasily Khoruzhick
2018-10-17 7:45 ` Maxime Ripard
2018-10-17 7:45 ` Maxime Ripard
2018-10-17 14:41 ` Vasily Khoruzhick
2018-10-17 14:41 ` Vasily Khoruzhick
2018-10-17 15:25 ` Maxime Ripard
2018-10-17 15:25 ` Maxime Ripard
2018-10-17 15:36 ` Vasily Khoruzhick
2018-10-17 15:36 ` Vasily Khoruzhick
2018-10-17 21:01 ` Maxime Ripard
2018-10-17 21:01 ` Maxime Ripard
2018-10-19 16:18 ` Vasily Khoruzhick
2018-10-19 16:18 ` Vasily Khoruzhick
2018-10-21 14:02 ` Mark Brown
2018-10-21 14:02 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.