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

[Pierre-Louis Bossart]

On 10/21/18 10:42 PM, Pierre-Louis Bossart wrote:
>
> 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?

I forgot to add another open on ACPI support: what would be the scope of 
the "DMIC" device? With ACPI we typically have a parent-child 
relationship, e.g. we put audio codec below the relevant I2C/SPI 
controller in the DSDT definitions. In the absence of a DMIC bus, you 
need to be careful how the DMIC device is added in the DSDT - it'd 
likely need to be below the scope of the HDaudio controller.

>
> -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);
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel