[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

Otherwise we may end up with shutting down I2S if shutdown() was
called for capture substream, but playback is still running.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index a9ea329dc046..787b67c4f845 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -649,6 +649,9 @@ static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
 {
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 
+	if (dai->active)
+		return 0;
+
 	/* Enable the whole hardware block */
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
 			   SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
@@ -667,6 +670,9 @@ static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
 {
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 
+	if (dai->active)
+		return;
+
 	clk_disable_unprepare(i2s->mod_clk);
 
 	/* Disable our output lines */
-- 
2.19.0

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

Otherwise we may end up with shutting down I2S if shutdown() was
called for capture substream, but playback is still running.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 sound/soc/sunxi/sun4i-i2s.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index a9ea329dc046..787b67c4f845 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -649,6 +649,9 @@ static int sun4i_i2s_startup(struct snd_pcm_substream *substream,
 {
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 
+	if (dai->active)
+		return 0;
+
 	/* Enable the whole hardware block */
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
 			   SUN4I_I2S_CTRL_GL_EN, SUN4I_I2S_CTRL_GL_EN);
@@ -667,6 +670,9 @@ static void sun4i_i2s_shutdown(struct snd_pcm_substream *substream,
 {
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 
+	if (dai->active)
+		return;
+
 	clk_disable_unprepare(i2s->mod_clk);
 
 	/* Disable our output lines */
-- 
2.19.0

Re: [PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Mark Brown]

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

On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> Otherwise we may end up with shutting down I2S if shutdown() was
> called for capture substream, but playback is still running.

Would it be cleaner and more robust to use runtime PM?  I'm wondering
what happens if some of the configuration stuff turns out to also need
some of the clocks for example.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Mark Brown]

On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> Otherwise we may end up with shutting down I2S if shutdown() was
> called for capture substream, but playback is still running.

Would it be cleaner and more robust to use runtime PM?  I'm wondering
what happens if some of the configuration stuff turns out to also need
some of the clocks for example.
-------------- 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/20181012/99d71b37/attachment-0001.sig>

Re: [PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > Otherwise we may end up with shutting down I2S if shutdown() was
> > called for capture substream, but playback is still running.
> 
> Would it be cleaner and more robust to use runtime PM?  I'm wondering
> what happens if some of the configuration stuff turns out to also need
> some of the clocks for example.

I guess. I'm not sure why this code was put into startup and shutdown 
callbacks in first place.

Maybe Marcus or Maxime know?

As for configuration - only bus clock is necessary for configuration and it's 
already enabled in runtime_resume() callback.

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > Otherwise we may end up with shutting down I2S if shutdown() was
> > called for capture substream, but playback is still running.
> 
> Would it be cleaner and more robust to use runtime PM?  I'm wondering
> what happens if some of the configuration stuff turns out to also need
> some of the clocks for example.

I guess. I'm not sure why this code was put into startup and shutdown 
callbacks in first place.

Maybe Marcus or Maxime know?

As for configuration - only bus clock is necessary for configuration and it's 
already enabled in runtime_resume() callback.

Re: [PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Mark Brown]

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

On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > called for capture substream, but playback is still running.

> > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > what happens if some of the configuration stuff turns out to also need
> > some of the clocks for example.

> I guess. I'm not sure why this code was put into startup and shutdown 
> callbacks in first place.

> Maybe Marcus or Maxime know?

> As for configuration - only bus clock is necessary for configuration and it's 
> already enabled in runtime_resume() callback.

Probably these really are only needed when audio is active so it's
saving that little bit of power in which case your fix looks good to me
but I'll leave a chance for the people with system specific knowledge to
review.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Mark Brown]

On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > called for capture substream, but playback is still running.

> > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > what happens if some of the configuration stuff turns out to also need
> > some of the clocks for example.

> I guess. I'm not sure why this code was put into startup and shutdown 
> callbacks in first place.

> Maybe Marcus or Maxime know?

> As for configuration - only bus clock is necessary for configuration and it's 
> already enabled in runtime_resume() callback.

Probably these really are only needed when audio is active so it's
saving that little bit of power in which case your fix looks good to me
but I'll leave a chance for the people with system specific knowledge to
review.
-------------- 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/20181012/18d50e17/attachment.sig>

Re: [PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Maxime Ripard]

On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > called for capture substream, but playback is still running.
> > 
> > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > what happens if some of the configuration stuff turns out to also need
> > some of the clocks for example.
> 
> I guess. I'm not sure why this code was put into startup and shutdown 
> callbacks in first place.
> 
> Maybe Marcus or Maxime know?

I can't really come up with a good explanation, so I guess there's
none :)

> As for configuration - only bus clock is necessary for configuration
> and it's already enabled in runtime_resume() callback.

Not really. Or at least, on the A64, yes, but shutting down the bus
clock on older SoCs will reset the controller. I'd just put the enable
bit on the runtime_pm hook.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Maxime Ripard]

On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > called for capture substream, but playback is still running.
> > 
> > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > what happens if some of the configuration stuff turns out to also need
> > some of the clocks for example.
> 
> I guess. I'm not sure why this code was put into startup and shutdown 
> callbacks in first place.
> 
> Maybe Marcus or Maxime know?

I can't really come up with a good explanation, so I guess there's
none :)

> As for configuration - only bus clock is necessary for configuration
> and it's already enabled in runtime_resume() callback.

Not really. Or at least, on the A64, yes, but shutting down the bus
clock on older SoCs will reset the controller. I'd just put the enable
bit on the runtime_pm hook.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

On Monday, October 15, 2018 1:11:51 AM PDT Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> > On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > > called for capture substream, but playback is still running.
> > > 
> > > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > > what happens if some of the configuration stuff turns out to also need
> > > some of the clocks for example.
> > 
> > I guess. I'm not sure why this code was put into startup and shutdown
> > callbacks in first place.
> > 
> > Maybe Marcus or Maxime know?
> 
> I can't really come up with a good explanation, so I guess there's
> none :)
> 
> > As for configuration - only bus clock is necessary for configuration
> > and it's already enabled in runtime_resume() callback.
> 
> Not really. Or at least, on the A64, yes, but shutting down the bus
> clock on older SoCs will reset the controller. I'd just put the enable
> bit on the runtime_pm hook.

OK, will do.

> Maxime

[PATCH] ASoC: sun4i-i2s: don't try to start up or shut down DAI if it's active

[Vasily Khoruzhick]

On Monday, October 15, 2018 1:11:51 AM PDT Maxime Ripard wrote:
> On Fri, Oct 12, 2018 at 09:28:56AM -0700, Vasily Khoruzhick wrote:
> > On Friday, October 12, 2018 9:09:05 AM PDT Mark Brown wrote:
> > > On Fri, Oct 12, 2018 at 08:32:54AM -0700, Vasily Khoruzhick wrote:
> > > > Otherwise we may end up with shutting down I2S if shutdown() was
> > > > called for capture substream, but playback is still running.
> > > 
> > > Would it be cleaner and more robust to use runtime PM?  I'm wondering
> > > what happens if some of the configuration stuff turns out to also need
> > > some of the clocks for example.
> > 
> > I guess. I'm not sure why this code was put into startup and shutdown
> > callbacks in first place.
> > 
> > Maybe Marcus or Maxime know?
> 
> I can't really come up with a good explanation, so I guess there's
> none :)
> 
> > As for configuration - only bus clock is necessary for configuration
> > and it's already enabled in runtime_resume() callback.
> 
> Not really. Or at least, on the A64, yes, but shutting down the bus
> clock on older SoCs will reset the controller. I'd just put the enable
> bit on the runtime_pm hook.

OK, will do.

> Maxime