RE: [PATCH 4/4] ASoC: cdns: Add Cadence I2S-MC controller driver

[Joakim Zhang (张强庆)]

Hello Chancel,

> -----Original Message-----
> From: Chancel Liu <chancel.liu@xxxxxxxxx>
> Sent: Wednesday, September 23, 2026 1:25 PM
> To: Joakim Zhang (张强庆) <joakim.zhang@xxxxxxxxxxx>; lgirdwood@xxxxxxxxx;
> broonie@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> perex@xxxxxxxx; tiwai@xxxxxxxx; p.zabel@xxxxxxxxxxxxxx
> Cc: cix-kernel-upstream <cix-kernel-upstream@xxxxxxxxxxx>; linux-
> sound@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/4] ASoC: cdns: Add Cadence I2S-MC controller driver

[...]
 
> > +static int cdns_i2s_mc_clks_enable(struct cdns_i2s_mc_priv
> > +*i2s_mc_priv) {
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(i2s_mc_priv->clk_hst);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = clk_prepare_enable(i2s_mc_priv->clk_i2s);
> > +     if (ret)
> > +             clk_disable_unprepare(i2s_mc_priv->clk_hst);
> > +
> > +     return ret;
> > +}
> > +
> > +static void cdns_i2s_mc_clks_disable(struct cdns_i2s_mc_priv
> > +*i2s_mc_priv) {
> > +     clk_disable_unprepare(i2s_mc_priv->clk_hst);
> > +     clk_disable_unprepare(i2s_mc_priv->clk_i2s);
> > +}
> > +
>
> It's better disable the clocks in reverse order of enablement。
OK.

> > +static void cdns_i2s_mc_adjust_pin_config(u8 *pin_mask, u32 slots) {
> > +     u8 mask = 0, num = 0;
> > +     int i;
> > +
> > +     /*
> > +      * Wired-out pins may sit at any index among the 8 data pins, so
> > +      * scan the whole mask and keep the lowest pins until enough slots
> > +      * are covered.
> > +      */
> > +     for (i = 0; i < BITS_PER_BYTE; i++) {
> > +             if (*pin_mask & (0x1 << i)) {
> > +                     mask |= (0x1 << i);
> > +                     if (++num == slots / 2) {
> > +                             *pin_mask = mask;
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +}
> > +
> > +static void cdns_i2s_mc_tx_config(struct cdns_i2s_mc_priv
> > +*i2s_mc_priv, bool on) {
> > +     u32 irq_mask = 0, clk_mask = 0, i2s_mask = 0;
> > +
> > +     irq_mask |= FIELD_PREP(I2S_CID_CTRL_I2S_MASK,
> > +                            i2s_mc_priv->pin_tx_mask_adjust);
> > +
> > +     clk_mask |= FIELD_PREP(I2S_CID_CTRL_I2S_STROBE,
> > +                            i2s_mc_priv->pin_tx_mask_adjust) |
> > +                 I2S_CID_CTRL_STROBE_TS;
> > +
> > +     i2s_mask |= FIELD_PREP(I2S_CTRL_I2S_EN,
> > + i2s_mc_priv->pin_tx_mask_adjust);
> > +
> > +     if (on) {
> > +             /* Transmitter data underrun interrupt unmask */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + irq_mask, irq_mask);
> > +
> > +             /* Transmitter clock enable */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + clk_mask, 0);
> > +
> > +             /*
> > +              * Transmitter enable
> > +              * Transmitter synchronizing unit out of reset
> > +              */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> > +                                i2s_mask | I2S_CTRL_TSYNC_RST,
> > +                                i2s_mask | I2S_CTRL_TSYNC_RST);
> > +     } else {
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> > +                                i2s_mask | I2S_CTRL_TSYNC_RST, 0);
> > +
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + clk_mask, clk_mask);
> > +
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL, irq_mask, 0);
> > +     }
> > +}
> > +
> > +static void cdns_i2s_mc_rx_config(struct cdns_i2s_mc_priv
> > +*i2s_mc_priv, bool on) {
> > +     u32 irq_mask = 0, clk_mask = 0, i2s_mask = 0;
> > +
> > +     irq_mask |= FIELD_PREP(I2S_CID_CTRL_I2S_MASK,
> > +                            i2s_mc_priv->pin_rx_mask_adjust);
> > +
> > +     clk_mask |= FIELD_PREP(I2S_CID_CTRL_I2S_STROBE,
> > +                            i2s_mc_priv->pin_rx_mask_adjust) |
> > +                 I2S_CID_CTRL_STROBE_RS;
> > +
> > +     i2s_mask |= FIELD_PREP(I2S_CTRL_I2S_EN,
> > + i2s_mc_priv->pin_rx_mask_adjust);
> > +
> > +     if (on) {
> > +             /* Receiver data overrun interrupt unmask */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + irq_mask, irq_mask);
> > +
> > +             /* Receiver clock enable */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + clk_mask, 0);
> > +
> > +             /*
> > +              * Receiver enable
> > +              * Receiver synchronizing unit out of reset
> > +              */
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> > +                                i2s_mask | I2S_CTRL_RSYNC_RST,
> > +                                i2s_mask | I2S_CTRL_RSYNC_RST);
> > +     } else {
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CTRL,
> > +                                i2s_mask | I2S_CTRL_RSYNC_RST, 0);
> > +
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL,
> > + clk_mask, clk_mask);
> > +
> > +             regmap_update_bits(i2s_mc_priv->regmap, I2S_CID_CTRL, irq_mask, 0);
> > +     }
> > +}
> > +
> 
> The TX and RX configuration look similar. Perhaps they be factored out into a helper
> to reduce duplication?
Fine.
 
> > +static int cdns_i2s_mc_hw_params(struct snd_pcm_substream *substream,
> > +                              struct snd_pcm_hw_params *params,
> > +                              struct snd_soc_dai *cpu_dai) {
> > +     struct cdns_i2s_mc_priv *i2s_mc_priv = snd_soc_dai_get_drvdata(cpu_dai);
> > +     struct device *dev = i2s_mc_priv->dev;
> > +     u32 rate, sample_rate = 0;
> > +     u32 slots, slot_width, resolution, ctrl;
> > +     unsigned long i2s_clk_rate;
> > +     u8 pin_tx_num, pin_rx_num;
> > +     struct clk *clk_parent;
> > +     bool is_master_mode;
> > +     int ret;
> > +
> > +     rate = params_rate(params);
> > +     slot_width = i2s_mc_priv->devtype_data->data_width;
> 
> Is the slot width fixed by the hardware? If it is configurable, it might be better to
> obtain it through .set_tdm_slot().
It depends on IP config, so adds it to devtype data for each SoC config independently as the drier data.

> > +     if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> > +             is_master_mode = ctrl & I2S_CTRL_T_MS;
> > +     else
> > +             is_master_mode = ctrl & I2S_CTRL_R_MS;
> > +
> 
> The master mode is already known in .set_fmt(). It might be cleaner to store the
> master state in the private data and use it here. This would avoid an unnecessary
> register read.
Both TX and RX can work as master or slave separately, not wanting to introduce two variables, this approach was adopted instead.

> > +static const struct snd_soc_dai_ops cdns_i2s_mc_dai_ops = {
> > +     .probe = cdns_i2s_mc_dai_probe,
> > +     .set_fmt = cdns_i2s_mc_set_fmt,
> > +
> > +     .hw_params = cdns_i2s_mc_hw_params,
> Nit: Remove blank line here.
OK.

Thanks,
Joakim