Re: [PATCH] ASoC: xilinx: formatter_pcm: Fix read-modify-write of AUD_CTRL in hw_params

["Gaddipati, Naveen Kumar" <naveenga@amd.com>]

On 10/23/2026 3:58 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> xlnx_formatter_pcm_hw_params() does a read-modify-write on XLNX_AUD_CTRL
> but only ORs in the new data width and active channel values without
> clearing the old ones. If hw_params is called more than once on an
> open stream, these fields may retain bits from the previous configuration,
> resulting in an incorrect sample width or channel count.
> 
> Clear both fields before writing the new values, and use FIELD_PREP() so
> the values cannot spill into neighbouring bits.
> 
> Fixes: 6f6c3c36f091 ("ASoC: xlnx: add pcm formatter platform driver")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
> 
> Compile-tested only.
> 
> The field masks follow the Control register layout in the Audio Formatter product guide (PG330).
> https://docs.amd.com/r/en-US/pg330-audio-formatter/S2MM-Control-Register-0x10
> 
>   sound/soc/xilinx/xlnx_formatter_pcm.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/xilinx/xlnx_formatter_pcm.c b/sound/soc/xilinx/xlnx_formatter_pcm.c
> index 304220d9aaf5..781508aef885 100644
> --- a/sound/soc/xilinx/xlnx_formatter_pcm.c
> +++ b/sound/soc/xilinx/xlnx_formatter_pcm.c
> @@ -6,6 +6,7 @@
>   //
>   // Author: Maruthi Srinivas Bayyavarapu <maruthis@xilinx.com>
>   
> +#include <linux/bitfield.h>
>   #include <linux/clk.h>
>   #include <linux/io.h>
>   #include <linux/module.h>
> @@ -55,8 +56,8 @@
>   #define CFG_S2MM_XFER_SHIFT	29
>   #define CFG_S2MM_PKG_MASK	BIT(28)
>   
> -#define AUD_CTRL_DATA_WIDTH_SHIFT	16
> -#define AUD_CTRL_ACTIVE_CH_SHIFT	19
> +#define AUD_CTRL_DATA_WIDTH_MASK	GENMASK(18, 16)
> +#define AUD_CTRL_ACTIVE_CH_MASK		GENMASK(22, 19)
>   #define PERIOD_CFG_PERIODS_SHIFT	16
>   
>   #define PERIODS_MIN		2
> @@ -506,28 +507,29 @@ static int xlnx_formatter_pcm_hw_params(struct snd_soc_component *component,
>   	writel(high, stream_data->mmio + XLNX_AUD_BUFF_ADDR_MSB);
>   
>   	val = readl(stream_data->mmio + XLNX_AUD_CTRL);
> +	val &= ~(AUD_CTRL_DATA_WIDTH_MASK | AUD_CTRL_ACTIVE_CH_MASK);
>   	bits_per_sample = params_width(params);
>   	switch (bits_per_sample) {
>   	case 8:
> -		val |= (BIT_DEPTH_8 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		val |= FIELD_PREP(AUD_CTRL_DATA_WIDTH_MASK, BIT_DEPTH_8);
>   		break;
>   	case 16:
> -		val |= (BIT_DEPTH_16 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		val |= FIELD_PREP(AUD_CTRL_DATA_WIDTH_MASK, BIT_DEPTH_16);
>   		break;
>   	case 20:
> -		val |= (BIT_DEPTH_20 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		val |= FIELD_PREP(AUD_CTRL_DATA_WIDTH_MASK, BIT_DEPTH_20);
>   		break;
>   	case 24:
> -		val |= (BIT_DEPTH_24 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		val |= FIELD_PREP(AUD_CTRL_DATA_WIDTH_MASK, BIT_DEPTH_24);
>   		break;
>   	case 32:
> -		val |= (BIT_DEPTH_32 << AUD_CTRL_DATA_WIDTH_SHIFT);
> +		val |= FIELD_PREP(AUD_CTRL_DATA_WIDTH_MASK, BIT_DEPTH_32);
>   		break;
>   	default:
>   		return -EINVAL;
>   	}
>   
> -	val |= active_ch << AUD_CTRL_ACTIVE_CH_SHIFT;
> +	val |= FIELD_PREP(AUD_CTRL_ACTIVE_CH_MASK, active_ch);
>   	writel(val, stream_data->mmio + XLNX_AUD_CTRL);
>   
>   	val = (params_periods(params) << PERIOD_CFG_PERIODS_SHIFT)

Reviewed-by: Gaddipati, Naveen <naveen.gaddipati@amd.com>

Thanks,
Naveen