[PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[Kuninori Morimoto]

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.

Tested-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v3 -> v4

 - use schedule_work() instead of schedule_delayed_work()

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

diff --git a/sound/soc/codecs/ak4613.c b/sound/soc/codecs/ak4613.c
index ee9e822..8d2c90b 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 work_struct 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,65 @@ 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);
+	struct snd_soc_component *component = priv->component;
+	unsigned int mgmt1;
+	unsigned int mgmt3;
+
+	/* wait 5 LR clocks */
+	udelay(5000000 / priv->rate);
+
+	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);
+
+	/*
+	 * 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 delay/dummy_write method from
+	 * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...),
+	 * call it once here.
+	 *
+	 * But, unfortunately, we can't "write" here because here is atomic
+	 * context (It uses I2C access for write).
+	 * Thus, use delayed work with 0 delay to switch to normal context
+	 * immediately, and wait 5 LR clocks and do dummy_write there.
+	 */
+
+	if ((cmd != SNDRV_PCM_TRIGGER_START) &&
+	    (cmd != SNDRV_PCM_TRIGGER_RESUME))
+		return 0;
+
+	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
+		return  0;
+
+	priv->component = &codec->component;
+	schedule_work(&priv->dummy_write_work);
+
+	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 +650,7 @@ static int ak4613_i2c_probe(struct i2c_client *i2c,
 	priv->iface		= NULL;
 	priv->cnt		= 0;
 	priv->sysclk		= 0;
+	INIT_WORK(&priv->dummy_write_work, ak4613_dummy_write);
 
 	mutex_init(&priv->lock);
 
-- 
1.9.1

Re: [PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[Takashi Sakamoto]

On Dec 1 2017 13:25, 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.
> 
> Tested-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v3 -> v4
> 
>   - use schedule_work() instead of schedule_delayed_work()
> 
>   sound/soc/codecs/ak4613.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
> 
> diff --git a/sound/soc/codecs/ak4613.c b/sound/soc/codecs/ak4613.c
> index ee9e822..8d2c90b 100644
> --- a/sound/soc/codecs/ak4613.c
> +++ b/sound/soc/codecs/ak4613.c
> ...
> +static void ak4613_dummy_write(struct work_struct *work)
> +{
> +	struct ak4613_priv *priv = container_of(work,
> +						struct ak4613_priv,
> +						dummy_write_work);
> +	struct snd_soc_component *component = priv->component;
> +	unsigned int mgmt1;
> +	unsigned int mgmt3;
> +
> +	/* wait 5 LR clocks */
> +	udelay(5000000 / priv->rate);
> +
> +	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);
> +}

In my understanding, it's better to have care of kernel preemption in 
this case because data transmission is already activated and these 
should be executed as quick as possible to prevent much presentation loss.

> +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);
> +
> +	/*
> +	 * 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 delay/dummy_write method from
> +	 * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...),
> +	 * call it once here.
> +	 *
> +	 * But, unfortunately, we can't "write" here because here is atomic
> +	 * context (It uses I2C access for write).
> +	 * Thus, use delayed work with 0 delay to switch to normal context
> +	 * immediately, and wait 5 LR clocks and do dummy_write there.

You don't use 'struct delayed_work' anymore.

> +	 */
> +
> +	if ((cmd != SNDRV_PCM_TRIGGER_START) &&
> +	    (cmd != SNDRV_PCM_TRIGGER_RESUME))
> +		return 0;
> +
> +	if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)
> +		return  0;
> +
> +	priv->component = &codec->component;
> +	schedule_work(&priv->dummy_write_work);
> +
> +	return 0;
> +}

Furthermore, you said that a task for the work is scheduled immediately 
after queueing. Renesas' R-Car SoC and its derivatives are designed to 
be multi core processor on which several threads are available. On the 
other hand, there're some SoCs which has a core for single thread. It's 
welcome to design this driver for such poor machine, in my opinion.

Overall, no information about possibilities of presentation loss in the 
beginning of playback, which I addressed several times.

Before posting revised patches, please have a time to re-evaluate it in 
your side. It often brings easy mistakes because some points are newly 
added against its original shape. The points can causes contradictions 
and lose consistency.


Regards

Takashi Sakamoto

Re: [PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[Kuninori Morimoto]

Hi Takashi

Thank you for your review

> > +static void ak4613_dummy_write(struct work_struct *work)
> > +{
> > +	struct ak4613_priv *priv = container_of(work,
> > +						struct ak4613_priv,
> > +						dummy_write_work);
> > +	struct snd_soc_component *component = priv->component;
> > +	unsigned int mgmt1;
> > +	unsigned int mgmt3;
> > +
> > +	/* wait 5 LR clocks */
> > +	udelay(5000000 / priv->rate);
> > +
> > +	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);
> > +}
> 
> In my understanding, it's better to have care of kernel preemption in
> this case because data transmission is already activated and these
> should be executed as quick as possible to prevent much presentation
> loss.

Hmm.. it was included from v2 patch.
I need to create v5 patch again...

> > +	/*
> > +	 * 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 delay/dummy_write method from
> > +	 * ak4613_set_bias_level() / SND_SOC_DAPM_DAC_E("DACx", ...),
> > +	 * call it once here.
> > +	 *
> > +	 * But, unfortunately, we can't "write" here because here is atomic
> > +	 * context (It uses I2C access for write).
> > +	 * Thus, use delayed work with 0 delay to switch to normal context
> > +	 * immediately, and wait 5 LR clocks and do dummy_write there.
> 
> You don't use 'struct delayed_work' anymore.

Ahh yes indeed.

> Overall, no information about possibilities of presentation loss in
> the beginning of playback, which I addressed several times.

Its my fault.
I will add it in v5

> Before posting revised patches, please have a time to re-evaluate it
> in your side. It often brings easy mistakes because some points are
> newly added against its original shape. The points can causes
> contradictions and lose consistency.

I don't understand where is contradictions.
Anyway, basically indeed it is my fault, but one reason is that
someone is pointing very randomly, thus the patch because to v4 now.
Now I got new point which exist from v1 patch.
If these random review were included into one, my easy mistakes
can be reduced :P

Best regards
---
Kuninori Morimoto

Re: [PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[Kuninori Morimoto]

Hi Takashi, again

> > +static void ak4613_dummy_write(struct work_struct *work)
> > +{
> > +	struct ak4613_priv *priv = container_of(work,
> > +						struct ak4613_priv,
> > +						dummy_write_work);
> > +	struct snd_soc_component *component = priv->component;
> > +	unsigned int mgmt1;
> > +	unsigned int mgmt3;
> > +
> > +	/* wait 5 LR clocks */
> > +	udelay(5000000 / priv->rate);
> > +
> > +	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);
> > +}
> 
> In my understanding, it's better to have care of kernel preemption in
> this case because data transmission is already activated and these
> should be executed as quick as possible to prevent much presentation
> loss.

To avoid v6 patch, I want to know more clearly.
Do you mean it needs spin lock ?
If so lock in trigger, and unlock here ?
Or lock/unlock on each ?

Best regards
---
Kuninori Morimoto

Re: [PATCH v4] ASoC: ak4613: call dummy write for PW_MGMT1/3 when Playback

[Takashi Sakamoto]

On Dec 5 2017 11:21, Kuninori Morimoto wrote:
> 
> Hi Takashi, again
> 
>>> +static void ak4613_dummy_write(struct work_struct *work)
>>> +{
>>> +	struct ak4613_priv *priv = container_of(work,
>>> +						struct ak4613_priv,
>>> +						dummy_write_work);
>>> +	struct snd_soc_component *component = priv->component;
>>> +	unsigned int mgmt1;
>>> +	unsigned int mgmt3;
>>> +
>>> +	/* wait 5 LR clocks */
>>> +	udelay(5000000 / priv->rate);
>>> +
>>> +	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);
>>> +}
>>
>> In my understanding, it's better to have care of kernel preemption in
>> this case because data transmission is already activated and these
>> should be executed as quick as possible to prevent much presentation
>> loss.
> 
> To avoid v6 patch, I want to know more clearly.
> Do you mean it needs spin lock ?
> If so lock in trigger, and unlock here ?
> Or lock/unlock on each ?

Hm.

Taking offload to scheduling tasks demands developers to have enough
knowledge about running process contexts on Operating System.
Apparently, you have little knowledge about it (and few interests in
it, in my opinion. I guess that you don't know Linux kernel adds
several kthreads for workqueues according to their priorities and
localities).

Please study about it, before proposing your insistences. Your
colleagues or offshore staffs might help your learning.


Regards

Takashi Sakamoto