Remove variable-length arrays for DMIC params

There is no reason why we should have a variable-length array in the struct sof_ipc_dai_dmic_params structure. And this results in DMIC-specific code at the lowest level of our topology-parsing routines.

We should instead reserve data for the worst-case, e.g. define how many PDM controller configurations are reasonable. the firmware can still use the 'num_pdm_active' field to know how many of the array elements are valid.

Do you have an ETA for ABI change? Though this change should be fairly simple to do.

ETA tomorrow :-)
I'd really like to know how many configurations we really need

OK! I'll work with this after I've fixed the ASRC capture stall issue.

I started with

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

struct sof_ipc_dai_dmic_params {
	uint32_t reserved0;
	uint32_t driver_ipc_version;	/**< Version (1..N) */

	uint32_t pdmclk_min;	/**< Minimum microphone clock in Hz (100000..N) */
	uint32_t pdmclk_max;	/**< Maximum microphone clock in Hz (min...N) */

	uint32_t fifo_fs;	/**< FIFO sample rate in Hz (8000..96000) */
	uint32_t reserved_1;	/**< Reserved */
	uint16_t fifo_bits;	/**< FIFO word length (16 or 32) */
	uint16_t reserved_2;	/**< Reserved */

	uint16_t duty_min;	/**< Min. mic clock duty cycle in % (20..80) */
	uint16_t duty_max;	/**< Max. mic clock duty cycle in % (min..80) */

	uint32_t num_pdm_active; /**< Number of active pdm controllers */

	uint32_t wake_up_time;      /**< Time from clock start to data (us) */
	uint32_t min_clock_on_time; /**< Min. time that clk is kept on (us) */
	uint32_t unmute_ramp_time;  /**< Length of logarithmic gain ramp (ms) */

	/* reserved for future use */
	uint32_t reserved[5];

	/**< Fixed number of pdm controller configurations */
	struct sof_ipc_dai_dmic_pdm_ctrl pdm[SOF_DAI_INTEL_DMIC_NUM_PDM];
} __attribute__((packed));

Would you like to preserve "num_pdm_active"? The IPC with fixed array size no more needs it. The driver code already checks if request for PDM[N] is zeros or populated since we support e.g. running of PDM[1] while PDM[0] is kept idle.

Removing it might force a major version bump. Should this be done only as minor version change?

@plbossart

Also this "id" could be removed for simple 1:1 mapping from IPC to PDM controllers and external microphone pins.

struct sof_ipc_dai_dmic_pdm_ctrl {
	uint32_t reserved0;
	uint16_t id;		/**< PDM controller ID */

	uint16_t enable_mic_a;	/**< Use A (left) channel mic (0 or 1)*/
	uint16_t enable_mic_b;	/**< Use B (right) channel mic (0 or 1)*/

	uint16_t polarity_mic_a; /**< Optionally invert mic A signal (0 or 1) */
	uint16_t polarity_mic_b; /**< Optionally invert mic B signal (0 or 1) */

	uint16_t clk_edge;	/**< Optionally swap data clock edge (0 or 1) */
	uint16_t skew;		/**< Adjust PDM data sampling vs. clock (0..15) */

	uint16_t reserved[3];	/**< Make sure the total size is 4 bytes aligned */
} __attribute__((packed));

It would simplify the IPC but delay until major version bump. Thoughts are welcome @plbossart @ranj063 @lgirdwood @lbetlej

Would you like to preserve "num_pdm_active"? The IPC with fixed array size no more needs it. The driver code already checks if request for PDM[N] is zeros or populated since we support e.g. running of PDM[1] while PDM[0] is kept idle.

Removing it might force a major version bump. Should this be done only as minor version change?

It's more on the firmware side that this matters, and if we already have something that tells use the number of valid structures it's simpler to use it.

Also no burning desire to remove the ID, just remove variable lengths when they are not really needed.

The PRs are now available for CI tests and review:

thesofproject/sof#2610
#1924

@kv2019i

@kv2019i @plbossart FW part is good. Ready to merge.

@kv2019i

@kv2019i what's the reasoning for not being in ABI minor ?

merged, closing

Navigation Menu

Repository navigation

Remove variable-length arrays for DMIC params #1801

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Remove variable-length arrays for DMIC params #1801

Description

Activity

singalsu commented on Feb 19, 2020

plbossart commented on Feb 19, 2020

singalsu commented on Feb 26, 2020

singalsu commented on Mar 3, 2020

singalsu commented on Mar 3, 2020

plbossart commented on Mar 3, 2020

singalsu commented on Mar 23, 2020

lgirdwood commented on Mar 26, 2020

lgirdwood commented on Mar 26, 2020

plbossart commented on Aug 11, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions