[alsa-devel] [PATCH 0/2] Introduce dmic mode switch delay parameter

[Pierre-Louis Bossart]

On 10/2/18 12:57 AM, Jenny TC wrote:
> Some DMICs need clock to be running for a specified duration as per the
> DMIC spec to complete the mode transition like sleep to mormal, off to normal
> etc.
>
> Jenny TC (2):
>    ASoC: dmic: Enable ACPI device entry
>    ASoC: dmic: Introduce mode switch delay
>
>   sound/soc/codecs/dmic.c | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>
Sorry for the late feedback.

Allowing some timing adjustments for the clock transitions is a good 
thing. The way it's done is questionable and raises a number of concerns.

First, there was an Intel internal discussion before my extended break 
on why this 'dmic-codec' is needed on Intel platforms. To the best of my 
knowledge we don't control the mics with GPIOs, which was the initial 
purpose of this driver. We have experimental evidence on ApolloLake and 
GeminiLake that using the soc-dummy/soc-dummy-dai definitions are 
enough, and it may be a good thing to agree on the direction here. If 
you want a parameter, you can still use a machine driver DMI-based 
kernel quirk and/or pass a kernel parameter, the need to extend this 
dmic-codec is far from obvious to me.

Assuming you still want to use this codec, then there are still major 
concerns about the ACPI directions. As Mark noted it, "DMIC" does not 
follow any of the guidelines or accepted definitions with an unambiguous 
vendor and part ID. We know we already have conflicts between 
Intel-defined ACPI IDs, e.g. for RT298 on multiple platforms, let's be 
careful here, shall we?

And I am coming to my last point. The Skylake driver already contains 
code to create the dmic devices by hand (see below the git grep 
results). So I wonder what happens if you use both ACPI-based 
enumeration AND manually create the dmic device - I view these solutions 
as mutually incompatible. Either you have not tested against the 
upstream code or something is missing from your patchset. What am I missing?

-Pierre

skl.c:static int skl_dmic_device_register(struct skl *skl)
skl.c:  /* SKL has one dmic port, so allocate dmic device for this */
skl.c:  pdev = platform_device_alloc("dmic-codec", -1);
skl.c:          dev_err(bus->dev, "failed to allocate dmic device\n");
skl.c:          dev_err(bus->dev, "failed to add dmic device: %d\n", ret);
skl.c:  skl->dmic_dev = pdev;
skl.c:static void skl_dmic_device_unregister(struct skl *skl)
skl.c:  if (skl->dmic_dev)
skl.c:          platform_device_unregister(skl->dmic_dev);
skl.c:  /* create device for soc dmic */
skl.c:  err = skl_dmic_device_register(skl);
skl.c:  skl_dmic_device_unregister(skl);