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@xxxxxxxxxxxx>
>
> 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?
> 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.
Thanks,
Trevor
