| Message ID | 20241203-mt8188-6359-unhardcode-dmic-v1-1-346e3e5cbe6d@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: mediatek: mt8188-mt6359: Remove hardcoded dmic codec | expand |
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>
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
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> > >
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
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.
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
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
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,
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(-) --- base-commit: b852e1e7a0389ed6168ef1d38eb0bad71a6b11e8 change-id: 20241203-mt8188-6359-unhardcode-dmic-ba7649f8a72b Best regards,