[v2] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback
diff mbox

Message ID 87609uubjp.wl%kuninori.morimoto.gx@renesas.com
State New
Headers show

Commit Message

Kuninori Morimoto Nov. 28, 2017, 7:15 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Power Down Release Command (PMVR, PMDAC, RSTN, PMDA1-PMDA6)
which are located on PW_MGMT1 / PW_MGMT3 register must be
write again after at least 5 LRCK cycle or later on each command.
Otherwise, Playback volume will be 0dB.
Basically, it should be

	1.   PowerDownRelease by Power Management1 <= call 1.x after 5LRCK
	1.x  Dummy write      to Power Management1
	2.   PowerDownRelease by Power Management3 <= call 2.x after 5LRCK
	2.x  Dummy write      to Power Management3

To avoid too many dummy write, this patch is merging these.

	1.   PowerDownRelease by Power Management1
	2.   PowerDownRelease by Power Management3   <= call after 5LRCK
	2.x  Dummy write      to Power Management1/3 <= merge dummy write

This patch adds dummy write when Playback Start timing.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - add more explain on log area
 - call dummy write after 5LRCK

 sound/soc/codecs/ak4613.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

Comments

Takashi Sakamoto Nov. 30, 2017, 10:47 p.m. UTC | #1
On Nov 26 2017 16:15, Kuninori Morimoto wrote:
> 
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Power Down Release Command (PMVR, PMDAC, RSTN, PMDA1-PMDA6)
> which are located on PW_MGMT1 / PW_MGMT3 register must be
> write again after at least 5 LRCK cycle or later on each command.
> Otherwise, Playback volume will be 0dB.
> Basically, it should be
> 
> 	1.   PowerDownRelease by Power Management1 <= call 1.x after 5LRCK
> 	1.x  Dummy write      to Power Management1
> 	2.   PowerDownRelease by Power Management3 <= call 2.x after 5LRCK
> 	2.x  Dummy write      to Power Management3
> 
> To avoid too many dummy write, this patch is merging these.
> 
> 	1.   PowerDownRelease by Power Management1
> 	2.   PowerDownRelease by Power Management3   <= call after 5LRCK
> 	2.x  Dummy write      to Power Management1/3 <= merge dummy write
> 
> This patch adds dummy write when Playback Start timing.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 
>   - add more explain on log area
>   - call dummy write after 5LRCK
> 
>   sound/soc/codecs/ak4613.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
> 
> diff --git a/sound/soc/codecs/ak4613.c b/sound/soc/codecs/ak4613.c
> index ee9e822..937cff4 100644
> --- a/sound/soc/codecs/ak4613.c
> +++ b/sound/soc/codecs/ak4613.c
> @@ -15,6 +15,7 @@
> ...
> +static int ak4613_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> +			      struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct ak4613_priv *priv = snd_soc_codec_get_drvdata(codec);
> +	unsigned long delay;
> +
> +	/*
> +	 * FIXME
> +	 *
> +	 * PW_MGMT1 / PW_MGMT3 needs dummy write at least after 5 LR clocks
> +	 * from Power Down Release. Otherwise, Playback volume will be 0dB.
> +	 * To avoid complex multiple delayed_work call from
> +	 * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...),
> +	 * call it once here.
> +	 */
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
> +		return  0;
> +
> +	delay = 5000000 / priv->rate;
> +	priv->component = &codec->component;
> +	schedule_delayed_work(&priv->dummy_write_work,
> +			      usecs_to_jiffies(delay));
> +
> +	return 0;
> +}

Please add enough descriptions that kernel's timer functionality cannot 
guarantee accuracy of expiration of timer for 5 phases of word select 
clock on usual serial sound interface, and my (and Mark's) concern about 
missing sound wave on analog part of hardware in the beginning of 
playback. Without the information, users of this codec driver will be 
puzzled in their hardware test. I can easily imagine that.

For example, 5 phases of word select clock can be calculated:
  - 0.113379 msec at 44.1 kHz
  - 0.104167 msec at 48.0 kHz
  - 0.026042 msec at 192.0 kHz

Kernel's timer service doesn't work by such accurate expiration time. At 
the best, 1 msec or more. On the other hand, typical drivers for any 
serial sound interface starts data transmission after a callback of 
.trigger.

Essentially, kernel's timer service is designed for 'rough' expiration 
to release system resources, such as socket buffer for any internet 
protocol. It surely guarantees timer expiration, but not for its accuracy.

But Mark Brown approved your idea under a certain compromise, due to 
current design of ALSA SoC part. The part includes no guarantee of 
timing to enable clocks on serial sound lines between .hw_params and 
.trigger, in my understanding.


Regards

Takashi Sakamoto
Kuninori Morimoto Dec. 1, 2017, 1:02 a.m. UTC | #2
Hi Sakamoto-san

> > Power Down Release Command (PMVR, PMDAC, RSTN, PMDA1-PMDA6)
> > which are located on PW_MGMT1 / PW_MGMT3 register must be
> > write again after at least 5 LRCK cycle or later on each command.
> > Otherwise, Playback volume will be 0dB.
> > Basically, it should be
> >
> > 	1.   PowerDownRelease by Power Management1 <= call 1.x after 5LRCK
> > 	1.x  Dummy write      to Power Management1
> > 	2.   PowerDownRelease by Power Management3 <= call 2.x after 5LRCK
> > 	2.x  Dummy write      to Power Management3
> >
> > To avoid too many dummy write, this patch is merging these.
> >
> > 	1.   PowerDownRelease by Power Management1
> > 	2.   PowerDownRelease by Power Management3   <= call after 5LRCK
> > 	2.x  Dummy write      to Power Management1/3 <= merge dummy write
> >
> > This patch adds dummy write when Playback Start timing.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
(snip)
> Please add enough descriptions that kernel's timer functionality
> cannot guarantee accuracy of expiration of timer for 5 phases of word
> select clock on usual serial sound interface, and my (and Mark's)
> concern about missing sound wave on analog part of hardware in the
> beginning of playback. Without the information, users of this codec
> driver will be puzzled in their hardware test. I can easily imagine
> that.
> 
> For example, 5 phases of word select clock can be calculated:
>  - 0.113379 msec at 44.1 kHz
>  - 0.104167 msec at 48.0 kHz
>  - 0.026042 msec at 192.0 kHz
> 
> Kernel's timer service doesn't work by such accurate expiration
> time. At the best, 1 msec or more. On the other hand, typical drivers
> for any serial sound interface starts data transmission after a
> callback of .trigger.
> 
> Essentially, kernel's timer service is designed for 'rough' expiration
> to release system resources, such as socket buffer for any internet
> protocol. It surely guarantees timer expiration, but not for its
> accuracy.
> 
> But Mark Brown approved your idea under a certain compromise, due to
> current design of ALSA SoC part. The part includes no guarantee of
> timing to enable clocks on serial sound lines between .hw_params and
> .trigger, in my understanding.

Thank you for your comment, yes, it should be indicated log or comment.

I noticed other approach in response to your remarks.
I will try to use it.

Best regards
---
Kuninori Morimoto

Patch
diff mbox

diff --git a/sound/soc/codecs/ak4613.c b/sound/soc/codecs/ak4613.c
index ee9e822..937cff4 100644
--- a/sound/soc/codecs/ak4613.c
+++ b/sound/soc/codecs/ak4613.c
@@ -15,6 +15,7 @@ 
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/of_device.h>
@@ -95,6 +96,9 @@  struct ak4613_priv {
 	struct mutex lock;
 	const struct ak4613_interface *iface;
 	struct snd_pcm_hw_constraint_list constraint;
+	struct delayed_work dummy_write_work;
+	struct snd_soc_component *component;
+	unsigned int rate;
 	unsigned int sysclk;
 
 	unsigned int fmt;
@@ -392,6 +396,7 @@  static int ak4613_dai_hw_params(struct snd_pcm_substream *substream,
 	default:
 		return -EINVAL;
 	}
+	priv->rate = rate;
 
 	/*
 	 * FIXME
@@ -467,11 +472,64 @@  static int ak4613_set_bias_level(struct snd_soc_component *component,
 	return 0;
 }
 
+static void ak4613_dummy_write(struct work_struct *work)
+{
+	struct ak4613_priv *priv = container_of(work,
+						struct ak4613_priv,
+						dummy_write_work.work);
+	struct snd_soc_component *component = priv->component;
+	unsigned int mgmt1;
+	unsigned int mgmt3;
+
+	snd_soc_component_read(component, PW_MGMT1, &mgmt1);
+	snd_soc_component_read(component, PW_MGMT3, &mgmt3);
+
+	snd_soc_component_write(component, PW_MGMT1, mgmt1);
+	snd_soc_component_write(component, PW_MGMT3, mgmt3);
+}
+
+static int ak4613_dai_trigger(struct snd_pcm_substream *substream, int cmd,
+			      struct snd_soc_dai *dai)
+{
+	struct snd_soc_codec *codec = dai->codec;
+	struct ak4613_priv *priv = snd_soc_codec_get_drvdata(codec);
+	unsigned long delay;
+
+	/*
+	 * FIXME
+	 *
+	 * PW_MGMT1 / PW_MGMT3 needs dummy write at least after 5 LR clocks
+	 * from Power Down Release. Otherwise, Playback volume will be 0dB.
+	 * To avoid complex multiple delayed_work call from
+	 * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...),
+	 * call it once here.
+	 */
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+		break;
+	default:
+		return 0;
+	}
+
+	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
+		return  0;
+
+	delay = 5000000 / priv->rate;
+	priv->component = &codec->component;
+	schedule_delayed_work(&priv->dummy_write_work,
+			      usecs_to_jiffies(delay));
+
+	return 0;
+}
+
 static const struct snd_soc_dai_ops ak4613_dai_ops = {
 	.startup	= ak4613_dai_startup,
 	.shutdown	= ak4613_dai_shutdown,
 	.set_sysclk	= ak4613_dai_set_sysclk,
 	.set_fmt	= ak4613_dai_set_fmt,
+	.trigger	= ak4613_dai_trigger,
 	.hw_params	= ak4613_dai_hw_params,
 };
 
@@ -591,6 +649,7 @@  static int ak4613_i2c_probe(struct i2c_client *i2c,
 	priv->iface		= NULL;
 	priv->cnt		= 0;
 	priv->sysclk		= 0;
+	INIT_DELAYED_WORK(&priv->dummy_write_work, ak4613_dummy_write);
 
 	mutex_init(&priv->lock);