Messages in this thread | | | Date | Mon, 14 Mar 2022 10:00:58 +0100 | Subject | Re: [PATCH 4/5] ASoC: mediatek: mt8195: add machine driver with mt6359, max98390 and rt5682 | From | AngeloGioacchino Del Regno <> |
| |
Il 12/03/22 17:18, Trevor Wu ha scritto: > On Thu, 2022-03-10 at 16:21 +0100, AngeloGioacchino Del Regno wrote: >> Il 08/03/22 08:24, Trevor Wu ha scritto: >>> This patch adds support for mt8195 board with mt6359, max98390 and >>> rt5682. >>> >>> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> >> >> Hello Trevor, >> thanks for the patch! However, there's something to improve... >> >>> --- >>> sound/soc/mediatek/Kconfig | 16 + >>> sound/soc/mediatek/mt8195/Makefile | 5 + >>> .../mt8195/mt8195-mt6359-max98390-rt5682.c | 1058 >>> +++++++++++++++++ >>> 3 files changed, 1079 insertions(+) >>> create mode 100644 sound/soc/mediatek/mt8195/mt8195-mt6359- >>> max98390-rt5682.c >>> >>> > > [...] >>> + >>> +static const struct snd_soc_dapm_widget >>> + mt8195_mt6359_max98390_rt5682_widgets[] = { >>> + SND_SOC_DAPM_SPK("Left Speaker", NULL), >>> + SND_SOC_DAPM_SPK("Right Speaker", NULL), >>> + SND_SOC_DAPM_HP("Headphone Jack", NULL), >> >> We can at least partially reuse existing UCM2 configuration if you >> slightly change the names for these controls. >> > > I don't know what the UCM2 configuration means. > Could you give me more information? >
UCM == Use Case Manager; In short, it's userspace (alsa-lib) configuration for sound cards, allowing to configure the various mixers for various usecases (speaker/headphone/HDMI playback, headset/internal microphone, etc).
Check this GitHub repository for more information: https://github.com/alsa-project/alsa-ucm-conf/tree/master/ucm2
> >> Specifically, MAX98090 (yes I know it's a different codec) has names >> "Speaker Left", "Speaker Right" instead, we will be able to at least >> partially reuse these (or get uniform naming, which is still good). >> As for the "Headphone Jack", it's simply "Headphone". >> >> Please note that the actual control names in userspace will be, >> exactly, >> >> "Speaker Left Switch", "Speaker Right Switch", >> "Headphone Left Switch", "Headphone Right Switch"... >> >> ....where "Switch" gets automatically appended because of the control >> type. >> >>> + SND_SOC_DAPM_MIC("Headset Mic", NULL), >> >> This "Headset Mic" name is fine. >> >>> + SND_SOC_DAPM_MIXER(SOF_DMA_DL2, SND_SOC_NOPM, 0, 0, NULL, 0), >>> + SND_SOC_DAPM_MIXER(SOF_DMA_DL3, SND_SOC_NOPM, 0, 0, NULL, 0), >>> + SND_SOC_DAPM_MIXER(SOF_DMA_UL4, SND_SOC_NOPM, 0, 0, NULL, 0), >>> + SND_SOC_DAPM_MIXER(SOF_DMA_UL5, SND_SOC_NOPM, 0, 0, NULL, 0), >>> +}; >>> + > [...] >>> + >>> +static struct snd_soc_dai_link >>> mt8195_mt6359_max98390_rt5682_dai_links[] = { >> >> >> ... again, different name, same contents ... >> >> >> And I won't go on repeating the same thing over and over again. >> I think that the best idea here is to either create a mt8195-mt6359- >> rt5682-common.c >> file, or to rename the others to something else and get them all in >> the same file. >> >> >> Regards, >> Angelo > > Hi Angelo, > > Thanks for your review. > Please forgive me for deleting some comments above. > I totally agree that most code can be reused. > I will try revising and merging all mt8195 machine drivers in a file.
No worries. Looking forward to see the next version. Thank you!
Regards, Angelo
> > Thanks, > Trevor >
| |