[Sound-open-firmware] [PATCH v3 3/7] topology: add DMIC tokens

[Seppo Ingalsuo]

Hi,

Sorry for some late comments, I should have commented the earlier RFC 
version. Though these are quite minor.


On 24.05.2018 05:47, Ranjani Sridharan wrote:
> This patch introduces the DMIC specific config parameter tokens
> in topology.
>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> ---
>   topology/sof/tokens.m4 | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/topology/sof/tokens.m4 b/topology/sof/tokens.m4
> index e9aa302..0dd2e00 100644
> --- a/topology/sof/tokens.m4
> +++ b/topology/sof/tokens.m4
> @@ -60,3 +60,16 @@ SectionVendorTokens."sof_ssp_tokens" {
>   	SOF_TKN_INTEL_SSP_BCLK_KEEP_ACTIVE	"501"
>   	SOF_TKN_INTEL_SSP_FS_KEEP_ACTIVE	"502"
>   }
> +
> +SectionVendorTokens."sof_dmic_tokens" {
> +	SOF_TKN_INTEL_DMIC_DRIVER_VERSION	"600"
Would it be useful to have comments for the tokens? For the version 
token the purpose is to ensure a topology and driver are compatible 
versions. If the FW would return an error it is recommended to check 
that microphone operates as specified before increasing version number 
to match. The intent is to increase in FW driver the IPC version if the 
clocks matching criteria would be changed.

> +	SOF_TKN_INTEL_DMIC_CLK_MIN		"601"
> +	SOF_TKN_INTEL_DMIC_CLK_MAX		"602"
> +	SOF_TKN_INTEL_DMIC_DUTY_MIN		"603"
> +	SOF_TKN_INTEL_DMIC_DUTY_MAX		"604"
Commenting again - The settings for these four values need to be looked 
from microphone data sheet. The datasheets specify the recommended clock 
ranges for various applications like speech capture, generic usage, 
ultrasound, etc. The unit is Hertz and typically these are within 1 MHz 
- 5 MHz.

The unit of duty cycle is an integer 0-100%. E.g. 40% and 60% max is 
quite typical duty cycle requirement.

If the driver fails to find a compatible mode it is recommended to look 
if some min/max range can be relaxed. If still no help possibly consider 
some other sample rate for application (below).

> +	SOF_TKN_INTEL_DMIC_REQUESTED_PDM_COUNT	"605"
> +	SOF_TKN_INTEL_DMIC_PDM_0_ENABLE		"606"
> +	SOF_TKN_INTEL_DMIC_PDM_1_ENABLE		"607"

Change proposal - The FW driver can happily work with these three 
controls to activate the needed microphones but there would be power 
saving opportunity by having possibility to express what active 
microphones combination is needed for PDM0 controller.

The PDM_0 token could be split to

SOF_TKN_INTEL_DMIC_PDM_0_LEFT_ENABLE
SOF_TKN_INTEL_DMIC_PDM_0_RIGHT_ENABLE

The driver can suspend the clocks of non-used microphones and save near 
1 mW of power for each by putting them to powered sleep state.

The PDM_COUNT token could be eliminated from topology by looking up the 
enabled max. PDM_x index plus one and set it by kernel driver.

There's also new DMIC HW with support for PDM_2 and PDM_3 controllers 
and max. 8 microphones. Should we add those tokens now to topology? 
Those need to be kept as zero for platforms like APL with max 4 
microphones capability (driver will issue an error if attempted).

There's even more configuration possibility in the IPC but exposing all 
of them to topology could be too much clutter while this should be 
sufficient for typical usage. Those additional options could be left for 
custom topology and kernel if need. If the kernel driver sets them to 
default 0 value it's simple to spot them and customize.

Thanks,
Seppo


> +	SOF_TKN_INTEL_DMIC_SAMPLE_RATE		"608"
> +	SOF_TKN_INTEL_DMIC_FIFO_WORD_LENGTH	"609"
> +}