| Message ID | 20220625190852.29130-1-jiaxin.yu@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8186 SoC | expand |
Le 25/06/2022 à 21:08, Jiaxin Yu a écrit : > Add mt8186 platform and affiliated driver. > > Signed-off-by: Jiaxin Yu <jiaxin.yu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> > --- > sound/soc/mediatek/Kconfig | 12 + > sound/soc/mediatek/Makefile | 1 + > sound/soc/mediatek/mt8186/Makefile | 19 + > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > .../soc/mediatek/mt8186/mt8186-afe-control.c | 255 ++ > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3011 +++++++++++++++++ > 6 files changed, 3533 insertions(+) > create mode 100644 sound/soc/mediatek/mt8186/Makefile > 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-pcm.c [...] > + MT8186_DAI_HOSTLESS_SRC_AAUDIO, > + MT8186_DAI_HOSTLESS_SRC_1, /* just an exmpale */ example? > + MT8186_DAI_HOSTLESS_SRC_BARGEIN, > + MT8186_DAI_HOSTLESS_UL1, [...] > +#define MTK_SPK_I2S_0_STR "MTK_SPK_I2S_0" > +#define MTK_SPK_I2S_1_STR "MTK_SPK_I2S_1" > +#define MTK_SPK_I2S_2_STR "MTK_SPK_I2S_2" > +#define MTK_SPK_I2S_3_STR "MTK_SPK_I2S_3" Out of curiosity, why no 4? Or, if related to mtk_spk_i2s_type below, why 6, 7, 8 and 9? > +#define MTK_SPK_I2S_5_STR "MTK_SPK_I2S_5" > +#define MTK_SPK_I2S_6_STR "MTK_SPK_I2S_6" > +#define MTK_SPK_I2S_7_STR "MTK_SPK_I2S_7" > +#define MTK_SPK_I2S_8_STR "MTK_SPK_I2S_8" > +#define MTK_SPK_I2S_9_STR "MTK_SPK_I2S_9" > + [...] > + > +enum mtk_spk_i2s_type { > + MTK_SPK_I2S_TYPE_INVALID = -1, > + MTK_SPK_I2S_0, > + MTK_SPK_I2S_1, > + MTK_SPK_I2S_2, > + MTK_SPK_I2S_3, > + MTK_SPK_I2S_5, > + MTK_SPK_I2S_TYPE_NUM > +}; [...] > +static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe; > + struct mt8186_afe_private *afe_priv; > + struct resource *res; > + struct reset_control *rstc; > + struct device *dev = &pdev->dev; > + int i, ret, irq_id; > + > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34)); > + if (ret) > + return ret; > + > + afe = devm_kzalloc(dev, sizeof(*afe), GFP_KERNEL); > + if (!afe) > + return -ENOMEM; > + platform_set_drvdata(pdev, afe); > + > + afe->platform_priv = devm_kzalloc(dev, sizeof(*afe_priv), GFP_KERNEL); > + if (!afe->platform_priv) > + return -ENOMEM; > + > + afe_priv = afe->platform_priv; > + afe->dev = &pdev->dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(afe->base_addr)) > + return PTR_ERR(afe->base_addr); > + > + /* init audio related clock */ > + ret = mt8186_init_clock(afe); > + if (ret) { > + dev_err(dev, "init clock error, ret %d\n", ret); > + return ret; > + } There is a mt8186_deinit_clock() call in the remove function. Should this also be called in the error handling path below? Or should a devm_add_action_or_reset() be used to ease error handling? > + > + /* init memif */ > + afe->memif_32bit_supported = 0; > + afe->memif_size = MT8186_MEMIF_NUM; > + afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe->memif), > + GFP_KERNEL); > + Nit: no need for an empty line here. > + if (!afe->memif) > + return -ENOMEM; > + [...] > + > + return 0; > + > +err_pm_disable: > + pm_runtime_put_noidle(dev); > + pm_runtime_set_suspended(dev); > + > + return ret; > +} > + > +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev) > +{ > + struct mtk_base_afe *afe = platform_get_drvdata(pdev); > + > + if (!pm_runtime_status_suspended(&pdev->dev)) > + mt8186_afe_runtime_suspend(&pdev->dev); Out of curiosity, is it normal to have some pm_runtime related code here that does not look the same as the one in the error handling of the probe? (I don't know much about pm, but usually, .remove() functions and error handling in the probe look quite close) > + > + mt8186_deinit_clock(afe); > + > + return 0; > +} > + [...]
On Sun, 2022-06-26 at 10:33 +0200, Christophe JAILLET wrote: > Le 25/06/2022 à 21:08, Jiaxin Yu a écrit : > > Add mt8186 platform and affiliated driver. > > > > Signed-off-by: Jiaxin Yu < > > jiaxin.yu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> > > --- > > sound/soc/mediatek/Kconfig | 12 + > > sound/soc/mediatek/Makefile | 1 + > > sound/soc/mediatek/mt8186/Makefile | 19 + > > sound/soc/mediatek/mt8186/mt8186-afe-common.h | 235 ++ > > .../soc/mediatek/mt8186/mt8186-afe-control.c | 255 ++ > > sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3011 > > +++++++++++++++++ > > 6 files changed, 3533 insertions(+) > > create mode 100644 sound/soc/mediatek/mt8186/Makefile > > 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-pcm.c > > [...] > > > + MT8186_DAI_HOSTLESS_SRC_AAUDIO, > > + MT8186_DAI_HOSTLESS_SRC_1, /* just an exmpale */ > > example? Yes, it's a typo. In fact, I should remove this description. > > > + MT8186_DAI_HOSTLESS_SRC_BARGEIN, > > + MT8186_DAI_HOSTLESS_UL1, > > [...] > > > +#define MTK_SPK_I2S_0_STR "MTK_SPK_I2S_0" > > +#define MTK_SPK_I2S_1_STR "MTK_SPK_I2S_1" > > +#define MTK_SPK_I2S_2_STR "MTK_SPK_I2S_2" > > +#define MTK_SPK_I2S_3_STR "MTK_SPK_I2S_3" > > Out of curiosity, why no 4? > Or, if related to mtk_spk_i2s_type below, why 6, 7, 8 and 9? Because the MT8186 don't have I2S4 hardware, so we continued to use the hardware name and skipped the number 4. However, the MT8186 does not have I2S 5/6/7/8/9, I will remove these. > > > +#define MTK_SPK_I2S_5_STR "MTK_SPK_I2S_5" > > +#define MTK_SPK_I2S_6_STR "MTK_SPK_I2S_6" > > +#define MTK_SPK_I2S_7_STR "MTK_SPK_I2S_7" > > +#define MTK_SPK_I2S_8_STR "MTK_SPK_I2S_8" > > +#define MTK_SPK_I2S_9_STR "MTK_SPK_I2S_9" > > + > > [...] > > > + > > +enum mtk_spk_i2s_type { > > + MTK_SPK_I2S_TYPE_INVALID = -1, > > + MTK_SPK_I2S_0, > > + MTK_SPK_I2S_1, > > + MTK_SPK_I2S_2, > > + MTK_SPK_I2S_3, > > + MTK_SPK_I2S_5, > > + MTK_SPK_I2S_TYPE_NUM > > +}; > > [...] > > > +static int mt8186_afe_pcm_dev_probe(struct platform_device *pdev) > > +{ > > + struct mtk_base_afe *afe; > > + struct mt8186_afe_private *afe_priv; > > + struct resource *res; > > + struct reset_control *rstc; > > + struct device *dev = &pdev->dev; > > + int i, ret, irq_id; > > + > > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34)); > > + if (ret) > > + return ret; > > + > > + afe = devm_kzalloc(dev, sizeof(*afe), GFP_KERNEL); > > + if (!afe) > > + return -ENOMEM; > > + platform_set_drvdata(pdev, afe); > > + > > + afe->platform_priv = devm_kzalloc(dev, sizeof(*afe_priv), > > GFP_KERNEL); > > + if (!afe->platform_priv) > > + return -ENOMEM; > > + > > + afe_priv = afe->platform_priv; > > + afe->dev = &pdev->dev; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + afe->base_addr = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(afe->base_addr)) > > + return PTR_ERR(afe->base_addr); > > + > > + /* init audio related clock */ > > + ret = mt8186_init_clock(afe); > > + if (ret) { > > + dev_err(dev, "init clock error, ret %d\n", ret); > > + return ret; > > + } > > There is a mt8186_deinit_clock() call in the remove function. > Should this also be called in the error handling path below? > Or should a devm_add_action_or_reset() be used to ease error > handling? > Yes, mt8186_deinit_clock() is required in the error handling path. I prefer to use devm_add_action_or_reset(), thank you for your comment. > > + > > + /* init memif */ > > + afe->memif_32bit_supported = 0; > > + afe->memif_size = MT8186_MEMIF_NUM; > > + afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe- > > >memif), > > + GFP_KERNEL); > > + > > Nit: no need for an empty line here. > Got it. > > + if (!afe->memif) > > + return -ENOMEM; > > + > > [...] > > > + > > + return 0; > > + > > +err_pm_disable: > > + pm_runtime_put_noidle(dev); > > + pm_runtime_set_suspended(dev); > > + > > + return ret; > > +} > > + > > +static int mt8186_afe_pcm_dev_remove(struct platform_device *pdev) > > +{ > > + struct mtk_base_afe *afe = platform_get_drvdata(pdev); > > + > > + if (!pm_runtime_status_suspended(&pdev->dev)) > > + mt8186_afe_runtime_suspend(&pdev->dev); > > Out of curiosity, is it normal to have some pm_runtime related code > here > that does not look the same as the one in the error handling of the > probe? > (I don't know much about pm, but usually, .remove() functions and > error > handling in the probe look quite close) > As I understand it, the .probe() function is like below: 1. allocate resources and initialize them 2. devm_pm_runtime_enable(dev); 3. pm_runtime_resume_and_get(dev); /* execute the runtime_resume callback */ 4. do regmap init that must power on the regulator and clock 5. pm_runtime_put_sync(dev); /* execute the runtime_suspend callback */ So the error handling is to process the errors from step 4/5. If the .probe() executes normally, the dev is in runtime suspend status. And we used devm_pm_runtime_enbale(), so maybe we don't need to do anything to the pm_runtime_xxx in the .remove() callback? Does this condition never established? if (!pm_runtime_status_suspended(&pdev->dev)) mt8186_afe_runtime_suspend(&pdev->dev); > > + > > + mt8186_deinit_clock(afe); > > + > > + return 0; > > +} > > + > > [...]
This series of patches adds support for Mediatek AFE of MT8186 Soc. Patches are based on broonie tree "for-next" branch. Changes since v7: - Optimize the code logic, such as the return value and position of a function. - Use devm_pm_runtime_enable() instead of pm_runtime_enable(). - Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(). Changes since v6: - The Makefile and Kconfig updates are moved into the driver patches so that can keep independence of each patch. Changes since v5: - fix build error about function snd_soc_card_jack_new that modified by: Link: https://lore.kernel.org/r/20220408041114.6024-1-akihiko.odaki@gmail.com - some have been accepted, details are in the link below: Link: https://lore.kernel.org/all/165459931885.399031.2621579592368573898.b4-ty@kernel.org/ Changes since v4: - [v5 07/20] - remove unsusd controls - [v5 09/20] - correct indent error - [v5 10/20] - [v5 13/20] - [v5 14/20] - fix the return value if the value is different from the previous value in mixer controls - [v5 17/20] - [v5 19/20] - correct the compatible name with '_' instead of '-' - [v5 18/20] - [v5 20/20] - correct the yaml after 'pip3 install dtschema --upgrade' 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 codec - 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 (8): dt-bindings: mediatek: mt6358: add new compatible for using mt6366 ASoC: mediatek: mt8186: add platform driver ASoC: mediatek: mt8186: add mt8186-mt6366 common 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 | 75 + .../sound/mt8186-mt6366-rt1019-rt5682s.yaml | 75 + 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 | 255 ++ sound/soc/mediatek/mt8186/mt8186-afe-pcm.c | 3011 +++++++++++++++++ .../mediatek/mt8186/mt8186-mt6366-common.c | 57 + .../mediatek/mt8186/mt8186-mt6366-common.h | 17 + .../mt8186/mt8186-mt6366-da7219-max98357.c | 1002 ++++++ .../mt8186/mt8186-mt6366-rt1019-rt5682s.c | 978 ++++++ 14 files changed, 5950 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-common.h create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.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