Re: [PATCH] ASoC: mediatek: mt8188-mt6359: Remove hardcoded dmic codec

[Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>]

On Tue, Jan 07, 2025 at 01:03:08PM +0800, Chen-Yu Tsai wrote:
> On Tue, Jan 7, 2025 at 1:33 AM Nícolas F. R. A. Prado
> <nfraprado@xxxxxxxxxxxxx> wrote:
> >
> > On Thu, Dec 26, 2024 at 04:30:17PM +0800, Chen-Yu Tsai wrote:
> > > Hi,
> > >
> > > On Wed, Dec 4, 2024 at 3:22 AM Nícolas F. R. A. Prado
> > > <nfraprado@xxxxxxxxxxxxx> wrote:
> > > >
> > > > Remove hardcoded dmic codec from the UL_SRC dai link to avoid requiring
> > > > a dmic codec to be present for the driver to probe, as not every
> > > > MT8188-based platform might need a dmic codec. The codec can be assigned
> > > > to the dai link through the dai-link property in Devicetree on the
> > > > platforms where it is needed.
> > >
> > > A followup question about this. The DMICs on the Chromebooks are attached
> > > to the PMIC codec's input side, which then converts the signals to standard
> > > I2S and passes them out to the SoC through its AIF1. So the original code
> > > was somewhat incorrect, though it works.
> > >
> > > How should we describe such a connection, given that the MediaTek sound
> > > bindings aren't a full graph?
> >
> > What you're describing is that the hardware topology looks like this:
> >
> > --------------------      --------------------
> > |    SoC           |      |   MT6359 PMIC    |
> > |        UL_SRC BE | <--- | AIF1   AIN0_DMIC | <-- DMic
> > --------------------      --------------------
> 
> Correct.
> 
> > But that the dailink definition in the machine driver had the DMic codec
> > connected directly to the UL_SRC BE instead, alongside the connection to the
> > PMIC, unlike the topology above.
> >
> > My understanding is that the dmic codec was added simply to allow the usage of
> > the wakeup-delays. From [1] it appears that DAI connections between two codecs
> > are possible, though rare. So the PMIC -> DMic connection description might be
> > possible in that way, although I'm not sure it brings any benefits besides
> > closer resembling the hardware topology.
> 
> I suspect we would want to keep the wakeup delays though. AFAICT they aren't
> the same number across the board (no pun intended), but actually differ
> between devices, perhaps due to differences in the actual DMIC used.

We can still keep the delays. We can keep assigning the dmic codec to the UL_SRC
BE, only through the DT now rather than hardcoded in the driver:

	dmic: dmic-codec {
		compatible = "dmic-codec";
		num-channels = <2>;
		wakeup-delay-ms = <50>;
		#sound-dai-cells = <0>;
	};

	&sound {
		...

		dai-link-1 {
			link-name = "UL_SRC_BE";

			codec {
				sound-dai = <&pmic 0>, <&dmic>;
			};
		};
	};

It still doesn't match the hardware topology, but the delay should work the same
as before.

> 
> If we don't want the full description, maybe we add the wakeup delay to
> the PMIC codec then?
> 
> AFAICT [1] is basically hardcoding in the dmic-codec in a different way,
> so basically reverting your original patch.

Hm, on a second look I think you're right.

Thanks,
Nícolas

> 
> ChenYu
> 
> > [1] https://www.kernel.org/doc/html/latest/sound/soc/codec-to-codec.html
> >
> > >
> > > > No Devicetree currently relies on it so it is safe to remove without
> > > > worrying about backward compatibility.
> > >
> > > Removing it didn't seem to cause any issues for the Chromebooks that
> > > do actually have DMICs. I suspect the only difference would be that
> > > the wakeup-delays no longer apply correctly.
> >
> > That's my guess too.
> >
> > Thanks,
> > Nícolas