[alsa-devel] [PATCH 1/2] ASoC: dmic: introduce mode switch delay

[Pierre-Louis Bossart]

On 11/16/18 7:29 AM, Jaroslav Kysela wrote:
> Dne 16.11.2018 v 12:10 Jenny TC napsal(a):
>> On startup, applications such as PulseAudio or CRAS enable playback or
>> capture on all PCM devices to verify that configurations are correct,
>> and close them immediately. For DMICs, this can result in the clock
>> being turned off very quickly, which may not compatible with internal
>> state machine transition requirements.
>>
>> This patch add a mode-switch delay which will prevent the clock from
>> being turned off without complying with manufacturer timing
>> specifications. While the DMIC clock may be controlled at a lower level,
>> be it with hardware or firmware, applying the delay during the
>> STOP_TRIGGER phase ensures that there is no race condition, e.g. with
>> the hardware/firmware turning off the clock earlier
>>
>> Signed-off-by: Sathyanarayana Nujella <sathyanarayana.nujella at intel.com>
>> Signed-off-by: Jairaj Arava <jairaj.arava at intel.com>
>> Signed-off-by: Harsha Priya <harshapriya.n at intel.com>
>> ---
>>   sound/soc/codecs/dmic.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/sound/soc/codecs/dmic.c b/sound/soc/codecs/dmic.c
>> index 8c4926d..0527bc2 100644
>> --- a/sound/soc/codecs/dmic.c
>> +++ b/sound/soc/codecs/dmic.c
>> @@ -30,9 +30,35 @@
>>   #include <sound/soc.h>
>>   #include <sound/soc-dapm.h>
>>   
>> +static int modeswitch_delay_ms;
>> +module_param(modeswitch_delay_ms, uint, 0644);
>> +
>>   struct dmic {
>>   	struct gpio_desc *gpio_en;
>>   	int wakeup_delay;
>> +	/* Delay after DMIC mode switch */
>> +	int modeswitch_delay_ms;
>> +};
>> +
>> +int dmic_daiops_trigger(struct snd_pcm_substream *substream,
>> +		int cmd, struct snd_soc_dai *dai)
>> +{
>> +	struct snd_soc_component *component = dai->component;
>> +	struct dmic *dmic = snd_soc_component_get_drvdata(component);
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +		if (dmic->modeswitch_delay_ms)
>> +			mdelay(dmic->modeswitch_delay_ms);
>> +
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct snd_soc_dai_ops dmic_dai_ops = {
>> +	.trigger	= dmic_daiops_trigger,
>>   };
>>   
>>   static int dmic_aif_event(struct snd_soc_dapm_widget *w,
>> @@ -68,6 +94,7 @@ static int dmic_aif_event(struct snd_soc_dapm_widget *w,
>>   			| SNDRV_PCM_FMTBIT_S24_LE
>>   			| SNDRV_PCM_FMTBIT_S16_LE,
>>   	},
>> +	.ops    = &dmic_dai_ops,
>>   };
>>   
>>   static int dmic_component_probe(struct snd_soc_component *component)
>> @@ -85,6 +112,10 @@ static int dmic_component_probe(struct snd_soc_component *component)
>>   
>>   	device_property_read_u32(component->dev, "wakeup-delay-ms",
>>   				 &dmic->wakeup_delay);
>> +	device_property_read_u32(component->dev, "modeswitch_delay_ms",
>> +				 &dmic->modeswitch_delay_ms);
> This should be probably consistent with the previous property name. Use
> "modeswitch-delay-ms".

Indeed. I'd go even further in the nitpicks department and only use the 
the -ms suffix in the property and use a modeswitch_delay field and 
parameter for naming consistency with the existing wake_delay.

The patches look good otherwise, it's a useful patch to add. Jenny, can 
you do this cosmetic fix and resubmit?

>
>> +	if (modeswitch_delay_ms)
>> +		dmic->modeswitch_delay_ms  = modeswitch_delay_ms;
>>   
>>   	snd_soc_component_set_drvdata(component, dmic);
>>   
>>
> 					Jaroslav
>