On 21-10-18, 22:42, 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.
I think in this case you can go with dummy codec. You are modelling an endpoint here, nothing else
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?
That is very valid point. Also DMIC clock is generated by Audio block and not really a system clock, so why shouldn't this be described in audio block.
Furthermore I recall we have NHLT table and some description for DMICs. The driver currently parses that information to get number of dmics to use (see skl_get_dmic_geo). I would think that adding this delay to NHLT would be more sensible and pass it on to whatever entity wishes to use it.