Message-ID: <92ecfdc3-3feb-41ec-8cc9-0379e3c951bb@kernel.org>
Date: Fri, 7 Mar 2025 16:32:39 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Darren.Ye" <darren.ye@...iatek.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 11/14] ASoC: mediatek: mt8196: add platform driver

On 07/03/2025 13:47, Darren.Ye wrote:
> +
> +static int mt8196_afe_runtime_suspend(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +	unsigned int value = 0;
> +	unsigned int tmp_reg = 0;
> +	int ret = 0, i;
> +
> +	dev_dbg(afe->dev, "%s() ready to stop\n", __func__);

Core already provides this. Drop.

> +
> +	if (!afe->regmap) {
> +		dev_info(afe->dev, "%s() skip regmap\n", __func__);
> +		goto skip_regmap;
> +	}
> +
> +	/* Add to be off for free run*/
> +	/* disable AFE */
> +	regmap_update_bits(afe->regmap, AUDIO_ENGEN_CON0, 0x1, 0x0);
> +
> +	ret = regmap_read_poll_timeout(afe->regmap,
> +				       AUDIO_ENGEN_CON0_MON,
> +				       value,
> +				       (value & AUDIO_ENGEN_MON_SFT) == 0,
> +				       20,
> +				       1 * 1000 * 1000);
> +	dev_dbg(afe->dev, "%s() read_poll ret %d\n", __func__, ret);
> +	if (ret)
> +		dev_info(afe->dev, "%s(), ret %d\n", __func__, ret);
> +
> +	/* make sure all irq status are cleared */
> +	for (i = 0; i < MT8196_IRQ_NUM; ++i) {
> +		regmap_read(afe->regmap, irq_data[i].irq_clr_reg, &tmp_reg);
> +		regmap_update_bits(afe->regmap, irq_data[i].irq_clr_reg,
> +				   AFE_IRQ_CLR_CFG_MASK_SFT | AFE_IRQ_MISS_FLAG_CLR_CFG_MASK_SFT,
> +				   tmp_reg ^ (AFE_IRQ_CLR_CFG_MASK_SFT |
> +				   AFE_IRQ_MISS_FLAG_CLR_CFG_MASK_SFT));
> +	}
> +
> +	/* reset sgen */
> +	regmap_write(afe->regmap, AFE_SINEGEN_CON0, 0x0);
> +	regmap_update_bits(afe->regmap, AFE_SINEGEN_CON1,
> +			   SINE_DOMAIN_MASK_SFT,
> +			   0x0 << SINE_DOMAIN_SFT);
> +	regmap_update_bits(afe->regmap, AFE_SINEGEN_CON1,
> +			   SINE_MODE_MASK_SFT,
> +			   0x0 << SINE_MODE_SFT);
> +	regmap_update_bits(afe->regmap, AFE_SINEGEN_CON1,
> +			   INNER_LOOP_BACKI_SEL_MASK_SFT,
> +			   0x0 << INNER_LOOP_BACKI_SEL_SFT);
> +	regmap_update_bits(afe->regmap, AFE_SINEGEN_CON1,
> +			   INNER_LOOP_BACK_MODE_MASK_SFT,
> +			   0xff << INNER_LOOP_BACK_MODE_SFT);
> +
> +	regmap_write(afe->regmap, AUDIO_TOP_CON4, 0x3fff);
> +
> +	/* reset audio 26M request */
> +	regmap_update_bits(afe->regmap,
> +			   AFE_SPM_CONTROL_REQ, 0x1, 0x0);
> +
> +	/* cache only */
> +	regcache_cache_only(afe->regmap, true);
> +	regcache_mark_dirty(afe->regmap);
> +
> +skip_regmap:
> +	mt8196_afe_disable_clock(afe);
> +	return 0;
> +}
> +
> +static int mt8196_afe_runtime_resume(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	ret = mt8196_afe_enable_clock(afe);
> +	dev_dbg(afe->dev, "%s(), enable_clock ret %d\n", __func__, ret);

So you are debugging every call. I think you do not trust your code, right?

> +
> +	if (ret)
> +		return ret;
> +
> +	if (!afe->regmap) {
> +		dev_info(afe->dev, "%s() skip regmap\n", __func__);

Why dev_info? How is this condition even possible?


> +		goto skip_regmap;
> +	}
> +	regcache_cache_only(afe->regmap, false);
> +	regcache_sync(afe->regmap);
> +
> +	/* set audio 26M request */
> +	regmap_update_bits(afe->regmap, AFE_SPM_CONTROL_REQ, 0x1, 0x1);
> +
> +	/* IPM2.0: Clear AUDIO_TOP_CON4 for enabling AP side module clk */
> +	regmap_write(afe->regmap, AUDIO_TOP_CON4, 0x0);
> +
> +	/* Add to be on for free run */
> +	regmap_write(afe->regmap, AUDIO_TOP_CON0, 0x0);
> +	regmap_write(afe->regmap, AUDIO_TOP_CON1, 0x0);
> +	regmap_write(afe->regmap, AUDIO_TOP_CON2, 0x0);
> +
> +	/* Can't set AUDIO_TOP_CON3 to be 0x0, it will hang in FPGA env */
> +	regmap_write(afe->regmap, AUDIO_TOP_CON3, 0x0);
> +
> +	regmap_update_bits(afe->regmap, AFE_CBIP_CFG0, 0x1, 0x1);
> +
> +	/* force cpu use 8_24 format when writing 32bit data */
> +	regmap_update_bits(afe->regmap, AFE_MEMIF_CON0,
> +			   CPU_HD_ALIGN_MASK_SFT, 0 << CPU_HD_ALIGN_SFT);
> +
> +	/* enable AFE */
> +	regmap_update_bits(afe->regmap, AUDIO_ENGEN_CON0, 0x1, 0x1);
> +
> +skip_regmap:
> +	return 0;


...

> +
> +typedef int (*dai_register_cb)(struct mtk_base_afe *);
> +static const dai_register_cb dai_register_cbs[] = {
> +	mt8196_dai_adda_register,
> +	mt8196_dai_i2s_register,
> +	mt8196_dai_tdm_register,
> +	mt8196_dai_memif_register,
> +};
> +
> +static int mt8196_afe_pcm_dev_probe(struct platform_device *pdev)
> +{
> +	int ret, i;
> +	unsigned int tmp_reg = 0;
> +	int irq_id;
> +	struct mtk_base_afe *afe;
> +	struct mt8196_afe_private *afe_priv;
> +	struct resource *res;
> +	struct device *dev;
> +
> +	pr_info("+%s()\n", __func__);

No, drop.

> +
> +	ret = of_reserved_mem_device_init(&pdev->dev);
> +	if (ret)
> +		dev_dbg(&pdev->dev, "failed to assign memory region: %d\n", ret);
> +
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(34));
> +	if (ret)
> +		return ret;
> +
> +	afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL);
> +	if (!afe)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, afe);
> +
> +	afe->platform_priv = devm_kzalloc(&pdev->dev, sizeof(*afe_priv),
> +					  GFP_KERNEL);
> +	if (!afe->platform_priv)
> +		return -ENOMEM;
> +
> +	afe_priv = afe->platform_priv;
> +
> +	afe->dev = &pdev->dev;
> +	dev = afe->dev;
> +
> +	/* init audio related clock */
> +	ret = mt8196_init_clock(afe);
> +	if (ret) {
> +		dev_info(dev, "init clock error: %d\n", ret);

How are you handling deferred probe? Why aren't you using dev_err_probe?

But more important - why do you keep printing errors multiple times,
including ENOMEM?

This is really poor coding style.

> +		return ret;
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	if (!pm_runtime_enabled(&pdev->dev))
> +		goto err_pm_disable;
> +
> +	/* Audio device is part of genpd.
> +	 * Set audio as syscore device to prevent
> +	 * genpd automatically power off audio
> +	 * device when suspend
> +	 */
> +	dev_pm_syscore_device(&pdev->dev, true);
> +
> +	/* regmap init */
> +	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);
> +
> +	/* enable clock for regcache get default value from hw */
> +	pm_runtime_get_sync(&pdev->dev);
> +
> +	afe->regmap = devm_regmap_init_mmio(&pdev->dev, afe->base_addr,
> +					    &mt8196_afe_regmap_config);
> +	if (IS_ERR(afe->regmap))
> +		return PTR_ERR(afe->regmap);
> +
> +	/* IPM2.0 clock flow, need debug */
> +
> +	regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &tmp_reg);
> +	regmap_write(afe->regmap, AFE_IRQ_MCU_EN, 0xffffffff);
> +	regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &tmp_reg);
> +	/* IPM2.0 clock flow, need debug */
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +
> +	regcache_cache_only(afe->regmap, true);
> +	regcache_mark_dirty(afe->regmap);
> +
> +	/* init gpio */
> +	ret = mt8196_afe_gpio_init(afe);
> +	if (ret)
> +		dev_info(dev, "init gpio error\n");

Do not print errors twice.

> +
> +	/* init memif */
> +	/* IPM2.0 no need banding */
> +	afe->memif_32bit_supported = 1;
> +	afe->memif_size = MT8196_MEMIF_NUM;
> +	afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe->memif),
> +				  GFP_KERNEL);
> +
> +	if (!afe->memif)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < afe->memif_size; i++) {
> +		afe->memif[i].data = &memif_data[i];
> +		afe->memif[i].irq_usage = memif_irq_usage[i];
> +		afe->memif[i].const_irq = 1;
> +	}
> +
> +	mutex_init(&afe->irq_alloc_lock);       /* needed when dynamic irq */
> +
> +	/* init irq */
> +	afe->irqs_size = MT8196_IRQ_NUM;
> +	afe->irqs = devm_kcalloc(dev, afe->irqs_size, sizeof(*afe->irqs),
> +				 GFP_KERNEL);
> +

Drop blank line

> +	if (!afe->irqs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < afe->irqs_size; i++)
> +		afe->irqs[i].irq_data = &irq_data[i];
> +
> +	/* request irq */
> +	irq_id = platform_get_irq(pdev, 0);
> +	if (irq_id <= 0) {

Please read documentation of platform_get_irq().

> +		dev_info(dev, "%pOFn no irq found\n", dev->of_node);

Why dev_info?

> +		return irq_id < 0 ? irq_id : -ENXIO;
> +	}
> +	ret = devm_request_irq(dev, irq_id, mt8196_afe_irq_handler,
> +			       IRQF_TRIGGER_NONE,
> +			       "Afe_ISR_Handle", (void *)afe);
> +	if (ret) {
> +		dev_info(dev, "could not request_irq for Afe_ISR_Handle\n");
> +		return ret;
> +	}
> +	ret = enable_irq_wake(irq_id);
> +	if (ret < 0)
> +		dev_info(dev, "enable_irq_wake %d err: %d\n", irq_id, ret);
> +
> +	/* init sub_dais */
> +	INIT_LIST_HEAD(&afe->sub_dais);
> +
> +	for (i = 0; i < ARRAY_SIZE(dai_register_cbs); i++) {
> +		ret = dai_register_cbs[i](afe);
> +		if (ret) {
> +			dev_info(afe->dev, "dai register i %d fail, ret %d\n",
> +				 i, ret);
> +			goto err_pm_disable;
> +		}
> +	}
> +
> +	/* init dai_driver and component_driver */
> +	ret = mtk_afe_combine_sub_dai(afe);
> +	if (ret) {
> +		dev_info(afe->dev, "mtk_afe_combine_sub_dai fail, ret %d\n",
> +			 ret);
> +		goto err_pm_disable;
> +	}
> +
> +	/* others */
> +	afe->mtk_afe_hardware = &mt8196_afe_hardware;
> +	afe->memif_fs = mt8196_memif_fs;
> +	afe->irq_fs = mt8196_irq_fs;
> +	afe->get_dai_fs = mt8196_get_dai_fs;
> +	afe->get_memif_pbuf_size = mt8196_get_memif_pbuf_size;
> +
> +	afe->runtime_resume = mt8196_afe_runtime_resume;
> +	afe->runtime_suspend = mt8196_afe_runtime_suspend;
> +
> +	afe->request_dram_resource = mt8196_afe_dram_request;
> +	afe->release_dram_resource = mt8196_afe_dram_release;
> +
> +	/* register component */
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &mt8196_afe_component,
> +					      afe->dai_drivers,
> +					      afe->num_dai_drivers);
> +	if (ret) {
> +		dev_info(dev, "afe component err: %d\n", ret);

Why not dev_err? You have this unusual pattern all over your code.

> +		goto err_pm_disable;
> +	}
> +	return 0;
> +
> +err_pm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;
> +}



Best regards,
Krzysztof

Powered by blists - more mailing lists