Hi Nicolin,
Thank you for your feedback, same here - just back from vacation.
On Jo, 2018-12-27 at 01:24 +0800, Nicolin Chen wrote:
Hi Viorel,
Sorry for the late response, having been on a long vacation.
The code looks pretty clean. Just some small concerns/questions below.
On Wed, Dec 19, 2018 at 12:30 AM Viorel Suman viorel.suman@nxp.com wrote:
This patch implements Audio Mixer CPU DAI driver for NXP iMX8 SOCs. The Audio Mixer is a on-chip functional module that allows mixing of two audio streams into a single audio stream.
Audio Mixer datasheet is available here: https://www.nxp.com/docs/en/reference-manual/IMX8DQXPRM.pdf
Signed-off-by: Viorel Suman viorel.suman@nxp.com
.../devicetree/bindings/sound/fsl,amix.txt | 45 ++ sound/soc/fsl/Kconfig | 7 + sound/soc/fsl/Makefile | 3 + sound/soc/fsl/fsl_amix.c | 554 +++++++++++++++++++++ sound/soc/fsl/fsl_amix.h | 101 ++++
I aimn't against the naming here, but it seems to be AUDMIX in RM?
Would it be better to align with that? It's your decision though.
To me "AUDMIX" sounds more like some RTL high level integration module, I would prefer to keep it as it is if there is no strong reason to rename it.
diff --git a/Documentation/devicetree/bindings/sound/fsl,amix.txt b/Documentation/devicetree/bindings/sound/fsl,amix.txt +================================= + - compatible : Compatible list, contains "fsl,imx8qm- amix"
+ - reg : Offset and length of the register set for the device.
+ - clocks : Must contain an entry for each entry in clock-names.
+ - clock-names : Must include the "ipg" for register access.
+ - power-domains : Must contain the phandle to the AMIX power domain node
+Device driver configuration example: +====================================== + amix: amix@59840000 { + compatible = "fsl,imx8qm-amix"; + reg = <0x0 0x59840000 0x0 0x10000>; + clocks = <&clk IMX8QXP_AUD_AMIX_IPG>; + clock-names = "ipg"; + power-domains = <&pd_amix>; + };
From the description of DT and RM, I don't see how it connects to SAIs.
Are they fixed to SAI0 and SAI1 in imx8qm? Wondering if it'd be better to have such information in the doc.
Please check chapter "16.1.2.2 Audio Mixer" in RM: it has two dedicated SAI interfaces, SAI4 and SAI5. Audio Mixer operates on bit clock of one of these interfaces.
diff --git a/sound/soc/fsl/fsl_amix.c b/sound/soc/fsl/fsl_amix.c
+static const char
- *width_sel[] = { "16b", "18b", "20b", "24b", "32b", },
- *pol_sel[] = { "Positive edge", "Negative edge", },
[...] +static const struct soc_enum fsl_amix_enum[] = { +/* FSL_AMIX_CTR enums */ [...] +SOC_ENUM_SINGLE_S(FSL_AMIX_CTR, FSL_AMIX_CTR_OUTWIDTH_SHIFT, width_sel), +SOC_ENUM_SINGLE_S(FSL_AMIX_CTR, FSL_AMIX_CTR_OUTCKPOL_SHIFT, pol_sel),
Should we handle the width in hw_param()?
The width of AMIX output (mixed) stream may be different than the width of the input streams, the assumption is that the user may want to control it from userspace.
Why do we change pol here? It feels like against set_fmt().
You're right, will remove it in the next version.
+static int fsl_amix_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{ + /* For playback the AMIX is slave, and for record is master */ + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBM_CFM: + case SND_SOC_DAIFMT_CBS_CFS:
So it's used either for playback or capture only, not both at same time?
From IP functional perspective AMIX capture is the result of AMIX playback - AMIX output represents the resulting mixed audio stream routed to SAI4 RX signals (bit & frame clocks and data). So once we have playback on either SAI4 or SAI5 (or both) - we can capture the AMIX output on SAI4.
I guess it would be nice to send the machine driver as part of this patchset also - it defines two input SAI interfaces as frontends and AMIX - as backend. Userspace sees only two SAI interfaces exposed, both of them having playback enabled, and only SAI4 having capture enabled.
Thanks Nicolin
Thank you, Viorel