Re: [RFC PATCH] ASoC: fsl: Add Audio Mixer CPU DAI driver

[Viorel Suman]

Hi Nicolin,

On Jo, 2019-01-03 at 12:03 -0800, Nicolin Chen wrote:
> Hi,
> 
> On Thu, Jan 03, 2019 at 03:56:46PM +0000, Viorel Suman wrote:
> > 
> > > 
> > > > 
> > > > Â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.
> We had AUDMUX, so "AUDMIX" doesn't sound bad to me at all. The only
> reason that we are discussing this is because RM uses more "AUDMIX"s
> over "amix"s. I'd have chosen AUDMIX if I were you, yet not strongly
> as I said. And it looks like Rob is asking you to use AUDMIX in DT
> binding doc.

Ok, I'll change in the next version the compatible string to "audmix"
as Rob requested.

> 
> > 
> > > 
> > > > 
> > > > +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.
> OK. I am actually more wondering how you connect it with SAI on
> the software level: for imx8qm, SAI4/5 are used, but later SoCs
> might use other SAI blocks. So it might be necessary to indicate
> the connections in DT.

See below.

> 
> > 
> > > 
> > > > 
> > > > +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.
> Ah, it sounds like a looping block then, receiving bclk from SAI4
> -- slave, and routing the bclk back to SAI4 -- master; SAI4 works
> at ASYNC mode?

Right. And yes, SAI4 works in ASYNC mode.

> 
> > 
> > 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.
> DPCM? So you are having the links in the sound DT nodes, i.e. the
> machine driver?

The machine driver DT configuration looks like:
================
sound-amix-saiÂ{
 compatible = "fsl,imx-audio-amix";
 model = "amix-audio-sai";
 dais = <&sai4>, <&sai5>;
 amix-controller = <&amix>;
};

The number of elements in "dais" array is at least two elements, DAPM
routes are generated dynamically in machine driver at probe as function
of number of SAI instances, the capture is enabled only for the first
SAI instance in the list.

> 
> Thanks
> Nicolin

Regards,
Viorel