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

[Trevor Wu]

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