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 [off-list ref]
wrote:

quoted

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 <redacted>
---
 .../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.

quoted

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.

quoted

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.

quoted

+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