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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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
>