[alsa-devel] [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 at 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 at renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx at 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