| Message ID | 20250307124841.23777-1-darren.ye@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8196 SoC | expand |
On 07/03/2025 13:47, Darren.Ye wrote: > + > +static int etdm_parse_dt(struct mtk_base_afe *afe) > +{ > + int ret; > + struct mt8196_afe_private *afe_priv = afe->platform_priv; > + struct mtk_afe_i2s_priv *i2sin4_priv = afe_priv->dai_priv[MT8196_DAI_I2S_IN4]; > + struct mtk_afe_i2s_priv *i2sout4_priv = afe_priv->dai_priv[MT8196_DAI_I2S_OUT4]; > + unsigned int ch_num_in; > + unsigned int ch_num_out; > + unsigned int sync_in; > + unsigned int sync_out; > + unsigned int ip_mode; > + > + /* get etdm ch */ > + ret = of_property_read_u32(afe->dev->of_node, "mediatek,etdm-out-ch", &ch_num_out); > + if (ret) { > + dev_info(afe->dev, "%s() failed to read mediatek,etdm-out-ch\n", __func__); > + return -EINVAL; > + } > + i2sout4_priv->ch_num = ch_num_out; > + dev_dbg(afe->dev, "%s() mediatek,etdm-out-ch: %d\n", __func__, ch_num_out); > + > + ret = of_property_read_u32(afe->dev->of_node, "mediatek,etdm-in-ch", &ch_num_in); > + if (ret) { > + dev_info(afe->dev, "%s() failed to read mediatek,etdm-in-ch\n", __func__); > + return -EINVAL; > + } > + i2sin4_priv->ch_num = ch_num_in; > + dev_dbg(afe->dev, "%s() mediatek,etdm-in-ch: %d\n", __func__, ch_num_in); > + > + /* get etdm sync */ > + ret = of_property_read_u32(afe->dev->of_node, "mediatek,etdm-out-sync", &sync_out); > + if (ret) { > + dev_info(afe->dev, "%s() failed to read mediatek,etdm-out-sync\n", __func__); > + return -EINVAL; > + } > + i2sout4_priv->sync = sync_out; > + dev_dbg(afe->dev, "%s() mediatek,etdm-out-sync: %d\n", __func__, sync_out); > + > + ret = of_property_read_u32(afe->dev->of_node, "mediatek,etdm-in-sync", &sync_in); > + if (ret) { > + dev_info(afe->dev, "%s() failed to read mediatek,etdm-in-sync\n", __func__); > + return -EINVAL; > + } > + i2sin4_priv->sync = sync_in; > + dev_dbg(afe->dev, "%s() mediatek,etdm-in-sync: %d\n", __func__, sync_in); > + > + /* get etdm ip mode */ > + ret = of_property_read_u32(afe->dev->of_node, "mediatek,etdm-ip-mode", &ip_mode); > + if (ret) { > + dev_info(afe->dev, "%s() failed to read mediatek,etdm-ip-mode\n", __func__); > + return -EINVAL; > + } > + i2sin4_priv->ip_mode = ip_mode; > + dev_dbg(afe->dev, "%s() mediatek,etdm-ip-mode: %d\n", __func__, ip_mode); > + > + return 0; > +} > + > +static int mt8196_dai_i2s_get_share(struct mtk_base_afe *afe) > +{ > + struct mt8196_afe_private *afe_priv = afe->platform_priv; > + const struct device_node *of_node = afe->dev->of_node; > + const char *of_str; > + const char *property_name; > + struct mtk_afe_i2s_priv *i2s_priv; > + int i; > + > + for (i = 0; i < DAI_I2S_NUM; i++) { > + i2s_priv = afe_priv->dai_priv[mt8196_i2s_priv[i].id]; > + property_name = mt8196_i2s_priv[i].share_property_name; > + if (of_property_read_string(of_node, property_name, &of_str)) > + continue; > + i2s_priv->share_i2s_id = get_i2s_id_by_name(afe, of_str); > + } > + return 0; > +} > + > +static int mt8196_dai_i2s_set_priv(struct mtk_base_afe *afe) > +{ > + int i; > + int ret; > + > + for (i = 0; i < DAI_I2S_NUM; i++) { > + ret = mt8196_dai_set_priv(afe, mt8196_i2s_priv[i].id, > + sizeof(struct mtk_afe_i2s_priv), > + &mt8196_i2s_priv[i]); > + if (ret) > + return ret; > + } > + return 0; > +} > + > +int mt8196_dai_i2s_register(struct mtk_base_afe *afe) > +{ > + struct mtk_base_afe_dai *dai; > + int ret; > + > + dev_dbg(afe->dev, "%s() successfully start\n", __func__); Well, no. Tracing is for debugging entry/exit of functions. Say something useful or just drop such entry/exist success messages. > + > + dai = devm_kzalloc(afe->dev, sizeof(*dai), GFP_KERNEL); > + if (!dai) > + return -ENOMEM; > + > + list_add(&dai->list, &afe->sub_dais); > + > + dai->dai_drivers = mtk_dai_i2s_driver; > + dai->num_dai_drivers = ARRAY_SIZE(mtk_dai_i2s_driver); > + > + dai->controls = mtk_dai_i2s_controls; > + dai->num_controls = ARRAY_SIZE(mtk_dai_i2s_controls); > + dai->dapm_widgets = mtk_dai_i2s_widgets; > + dai->num_dapm_widgets = ARRAY_SIZE(mtk_dai_i2s_widgets); > + dai->dapm_routes = mtk_dai_i2s_routes; > + dai->num_dapm_routes = ARRAY_SIZE(mtk_dai_i2s_routes); > + > + /* set all dai i2s private data */ > + ret = mt8196_dai_i2s_set_priv(afe); > + if (ret) > + return ret; > + > + /* parse share i2s */ > + ret = mt8196_dai_i2s_get_share(afe); > + if (ret) > + return ret; > + > + /* for customer to change ch_num & sync & ipmode from dts */ > + ret = etdm_parse_dt(afe); > + if (ret) { > + dev_info(afe->dev, "%s() fail to parse dts: %d\n", __func__, ret); Why do you print errors twice? > + return ret; > + } > + > + return 0; > +} Best regards, Krzysztof
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
From: Darren Ye <darren.ye@mediatek.com> This series of patches adds support for Mediatek AFE of MT8196 SoC. Patches are based on broonie tree "for-next" branch. --- This series patches dependent on: [1] https://lore.kernel.org/all/20250307032942.10447-1-guangjie.song@mediatek.com/ Darren Ye (14): ASoC: mediatek: common: modify mtk afe common driver for mt8196 ASoC: mediatek: common: modify mtk afe platform driver for mt8196 ASoC: mediatek: mt8196: add common header ASoC: mediatek: mt8196: add common interface for mt8196 DAI driver ASoC: mediatek: mt8196: support audio clock control ASoC: mediatek: mt8196: support audio GPIO control ASoC: mediatek: mt8196: support ADDA in platform driver ASoC: mediatek: mt8196: support I2S in platform driver ASoC: mediatek: mt8196: support TDM in platform driver ASoC: mediatek: mt8196: support CM in platform driver ASoC: mediatek: mt8196: add platform driver dt-bindings: mediatek: mt8196: add audio AFE document ASoC: mediatek: mt8196: add machine driver with mt6681 dt-bindings: mediatek: mt8196: add mt8196-mt6681 document .../bindings/sound/mediatek,mt8196-afe.yaml | 259 + .../sound/mediatek,mt8196-mt6681.yaml | 122 + sound/soc/mediatek/Kconfig | 30 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/common/mtk-afe-fe-dai.c | 30 +- sound/soc/mediatek/common/mtk-afe-fe-dai.h | 6 + .../mediatek/common/mtk-afe-platform-driver.c | 63 +- .../mediatek/common/mtk-afe-platform-driver.h | 5 + sound/soc/mediatek/common/mtk-base-afe.h | 13 + sound/soc/mediatek/mt8196/Makefile | 19 + sound/soc/mediatek/mt8196/mt8196-afe-clk.c | 698 + sound/soc/mediatek/mt8196/mt8196-afe-clk.h | 313 + sound/soc/mediatek/mt8196/mt8196-afe-cm.c | 94 + sound/soc/mediatek/mt8196/mt8196-afe-cm.h | 23 + sound/soc/mediatek/mt8196/mt8196-afe-common.h | 290 + .../soc/mediatek/mt8196/mt8196-afe-control.c | 109 + sound/soc/mediatek/mt8196/mt8196-afe-gpio.c | 239 + sound/soc/mediatek/mt8196/mt8196-afe-gpio.h | 58 + sound/soc/mediatek/mt8196/mt8196-afe-pcm.c | 5134 +++++++ sound/soc/mediatek/mt8196/mt8196-dai-adda.c | 2207 +++ sound/soc/mediatek/mt8196/mt8196-dai-i2s.c | 4399 ++++++ sound/soc/mediatek/mt8196/mt8196-dai-tdm.c | 837 ++ .../mediatek/mt8196/mt8196-interconnection.h | 121 + sound/soc/mediatek/mt8196/mt8196-mt6681.c | 879 ++ sound/soc/mediatek/mt8196/mt8196-reg.h | 12133 ++++++++++++++++ 25 files changed, 28055 insertions(+), 27 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt8196-mt6681.yaml create mode 100644 sound/soc/mediatek/mt8196/Makefile create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-clk.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-clk.h create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-cm.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-cm.h create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-common.h create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-control.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-gpio.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-gpio.h create mode 100644 sound/soc/mediatek/mt8196/mt8196-afe-pcm.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-dai-adda.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-dai-i2s.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-dai-tdm.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-interconnection.h create mode 100644 sound/soc/mediatek/mt8196/mt8196-mt6681.c create mode 100644 sound/soc/mediatek/mt8196/mt8196-reg.h