| 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:04PM +0800, Jiaxin Yu wrote: > Mt6366 is a new version of mt6358, and they are same about audio part. > So we can reuse the driver of mt6358. > snd-soc-mt6359-accdet-objs := mt6359-accdet.o > +snd-soc-mt6366-objs := mt6358.o > snd-soc-mt6660-objs := mt6660.o > snd-soc-nau8315-objs := nau8315.o > snd-soc-nau8540-objs := nau8540.o > @@ -465,6 +466,7 @@ obj-$(CONFIG_SND_SOC_MT6351) += snd-soc-mt6351.o > obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o > obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o > obj-$(CONFIG_SND_SOC_MT6359_ACCDET) += mt6359-accdet.o > +obj-$(CONFIG_SND_SOC_MT6366) += snd-soc-mt6366.o Why build a separate module here? That'll just make the binary larger but not otherwise do anything. Usually we'd just add a new ID to the existing driver.
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:08PM +0800, Jiaxin Yu wrote: Again, mostly looks good just some minor issues (I've not repeated some that applied to the prior code): > +static const struct snd_kcontrol_new mtk_hw_gain2_in_ch1_mix[] = { > + SOC_DAPM_SINGLE_AUTODISABLE("ADDA_UL_CH1", AFE_CONN15, > + I_ADDA_UL_CH1, 1, 0), > +}; These end up as regular user visible controls so should have standard control names - in this case ending in Switch since it's a simple on/off. A similar issue was there in the earlier patches. > +static const struct snd_kcontrol_new mtk_hw_gain_controls[] = { > + SOC_SINGLE("HW Gain 1", AFE_GAIN1_CON1, > + GAIN1_TARGET_SFT, GAIN1_TARGET_MASK, 0), > + SOC_SINGLE("HW Gain 2", AFE_GAIN2_CON1, > + GAIN2_TARGET_SFT, GAIN2_TARGET_MASK, 0), These should have standard names like "HW 1 Volume" so userspace has a better idea how to display them.
On Fri, Feb 11, 2022 at 06:38:12PM +0800, Jiaxin Yu wrote: Again, mostly looks good just fairly small and easily fixable issues: > +static int mtk_tdm_hd_en_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); > + > + dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n", > + __func__, w->name, event); > + > + return 0; > +} This does nothing, you can just remove it. > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { > + case SND_SOC_DAIFMT_NB_NF: > + tdm_priv->bck_invert = TDM_BCK_NON_INV; > + tdm_priv->lck_invert = TDM_LCK_NON_INV; > + break; > + case SND_SOC_DAIFMT_NB_IF: > + tdm_priv->bck_invert = TDM_BCK_NON_INV; > + tdm_priv->lck_invert = TDM_LCK_INV; > + break; > + case SND_SOC_DAIFMT_IB_NF: > + tdm_priv->bck_invert = TDM_BCK_INV; > + tdm_priv->lck_invert = TDM_LCK_NON_INV; > + break; > + case SND_SOC_DAIFMT_IB_IF: > + default: > + tdm_priv->bck_invert = TDM_BCK_INV; > + tdm_priv->lck_invert = TDM_LCK_INV; You should return an error in the default case rather than just picking one of the behaviours to help spot any configuration errors. > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > + case SND_SOC_DAIFMT_CBM_CFM: > + tdm_priv->slave_mode = false; > + break; > + case SND_SOC_DAIFMT_CBS_CFS: > + tdm_priv->slave_mode = true; We're trying to move away from these defines and the master/slave terminology to talk about clock providers instead - the new defines are _PROVIDER_MASK, _DAIFMT_CBP_CFP and _DAIFMT_CBC_CFC.
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, 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.
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.
On Fri, 2022-02-11 at 15:24 +0000, Mark Brown wrote: > On Fri, Feb 11, 2022 at 06:38:12PM +0800, Jiaxin Yu wrote: > > Again, mostly looks good just fairly small and easily fixable issues: > > > +static int mtk_tdm_hd_en_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); > > + > > + dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n", > > + __func__, w->name, event); > > + > > + return 0; > > +} > > This does nothing, you can just remove it. Yes, this is readlly useless, removed it in the v2 version. > > > + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { > > + case SND_SOC_DAIFMT_NB_NF: > > + tdm_priv->bck_invert = TDM_BCK_NON_INV; > > + tdm_priv->lck_invert = TDM_LCK_NON_INV; > > + break; > > + case SND_SOC_DAIFMT_NB_IF: > > + tdm_priv->bck_invert = TDM_BCK_NON_INV; > > + tdm_priv->lck_invert = TDM_LCK_INV; > > + break; > > + case SND_SOC_DAIFMT_IB_NF: > > + tdm_priv->bck_invert = TDM_BCK_INV; > > + tdm_priv->lck_invert = TDM_LCK_NON_INV; > > + break; > > + case SND_SOC_DAIFMT_IB_IF: > > + default: > > + tdm_priv->bck_invert = TDM_BCK_INV; > > + tdm_priv->lck_invert = TDM_LCK_INV; > > You should return an error in the default case rather than just > picking > one of the behaviours to help spot any configuration errors. Done in the v2 version. > > > + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { > > + case SND_SOC_DAIFMT_CBM_CFM: > > + tdm_priv->slave_mode = false; > > + break; > > + case SND_SOC_DAIFMT_CBS_CFS: > > + tdm_priv->slave_mode = true; > > We're trying to move away from these defines and the master/slave > terminology to talk about clock providers instead - the new defines > are > _PROVIDER_MASK, _DAIFMT_CBP_CFP and _DAIFMT_CBC_CFC. Done in the v2 version.
On Fri, 2022-02-11 at 15:15 +0000, Mark Brown wrote: > On Fri, Feb 11, 2022 at 06:38:08PM +0800, Jiaxin Yu wrote: > > Again, mostly looks good just some minor issues (I've not repeated > some > that applied to the prior code): > > > +static const struct snd_kcontrol_new mtk_hw_gain2_in_ch1_mix[] = { > > + SOC_DAPM_SINGLE_AUTODISABLE("ADDA_UL_CH1", AFE_CONN15, > > + I_ADDA_UL_CH1, 1, 0), > > +}; > > These end up as regular user visible controls so should have standard > control names - in this case ending in Switch since it's a simple > on/off. A similar issue was there in the earlier patches. Yes, I have corrected the control names in the other patches together. > > > +static const struct snd_kcontrol_new mtk_hw_gain_controls[] = { > > + SOC_SINGLE("HW Gain 1", AFE_GAIN1_CON1, > > + GAIN1_TARGET_SFT, GAIN1_TARGET_MASK, 0), > > + SOC_SINGLE("HW Gain 2", AFE_GAIN2_CON1, > > + GAIN2_TARGET_SFT, GAIN2_TARGET_MASK, 0), > > These should have standard names like "HW 1 Volume" so userspace has > a > better idea how to display them. Ok, I see.
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