| Message ID | 20220428093355.16172-1-jiaxin.yu@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8186 SoC | expand |
On Thu, Apr 28, 2022 at 05:33:50PM +0800, Jiaxin Yu wrote: > Add mt8186 platform and affiliated driver. > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > --- > sound/soc/mediatek/Kconfig | 44 + > sound/soc/mediatek/Makefile | 1 + > sound/soc/mediatek/mt8186/Makefile | 22 + > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3005 +++++++++++++++++ > .../mediatek/mt8186/mt8186-interconnection.h | 69 + > .../soc/mediatek/mt8186/mt8186-misc-control.c | 294 ++ > .../mediatek/mt8186/mt8186-mt6366-common.c | 59 + > .../mediatek/mt8186/mt8186-mt6366-common.h | 17 + > sound/soc/mediatek/mt8186/mt8186-reg.h | 2913 ++++++++++++++++ > 11 files changed, 6920 insertions(+) This looks mostly good though it is enormous so I might've missed some things. The patch series is already very large but it might still be worth splitting this up a bit more, perhaps split the code and data tables/register definitions into separate patches? A few relatively minor issues with the controls. > +/* this order must match reg bit amp_div_ch1/2 */ > +static const char * const mt8186_sgen_amp_str[] = { > + "1/128", "1/64", "1/32", "1/16", "1/8", "1/4", "1/2", "1" }; > +static const char * const mt8186_sgen_mute_str[] = { > + "Off", "On" > +}; On/off controls should be normal Switch controls not enums so userspace can display things sensibly. > +static int mt8186_sgen_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 mode; > + int mode_idx; > + > + if (ucontrol->value.enumerated.item[0] >= e->items) > + return -EINVAL; ... > + 0x3f << INNER_LOOP_BACK_MODE_SFT); > + } > + > + afe_priv->sgen_mode = mode; > + > + return 0; > +} This should return 1 if the value is different from the previous value so event generation works, please run mixer-test against a system using the driver to help spot issues like this. The same issue applies to at least some of the other custom controls. > +static int mt8186_sgen_mute_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 soc_enum *e = (struct soc_enum *)kcontrol->private_value; > + int mute; > + > + if (ucontrol->value.enumerated.item[0] >= e->items) > + return -EINVAL; > + > + mute = ucontrol->value.integer.value[0]; > + > + dev_dbg(afe->dev, "%s(), kcontrol name %s, mute %d\n", > + __func__, kcontrol->id.name, mute); > + > + if (strcmp(kcontrol->id.name, SGEN_MUTE_CH1_KCONTROL_NAME) == 0) { > + regmap_update_bits(afe->regmap, AFE_SINEGEN_CON0, > + MUTE_SW_CH1_MASK_SFT, > + mute << MUTE_SW_CH1_SFT); > + } else { > + regmap_update_bits(afe->regmap, AFE_SINEGEN_CON0, > + MUTE_SW_CH2_MASK_SFT, > + mute << MUTE_SW_CH2_SFT); > + } > + > + return 0; > +} I can't tell why some of these are done with custom code rather than using a normal SOC_SINGLE()?
On Thu, Apr 28, 2022 at 05:33:37PM +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 looks mostly good from a framework point of view - I did notice a few minor issues which I commented on (some of the control things are repeated in other patches) but the overwhelming bulk of the code looks good.
Hi Jiaxin,
Gmail tends to mark your patches as spam.
Can you please make sure to use "PATCH" in the subject line, e.g.
"[PATCH v4 00/18] ASoC: mediatek: Add support for MT8186 SoC"?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Fri, 2022-04-29 at 10:47 +0200, Geert Uytterhoeven wrote: > Hi Jiaxin, > > Gmail tends to mark your patches as spam. > Can you please make sure to use "PATCH" in the subject line, e.g. > "[PATCH v4 00/18] ASoC: mediatek: Add support for MT8186 SoC"? > > Thanks! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a > hacker. But > when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds Hi Geert, Sorry for this mistake, I usually use "git format-patch --subject- prefix "v4" --cover-letter -x" to generate a series of patches. So it automatically removes "PATCH". I will correct the cmd to "git format-patch --subject-prefix "PATCH v4" --cover-letter -x". Thanks, Jiaxin.Yu
On Thu, 2022-04-28 at 13:19 +0100, Mark Brown wrote: > On Thu, Apr 28, 2022 at 05:33:37PM +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 looks mostly good from a framework point of view - I did notice > a > few minor issues which I commented on (some of the control things are > repeated in other patches) but the overwhelming bulk of the code > looks > good. Hi Mark, Thank you very much for your review. I will check the codes with your comments carefully. Best Regards Jiaxin.Yu
On Thu, 2022-04-28 at 13:02 +0100, Mark Brown wrote: > On Thu, Apr 28, 2022 at 05:33:50PM +0800, Jiaxin Yu wrote: > > Add mt8186 platform and affiliated driver. > > > > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com> > > --- > > sound/soc/mediatek/Kconfig | 44 + > > sound/soc/mediatek/Makefile | 1 + > > sound/soc/mediatek/mt8186/Makefile | 22 + > > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > > .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ > > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3005 > > +++++++++++++++++ > > .../mediatek/mt8186/mt8186-interconnection.h | 69 + > > .../soc/mediatek/mt8186/mt8186-misc-control.c | 294 ++ > > .../mediatek/mt8186/mt8186-mt6366-common.c | 59 + > > .../mediatek/mt8186/mt8186-mt6366-common.h | 17 + > > sound/soc/mediatek/mt8186/mt8186-reg.h | 2913 > > ++++++++++++++++ > > 11 files changed, 6920 insertions(+) > > This looks mostly good though it is enormous so I might've missed > some > things. The patch series is already very large but it might still be > worth splitting this up a bit more, perhaps split the code and data > tables/register definitions into separate patches? > Yes, agree with you. I will spit them into three patches: PATCH 1: - mt8186-reg.h - mt8186-interconnection.h - mt8186-misc-control.c PATCH 2: - mt8186-mt6366-common.c - mt8186-mt6366-common.h PATCH 3: - sound/soc/mediatek/Kconfig - sound/soc/mediatek/Makefile - sound/soc/mediatek/mt8186/Makefile - sound/soc/mediatek/mt8186/mt8186-afe-common.h - .../soc/mediatek/mt8186/mt8186-afe-control.c - sound/soc/mediatek/mt8186/mt8186-afe-pcm.c > A few relatively minor issues with the controls. > > > +/* this order must match reg bit amp_div_ch1/2 */ > > +static const char * const mt8186_sgen_amp_str[] = { > > + "1/128", "1/64", "1/32", "1/16", "1/8", "1/4", "1/2", "1" }; > > +static const char * const mt8186_sgen_mute_str[] = { > > + "Off", "On" > > +}; > > On/off controls should be normal Switch controls not enums so > userspace > can display things sensibly. > > > +static int mt8186_sgen_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 mode; > > + int mode_idx; > > + > > + if (ucontrol->value.enumerated.item[0] >= e->items) > > + return -EINVAL; > > ... > > > + 0x3f << INNER_LOOP_BACK_MODE_SFT); > > + } > > + > > + afe_priv->sgen_mode = mode; > > + > > + return 0; > > +} > > This should return 1 if the value is different from the previous > value > so event generation works, please run mixer-test against a system > using > the driver to help spot issues like this. The same issue applies to > at > least some of the other custom controls. > Got it. > > +static int mt8186_sgen_mute_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 soc_enum *e = (struct soc_enum *)kcontrol- > > >private_value; > > + int mute; > > + > > + if (ucontrol->value.enumerated.item[0] >= e->items) > > + return -EINVAL; > > + > > + mute = ucontrol->value.integer.value[0]; > > + > > + dev_dbg(afe->dev, "%s(), kcontrol name %s, mute %d\n", > > + __func__, kcontrol->id.name, mute); > > + > > + if (strcmp(kcontrol->id.name, SGEN_MUTE_CH1_KCONTROL_NAME) == > > 0) { > > + regmap_update_bits(afe->regmap, AFE_SINEGEN_CON0, > > + MUTE_SW_CH1_MASK_SFT, > > + mute << MUTE_SW_CH1_SFT); > > + } else { > > + regmap_update_bits(afe->regmap, AFE_SINEGEN_CON0, > > + MUTE_SW_CH2_MASK_SFT, > > + mute << MUTE_SW_CH2_SFT); > > + } > > + > > + return 0; > > +} > > I can't tell why some of these are done with custom code rather than > using a normal SOC_SINGLE()? Yes, it's better to use SOC_SINGLE. I will fix them in next version. Thanks, Jiaxin.yu
Hi Jiaxin, On Fri, Apr 29, 2022 at 11:32 AM Jiaxin Yu <jiaxin.yu@mediatek.com> wrote: > On Fri, 2022-04-29 at 10:47 +0200, Geert Uytterhoeven wrote: > > Gmail tends to mark your patches as spam. > > Can you please make sure to use "PATCH" in the subject line, e.g. > > "[PATCH v4 00/18] ASoC: mediatek: Add support for MT8186 SoC"? > Sorry for this mistake, I usually use "git format-patch --subject- > prefix "v4" --cover-letter -x" to generate a series of patches. > So it automatically removes "PATCH". I will correct the cmd to "git > format-patch --subject-prefix "PATCH v4" --cover-letter -x". You can just use e.g. "-v4" instead of the --subject-prefix option. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
This series of patches adds support for Mediatek AFE of MT8186 Soc. Patches are based on broonie tree "for-next" branch. Changes since v3: - [v4 09/18] - remove DEBUG_COEFF debugging code - [v4 10/18] - simplify the logic of the code - [v4 13/18] - split out the MT6366 bits into mt8186-mt6366-commom.c - fix build error of "error: 'struct dev_pm_info' has no member named 'runtime_error'" - fix bug of adda dai driver - add route for pcm interface channel 2. - [v4 15/18] - [v4 17/18] - commonize the configuration of the codecs - move MT6366 common bits into mt8186-mt6366-common.c Changes since v2: - add a new compatible string "mediatek,mt6366-sound" - modify the log level for simplicity - use dev_err_probe(...) instead of dev_err(...) in dev probe() - optimized the logic of some code - use BIT() and GENMASK() macros to descript the registers Thanks for AngeloGioacchino's careful reviews. Changes since v1: [v2 01/17] - add a new ID to the existing mt6358 codec driver [v2 03/17] - modify log level in DAPM events - use standard numeric control with name ending in Switch - return 1 when the value changed in mixer control's .get callback [v2 05/17] - ending in Switch to the standard on/off controls - change to "HW Gain 1 Volume" and "HW Gain 2 Volume" [v2 09/17] - return an error in the default case rather than just picking one of the behaviours when do .set_fmt - use the new defines that are _PROVIDER_MASK, _DAIFMT_CBP_CFP and _DAIFMT_CBC_CFC [v2 10/17] [v2 11/17] - the clock and gpio are aplit out into separate patches The source file's GPL comment use c++ style, and the header fils's GPL comment use c style. We have added "Switch" after the names of all the controls that just are simple on/off. Jiaxin Yu (18): ASoC: mediatek: mt6366: support for mt6366 codec dt-bindings: mediatek: mt6358: add new compatible for using mt6366 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: support audio clock control in platform driver ASoC: mediatek: mt8186: support gpio control 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 rt5682s dt-bindings: mediatek: mt8186: add mt8186-mt6366-rt1019-rt5682s document .../devicetree/bindings/sound/mt6358.txt | 4 +- .../bindings/sound/mt8186-afe-pcm.yaml | 175 + .../sound/mt8186-mt6366-da7219-max98357.yaml | 71 + .../sound/mt8186-mt6366-rt1019-rt5682s.yaml | 71 + sound/soc/codecs/mt6358.c | 1 + sound/soc/mediatek/Kconfig | 44 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/mt8186/Makefile | 22 + sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 651 ++++ sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 106 + sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ .../soc/mediatek/mt8186/mt8186-afe-control.c | 261 ++ sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 244 ++ sound/soc/mediatek/mt8186/mt8186-afe-gpio.h | 19 + sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3005 +++++++++++++++++ sound/soc/mediatek/mt8186/mt8186-audsys-clk.c | 150 + 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 | 873 +++++ .../soc/mediatek/mt8186/mt8186-dai-hostless.c | 298 ++ .../soc/mediatek/mt8186/mt8186-dai-hw-gain.c | 236 ++ sound/soc/mediatek/mt8186/mt8186-dai-i2s.c | 1355 ++++++++ sound/soc/mediatek/mt8186/mt8186-dai-pcm.c | 423 +++ sound/soc/mediatek/mt8186/mt8186-dai-src.c | 695 ++++ sound/soc/mediatek/mt8186/mt8186-dai-tdm.c | 695 ++++ .../mediatek/mt8186/mt8186-interconnection.h | 69 + .../soc/mediatek/mt8186/mt8186-misc-control.c | 294 ++ .../mediatek/mt8186/mt8186-mt6366-common.c | 59 + .../mediatek/mt8186/mt8186-mt6366-common.h | 17 + .../mt8186/mt8186-mt6366-da7219-max98357.c | 1003 ++++++ .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 979 ++++++ sound/soc/mediatek/mt8186/mt8186-reg.h | 2913 ++++++++++++++++ 32 files changed, 15028 insertions(+), 1 deletion(-) 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-rt5682s.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-common.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-common.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-reg.h