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
