diff mbox series

[4/4] ASoC: wm8524: Delay some time to follow power up sequency

Message ID 20230222113945.3390672-4-chancel.liu@nxp.com
State New
Headers
Series [1/4] ASoC: dt-bindings: wlf,wm8524: Convert to json-schema |

Commit Message

Chancel Liu Feb. 22, 2023, 11:39 a.m. UTC 
  The recommended power sequence needs to be followed to ensure the best
performance. After MCLK, BCLK and MUTE=1 are ready, this device has to
wait some time before ready to output audio. Otherwise the beginning
data may be lost. For more details about the timing constraints, please
refer to WTN0302 on https://www.cirrus.com/products/wm8524/

Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
---
 sound/soc/codecs/wm8524.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Mark Brown Feb. 22, 2023, 12:48 p.m. UTC | #1 
On Wed, Feb 22, 2023 at 07:39:45PM +0800, Chancel Liu wrote:

> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		mdelay(wm8524->power_up_delay);
> +		break;

Doing a 100ms busy wait in atomic context does not seem like a great
idea, never mind a 1.5s one.  This shouldn't be done in trigger, it
needs to be done later - digital_mute() might be a better time to hook
in, though longer delays like this are really quite bad.
Chancel Liu Feb. 24, 2023, 10:54 a.m. UTC | #2 
> On Wed, Feb 22, 2023 at 07:39:45PM +0800, Chancel Liu wrote:
> 
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +	case SNDRV_PCM_TRIGGER_RESUME:
> > +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> > +		mdelay(wm8524->power_up_delay);
> > +		break;
> 
> Doing a 100ms busy wait in atomic context does not seem like a great
> idea, never mind a 1.5s one.  This shouldn't be done in trigger, it
> needs to be done later - digital_mute() might be a better time to hook
> in, though longer delays like this are really quite bad.

Yes, such long time delay in driver is very bad. But this device requires
waiting some time before able to output audio. We have to wait otherwise the
beginning data may be lost.

The power up to audio out timing occurs after MCLK, BCLK and MUTE=1 are ready.
I added the delay in trigger() because some CPU DAI drivers enable BCLK in
trigger(). You suggested moving the delay to digital_mute(). It seems
digital_mute() is called before cpu_dai->trigger. Please correct me if I'm
wrong.

Regards, 
Chancel Liu
Mark Brown Feb. 24, 2023, 12:30 p.m. UTC | #3 
On Fri, Feb 24, 2023 at 10:54:59AM +0000, Chancel Liu wrote:
> > On Wed, Feb 22, 2023 at 07:39:45PM +0800, Chancel Liu wrote:

> > Doing a 100ms busy wait in atomic context does not seem like a great
> > idea, never mind a 1.5s one.  This shouldn't be done in trigger, it
> > needs to be done later - digital_mute() might be a better time to hook
> > in, though longer delays like this are really quite bad.

> Yes, such long time delay in driver is very bad. But this device requires
> waiting some time before able to output audio. We have to wait otherwise the
> beginning data may be lost.

It's not just that it's doing this in the driver, it's doing it in the
trigger() function which runs in atomic context.  That's unreasonable.

> The power up to audio out timing occurs after MCLK, BCLK and MUTE=1 are ready.
> I added the delay in trigger() because some CPU DAI drivers enable BCLK in
> trigger(). You suggested moving the delay to digital_mute(). It seems
> digital_mute() is called before cpu_dai->trigger. Please correct me if I'm
> wrong.

Hrm, right - in any case, it needs to be somewhere that isn't atomic
context.
Chancel Liu Feb. 27, 2023, 9:07 a.m. UTC | #4 
> > > On Wed, Feb 22, 2023 at 07:39:45PM +0800, Chancel Liu wrote:
> > > Doing a 100ms busy wait in atomic context does not seem like a great
> > > idea, never mind a 1.5s one.  This shouldn't be done in trigger, it
> > > needs to be done later - digital_mute() might be a better time to hook
> > > in, though longer delays like this are really quite bad.
>
> > Yes, such long time delay in driver is very bad. But this device requires
> > waiting some time before able to output audio. We have to wait otherwise
> > the beginning data may be lost.
>
> It's not just that it's doing this in the driver, it's doing it in the
> trigger() function which runs in atomic context.  That's unreasonable.
>
> > The power up to audio out timing occurs after MCLK, BCLK and MUTE=1 are
> > ready. I added the delay in trigger() because some CPU DAI drivers enable BCLK in
> > trigger(). You suggested moving the delay to digital_mute(). It seems
> > digital_mute() is called before cpu_dai->trigger. Please correct me if I'm
> > wrong.
> 
> Hrm, right - in any case, it needs to be somewhere that isn't atomic
> context.

OK. I will keep PATCH 1 and PATCH 3. 

For PATCH 2 and PATCH 4, I have to find a better way in which long time delay
can be avoided in atomic context.

Thank you very much for your comments.

Regards, 
Chancel Liu
diff mbox series

Patch

diff --git a/sound/soc/codecs/wm8524.c b/sound/soc/codecs/wm8524.c
index 8f2130e05b32..f61967b66c3b 100644
--- a/sound/soc/codecs/wm8524.c
+++ b/sound/soc/codecs/wm8524.c
@@ -21,6 +21,7 @@ 
 #include <sound/soc.h>
 #include <sound/initval.h>
 
+#define WM8524_POWER_UP_DELAY_MS 100
 #define WM8524_NUM_RATES 7
 
 /* codec private data */
@@ -29,6 +30,7 @@  struct wm8524_priv {
 	unsigned int sysclk;
 	unsigned int rate_constraint_list[WM8524_NUM_RATES];
 	struct snd_pcm_hw_constraint_list rate_constraint;
+	unsigned int power_up_delay;
 };
 
 
@@ -157,6 +159,28 @@  static int wm8524_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
 	return 0;
 }
 
+static int wm8524_trigger(struct snd_pcm_substream *substream, int cmd,
+			  struct snd_soc_dai *dai)
+{
+	struct wm8524_priv *wm8524 = snd_soc_dai_get_drvdata(dai);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		mdelay(wm8524->power_up_delay);
+		break;
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+};
+
 #define WM8524_RATES SNDRV_PCM_RATE_8000_192000
 
 #define WM8524_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
@@ -169,6 +193,7 @@  static const struct snd_soc_dai_ops wm8524_dai_ops = {
 	.set_sysclk	= wm8524_set_dai_sysclk,
 	.set_fmt	= wm8524_set_fmt,
 	.mute_stream	= wm8524_mute_stream,
+	.trigger	= wm8524_trigger,
 };
 
 static struct snd_soc_dai_driver wm8524_dai = {
@@ -213,6 +238,7 @@  MODULE_DEVICE_TABLE(of, wm8524_of_match);
 
 static int wm8524_codec_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct wm8524_priv *wm8524;
 	int ret;
 
@@ -230,6 +256,9 @@  static int wm8524_codec_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	wm8524->power_up_delay = WM8524_POWER_UP_DELAY_MS;
+	of_property_read_u32(np, "wlf,power-up-delay-ms", &wm8524->power_up_delay);
+
 	ret = devm_snd_soc_register_component(&pdev->dev,
 			&soc_component_dev_wm8524, &wm8524_dai, 1);
 	if (ret < 0)