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@xxxxxxx> > 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@xxxxxxx> > > --- > > .../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
