Re: [RFC PATCH 1/2] ASoC: loongson: Fix IISConfig0 update in hw_params for rev 1

[Binbin Zhou]

Hi phucduc:

Thanks for your patch.

On Thu, Oct 22, 2026 at 4:46 PM <phucduc.bui@xxxxxxxxx> wrote:
>
> From: bui duc phuc <phucduc.bui@xxxxxxxxx>
>
> On revision 1 controllers, loongson_i2s_hw_params() reads LS_I2S_CFG
> and ORs the new LR_LEN, BCLK_RATIO and RES_DEPTH values into it
> without clearing the old ones.
>
> The first stream after boot is configured correctly, but any later
> stream with a different rate or sample width ends up with a mix of
> old and new bits, so it may run with the wrong rate or sample width.

I tested this patch on a Loongson-2K0300 and Loongson-2K2000 board
(LoongsonASoC sound card).

I ran consecutive playback and capture tests alternating between
S16_LE/44100 Hz and S24_LE/48000 Hz, both in stereo. After applying
this patch, switching between different sample formats and rates, works
correctly.

>
> Use regmap_update_bits() to replace LR_LEN, BCLK_RATIO and the
> RES_DEPTH field for the current direction while preserving the
> other direction's RES_DEPTH field.
>
> Fixes: d84881e06836 ("ASoC: Add support for Loongson I2S controller")
> Signed-off-by: bui duc phuc <phucduc.bui@xxxxxxxxx>
> ---
>
> Compile-tested only, I don't have the hardware.
> A Tested-by from someone with a 2K0300, 2K2000 board would be appreciated.
>
> The field layout follows the Loongson-2K0300 user manual (IISConfig0) [1].
>
> Not sure whether concurrent playback and capture is supported on this controller.
> If it is, note that LR_LEN and BCLK_RATIO are shared, so the stream started later
> would overwrite them when the sample width or channel count differs.
> I can follow up with symmetric_channels and symmetric_sample_bits if that is
> the right fix.
>
> [1] https://github.com/loongson-community/docs/blob/master/2K0300/2025060909243029508.%E9%BE%99%E8%8A%AF2K0300%E5%A4%84%E7%90%86%E5%99%A8%E7%94%A8%E6%88%B7%E6%89%8B%E5%86%8C_V1.01.pdf
>
>  sound/soc/loongson/loongson_i2s.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/sound/soc/loongson/loongson_i2s.c b/sound/soc/loongson/loongson_i2s.c
> index da0d029c028a..109cb61c6ddf 100644
> --- a/sound/soc/loongson/loongson_i2s.c
> +++ b/sound/soc/loongson/loongson_i2s.c
> @@ -69,6 +69,7 @@ static int loongson_i2s_hw_params(struct snd_pcm_substream *substream,
>         u32 fs = params_rate(params);
>         u32 bclk_ratio, mclk_ratio;
>         u32 mclk_ratio_frac;
> +       u32 mask;
>         u32 val = 0;
>
>         switch (i2s->rev_id) {
> @@ -92,14 +93,19 @@ static int loongson_i2s_hw_params(struct snd_pcm_substream *substream,
>                 mclk_ratio_frac = DIV_ROUND_CLOSEST_ULL(((u64)clk_rate << 16),
>                                                     sysclk) - (mclk_ratio << 16);
>
> -               regmap_read(i2s->regmap, LS_I2S_CFG, &val);
> -               val |= (bits << 24);
> -               val |= (bclk_ratio << 8);
> -               if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +               /* Shared fields: LR_LEN [31:24], BCLK_RATIO [15:8] */
> +               mask = GENMASK(31, 24) | GENMASK(15, 8);
> +               val = (bits << 24) | (bclk_ratio << 8);
> +               if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +                       /* TX_RES_DEPTH */
> +                       mask |= GENMASK(23, 16);
>                         val |= (bits << 16);
> -               else
> +               } else {
> +                       /* RX_RES_DEPTH */
> +                       mask |= GENMASK(7, 0);

One minor suggestion: would it be possible to define these bit fields via
macros, or use FIELD_PREP()/FIELD_GET() with GENMASK(), instead of
open-coded shifts and masks? For example:

#define LS_I2S_CFG_LR_LEN_MASK          GENMASK(31, 24)
#define LS_I2S_CFG_TX_RES_DEPTH_MASK    GENMASK(23, 16)
#define LS_I2S_CFG_BCLK_RATIO_MASK      GENMASK(15, 8)
#define LS_I2S_CFG_RX_RES_DEPTH_MASK    GENMASK(7, 0)

>                         val |= bits;
> -               regmap_write(i2s->regmap, LS_I2S_CFG, val);
> +               }
> +               regmap_update_bits(i2s->regmap, LS_I2S_CFG, mask, val);
>
>                 val = (mclk_ratio_frac << 16) | mclk_ratio;
>                 regmap_write(i2s->regmap, LS_I2S_CFG1, val);
> --
> 2.43.0
>
>

-- 
Thanks.
Binbin