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

[Chancel Liu <chancel.liu@gmail.com>]

> +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。

> +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?

> +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().

> +	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.
> +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.

Regards,
Chancel Liu