Re: [PATCH 4/5] ASoC: mediatek: mt8195: add machine driver with mt6359, max98390 and rt5682

[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@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?

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