Re: [PATCH 01/14] ASoC: mediatek: common: modify mtk afe common driver for mt8196

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 07/03/25 13:47, Darren.Ye ha scritto:
> From: Darren Ye <darren.ye@mediatek.com>
> 
> Export register read and write interface, add sample reate interface, and
> update the mtk_memif_set_channel interface for the mt8196 platform.
> 
> Signed-off-by: Darren Ye <darren.ye@mediatek.com>
> ---
>   sound/soc/mediatek/common/mtk-afe-fe-dai.c | 30 ++++++++++++++--------
>   sound/soc/mediatek/common/mtk-afe-fe-dai.h |  6 +++++
>   sound/soc/mediatek/common/mtk-base-afe.h   | 13 ++++++++++
>   3 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/mediatek/common/mtk-afe-fe-dai.c b/sound/soc/mediatek/common/mtk-afe-fe-dai.c
> index 3809068f5620..c36dae520f04 100644
> --- a/sound/soc/mediatek/common/mtk-afe-fe-dai.c
> +++ b/sound/soc/mediatek/common/mtk-afe-fe-dai.c
> @@ -18,7 +18,7 @@
>   
>   #define AFE_BASE_END_OFFSET 8
>   
> -static int mtk_regmap_update_bits(struct regmap *map, int reg,
> +int mtk_regmap_update_bits(struct regmap *map, int reg,
>   			   unsigned int mask,
>   			   unsigned int val, int shift)
>   {
> @@ -26,13 +26,16 @@ static int mtk_regmap_update_bits(struct regmap *map, int reg,
>   		return 0;
>   	return regmap_update_bits(map, reg, mask << shift, val << shift);
>   }
> +EXPORT_SYMBOL(mtk_regmap_update_bits);

Please don't export this function: it's not the greatest, `reg` should be unsigned
and so it should the shift - it's kinda wrong in principle, and you can simply use
regmap_update_bits() directly in your drivers. Please, do so.

> +
> +int mtk_regmap_write(struct regmap *map, int reg, unsigned int val)
>   
> -static int mtk_regmap_write(struct regmap *map, int reg, unsigned int val)
>   {
>   	if (reg < 0)
>   		return 0;
>   	return regmap_write(map, reg, val);
>   }
> +EXPORT_SYMBOL(mtk_regmap_write);
>   

Same here

>   int mtk_afe_fe_startup(struct snd_pcm_substream *substream,
>   		       struct snd_soc_dai *dai)
> @@ -459,8 +462,12 @@ int mtk_memif_set_channel(struct mtk_base_afe *afe,
>   	struct mtk_base_afe_memif *memif = &afe->memif[id];
>   	unsigned int mono;
>   
> -	if (memif->data->mono_shift < 0)
> -		return 0;
> +	dev_info(afe->dev, "%s(), id: %d, channel: %d\n", __func__, id, channel);

If you really really really need this print, it's for debugging, so this should be
dev_dbg() instead... but otherwise just delete it.

> +	mono = memif->data->mono_invert ^ (channel == 1);
> +
> +	if (memif->data->mono_shift > 0)
> +		mtk_regmap_update_bits(afe->regmap, memif->data->mono_reg,
> +				       0x1, mono, memif->data->mono_shift);
>   
>   	if (memif->data->quad_ch_mask) {
>   		unsigned int quad_ch = (channel == 4) ? 1 : 0;
> @@ -470,11 +477,6 @@ int mtk_memif_set_channel(struct mtk_base_afe *afe,
>   				       quad_ch, memif->data->quad_ch_shift);
>   	}
>   
> -	if (memif->data->mono_invert)
> -		mono = (channel == 1) ? 0 : 1;
> -	else
> -		mono = (channel == 1) ? 1 : 0;
> -
>   	/* for specific configuration of memif mono mode */
>   	if (memif->data->int_odd_flag_reg)
>   		mtk_regmap_update_bits(afe->regmap,
> @@ -482,8 +484,14 @@ int mtk_memif_set_channel(struct mtk_base_afe *afe,
>   				       1, mono,
>   				       memif->data->int_odd_flag_shift);
>   
> -	return mtk_regmap_update_bits(afe->regmap, memif->data->mono_reg,
> -				      1, mono, memif->data->mono_shift);

Don't break older platforms. You're removing functionality here.

> +	if (memif->data->ch_num_maskbit) {
> +		dev_info(afe->dev, "%s(), set ch num id: %d, channel: %d\n", __func__, id, channel);

Same comment applies (plus, this is a double print, even worse).

> +		mtk_regmap_update_bits(afe->regmap, memif->data->ch_num_reg,
> +				       memif->data->ch_num_maskbit,
> +				       channel, memif->data->ch_num_shift);
> +	}
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL_GPL(mtk_memif_set_channel);
>   
> diff --git a/sound/soc/mediatek/common/mtk-afe-fe-dai.h b/sound/soc/mediatek/common/mtk-afe-fe-dai.h
> index b6d0f2b27e86..64b10ccba291 100644
> --- a/sound/soc/mediatek/common/mtk-afe-fe-dai.h
> +++ b/sound/soc/mediatek/common/mtk-afe-fe-dai.h
> @@ -12,7 +12,13 @@
>   struct snd_soc_dai_ops;
>   struct mtk_base_afe;
>   struct mtk_base_afe_memif;
> +struct mtk_base_irq_data;
>   
> +int mtk_regmap_update_bits(struct regmap *map, int reg,
> +			   unsigned int mask, unsigned int val,
> +			   int shift);
> +int mtk_regmap_write(struct regmap *map, int reg,
> +		     unsigned int val);

No, don't export these. Use regmap directly.

Regards,
Angelo