| Message ID | 1603526339-15005-1-git-send-email-jiaxin.yu@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Mediatek: Add support for MT8192 SoC | expand |
On Sat, Oct 24, 2020 at 03:58:53PM +0800, Jiaxin Yu wrote: > +static const struct soc_enum mt8192_i2s_enum[] = { > + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_i2s_hd_str), > + mt8192_i2s_hd_str), > +}; Why is this declared as a single element array? It just makes all the usages look odd for no obvious gain. > +static int mtk_i2s_en_event(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, > + int event) > + dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n", > + __func__, w->name, event); This should be dev_dbg() at most, _info() will be too noisy in the logs. Same for a lot of functions, including the stream callbacks. > +static int mtk_i2s_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 should just be removed entirely, there's trace in the core if you need logging in production systems - the tracepoints in particular are good for just leaving on all the time without adding overhead. > + return (i2s_need_apll == cur_apll) ? 1 : 0; Please write normal conditional statements to improve legiblity. > + if (rate == 44100) > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000); > + else if (rate == 32000) > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000); > + else > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000); This would be better written as a switch statement. > + /* Calibration setting */ > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000); > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000); > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00); > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4); > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986); Are you sure this isn't system dependant?
On Sat, Oct 24, 2020 at 03:58:50PM +0800, Jiaxin Yu wrote: > This series of patches adds support for Mediatek AFE for MT8192 SoC. At the same > time, the calibration function of MT6359 is completed with real machine driver. > The patch is based on broonie tree "for-next" branch. I had some small comments but they were all pretty minor - overall this is basically fine once those are addressed.
On Fri, 2020-10-30 at 14:37 +0000, Mark Brown wrote: > On Sat, Oct 24, 2020 at 03:58:53PM +0800, Jiaxin Yu wrote: > > > +static const struct soc_enum mt8192_i2s_enum[] = { > > + SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_i2s_hd_str), > > + mt8192_i2s_hd_str), > > +}; > > Why is this declared as a single element array? It just makes all the > usages look odd for no obvious gain. > > > +static int mtk_i2s_en_event(struct snd_soc_dapm_widget *w, > > + struct snd_kcontrol *kcontrol, > > + int event) > > > + dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n", > > + __func__, w->name, event); > > This should be dev_dbg() at most, _info() will be too noisy in the logs. > Same for a lot of functions, including the stream callbacks. > > > +static int mtk_i2s_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 should just be removed entirely, there's trace in the core if you > need logging in production systems - the tracepoints in particular are > good for just leaving on all the time without adding overhead. > > > + return (i2s_need_apll == cur_apll) ? 1 : 0; > > Please write normal conditional statements to improve legiblity. > > > + if (rate == 44100) > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000); > > + else if (rate == 32000) > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000); > > + else > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000); > > This would be better written as a switch statement. > > > + /* Calibration setting */ > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000); > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000); > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00); > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4); > > + regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986); > > Are you sure this isn't system dependant? Hi Mark, Yes, this is a system independent setting. And I fixed other comments you pointed out then send "PATCH v4".
This series of patches adds support for Mediatek AFE for MT8192 SoC. At the same time, the calibration function of MT6359 is completed with real machine driver. The patch is based on broonie tree "for-next" branch. Change since v2: - split the dai driver files as a seperate patch - fix dt-bindings to GPL-2.0-only License - remove unnecessary preperty descriptions such as 'maxItems' Change since v1: - fixed some typos related to dt-bindings in [v1,3/5] and [v1,5/5] - add vendor prefix to the properties, such as "mediatek,apmixedsys" - add a dependency description to indicate the required header files Jiaxin Yu (9): ASoC: mediatek: mt6359: add the calibration functions ASoC: mediatek: mt8192: add platform driver ASoC: mediatek: mt8192: support i2s in platform driver ASoC: mediatek: mt8192: support add in platform driver ASoC: mediatek: mt8192: support pcm in platform driver ASoC: mediatek: mt8192: support tdm in platform driver dt-bindings: mediatek: mt8192: add audio afe document ASoC: mediatek: mt8192: add machine driver with mt6359, rt1015 and rt5682 dt-bindings: mediatek: mt8192: add mt8192-mt6358-rt1015-rt5682 document .../bindings/sound/mt8192-afe-pcm.yaml | 100 + .../sound/mt8192-mt6359-rt1015-rt5682.yaml | 42 + sound/soc/codecs/mt6359.c | 110 + sound/soc/codecs/mt6359.h | 7 + sound/soc/mediatek/Kconfig | 23 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/common/mtk-afe-fe-dai.c | 13 +- sound/soc/mediatek/common/mtk-base-afe.h | 1 + sound/soc/mediatek/mt8192/Makefile | 16 + sound/soc/mediatek/mt8192/mt8192-afe-clk.c | 669 ++++ sound/soc/mediatek/mt8192/mt8192-afe-clk.h | 244 ++ sound/soc/mediatek/mt8192/mt8192-afe-common.h | 170 + .../soc/mediatek/mt8192/mt8192-afe-control.c | 163 + sound/soc/mediatek/mt8192/mt8192-afe-gpio.c | 306 ++ sound/soc/mediatek/mt8192/mt8192-afe-gpio.h | 19 + sound/soc/mediatek/mt8192/mt8192-afe-pcm.c | 2389 +++++++++++++ sound/soc/mediatek/mt8192/mt8192-dai-adda.c | 1489 ++++++++ sound/soc/mediatek/mt8192/mt8192-dai-i2s.c | 2139 +++++++++++ sound/soc/mediatek/mt8192/mt8192-dai-pcm.c | 409 +++ sound/soc/mediatek/mt8192/mt8192-dai-tdm.c | 778 ++++ .../mediatek/mt8192/mt8192-interconnection.h | 65 + .../mt8192/mt8192-mt6359-rt1015-rt5682.c | 1058 ++++++ sound/soc/mediatek/mt8192/mt8192-reg.h | 3131 +++++++++++++++++ 23 files changed, 13338 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml create mode 100644 Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml create mode 100644 sound/soc/mediatek/mt8192/Makefile create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-clk.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-clk.h create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-common.h create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-control.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-gpio.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-gpio.h create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-pcm.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-adda.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-i2s.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-pcm.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-tdm.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-interconnection.h create mode 100644 sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c create mode 100644 sound/soc/mediatek/mt8192/mt8192-reg.h