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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b5fd1376-6abb-40c1-b3f0-238e2b0056fa@collabora.com>
Date: Mon, 10 Mar 2025 16:23:15 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: "Darren.Ye" <darren.ye@...iatek.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 01/14] ASoC: mediatek: common: modify mtk afe common
 driver for mt8196

Il 07/03/25 13:47, Darren.Ye ha scritto:
> From: Darren Ye <darren.ye@...iatek.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@...iatek.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