Skip to contentSkip to conversation

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

PATCH In review Vasily Khoruzhick 3 participants 6 messages Oct 12, 2018 – Oct 15, 2018

Subsystems (3)
Sound ASoC Arm/Allwinner sunXi SoC support
lore
Share & tools
lore

Vasily Khoruzhick

author
#0 lore raw

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

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 <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 6 ++++++
 1 file changed, 6 insertions(+)
sound/soc/sunxi/sun4i-i2s.c +6 -0
index a9ea329dc046..787b67c4f845 100644
@@ -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 */
Signature · 1 line
2.19.0
maintainer
#1 lore raw
Quoted 2 lines · Vasily Khoruzhick

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>

author
#2 lore raw
Quoted 7 lines · PDT Mark Brown
Quoted 2 lines · Vasily Khoruzhick

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.

maintainer
#3 lore raw
Quoted 9 lines · Vasily Khoruzhick
Quoted 7 lines · PDT Mark Brown
Quoted 2 lines · Vasily Khoruzhick

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.

Quoted 7 lines

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>

#4 lore raw
In reply to Vasily Khoruzhick
Quoted 5 lines · Vasily Khoruzhick
Quoted 4 lines · PDT Mark Brown
Quoted 2 lines · Vasily Khoruzhick

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

Quoted 8 lines
Quoted 3 lines

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 :)

Quoted 2 lines

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

Signature · 3 lines
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
author
#5 lore raw
Quoted 18 lines · PDT Maxime Ripard
Quoted 5 lines · Vasily Khoruzhick
Quoted 4 lines · PDT Mark Brown
Quoted 2 lines · Vasily Khoruzhick

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

Quoted 8 lines
Quoted 3 lines

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 :)

Quoted 6 lines
Quoted 2 lines

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.

Quoted 1 line

Maxime