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

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

[Nícolas F. R. A. Prado]

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.

No Devicetree currently relies on it so it is safe to remove without
worrying about backward compatibility.

Fixes: 9f08dcbddeb3 ("ASoC: mediatek: mt8188-mt6359: support new board with nau88255")
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 sound/soc/mediatek/mt8188/mt8188-mt6359.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
index 08ae962afeb92965109b303439419bc6e7c2a896..1550e56ab57d54b179ebe5cbd60db1660bb0bd2c 100644
--- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
+++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
@@ -188,9 +188,7 @@ SND_SOC_DAILINK_DEFS(pcm1,
 SND_SOC_DAILINK_DEFS(ul_src,
 		     DAILINK_COMP_ARRAY(COMP_CPU("UL_SRC")),
 		     DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound",
-						   "mt6359-snd-codec-aif1"),
-					COMP_CODEC("dmic-codec",
-						   "dmic-hifi")),
+						   "mt6359-snd-codec-aif1")),
 		     DAILINK_COMP_ARRAY(COMP_EMPTY()));
 
 SND_SOC_DAILINK_DEFS(AFE_SOF_DL2,

---
base-commit: b852e1e7a0389ed6168ef1d38eb0bad71a6b11e8
change-id: 20241203-mt8188-6359-unhardcode-dmic-ba7649f8a72b

Best regards,
-- 
Nícolas F. R. A. Prado <nfraprado@collabora.com>

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

[AngeloGioacchino Del Regno]

Il 03/12/24 20:20, Nícolas F. R. A. Prado ha scritto:
> 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.
> 
> No Devicetree currently relies on it so it is safe to remove without
> worrying about backward compatibility.
> 
> Fixes: 9f08dcbddeb3 ("ASoC: mediatek: mt8188-mt6359: support new board with nau88255")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

[Mark Brown]

On Tue, 03 Dec 2024 16:20:58 -0300, Nícolas F. R. A. Prado 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.
> 
> No Devicetree currently relies on it so it is safe to remove without
> worrying about backward compatibility.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: mediatek: mt8188-mt6359: Remove hardcoded dmic codec
      commit: ec16a3cdf37e507013062f9c4a2067eacdd12b62

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Chen-Yu Tsai]

Hi,

On Wed, Dec 4, 2024 at 3:22 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> 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?

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


Thanks
ChenYu

> Fixes: 9f08dcbddeb3 ("ASoC: mediatek: mt8188-mt6359: support new board with nau88255")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  sound/soc/mediatek/mt8188/mt8188-mt6359.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> index 08ae962afeb92965109b303439419bc6e7c2a896..1550e56ab57d54b179ebe5cbd60db1660bb0bd2c 100644
> --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> @@ -188,9 +188,7 @@ SND_SOC_DAILINK_DEFS(pcm1,
>  SND_SOC_DAILINK_DEFS(ul_src,
>                      DAILINK_COMP_ARRAY(COMP_CPU("UL_SRC")),
>                      DAILINK_COMP_ARRAY(COMP_CODEC("mt6359-sound",
> -                                                  "mt6359-snd-codec-aif1"),
> -                                       COMP_CODEC("dmic-codec",
> -                                                  "dmic-hifi")),
> +                                                  "mt6359-snd-codec-aif1")),
>                      DAILINK_COMP_ARRAY(COMP_EMPTY()));
>
>  SND_SOC_DAILINK_DEFS(AFE_SOF_DL2,
>
> ---
> base-commit: b852e1e7a0389ed6168ef1d38eb0bad71a6b11e8
> change-id: 20241203-mt8188-6359-unhardcode-dmic-ba7649f8a72b
>
> Best regards,
> --
> Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
>

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

[Nícolas F. R. A. Prado]

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@collabora.com> 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
--------------------      --------------------

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.

[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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On Mon, Jan 06, 2025 at 02:33:10PM -0300, Nícolas F. R. A. Prado wrote:

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

> [1] https://www.kernel.org/doc/html/latest/sound/soc/codec-to-codec.html

That's quite widely used in phones FWIW.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Chen-Yu Tsai]

On Tue, Jan 7, 2025 at 1:33 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> 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@collabora.com> 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.

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.

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

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

[Nícolas F. R. A. Prado]

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@collabora.com> 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@collabora.com> 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