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