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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <db4d4b19-0c44-0bad-3544-01237bbb6c07@collabora.com>
Date:   Mon, 14 Mar 2022 10:00:58 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Trevor Wu <trevor.wu@...iatek.com>, broonie@...nel.org,
        tiwai@...e.com, robh+dt@...nel.org, matthias.bgg@...il.com
Cc:     alsa-devel@...a-project.org, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, aaronyu@...gle.com,
        yc.hung@...iatek.com
Subject: Re: [PATCH 4/5] ASoC: mediatek: mt8195: add machine driver with
 mt6359, max98390 and rt5682

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@...iatek.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
>