lkml.org 
[lkml]   [2022]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 4/5] ASoC: mediatek: mt8195: add machine driver with mt6359, max98390 and rt5682
    From
    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
    >


    \
     
     \ /
      Last update: 2022-03-14 10:03    [W:2.851 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site