Re: [PATCH 11/18] ASoC: mediatek: mt8365: Add platform driver

[Krzysztof Kozlowski]

On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add mt8365 platform driver.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@xxxxxxxxxxxx>

..

> +	memif->substream = substream;
> +
> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
> +		SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 16);
> +
> +	snd_soc_set_runtime_hwparams(substream, mtk_afe_hardware);
> +
> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +		SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		dev_err(afe->dev, "snd_pcm_hw_constraint_integer failed\n");
> +
> +	mt8365_afe_enable_main_clk(afe);
> +	return ret;
> +}
> +
> +static void mt8365_afe_fe_shutdown(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	int memif_num = snd_soc_rtd_to_cpu(rtd, 0)->id;
> +	struct mtk_base_afe_memif *memif = &afe->memif[memif_num];
> +
> +	dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name);

Drop trace-like prints. Tracing is for this. Before upstreaming driver
from vendor repository, such stuff should be cleaned.


> +
> +	memif->substream = NULL;
> +
> +	mt8365_afe_disable_main_clk(afe);
> +}
> +
> +static int mt8365_afe_fe_hw_params(struct snd_pcm_substream *substream,
> +	struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id;
> +	struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data;
> +	struct mtk_base_afe_memif *memif = &afe->memif[dai_id];
> +	struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id];
> +	const size_t request_size = params_buffer_bytes(params);
> +	unsigned int channels = params_channels(params);
> +	unsigned int rate = params_rate(params);
> +	int ret, fs = 0;
> +	unsigned int base_end_offset = 8;
> +
> +	dev_info(afe->dev,
> +		"%s %s period = %d rate = %d channels = %d\n",
> +		__func__, memif->data->name, params_period_size(params),
> +		rate, channels);
> +
> +	if (dai_id == MT8365_AFE_MEMIF_VUL2) {
> +		if (!ctrl_data->bypass_cm1)
> +			/* configure cm1 */
> +			mt8365_afe_configure_cm(afe,
> +				MT8365_CM1, channels, rate);
> +		else
> +			regmap_update_bits(afe->regmap, AFE_CM1_CON0,
> +				CM_AFE_CM1_VUL_SEL, CM_AFE_CM1_VUL_SEL);
> +	} else if (dai_id == MT8365_AFE_MEMIF_TDM_IN) {
> +		if (!ctrl_data->bypass_cm2)
> +			/* configure cm2 */
> +			mt8365_afe_configure_cm(afe,
> +				MT8365_CM2, channels, rate);
> +		else
> +			regmap_update_bits(afe->regmap, AFE_CM2_CON0,
> +				CM_AFE_CM2_TDM_SEL, ~CM_AFE_CM2_TDM_SEL);
> +
> +		base_end_offset = 4;
> +	}
> +
> +	if (request_size > fe_data->sram_size) {
> +		ret = snd_pcm_lib_malloc_pages(substream, request_size);
> +		if (ret < 0) {
> +			dev_err(afe->dev,
> +				"%s %s malloc pages %zu bytes failed %d\n",
> +				__func__, memif->data->name, request_size, ret);
> +			return ret;
> +		}
> +
> +		fe_data->use_sram = false;
> +
> +		mt8365_afe_emi_clk_on(afe);
> +	} else {
> +		struct snd_dma_buffer *dma_buf = &substream->dma_buffer;
> +
> +		dma_buf->dev.type = SNDRV_DMA_TYPE_DEV;
> +		dma_buf->dev.dev = substream->pcm->card->dev;
> +		dma_buf->area = (unsigned char *)fe_data->sram_vir_addr;
> +		dma_buf->addr = fe_data->sram_phy_addr;
> +		dma_buf->bytes = request_size;
> +		snd_pcm_set_runtime_buffer(substream, dma_buf);
> +
> +		fe_data->use_sram = true;
> +	}
> +
> +	memif->phys_buf_addr = lower_32_bits(substream->runtime->dma_addr);
> +	memif->buffer_size = substream->runtime->dma_bytes;
> +
> +	/* start */
> +	regmap_write(afe->regmap, memif->data->reg_ofs_base,
> +		memif->phys_buf_addr);
> +	/* end */
> +	regmap_write(afe->regmap,
> +		memif->data->reg_ofs_base + base_end_offset,
> +		memif->phys_buf_addr + memif->buffer_size - 1);
> +
> +	/* set channel */
> +	if (memif->data->mono_shift >= 0) {
> +		unsigned int mono = (params_channels(params) == 1) ? 1 : 0;
> +
> +		if (memif->data->mono_reg < 0)
> +			dev_info(afe->dev, "%s mono_reg is NULL\n", __func__);
> +		else
> +			regmap_update_bits(afe->regmap, memif->data->mono_reg,
> +				1 << memif->data->mono_shift,
> +				mono << memif->data->mono_shift);
> +	}
> +
> +	/* set rate */
> +	if (memif->data->fs_shift < 0)
> +		return 0;
> +
> +	fs = afe->memif_fs(substream, params_rate(params));
> +
> +	if (fs < 0)
> +		return -EINVAL;
> +
> +	if (memif->data->fs_reg < 0)
> +		dev_info(afe->dev, "%s fs_reg is NULL\n", __func__);
> +	else
> +		regmap_update_bits(afe->regmap, memif->data->fs_reg,
> +			memif->data->fs_maskbit << memif->data->fs_shift,
> +			fs << memif->data->fs_shift);
> +
> +	return 0;
> +}
> +
> +static int mt8365_afe_fe_hw_free(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id;
> +	struct mtk_base_afe_memif *memif = &afe->memif[dai_id];
> +	struct mt8365_fe_dai_data *fe_data = &afe_priv->fe_data[dai_id];
> +	int ret = 0;
> +
> +	dev_dbg(afe->dev, "%s %s\n", __func__, memif->data->name);

Drop

> +
> +	if (fe_data->use_sram) {
> +		snd_pcm_set_runtime_buffer(substream, NULL);
> +	} else {
> +		ret = snd_pcm_lib_free_pages(substream);
> +
> +		mt8365_afe_emi_clk_off(afe);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mt8365_afe_fe_prepare(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id;
> +	struct mtk_base_afe_memif *memif = &afe->memif[dai_id];
> +
> +	/* set format */
> +	if (memif->data->hd_reg >= 0) {
> +		switch (substream->runtime->format) {
> +		case SNDRV_PCM_FORMAT_S16_LE:
> +			regmap_update_bits(afe->regmap, memif->data->hd_reg,
> +				3 << memif->data->hd_shift,
> +				0 << memif->data->hd_shift);
> +			break;
> +		case SNDRV_PCM_FORMAT_S32_LE:
> +			regmap_update_bits(afe->regmap, memif->data->hd_reg,
> +				3 << memif->data->hd_shift,
> +				3 << memif->data->hd_shift);
> +
> +			if (dai_id == MT8365_AFE_MEMIF_TDM_IN) {
> +				regmap_update_bits(afe->regmap,
> +					memif->data->hd_reg,
> +					3 << memif->data->hd_shift,
> +					1 << memif->data->hd_shift);
> +				regmap_update_bits(afe->regmap,
> +					memif->data->hd_reg,
> +					1 << memif->data->hd_align_mshift,
> +					1 << memif->data->hd_align_mshift);
> +			}
> +			break;
> +		case SNDRV_PCM_FORMAT_S24_LE:
> +			regmap_update_bits(afe->regmap, memif->data->hd_reg,
> +				3 << memif->data->hd_shift,
> +				1 << memif->data->hd_shift);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mt8365_afe_irq_direction_enable(afe,
> +		memif->irq_usage,
> +		MT8365_AFE_IRQ_DIR_MCU);
> +
> +	return 0;
> +}
> +
> +int mt8365_afe_fe_trigger(struct snd_pcm_substream *substream, int cmd,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	int dai_id = snd_soc_rtd_to_cpu(rtd, 0)->id;
> +	struct mt8365_control_data *ctrl_data = &afe_priv->ctrl_data;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		/* enable channel merge */
> +		if (dai_id == MT8365_AFE_MEMIF_VUL2 &&
> +		    !ctrl_data->bypass_cm1) {
> +			regmap_update_bits(afe->regmap,
> +				AFE_CM1_CON0,
> +				CM_AFE_CM_ON, CM_AFE_CM_ON);
> +		} else if (dai_id == MT8365_AFE_MEMIF_TDM_IN &&
> +			   !ctrl_data->bypass_cm2) {
> +			regmap_update_bits(afe->regmap,
> +				AFE_CM2_CON0,
> +				CM_AFE_CM_ON, CM_AFE_CM_ON);
> +		}
> +		break;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +		/* disable channel merge */
> +		if (dai_id == MT8365_AFE_MEMIF_VUL2 &&
> +		    !ctrl_data->bypass_cm1) {
> +			regmap_update_bits(afe->regmap,
> +				AFE_CM1_CON0,
> +				CM_AFE_CM_ON, ~CM_AFE_CM_ON);
> +		} else if (dai_id == MT8365_AFE_MEMIF_TDM_IN &&
> +			   !ctrl_data->bypass_cm2) {
> +			regmap_update_bits(afe->regmap,
> +				AFE_CM2_CON0,
> +				CM_AFE_CM_ON, ~CM_AFE_CM_ON);
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return mtk_afe_fe_trigger(substream, cmd, dai);
> +}
> +
> +static int mt8365_afe_hw_gain1_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +
> +	mt8365_afe_enable_main_clk(afe);
> +	return 0;
> +}
> +
> +static void mt8365_afe_hw_gain1_shutdown(struct snd_pcm_substream *substream,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	struct mt8365_be_dai_data *be =
> +		&afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE];
> +
> +	const unsigned int stream = substream->stream;

Why do you const local scalars? What is the point of this variable anyway?

> +
> +	if (be->prepared[stream]) {
> +		regmap_update_bits(afe->regmap, AFE_GAIN1_CON0,
> +			AFE_GAIN1_CON0_EN_MASK, 0);
> +		be->prepared[stream] = false;
> +	}
> +	mt8365_afe_disable_main_clk(afe);
> +}
> +
> +static int mt8365_afe_hw_gain1_prepare(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct snd_pcm_runtime * const runtime = substream->runtime;
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	struct mt8365_be_dai_data *be =
> +		&afe_priv->be_data[dai->id - MT8365_AFE_BACKEND_BASE];
> +
> +	const unsigned int rate = runtime->rate;
> +	const unsigned int stream = substream->stream;

So this const is everywhere? How does it change it from code safety? No
one expects to overwrite such variables anyway, so there is no increase
in readability, either.

> +	int fs;
> +	unsigned int val1 = 0, val2 = 0;
> +
> +	if (be->prepared[stream]) {
> +		dev_info(afe->dev, "%s prepared already\n", __func__);
> +		return 0;
> +	}
> +
> +	fs = mt8365_afe_fs_timing(rate);
> +	regmap_update_bits(afe->regmap, AFE_GAIN1_CON0,
> +		AFE_GAIN1_CON0_MODE_MASK, (unsigned int)fs<<4);
> +
> +	regmap_read(afe->regmap, AFE_GAIN1_CON1, &val1);
> +	regmap_read(afe->regmap, AFE_GAIN1_CUR, &val2);
> +	if ((val1 & AFE_GAIN1_CON1_MASK) != (val2 & AFE_GAIN1_CUR_MASK))
> +		regmap_update_bits(afe->regmap, AFE_GAIN1_CUR,
> +		AFE_GAIN1_CUR_MASK, val1);
> +
> +	regmap_update_bits(afe->regmap, AFE_GAIN1_CON0,
> +		AFE_GAIN1_CON0_EN_MASK, 1);
> +	be->prepared[stream] = true;
> +
> +	return 0;
> +}
> +
> +static const struct snd_pcm_hardware mt8365_hostless_hardware = {
> +	.info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED |
> +		 SNDRV_PCM_INFO_MMAP_VALID),
> +	.period_bytes_min = 256,
> +	.period_bytes_max = 4 * 48 * 1024,
> +	.periods_min = 2,
> +	.periods_max = 256,
> +	.buffer_bytes_max = 8 * 48 * 1024,
> +	.fifo_size = 0,
> +};
> +
> +/* dai ops */
> +static int mtk_dai_hostless_startup(struct snd_pcm_substream *substream,
> +				    struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int ret;
> +
> +	snd_soc_set_runtime_hwparams(substream, &mt8365_hostless_hardware);
> +
> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		dev_info(afe->dev, "snd_pcm_hw_constraint_integer failed\n");
> +	return ret;
> +}
> +
> +/* FE DAIs */
> +static const struct snd_soc_dai_ops mt8365_afe_fe_dai_ops = {
> +	.startup	= mt8365_afe_fe_startup,
> +	.shutdown	= mt8365_afe_fe_shutdown,
> +	.hw_params	= mt8365_afe_fe_hw_params,
> +	.hw_free	= mt8365_afe_fe_hw_free,
> +	.prepare	= mt8365_afe_fe_prepare,
> +	.trigger	= mt8365_afe_fe_trigger,
> +};
> +
> +static const struct snd_soc_dai_ops mt8365_dai_hostless_ops = {
> +	.startup = mtk_dai_hostless_startup,
> +};
> +

..

> +static int mt8365_afe_cm2_io_input_mux_get(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	ucontrol->value.integer.value[0] = mCM2Input;
> +
> +	return 0;
> +}
> +
> +static int mt8365_afe_cm2_io_input_mux_put(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)

Run checkpatch --strict

> +{
> +	struct snd_soc_dapm_context *dapm =
> +		snd_soc_dapm_kcontrol_dapm(kcontrol);
> +	struct snd_soc_component *comp = snd_soc_dapm_to_component(dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(comp);
> +	struct mt8365_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	mCM2Input = ucontrol->value.enumerated.item[0];
> +
> +	afe_priv->cm2_mux_input = mCM2Input;
> +	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
> +
> +	return ret;
> +}
> +

..

> +
> +static const struct regmap_config mt8365_afe_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = MAX_REGISTER,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static irqreturn_t mt8365_afe_irq_handler(int irq, void *dev_id)
> +{
> +	struct mtk_base_afe *afe = dev_id;
> +	unsigned int reg_value;
> +	unsigned int mcu_irq_mask;
> +	int i, ret;
> +
> +	ret = regmap_read(afe->regmap, AFE_IRQ_MCU_STATUS, &reg_value);
> +	if (ret) {
> +		dev_err(afe->dev, "%s irq status err\n", __func__);

I can imagine happy user seeing flooded dmesg on each IRQ with these
messages...

> +		reg_value = AFE_IRQ_STATUS_BITS;
> +		goto err_irq;
> +	}
> +
> +	ret = regmap_read(afe->regmap, AFE_IRQ_MCU_EN, &mcu_irq_mask);
> +	if (ret) {
> +		dev_err(afe->dev, "%s irq mcu_en err\n", __func__);
> +		reg_value = AFE_IRQ_STATUS_BITS;
> +		goto err_irq;
> +	}
> +
> +	/* only clr cpu irq */
> +	reg_value &= mcu_irq_mask;
> +
> +	for (i = 0; i < MT8365_AFE_MEMIF_NUM; i++) {
> +		struct mtk_base_afe_memif *memif = &afe->memif[i];
> +		struct mtk_base_afe_irq *mcu_irq;
> +
> +		if (memif->irq_usage < 0)
> +			continue;
> +
> +		mcu_irq = &afe->irqs[memif->irq_usage];
> +
> +		if (!(reg_value & (1 << mcu_irq->irq_data->irq_clr_shift)))
> +			continue;
> +
> +		snd_pcm_period_elapsed(memif->substream);
> +	}
> +
> +err_irq:
> +	/* clear irq */
> +	regmap_write(afe->regmap, AFE_IRQ_MCU_CLR,
> +		reg_value & AFE_IRQ_STATUS_BITS);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __maybe_unused mt8365_afe_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int mt8365_afe_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int __maybe_unused mt8365_afe_suspend(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +	struct regmap *regmap = afe->regmap;
> +	int i;
> +
> +	mt8365_afe_enable_main_clk(afe);
> +
> +	if (!afe->reg_back_up)
> +		afe->reg_back_up =
> +			devm_kcalloc(dev, afe->reg_back_up_list_num,
> +				     sizeof(unsigned int), GFP_KERNEL);
> +
> +	for (i = 0; i < afe->reg_back_up_list_num; i++)
> +		regmap_read(regmap, afe->reg_back_up_list[i],
> +			    &afe->reg_back_up[i]);
> +
> +	mt8365_afe_disable_main_clk(afe);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mt8365_afe_resume(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +	struct regmap *regmap = afe->regmap;
> +	int i = 0;
> +
> +	if (!afe->reg_back_up) {
> +		dev_dbg(dev, "%s no reg_backup\n", __func__);
> +		return 0;
> +	}
> +
> +	mt8365_afe_enable_main_clk(afe);
> +
> +	for (i = 0; i < afe->reg_back_up_list_num; i++)
> +		regmap_write(regmap, afe->reg_back_up_list[i],
> +				 afe->reg_back_up[i]);
> +
> +	mt8365_afe_disable_main_clk(afe);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mt8365_afe_dev_runtime_suspend(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +
> +	dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__,
> +		pm_runtime_status_suspended(dev), afe->suspended);

Drop silly trace-like prints. Tracing is for this.

> +
> +	if (pm_runtime_status_suspended(dev) || afe->suspended)
> +		return 0;
> +
> +	mt8365_afe_suspend(dev);
> +
> +	afe->suspended = true;
> +
> +	dev_dbg(afe->dev, "%s <<\n", __func__);

Drop silly trace-like prints. Tracing is for this.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mt8365_afe_dev_runtime_resume(struct device *dev)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dev);
> +
> +	dev_dbg(afe->dev, "%s suspend %d %d >>\n", __func__,
> +		pm_runtime_status_suspended(dev), afe->suspended);

Drop silly trace-like prints. Tracing is for this.

> +
> +	if (pm_runtime_status_suspended(dev) || !afe->suspended)
> +		return 0;
> +
> +	mt8365_afe_resume(dev);
> +
> +	afe->suspended = false;
> +
> +	dev_dbg(afe->dev, "%s <<\n", __func__);

Drop silly trace-like prints. Tracing is for this.

> +
> +	return 0;
> +}
> +
> +static int mt8365_afe_init_registers(struct mtk_base_afe *afe)
> +{
> +	size_t i;
> +
> +	static struct {
> +		unsigned int reg;
> +		unsigned int mask;
> +		unsigned int val;
> +	} init_regs[] = {
> +		{ AFE_CONN_24BIT, GENMASK(31, 0), GENMASK(31, 0) },
> +		{ AFE_CONN_24BIT_1, GENMASK(21, 0), GENMASK(21, 0) },
> +	};
> +
> +	mt8365_afe_enable_main_clk(afe);
> +
> +	for (i = 0; i < ARRAY_SIZE(init_regs); i++)
> +		regmap_update_bits(afe->regmap, init_regs[i].reg,
> +				   init_regs[i].mask, init_regs[i].val);
> +
> +	mt8365_afe_disable_main_clk(afe);
> +
> +	return 0;
> +}
> +
> +static int mt8365_afe_component_probe(struct snd_soc_component *component)
> +{
> +	return mtk_afe_add_sub_dai_control(component);
> +}
> +
> +static const struct snd_soc_component_driver mt8365_afe_component = {
> +	.name		= AFE_PCM_NAME,
> +	.probe		= mt8365_afe_component_probe,
> +	.pointer	= mtk_afe_pcm_pointer,
> +	.pcm_construct	= mtk_afe_pcm_new,
> +};
> +
> +static const struct snd_soc_component_driver mt8365_afe_pcm_component = {
> +	.name = "mt8365-afe-pcm-dai",
> +};
> +
> +static int mt8365_dai_memif_register(struct mtk_base_afe *afe)
> +{
> +	struct mtk_base_afe_dai *dai;
> +
> +	dai = devm_kzalloc(afe->dev, sizeof(*dai), GFP_KERNEL);
> +	if (!dai)
> +		return -ENOMEM;
> +
> +	list_add(&dai->list, &afe->sub_dais);
> +
> +	dai->dai_drivers = mt8365_memif_dai_driver;
> +	dai->num_dai_drivers = ARRAY_SIZE(mt8365_memif_dai_driver);
> +
> +	dai->dapm_widgets = mt8365_memif_widgets;
> +	dai->num_dapm_widgets = ARRAY_SIZE(mt8365_memif_widgets);
> +	dai->dapm_routes = mt8365_memif_routes;
> +	dai->num_dapm_routes = ARRAY_SIZE(mt8365_memif_routes);
> +	return 0;
> +}
> +
> +typedef int (*dai_register_cb)(struct mtk_base_afe *);
> +static const dai_register_cb dai_register_cbs[] = {
> +	mt8365_dai_pcm_register,
> +	mt8365_dai_i2s_register,
> +	mt8365_dai_adda_register,
> +	mt8365_dai_dmic_register,
> +	mt8365_dai_memif_register,
> +};
> +
> +static int mt8365_afe_pcm_dev_probe(struct platform_device *pdev)
> +{
> +	struct mtk_base_afe *afe;
> +	struct mt8365_afe_private *afe_priv;
> +	struct device *dev;
> +	int ret, i, sel_irq;
> +	unsigned int irq_id;
> +	struct resource *res;
> +
> +	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;
> +
> +	spin_lock_init(&afe_priv->afe_ctrl_lock);
> +	mutex_init(&afe_priv->afe_clk_mutex);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	afe->base_addr = devm_ioremap_resource(&pdev->dev, res);

Why not using helper combining these two?

> +	if (IS_ERR(afe->base_addr))
> +		return PTR_ERR(afe->base_addr);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res) {
> +		afe_priv->afe_sram_vir_addr =
> +			devm_ioremap_resource(&pdev->dev, res);
> +		if (!IS_ERR(afe_priv->afe_sram_vir_addr)) {
> +			afe_priv->afe_sram_phy_addr = res->start;
> +			afe_priv->afe_sram_size = resource_size(res);
> +		}
> +	}
> +
> +	/* initial audio related clock */
> +	ret = mt8365_afe_init_audio_clk(afe);
> +	if (ret) {
> +		dev_err(afe->dev, "mt8365_afe_init_audio_clk fail\n");

return dev_err_probe()

> +		return ret;
> +	}
> +
> +	afe->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "top_audio_sel",
> +		afe->base_addr,	&mt8365_afe_regmap_config);
> +	if (IS_ERR(afe->regmap))
> +		return PTR_ERR(afe->regmap);
> +
> +	/* memif % irq initialize*/
> +	afe->memif_size = MT8365_AFE_MEMIF_NUM;
> +	afe->memif = devm_kcalloc(afe->dev, afe->memif_size,
> +				  sizeof(*afe->memif), GFP_KERNEL);
> +	if (!afe->memif)
> +		return -ENOMEM;
> +
> +	afe->irqs_size = MT8365_AFE_IRQ_NUM;
> +	afe->irqs = devm_kcalloc(afe->dev, afe->irqs_size,
> +				 sizeof(*afe->irqs), GFP_KERNEL);
> +	if (!afe->irqs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < afe->irqs_size; i++)
> +		afe->irqs[i].irq_data = &irq_data[i];
> +
> +	irq_id = platform_get_irq(pdev, 0);
> +	if (!irq_id) {

That does not look correct. 0 is valid. Look at help of this function.

> +		dev_err(afe->dev, "np %s no irq\n", afe->dev->of_node->name);

return dev_err_probe()

> +		return -ENXIO;
> +	}
> +	ret = devm_request_irq(afe->dev, irq_id, mt8365_afe_irq_handler,
> +			       0, "Afe_ISR_Handle", (void *)afe);
> +	if (ret) {
> +		dev_err(afe->dev, "could not request_irq\n");

return dev_err_probe()

> +		return 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_warn(afe->dev, "dai register i %d fail, ret %d\n",
> +				 i, ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* init dai_driver and component_driver */
> +	ret = mtk_afe_combine_sub_dai(afe);
> +	if (ret) {
> +		dev_warn(afe->dev, "mtk_afe_combine_sub_dai fail, ret %d\n",
> +			 ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < afe->memif_size; i++) {
> +		afe->memif[i].data = &memif_data[i];
> +		sel_irq = memif_specified_irqs[i];
> +		if (sel_irq >= 0) {
> +			afe->memif[i].irq_usage = sel_irq;
> +			afe->memif[i].const_irq = 1;
> +			afe->irqs[sel_irq].irq_occupyed = true;
> +		} else {
> +			afe->memif[i].irq_usage = -1;
> +		}
> +	}
> +
> +	afe->mtk_afe_hardware = &mt8365_afe_hardware;
> +	afe->memif_fs = mt8365_memif_fs;
> +	afe->irq_fs = mt8365_irq_fs;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +	afe->reg_back_up_list = mt8365_afe_backup_list;
> +	afe->reg_back_up_list_num = ARRAY_SIZE(mt8365_afe_backup_list);
> +	afe->runtime_resume = mt8365_afe_runtime_resume;
> +	afe->runtime_suspend = mt8365_afe_runtime_suspend;
> +
> +	/* open afe pdn for dapm read/write audio register */
> +	mt8365_afe_enable_top_cg(afe, MT8365_TOP_CG_AFE);
> +
> +	/* Set 26m parent clk */
> +	mt8365_afe_set_clk_parent(afe,
> +		afe_priv->clocks[MT8365_CLK_TOP_AUD_SEL],
> +		afe_priv->clocks[MT8365_CLK_CLK26M]);
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &mt8365_afe_component, NULL, 0);
> +	if (ret) {
> +		dev_warn(dev, "err_platform\n");
> +		return ret;
> +	}
> +
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &mt8365_afe_pcm_component,
> +					      afe->dai_drivers,
> +					      afe->num_dai_drivers);
> +	if (ret) {
> +		dev_warn(dev, "err_dai_component\n");
> +		return ret;
> +	}
> +
> +	mt8365_afe_init_registers(afe);
> +
> +	dev_info(&pdev->dev, "MT8365 AFE driver initialized.\n");

Drop simple probe success messages everywhere. They are not needed and
they don't bring useful information.

Best regards,
Krzysztof