Re: [PATCH v1 2/6] ASoC: codecs: lpass-wsa-macro: Add support to WSA Macro

[Pierre-Louis Bossart]

> +static int wsa_macro_set_prim_interpolator_rate(struct snd_soc_dai *dai,
> +						u8 int_prim_fs_rate_reg_val,
> +						u32 sample_rate)
> +{
> +	u8 int_1_mix1_inp;
> +	u32 j, port;
> +	u16 int_mux_cfg0, int_mux_cfg1;
> +	u16 int_fs_reg;
> +	u8 int_mux_cfg0_val, int_mux_cfg1_val;
> +	u8 inp0_sel, inp1_sel, inp2_sel;
> +	struct snd_soc_component *component = dai->component;
> +	struct wsa_macro *wsa = snd_soc_component_get_drvdata(component);
> +
> +	for_each_set_bit(port, &wsa->active_ch_mask[dai->id],
> +			 WSA_MACRO_RX_MAX) {
> +		int_1_mix1_inp = port;
> +		if ((int_1_mix1_inp < WSA_MACRO_RX0) ||
> +			(int_1_mix1_inp > WSA_MACRO_RX_MIX1)) {

nit-pick: alignment seems off, use one line for the test and for_each loop?

[...]

> +static int wsa_macro_set_mix_interpolator_rate(struct snd_soc_dai *dai,
> +					       u8 int_mix_fs_rate_reg_val,
> +					       u32 sample_rate)
> +{
> +	u8 int_2_inp;
> +	u32 j, port;
> +	u16 int_mux_cfg1, int_fs_reg;
> +	u8 int_mux_cfg1_val;
> +	struct snd_soc_component *component = dai->component;
> +	struct wsa_macro *wsa = snd_soc_component_get_drvdata(component);
> +
> +	for_each_set_bit(port, &wsa->active_ch_mask[dai->id],
> +			 WSA_MACRO_RX_MAX) {
> +		int_2_inp = port;
> +		if ((int_2_inp < WSA_MACRO_RX0) ||
> +			(int_2_inp > WSA_MACRO_RX_MIX1)) {

same weird alignment.

> +static int wsa_macro_hw_params(struct snd_pcm_substream *substream,
> +			       struct snd_pcm_hw_params *params,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	int ret;
> +
> +	switch (substream->stream) {
> +	case SNDRV_PCM_STREAM_PLAYBACK:
> +		ret = wsa_macro_set_interpolator_rate(dai, params_rate(params));
> +		if (ret) {
> +			dev_err(component->dev,
> +				"%s: cannot set sample rate: %u\n",
> +				__func__, params_rate(params));
> +			return ret;
> +		}
> +		break;
> +	case SNDRV_PCM_STREAM_CAPTURE:

nit-pick: might as well remove the capture or add a comment on why it's 
listed but not used?

> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int wsa_macro_get_channel_map(struct snd_soc_dai *dai,
> +				unsigned int *tx_num, unsigned int *tx_slot,
> +				unsigned int *rx_num, unsigned int *rx_slot)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct wsa_macro *wsa = snd_soc_component_get_drvdata(component);
> +	u16 val = 0, mask = 0, cnt = 0, temp = 0;

nit-pick: temp does not need to be initialized.

> +
> +	wsa = dev_get_drvdata(component->dev);
> +	if (!wsa)
> +		return -EINVAL;
> +
> +	switch (dai->id) {
> +	case WSA_MACRO_AIF_VI:
> +		*tx_slot = wsa->active_ch_mask[dai->id];
> +		*tx_num = wsa->active_ch_cnt[dai->id];
> +		break;
> +	case WSA_MACRO_AIF1_PB:
> +	case WSA_MACRO_AIF_MIX1_PB:
> +		for_each_set_bit(temp, &wsa->active_ch_mask[dai->id],
> +					WSA_MACRO_RX_MAX) {
> +			mask |= (1 << temp);
> +			if (++cnt == WSA_MACRO_MAX_DMA_CH_PER_PORT)
> +				break;
> +		}
> +		if (mask & 0x0C)
> +			mask = mask >> 0x2;
> +		*rx_slot = mask;
> +		*rx_num = cnt;
> +		break;
> +	case WSA_MACRO_AIF_ECHO:
> +		val = snd_soc_component_read(component,
> +			CDC_WSA_RX_INP_MUX_RX_MIX_CFG0);
> +		if (val & WSA_MACRO_EC_MIX_TX1_MASK) {
> +			mask |= 0x2;
> +			cnt++;
> +		}
> +		if (val & WSA_MACRO_EC_MIX_TX0_MASK) {
> +			mask |= 0x1;
> +			cnt++;
> +		}
> +		*tx_slot = mask;
> +		*tx_num = cnt;
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: Invalid AIF\n", __func__);
> +		break;
> +	}
> +	return 0;

[...]

> +static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
> +{
> +	struct device *dev = wsa->dev;
> +	struct device_node *np = dev->of_node;
> +	const char *parent_clk_name = NULL;

initialization not needed, overriden below

> +	const char *clk_name = "mclk";
> +	struct clk_hw *hw;
> +	struct clk_init_data init;
> +	int ret;
> +
> +	parent_clk_name = __clk_get_name(wsa->clks[2].clk);
> +
> +	init.name = clk_name;
> +	init.ops = &swclk_gate_ops;
> +	init.flags = 0;
> +	init.parent_names = &parent_clk_name;
> +	init.num_parents = 1;
> +	wsa->hw.init = &init;
> +	hw = &wsa->hw;
> +	ret = clk_hw_register(wsa->dev, hw);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
> +
> +	return NULL;
> +}