Re: [PATCH 05/10] ASoC: soc-dai: add snd_soc_dai_symmetric_get_params()

["Alvin Šipraga" <alvin.sipraga@analog.com>]

Hi Kuninori,

A few more naming ideas below.

On Mon, Sep 14, 2026 at 01:45:14AM +0000, Kuninori Morimoto wrote:
> DAI parameter will be capsuled soon, will be not enable to access from
> each drivers. DAI symmetric_xxx has been mainly referred from Mediatek
> drivers. Let's add snd_soc_dai_symmetric_get_params() for it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/soc-dai.h                     |  4 ++++
>  sound/soc/mediatek/mt8188/mt8188-dai-pcm.c  |  5 ++++-
>  sound/soc/mediatek/mt8195/mt8195-dai-pcm.c  |  5 ++++-
>  sound/soc/mediatek/mt8365/mt8365-dai-dmic.c | 10 +++++++---
>  sound/soc/mediatek/mt8365/mt8365-dai-pcm.c  |  4 +++-
>  sound/soc/soc-dai.c                         | 14 ++++++++++++++
>  sound/soc/soc-pcm.c                         |  5 ++++-
>  7 files changed, 40 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index bd714c3bbf3df..c5544f1789425 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -563,6 +563,10 @@ struct snd_soc_dai *snd_soc_dai_register(struct snd_soc_component *component,
>  void snd_soc_dai_unregister(struct snd_soc_dai *dai);
>  struct snd_soc_dai *snd_soc_dai_from_list(struct list_head *list);
>  struct list_head *snd_soc_dai_to_list(struct snd_soc_dai *dai);
> +void snd_soc_dai_symmetric_get_params(struct snd_soc_dai *dai,
> +				      unsigned int *symmetric_rate,
> +				      unsigned int *symmetric_channels,
> +				      unsigned int *symmetric_sample_bits);

Why not split into 3 functions:

snd_soc_dai_get_symmetric_rate()
snd_soc_dai_get_symmetric_channels()
snd_soc_dai_get_symmetric_sample_bits()

Or if not, similar to my comment in your other patch, consider
snd_soc_dai_get_symmetric_params() instead. I think it'd be more
consistent with the rest of the API.

>  
>  /* REMOVE ME */
>  #define snd_soc_dai_get_pcm_stream			snd_soc_dai_pcm_stream_get
> diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-pcm.c b/sound/soc/mediatek/mt8188/mt8188-dai-pcm.c
> index 8ca7cc75e21dc..766893ca56d45 100644
> --- a/sound/soc/mediatek/mt8188/mt8188-dai-pcm.c
> +++ b/sound/soc/mediatek/mt8188/mt8188-dai-pcm.c
> @@ -128,7 +128,7 @@ static int mtk_dai_pcm_configure(struct snd_pcm_substream *substream,
>  	unsigned int lrck_inv;
>  	unsigned int bck_inv;
>  	unsigned int fmt;
> -	unsigned int bit_width = dai->symmetric_sample_bits;
> +	unsigned int bit_width;

Alternative:

	unsigned int bit_width = snd_soc_dai_get_symmetric_sample_bits(dai);

>  	unsigned int val = 0;
>  	unsigned int mask = 0;
>  	int fs = 0;
> @@ -137,7 +137,10 @@ static int mtk_dai_pcm_configure(struct snd_pcm_substream *substream,
>  	if (dai->id < 0)
>  		return -EINVAL;
>  
> +	snd_soc_dai_symmetric_get_params(dai, NULL, NULL, &bit_width);
> +
>  	pcmif_priv = afe_priv->dai_priv[dai->id];
> +
>  	slave_mode = pcmif_priv->slave_mode;
>  	lrck_inv = pcmif_priv->lrck_inv;
>  	bck_inv = pcmif_priv->bck_inv;
> diff --git a/sound/soc/mediatek/mt8195/mt8195-dai-pcm.c b/sound/soc/mediatek/mt8195/mt8195-dai-pcm.c
> index cdc16057d50e2..a090995ac9a7f 100644
> --- a/sound/soc/mediatek/mt8195/mt8195-dai-pcm.c
> +++ b/sound/soc/mediatek/mt8195/mt8195-dai-pcm.c
> @@ -127,7 +127,7 @@ static int mtk_dai_pcm_configure(struct snd_pcm_substream *substream,
>  	unsigned int lrck_inv;
>  	unsigned int bck_inv;
>  	unsigned int fmt;
> -	unsigned int bit_width = dai->symmetric_sample_bits;
> +	unsigned int bit_width;

Ditto:

	unsigned int bit_width = snd_soc_dai_get_symmetric_sample_bits(dai);

>  	unsigned int val = 0;
>  	unsigned int mask = 0;
>  	int fs = 0;
> @@ -136,7 +136,10 @@ static int mtk_dai_pcm_configure(struct snd_pcm_substream *substream,
>  	if (dai->id != MT8195_AFE_IO_PCM)
>  		return -EINVAL;
>  
> +	snd_soc_dai_symmetric_get_params(dai, NULL, NULL, &bit_width);
> +
>  	pcmif_priv = afe_priv->dai_priv[dai->id];
> +
>  	slave_mode = pcmif_priv->slave_mode;
>  	lrck_inv = pcmif_priv->lrck_inv;
>  	bck_inv = pcmif_priv->bck_inv;
> diff --git a/sound/soc/mediatek/mt8365/mt8365-dai-dmic.c b/sound/soc/mediatek/mt8365/mt8365-dai-dmic.c
> index 0bac143b48bfb..6e1d48f757d22 100644
> --- a/sound/soc/mediatek/mt8365/mt8365-dai-dmic.c
> +++ b/sound/soc/mediatek/mt8365/mt8365-dai-dmic.c
> @@ -118,13 +118,17 @@ static int mt8365_dai_configure_dmic(struct mtk_base_afe *afe,
>  	unsigned int clk_phase_sel_ch1 = dmic_data->clk_phase_sel_ch1;
>  	unsigned int clk_phase_sel_ch2 = dmic_data->clk_phase_sel_ch2;
>  	unsigned int val = 0;
> -	unsigned int rate = dai->symmetric_rate;
> -	int reg = get_chan_reg(dai->symmetric_channels);
> +	unsigned int rate;
> +	unsigned int channel;

Alternative:

	unsigned int rate = snd_soc_dai_get_symmetric_rate(dai);
	unsigned int channels = snd_soc_dai_get_symmetric_channels(dai);
	int reg = get_chan_reg(channels);

you get the idea.

[...]

> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index a446e9c1c9887..aaba9ca724e61 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -453,6 +453,8 @@ static int soc_pcm_shared_bclk_rule_rate(struct snd_pcm_hw_params *params,
>  	/* Scan all DAIs on the card for an active peer sharing the same BCLK */
>  	for_each_card_rtds(card, rtd) {
>  		for_each_rtd_cpu_dais(rtd, i, other_dai) {
> +			unsigned int symmetric_rate;
> +
>  			if (other_dai == dai)
>  				continue;
>  			if (!other_dai->bclk)
> @@ -465,7 +467,8 @@ static int soc_pcm_shared_bclk_rule_rate(struct snd_pcm_hw_params *params,
>  			 * after snd_soc_dai_hw_params(), so non-zero means
>  			 * the DAI's clk_set_rate() has already executed.
>  			 */
> -			if (!other_dai->symmetric_rate)
> +			snd_soc_dai_symmetric_get_params(other_dai, &symmetric_rate, NULL, NULL);
> +			if (!symmetric_rate)
>  				continue;

 for_each_rtd_cpu_dais(rtd, i, other_dai) {
   ...

   if (!snd_soc_dai_get_symmetric_rate(other_dai))
     continue;
 }

It's a bit more explicit, and avoids the NULL, NULL stuff.

Kind regards,
Alvin