| Message ID | 20220211103818.8266-1-jiaxin.yu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8186 SoC | expand |
On Fri, Feb 11, 2022 at 06:38:06PM +0800, Jiaxin Yu wrote: This looks pretty good, there's some issues below but they're all very minor, mostly stylistic things rather than anything substantial. > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * MediaTek ALSA SoC Audio DAI ADDA Control > + * > + * Copyright (c) 2022 MediaTek Inc. > + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com> > + */ Please make the entire comment a C++ one so things look more intentional. > +static int mtk_adda_ul_event(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > +{ > + struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + int mtkaif_dmic = afe_priv->mtkaif_dmic; > + > + dev_info(afe->dev, "%s(), name %s, event 0x%x, mtkaif_dmic %d\n", > + __func__, w->name, event, mtkaif_dmic); This should be dev_dbg() at most, otherwise the logs will get very noisy (but note that there are trace points in the core which cover this). There's a bunch of other dev_info() calls like this on DAPM events. > + if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2_CLK_P2) > + regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x38); > + else if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2) > + regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30); > + else > + regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30); This could be more clearly written as a switch statement. > + if (strcmp(w->name, "ADDA_MTKAIF_CFG") == 0) { > + if (afe_priv->mtkaif_chosen_phase[0] < 0 && > + afe_priv->mtkaif_chosen_phase[1] < 0) { > + dev_info(afe->dev, > + "%s(), calib fail mtkaif_chosen_phase[0/1]:%d/%d\n", > + __func__, Should this be a dev_err() given that the calibration failed? > +/* mtkaif dmic */ > +static const char * const mt8186_adda_off_on_str[] = { > + "Off", "On" > +}; > + > +static const struct soc_enum mt8186_adda_enum[] = { > + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8186_adda_off_on_str), > + mt8186_adda_off_on_str), > +}; This is a simple on/off control so should be a standard numeric control with a name ending in Switch to help UIs handle it properly. > +static int mt8186_adda_dmic_set(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol); > + struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt); > + struct mt8186_afe_private *afe_priv = afe->platform_priv; > + struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; > + int dmic_on; > + > + if (ucontrol->value.enumerated.item[0] >= e->items) > + return -EINVAL; > + > + dmic_on = ucontrol->value.integer.value[0]; > + > + dev_info(afe->dev, "%s(), kcontrol name %s, dmic_on %d\n", > + __func__, kcontrol->id.name, dmic_on); > + > + afe_priv->mtkaif_dmic = dmic_on; > + return 0; This should return 1 if the value changed so an event is generated for userspace. You might want to run the mixer-test kselftest (ideally the version that's in -next as there were a few bits added very recently), it should detect issues like this.
On Fri, Feb 11, 2022 at 06:38:13PM +0800, Jiaxin Yu wrote: > sound/soc/mediatek/Kconfig | 44 + > sound/soc/mediatek/Makefile | 1 + > sound/soc/mediatek/mt8186/Makefile | 21 + > sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719 ++++ > sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 + > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 245 ++ > .../soc/mediatek/mt8186/mt8186-afe-control.c | 262 ++ > sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 211 + > sound/soc/mediatek/mt8186/mt8186-afe-gpio.h | 19 + > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3030 +++++++++++++++ > .../mediatek/mt8186/mt8186-interconnection.h | 69 + > .../soc/mediatek/mt8186/mt8186-misc-control.c | 1729 +++++++++ > sound/soc/mediatek/mt8186/mt8186-reg.h | 3433 +++++++++++++++++ I know it's already a long series but perhaps the clock and GPIO bits could be split out into separate patches? This one patch is over 300K which is a bit much in one go, especially when it's not just all big tables.
On Fri, 2022-02-11 at 16:14 +0000, Mark Brown wrote: > On Fri, Feb 11, 2022 at 06:38:03PM +0800, Jiaxin Yu wrote: > > This series of patches adds support for Mediatek AFE of MT8186 Soc. > > Patches are based on broonie tree "for-next" branch. > > This all looks basically good, there are some issues which I've > highlighted on the patches but like I kept on saying they're all > fairly > small and hopefully easy to address - there's no big structural > problems > or anything that I noticed. Thank you for helping to review my submitted patches. I will go through your comments carefully and fix them.
On Fri, 2022-02-11 at 16:12 +0000, Mark Brown wrote: > On Fri, Feb 11, 2022 at 06:38:13PM +0800, Jiaxin Yu wrote: > > > sound/soc/mediatek/Kconfig | 44 + > > sound/soc/mediatek/Makefile | 1 + > > sound/soc/mediatek/mt8186/Makefile | 21 + > > sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719 ++++ > > sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 + > > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 245 ++ > > .../soc/mediatek/mt8186/mt8186-afe-control.c | 262 ++ > > sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 211 + > > sound/soc/mediatek/mt8186/mt8186-afe-gpio.h | 19 + > > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3030 > > +++++++++++++++ > > .../mediatek/mt8186/mt8186-interconnection.h | 69 + > > .../soc/mediatek/mt8186/mt8186-misc-control.c | 1729 +++++++++ > > sound/soc/mediatek/mt8186/mt8186-reg.h | 3433 > > +++++++++++++++++ > > I know it's already a long series but perhaps the clock and GPIO bits > could be split out into separate patches? This one patch is over > 300K > which is a bit much in one go, especially when it's not just all big > tables. Ok, I see. I've split them out into separate patches in v2 version.
This series of patches adds support for Mediatek AFE of MT8186 Soc. Patches are based on broonie tree "for-next" branch. Jiaxin Yu (15): ASoC: mediatek: mt6366: add codec driver ASoC: mediatek: mt8186: support audsys clock control ASoC: mediatek: mt8186: support adda in platform driver ASoC: mediatek: mt8186: support hostless in platform driver ASoC: mediatek: mt8186: support hw gain in platform driver ASoC: mediatek: mt8186: support i2s in platform driver ASoC: mediatek: mt8186: support pcm in platform driver ASoC: mediatek: mt8186: support src in platform driver ASoC: mediatek: mt8186: support tdm in platform driver ASoC: mediatek: mt8186: add platform driver dt-bindings: mediatek: mt8186: add audio afe document ASoC: mediatek: mt8186: add machine driver with mt6366, da7219 and max98357 dt-bindings: mediatek: mt8186: add mt8186-mt6366-da7219-max98357 document ASoC: mediatek: mt8186: add machine driver with mt6366, rt1019 and rt5682 dt-bindings: mediatek: mt8186: add mt8186-mt6366-rt1019-rt5682 document .../bindings/sound/mt8186-afe-pcm.yaml | 175 + .../sound/mt8186-mt6366-da7219-max98357.yaml | 47 + .../sound/mt8186-mt6366-rt1019-rt5682.yaml | 47 + sound/soc/codecs/Kconfig | 8 + sound/soc/codecs/Makefile | 2 + sound/soc/mediatek/Kconfig | 44 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/mt8186/Makefile | 21 + sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719 ++++ sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 + sound/soc/mediatek/mt8186/mt8186-afe-common.h | 245 ++ .../soc/mediatek/mt8186/mt8186-afe-control.c | 262 ++ sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 211 + sound/soc/mediatek/mt8186/mt8186-afe-gpio.h | 19 + sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3030 +++++++++++++++ sound/soc/mediatek/mt8186/mt8186-audsys-clk.c | 151 + sound/soc/mediatek/mt8186/mt8186-audsys-clk.h | 15 + .../soc/mediatek/mt8186/mt8186-audsys-clkid.h | 45 + sound/soc/mediatek/mt8186/mt8186-dai-adda.c | 897 +++++ .../soc/mediatek/mt8186/mt8186-dai-hostless.c | 296 ++ .../soc/mediatek/mt8186/mt8186-dai-hw-gain.c | 246 ++ sound/soc/mediatek/mt8186/mt8186-dai-i2s.c | 1352 +++++++ sound/soc/mediatek/mt8186/mt8186-dai-pcm.c | 433 +++ sound/soc/mediatek/mt8186/mt8186-dai-src.c | 758 ++++ sound/soc/mediatek/mt8186/mt8186-dai-tdm.c | 723 ++++ .../mediatek/mt8186/mt8186-interconnection.h | 69 + .../soc/mediatek/mt8186/mt8186-misc-control.c | 1729 +++++++++ .../mt8186/mt8186-mt6366-da7219-max98357.c | 903 +++++ .../mt8186/mt8186-mt6366-rt1019-rt5682.c | 887 +++++ sound/soc/mediatek/mt8186/mt8186-reg.h | 3433 +++++++++++++++++ 30 files changed, 16978 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mt8186-afe-pcm.yaml create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-da7219-max98357.yaml create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-rt1019-rt5682.yaml create mode 100644 sound/soc/mediatek/mt8186/Makefile create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clk.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clk.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clkid.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-hostless.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-hw-gain.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-i2s.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-pcm.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-src.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-tdm.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-interconnection.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-misc-control.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-reg.h