Re: [PATCH v8 2/8] ASoC: mediatek: mt8186: add platform driver

[Jiaxin Yu]

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