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;
+}
[...]