ASoC: SOF: DMIC: Fixed length IPC #1924

singalsu · 2020-03-23T16:15:44Z

No description provided.

singalsu · 2020-03-23T16:20:29Z

The similar PR for firmware would increase ABI minor to 15. Since the ABI minor for current kernel is behind by one this would increase ABI minor to 14 that is not consistent. Probably the feature from firmware should be merged first to get to version 14 and have 15 on both sides with this merged.

I've tested this with all combinations of new/new, new/old, old/new for kernel and FW on UP2+DMIC kit and no issues seen.

plbossart

minor comments, looks good otherwise.
Need to align on ABI change, there are already two changes competing for ABI 15.

sound/soc/sof/topology.c

include/sound/sof/dai-intel.h

ranj063

@singalsu but for the one change in comment as requested by Pierre, the patch looks good. Thanks!

include/uapi/sound/sof/abi.h

sound/soc/sof/topology.c

kv2019i

Thanks @singalsu Minor comments to documentation, and we need to clarify what is the right ABI version to use.

include/sound/sof/dai-intel.h

include/uapi/sound/sof/abi.h

singalsu · 2020-03-24T16:47:38Z

I've addressed the comments in my way except the sdev->private and ABI minor version.

singalsu · 2020-03-24T17:00:07Z

On FW side review @lgirdwood said this should have ABI minor version 14 so my guess was wrong. I'll wait for other comments before I update this.

kv2019i

Looks good now @singalsu

Not sure who should do this, but I manually added this PR to the ABI classifier as well:
https://github.com/orgs/thesofproject/projects/2?card_filter_query=dmic

sound/soc/sof/topology.c

singalsu · 2020-03-25T07:57:48Z

Looks good now @singalsu

Not sure who should do this, but I manually added this PR to the ABI classifier as well:
https://github.com/orgs/thesofproject/projects/2?card_filter_query=dmic

Thanks! I didn't know about that. I'll learn how to do it.

singalsu · 2020-03-25T15:00:19Z

I just pushed new version with extra if statement brackets removed and changed ABI minor back to 14.

singalsu · 2020-03-25T17:58:59Z

Thanks for quick review work to everyone. The firmware proposal PR is now updated to match this one.

kv2019i · 2020-03-26T07:55:58Z

Just waiting on thesofproject/sof#2610 now..
UPDATE: sof#2610 now approved. I'll let FW merge first and then I'll merge this.

This patch fixes the typo in word "microphone". Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch changes the flexible array member pdm[] into a fixed array of four that is the max. number of stereo PDM controllers in the current Intel platforms. The change simplifies DMIC DAI load code and aligns the IPC with other DAI types. The change is compatible with old and new firmware with similar change. The ABI minor version is increased due to change in IPC headers. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

This patch restores the field name to fifo_bits_b since the legacy firmware compatibility code (for firmware ABI 3.0.0 or earlier) sets it in sof_link_dmic_load() function in topology.c. Setting of reserved_2 didn't look appropriate. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2020-04-03T11:54:45Z

This leaves abi.h at minor 15. No other changes into this PR.

kv2019i

Agreed with @lgirdwood to reclassify this to ABI15. Updated to latest sof-dev and as no changes otherwise, seems ready to go.

lyakh · 2020-04-06T07:31:11Z

sound/soc/sof/topology.c

 	 * Used to track the pdm config array index currently being parsed
 	 */
 	sdev->private = kzalloc(sizeof(u32), GFP_KERNEL);
-	if (!sdev->private) {


Do I understand correctly that this now conflicts with #1766? But the conflict seems to be easy to resolve.

@juimonen FYI, Seppo is out this week, so maybe let's put this #1924 in first, and you'll update the other one?

@kv2019i you want me to update it? Sure can do, but so far I'm not aware of many cases of such take-overs. Is it urgent? Or was it just a typo?

lyakh · 2020-04-06T07:35:08Z

include/sound/sof/dai-intel.h

 	uint32_t reserved_1;	/**< Reserved */
 	uint16_t fifo_bits;	/**< FIFO word length (16 or 32) */
-	uint16_t reserved_2;	/**< Reserved */
+	uint16_t fifo_bits_b;	/**< Deprecated since firmware ABI 3.0.1 */


I'm wondering how we're going to handle such deprecation? Do we set a grace period of, say, a year and then remove this backward compatibility and then claim the field as "reserved?"

@lyakh I don't think you really ever get back reserved bits via deprecation unless you end up in a situation where you absolutely have to. I.e. rare enough event, so better worry about the process when the time comes.

lyakh

@singalsu would be good to have those additional remarks addressed in a new PR

lyakh · 2020-04-07T06:15:34Z

include/sound/sof/dai-intel.h

 #define SOF_DAI_INTEL_SSP_CLKCTRL_BCLK_IDLE_HIGH	BIT(5)

+/* DMIC max. four controllers for eight microphone channels */
+#define SOF_DAI_INTEL_DMIC_NUM_CTRL			4


sorry for chiming in so late with this. This can be made a separate PR: should we add a check of num_pdm_active <= SOF_DAI_INTEL_DMIC_NUM_CTRL? I think it would be good to tighten up our topology consistency checks.

lyakh · 2020-04-07T06:22:27Z

sound/soc/sof/topology.c

-	 * that does not include the PDM controller config array
-	 * Set the common params to 0.
-	 */
+	/* Ensure the entire DMIC config struct is zeros */


This comment could be just drop, it doesn't seem to add a lot of information. In fact, looking at it: the below memset() is completely redundant. The caller has already 0-initialised the whole config. @singalsu could you make an additional PR to remove this too?

kv2019i · 2020-04-08T07:47:14Z

Thanks @lyakh . Let' s merge this version if the FW side progresses this week, and handle updates in separate PR.

kv2019i · 2020-04-09T08:23:47Z

FW part merged, merging this.
@singalsu Please check the additional comments from @lyakh and address in follow-up if still valid.

singalsu requested review from kv2019i, lgirdwood, plbossart, ranj063 and tlauda March 23, 2020 16:15

singalsu requested review from bardliao, dbaluta, juimonen and lyakh as code owners March 23, 2020 16:15

plbossart requested changes Mar 23, 2020

View reviewed changes

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

include/sound/sof/dai-intel.h Outdated Show resolved Hide resolved

include/sound/sof/dai-intel.h Show resolved Hide resolved

include/sound/sof/dai-intel.h Outdated Show resolved Hide resolved

singalsu mentioned this pull request Mar 23, 2020

Remove variable-length arrays for DMIC params #1801

Closed

singalsu added the ABI involves ABI changes, need extra attention label Mar 23, 2020

singalsu force-pushed the dmic_fixed_length_ipc branch from 97c2fb4 to f9a84c5 Compare March 23, 2020 17:41

ranj063 reviewed Mar 23, 2020

View reviewed changes

kv2019i mentioned this pull request Mar 24, 2020

ASoC: SOF: make sof_ipc_cc_version to fixed length and fix ABI mismatch #1890

Merged

lyakh requested changes Mar 24, 2020

View reviewed changes

include/uapi/sound/sof/abi.h Outdated Show resolved Hide resolved

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

sound/soc/sof/topology.c Outdated Show resolved Hide resolved

sound/soc/sof/topology.c Show resolved Hide resolved

kv2019i requested changes Mar 24, 2020

View reviewed changes

include/sound/sof/dai-intel.h Outdated Show resolved Hide resolved

include/uapi/sound/sof/abi.h Outdated Show resolved Hide resolved

singalsu force-pushed the dmic_fixed_length_ipc branch from f9a84c5 to b14d68b Compare March 24, 2020 16:38

singalsu requested review from kv2019i, lyakh and plbossart March 24, 2020 16:44

kv2019i reviewed Mar 24, 2020

View reviewed changes

lyakh reviewed Mar 25, 2020

View reviewed changes

sound/soc/sof/topology.c Show resolved Hide resolved

singalsu force-pushed the dmic_fixed_length_ipc branch from b14d68b to 728154c Compare March 25, 2020 14:55

singalsu requested a review from lyakh March 25, 2020 15:00

singalsu dismissed stale reviews from ranj063 and kv2019i via a245cbe March 25, 2020 17:18

singalsu force-pushed the dmic_fixed_length_ipc branch from 728154c to a245cbe Compare March 25, 2020 17:18

singalsu requested a review from plbossart March 25, 2020 17:20

ranj063 previously approved these changes Mar 25, 2020

View reviewed changes

plbossart previously approved these changes Mar 25, 2020

View reviewed changes

lyakh previously approved these changes Mar 26, 2020

View reviewed changes

kv2019i previously approved these changes Mar 26, 2020

View reviewed changes

kv2019i mentioned this pull request Apr 3, 2020

DMIC: Fixed length IPC thesofproject/sof#2610

Merged

singalsu added 3 commits April 3, 2020 14:43

ASoC: SOF: Intel: Fix typo in header file comment text

d767e9c

This patch fixes the typo in word "microphone". Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu dismissed stale reviews from kv2019i, lyakh, plbossart, and ranj063 via 57f8967 April 3, 2020 11:53

singalsu force-pushed the dmic_fixed_length_ipc branch from a245cbe to 57f8967 Compare April 3, 2020 11:53

singalsu requested review from kv2019i, lyakh, plbossart and ranj063 April 3, 2020 11:53

kv2019i approved these changes Apr 3, 2020

View reviewed changes

lyakh reviewed Apr 6, 2020

View reviewed changes

lyakh approved these changes Apr 7, 2020

View reviewed changes

kv2019i merged commit 0bebc01 into thesofproject:topic/sof-dev Apr 9, 2020

ASoC: SOF: DMIC: Fixed length IPC #1924

ASoC: SOF: DMIC: Fixed length IPC #1924

Uh oh!

Conversation

singalsu commented Mar 23, 2020

Uh oh!

singalsu commented Mar 23, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

singalsu commented Mar 24, 2020

Uh oh!

singalsu commented Mar 24, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singalsu commented Mar 25, 2020

Uh oh!

singalsu commented Mar 25, 2020

Uh oh!

singalsu commented Mar 25, 2020

Uh oh!

kv2019i commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singalsu commented Apr 3, 2020

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

lyakh Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

lyakh Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i Apr 6, 2020

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

lyakh Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Apr 8, 2020

Uh oh!

kv2019i commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

kv2019i commented Mar 26, 2020 •

edited

Loading