Re: [PATCH v4 4/4] ASoC: airoha: Add AFE and machine driver for Airoha AN7581

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 7/31/2026 8:43 AM, Christian Marangi wrote:
> Add support for the Sound system present on Airoha AN7581 SoC. This is
> based on the mediatek AFE drivers.
> 
> Also add the machine driver to create an actual sound card for the AFE.

...

> --- /dev/null
> +++ b/sound/soc/mediatek/an7581/an7581-afe-common.h
> @@ -0,0 +1,50 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * an7581-afe-common.h  --  Airoha AN7581 audio driver definitions
> + */
> +
> +#ifndef _AN7581_AFE_COMMON_H_
> +#define _AN7581_AFE_COMMON_H_
> +
> +#include <linux/atomic.h>
> +#include <sound/soc.h>
> +#include <linux/list.h>
> +#include <linux/regmap.h>

Sort these, linux/ comes before sound/.

> +#include "../../mediatek/common/mtk-base-afe.h"
> +
> +enum {
> +	AN7581_MEMIF_DL1,
> +	AN7581_MEMIF_UL1,
> +	AN7581_MEMIF_NUM,
> +	AN7581_DAI_ETDM = AN7581_MEMIF_NUM,
> +	AN7581_DAI_NUM,
> +};
> +
> +enum {
> +	AN7581_IRQ_0,
> +	AN7581_IRQ_1,
> +	AN7581_IRQ_NUM,
> +};
> +
> +struct an7581_memif_irq_desc {
> +	u8 irq;
> +	u32 status_bit;
> +	u32 clear_reg;
> +};
> +
> +struct an7581_afe_private {
> +	unsigned int users;
> +	/* protect concurrent ETDM user tracking */
> +	struct mutex user_lock;
> +
> +	/* dai */
> +	void *dai_priv[AN7581_DAI_NUM];

The comment is redundant.

> +};
> +
> +unsigned int an7581_afe_rate_transform(struct device *dev,
> +				       unsigned int rate);
> +
> +/* dai register */
> +int an7581_dai_etdm_register(struct mtk_base_afe *afe);

Ditto.

> +
> +#endif

...

> +unsigned int an7581_afe_rate_transform(struct device *dev, unsigned int rate)
> +{
> +	switch (rate) {
> +	case 7350:
> +		return ARH_AFE_RATE_7K;
> +	case 8000:
> +		return ARH_AFE_RATE_8K;
> +	case 11025:
> +		return ARH_AFE_RATE_11K;
> +	case 12000:
> +		return ARH_AFE_RATE_12K;
> +	case 14700:
> +		return ARH_AFE_RATE_14K;
> +	case 16000:
> +		return ARH_AFE_RATE_16K;
> +	case 22050:
> +		return ARH_AFE_RATE_22K;
> +	case 24000:
> +		return ARH_AFE_RATE_24K;
> +	case 29400:
> +		return ARH_AFE_RATE_29K;
> +	case 32000:
> +		return ARH_AFE_RATE_32K;
> +	case 44100:
> +		return ARH_AFE_RATE_44K;
> +	case 48000:
> +		return ARH_AFE_RATE_48K;
> +	case 88200:
> +		return ARH_AFE_RATE_88K;
> +	case 96000:
> +		return ARH_AFE_RATE_96K;
> +	case 176400:
> +		return ARH_AFE_RATE_176K;
> +	case 192000:
> +		return ARH_AFE_RATE_192K;
> +	case 352800:
> +		return ARH_AFE_RATE_352K;
> +	case 384000:
> +		return ARH_AFE_RATE_384K;
> +	default:
> +		dev_warn(dev, "%s(), rate %u invalid, using %d!!!\n",
> +			 __func__, rate, ARH_AFE_RATE_48K);

Three '!', really? :)

> +		return ARH_AFE_RATE_48K;
> +	}
> +}

> +static const struct snd_pcm_hardware an7581_afe_hardware = {
> +	.info = SNDRV_PCM_INFO_MMAP |
> +		SNDRV_PCM_INFO_INTERLEAVED |
> +		SNDRV_PCM_INFO_MMAP_VALID,
> +	.formats = SNDRV_PCM_FMTBIT_S16_LE |
> +		   SNDRV_PCM_FMTBIT_S24_LE |
> +		   SNDRV_PCM_FMTBIT_S32_LE,
> +	.period_bytes_min = 512,
> +	.period_bytes_max = 128 * 1024,
> +	.periods_min = 2,
> +	.periods_max = 256,
> +	.buffer_bytes_max = 256 * 1024,
> +	.fifo_size = 0,

static takes care of this unless you want to explicitly state the 
fifo_size must be 0 - in such case comment is necessary, otherwise drop 
the assignment.

> +};
> +
> +static int an7581_memif_fs(struct snd_pcm_substream *substream,
> +			   unsigned int rate)

For the entire file - char limit is 100, there is no reason not to 
utilize it.

> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(component);
> +
> +	return an7581_afe_rate_transform(afe->dev, rate);
> +}
> +
> +static int an7581_irq_fs(struct snd_pcm_substream *substream,
> +			 unsigned int rate)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_component *component = snd_soc_rtdcom_lookup(rtd, AFE_PCM_NAME);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(component);
> +
> +	return an7581_afe_rate_transform(afe->dev, rate);
> +}

These two functions seem to be identical.

> +
> +static int an7581_afe_fe_startup(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	int ret;
> +
> +	ret = mtk_afe_fe_startup(substream, dai);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		ret = snd_pcm_hw_constraint_minmax(runtime,
> +						   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
> +						   0x2000, UINT_MAX);
> +		if (ret < 0)
> +			dev_err(afe->dev, "hw_constraint_minmax failed\n");
> +	}
> +
> +	return ret;

mtk_afe_fe_startup() acquires irq via mtk_dynamic_irq_acquire(). Do we 
need to unwind the process if snd_pcm_hw_constraint_minmax() fails?

> +}

...

> +static int an7581_afe_pcm_dev_probe(struct platform_device *pdev)
> +{
> +	struct an7581_afe_private *afe_priv;
> +	struct reset_control *reset;
> +	struct mtk_base_afe *afe;
> +	struct device *dev;
> +	int i, irq_id, ret;
> +	void *base;
> +
> +	afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL);
> +	if (!afe)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, afe);

Typically drvdata is assigned last, one we know probe() is successful. 
Is there some specific reason why you assign the pointer immediately?
> +
> +	afe->platform_priv = devm_kzalloc(&pdev->dev, sizeof(*afe_priv),
> +					  GFP_KERNEL);
> +	if (!afe->platform_priv)
> +		return -ENOMEM;

Especially when the memory allocation is still ongoing..

> +
> +	afe_priv = afe->platform_priv;
> +	mutex_init(&afe_priv->user_lock);
> +	afe->dev = &pdev->dev;
> +	dev = afe->dev;
> +
> +	reset = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
> +
> +	/* Global reset I2S */
> +	reset_control_assert(reset);
> +	usleep_range(10, 20);
> +	reset_control_deassert(reset);
> +
> +	afe->base_addr = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(afe->base_addr))
> +		return PTR_ERR(afe->base_addr);
> +
> +	base = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	afe->regmap = devm_regmap_init_mmio(&pdev->dev, afe->base_addr,
> +					    &an7581_afe_regmap_config);
> +	if (IS_ERR(afe->regmap))
> +		return PTR_ERR(afe->regmap);
> +
> +	mutex_init(&afe->irq_alloc_lock);
> +
> +	/* irq initialize */
> +	afe->irqs_size = AN7581_IRQ_NUM;
> +	afe->irqs = devm_kcalloc(dev, afe->irqs_size, sizeof(*afe->irqs),
> +				 GFP_KERNEL);
> +	if (!afe->irqs)
> +		return -ENOMEM;

If the size is known upfront, I'd just move all the allocations to the top.

> +
> +	for (i = 0; i < afe->irqs_size; i++)
> +		afe->irqs[i].irq_data = &irq_data[i];
> +
> +	afe->irqs[AN7581_IRQ_0].regmap = afe->regmap;
> +	afe->irqs[AN7581_IRQ_1].regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +							       &an7581_afe_irq1_regmap_config);
> +	if (IS_ERR(afe->irqs[AN7581_IRQ_1].regmap))
> +		return PTR_ERR(afe->irqs[AN7581_IRQ_1].regmap);
> +
> +	/* request irq */
> +	irq_id = platform_get_irq(pdev, 0);
> +	if (irq_id < 0)
> +		return irq_id;
> +
> +	/* init memif */
> +	afe->memif_size = AN7581_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++) {
> +		int sel_irq = an7581_memif_irq_descs[i].irq;
> +
> +		afe->memif[i].data = &memif_data[i];
> +		afe->memif[i].irq_usage = sel_irq;
> +		afe->memif[i].const_irq = 1;
> +		afe->irqs[sel_irq].irq_occupyed = true;
> +	}
> +
> +	/* init sub_dais */
> +	INIT_LIST_HEAD(&afe->sub_dais);
> +
> +	ret = devm_request_irq(dev, irq_id, an7581_afe_irq_handler,
> +			       IRQF_TRIGGER_NONE, "asys-isr", (void *)afe);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq for asys-isr\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(dai_register_cbs); i++) {
> +		ret = dai_register_cbs[i](afe);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "DAI register failed, i: %d\n", i);
> +	}
> +
> +	/* init dai_driver and component_driver */
> +	ret = mtk_afe_combine_sub_dai(afe);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "mtk_afe_combine_sub_dai fail\n");
> +
> +	afe->mtk_afe_hardware = &an7581_afe_hardware;
> +	afe->memif_fs = an7581_memif_fs;
> +	afe->irq_fs = an7581_irq_fs;
> +
> +	/* register component */
> +	ret = devm_snd_soc_register_component(&pdev->dev,
> +					      &mtk_afe_pcm_platform,
> +					      NULL, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register AFE component\n");
> +
> +	ret = devm_snd_soc_register_component(afe->dev,
> +					      &an7581_afe_pcm_dai_component,
> +					      afe->dai_drivers,
> +					      afe->num_dai_drivers);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot register PCM DAI component\n");
> +
> +	return 0;
> +}

...

> +/* dai ops */
> +static int mtk_dai_etdm_startup(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +
> +	regmap_set_bits(afe->regmap, AFE_DAC_CON0,
> +			BIT(AFE_AFE_ENABLE_SHIFT));
> +
> +	mutex_lock(&afe_priv->user_lock);

I'd suggest to switch to guards, see cleanup.h.

> +	afe_priv->users++;
> +	mutex_unlock(&afe_priv->user_lock);
> +
> +	return 0;

Why skip the return code of regmap_set_bits()?

> +}
> +
> +static void mtk_dai_etdm_shutdown(struct snd_pcm_substream *substream,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +
> +	mutex_lock(&afe_priv->user_lock);
> +	afe_priv->users--;
> +	if (!afe_priv->users)
> +		regmap_clear_bits(afe->regmap, AFE_DAC_CON0,
> +				  BIT(AFE_AFE_ENABLE_SHIFT));
> +	mutex_unlock(&afe_priv->user_lock);

Well, here we clear bits in locked state whereas above they're set 
unlocked. Typically such inconsistency translates to an issue.

> +}
> +
> +static unsigned int get_etdm_ch_fixup(unsigned int channels)
> +{
> +	if (channels > 16)
> +		return 24;
> +	else if (channels > 8)
> +		return 16;
> +	else if (channels > 4)
> +		return 8;
> +	else if (channels > 2)
> +		return 4;
> +	else
> +		return 2;

Refactor, not a single 'else-if' is needed here as well as 'else'.
If a preceding if-statement ends with 'return' there is no elsing anything.

> +}
> +
> +static int mtk_dai_etdm_config(struct mtk_base_afe *afe,
> +			       struct snd_pcm_hw_params *params,
> +			       struct snd_soc_dai *dai,
> +			       int stream)
> +{
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_dai_etdm_priv *etdm_data = afe_priv->dai_priv[dai->id];
> +	unsigned int rate = params_rate(params);
> +	unsigned int etdm_rate = an7581_etdm_rate_transform(afe->dev, rate);
> +	unsigned int channels = params_channels(params);
> +	unsigned int bit_width = params_width(params);
> +	unsigned int wlen = get_etdm_wlen(bit_width);

All the format variables are used once or twice. I'd drop them. Right 
now you have 11 (!) locals on the stack.

> +	unsigned int val = 0, val1 = 0;
> +	unsigned int mask = 0, mask1 = 0;
> +
> +	dev_dbg(afe->dev, "%s(), stream %d, rate %u, bitwidth %u\n",
> +		__func__, stream, rate, bit_width);
> +
> +	/* CON0 */
> +	mask |= ETDM_SLAVE_MODE;
> +	if (etdm_data->slave_mode)
> +		val |= ETDM_SLAVE_MODE;
> +	mask |= ETDM_BIT_LEN;
> +	val |= FIELD_PREP(ETDM_BIT_LEN, bit_width - 1);
> +	mask |= ETDM_WRD_LEN;
> +	val |= FIELD_PREP(ETDM_WRD_LEN, wlen - 1);
> +	mask |= ETDM_FMT;
> +	val |= FIELD_PREP(ETDM_FMT, etdm_data->format);
> +	mask |= ETDM_CH_NUM;
> +	val |= FIELD_PREP(ETDM_CH_NUM, get_etdm_ch_fixup(channels) - 1);

I'd group 'mask' and 'val' together, easier to follow. These do not seem 
intertwined.

> +
> +	/* CON1 */
> +	mask1 |= EDTM_LRCK_AUTO_MODE;
> +	val1 |= EDTM_LRCK_AUTO_MODE;
> +	mask1 |= EDTM_CKEN_SEL;
> +	val1 |= EDTM_CKEN_SEL;
> +	mask1 |= EDTM_LRCK_AUTO_OFF;
> +	val1 |= EDTM_LRCK_AUTO_OFF;
> +	mask1 |= EDTM_INITIAL_POINT;
> +	val1 |= FIELD_PREP(EDTM_INITIAL_POINT, 14);
> +	mask1 |= EDTM_INITIAL_COUNT;
> +	val1 |= FIELD_PREP(EDTM_INITIAL_COUNT, 14);

Ditto.

> +
> +	switch (stream) {
> +	case SNDRV_PCM_STREAM_PLAYBACK:
> +		/* set ETDM_OUT1_CON0 */
> +		regmap_update_bits(afe->regmap, ETDM_OUT1_CON0, mask, val);
> +
> +		mask1 |= EDTM_DIRECT_INPUT_MASTER_BCK;
> +		val1 |= EDTM_DIRECT_INPUT_MASTER_BCK;
> +
> +		/* set ETDM_OUT1_CON1 */
> +		regmap_update_bits(afe->regmap, ETDM_OUT1_CON1, mask1, val1);
> +
> +		/* set ETDM_OUT1_CON4 */
> +		regmap_update_bits(afe->regmap, ETDM_OUT1_CON4, OUT_SEL_FS,
> +				   FIELD_PREP(OUT_SEL_FS, etdm_rate));
> +
> +		regmap_update_bits(afe->irqs[AN7581_IRQ_0].regmap,
> +				   afe->irqs[AN7581_IRQ_0].irq_data->irq_en_reg,
> +				   AFE_IRQ_EN_SEL, AFE_IRQ_EN_SEL_I2SOUT);
> +		break;
> +	case SNDRV_PCM_STREAM_CAPTURE:
> +		/* set ETDM_IN1_CON0 */
> +		regmap_update_bits(afe->regmap, ETDM_IN1_CON0, mask, val);
> +
> +		/* set ETDM_IN1_CON1 */
> +		regmap_update_bits(afe->regmap, ETDM_IN1_CON1, mask1, val1);
> +
> +		/* set ETDM_IN1_CON3 */

All the comments found in this function do not explain anything. Drop them.

> +		regmap_update_bits(afe->regmap, ETDM_IN1_CON3, IN_SEL_FS,
> +				   FIELD_PREP(IN_SEL_FS, etdm_rate));
> +
> +		regmap_update_bits(afe->irqs[AN7581_IRQ_1].regmap,
> +				   afe->irqs[AN7581_IRQ_1].irq_data->irq_en_reg,
> +				   AFE_IRQ_EN_SEL, AFE_IRQ_EN_SEL_I2SIN);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_dai_etdm_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params,
> +				  struct snd_soc_dai *dai)
> +{
> +	unsigned int rate = params_rate(params);
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +
> +	regmap_update_bits(afe->regmap, ETDM_COWORK_CON0,
> +			   EDTM_IN1_SLAVE_SEL,
> +			   EDTM_IN1_SLAVE_FROM_ETDMIN1_SLAVE);
> +	regmap_update_bits(afe->regmap, ETDM_COWORK_CON0,
> +			   EDTM_OUT1_SLAVE_SEL,
> +			   EDTM_OUT1_SLAVE_FROM_ETDMOUT1_SLAVE);
> +	regmap_update_bits(afe->regmap, ETDM_COWORK_CON1,
> +			   EDTM_IN1_SDATA0_SEL,
> +			   EDTM_IN1_SDATA0_FROM_PAD);
> +
> +	switch (rate) {
> +	case 7350:
> +	case 8000:
> +	case 11025:
> +	case 12000:
> +	case 14700:
> +	case 16000:
> +	case 22050:
> +	case 24000:
> +	case 29400:
> +	case 32000:
> +	case 44100:
> +	case 48000:
> +	case 88200:
> +	case 96000:
> +	case 176400:
> +	case 192000:
> +	case 352800:
> +	case 384000:
> +		mtk_dai_etdm_config(afe, params, dai, substream->stream);
> +		return 0;

Again, skipping return code of the main procedure. I'd avoid blind 
'return 0'.

> +	default:
> +		dev_err(afe->dev,
> +			"Sample rate %d invalid\n",
> +			rate);
> +		return -EINVAL;
> +	}
> +}

...

> +static int mtk_dai_etdm_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct an7581_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_dai_etdm_priv *etdm_data;
> +	void *priv_data;
> +
> +	switch (dai->id) {
> +	case AN7581_DAI_ETDM:
> +		break;
> +	default:
> +		dev_warn(afe->dev, "%s(), id %d not support\n",
> +			 __func__, dai->id);

s/support/supported/

Though this entire check looks weird. Why should set_fmt() care about 
DAI's ID? Such check shall be done once, early with the rest of the 
runtime doing just the important stuff.

> +		return -EINVAL;
> +	}
> +
> +	priv_data = devm_kzalloc(afe->dev, sizeof(struct mtk_dai_etdm_priv),
> +				 GFP_KERNEL);
> +	if (!priv_data)
> +		return -ENOMEM;
> +
> +	afe_priv->dai_priv[dai->id] = priv_data;
> +	etdm_data = afe_priv->dai_priv[dai->id];
> +
> +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> +	case SND_SOC_DAIFMT_I2S:
> +		etdm_data->format = MTK_DAI_ETDM_FORMAT_I2S;
> +		break;
> +	case SND_SOC_DAIFMT_DSP_A:
> +		etdm_data->format = MTK_DAI_ETDM_FORMAT_DSPA;
> +		break;
> +	case SND_SOC_DAIFMT_DSP_B:
> +		etdm_data->format = MTK_DAI_ETDM_FORMAT_DSPB;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Please waterfall the points to all the functions/files. I can do another 
round once this batch is addressed or explained first.

Kind regards,
Czarek