Re: [PATCH] ASoC: nau8821: new driver

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 8/9/2021 12:10 PM, Seven Lee wrote:
> Add driver for NAU88L21.
> 
> Signed-off-by: Seven Lee <wtli@nuvoton.com>
> ---

...

> +
> +static int dmic_clock_control(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *k, int  event)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_dapm_to_component(w->dapm);
> +	struct nau8821 *nau8821 = snd_soc_component_get_drvdata(component);
> +	int i, speed_selection, clk_adc_src, clk_adc;
> +	unsigned int clk_divider_r03;
> +
> +	/* The DMIC clock is gotten from adc clock divided by
> +	 * CLK_DMIC_SRC (1, 2, 4, 8). The clock has to be equal or
> +	 * less than nau8821->dmic_clk_threshold.
> +	 */
> +	regmap_read(nau8821->regmap, NAU8821_R03_CLK_DIVIDER,
> +		&clk_divider_r03);
> +	clk_adc_src = (clk_divider_r03 & NAU8821_CLK_ADC_SRC_MASK)
> +		>> NAU8821_CLK_ADC_SRC_SFT;
> +
> +	switch (clk_adc_src) {
> +	case 0:
> +		clk_adc = nau8821->fs * 256;
> +		break;
> +	case 1:
> +		clk_adc = (nau8821->fs * 256) >> 1;
> +		break;
> +	case 2:
> +		clk_adc = (nau8821->fs * 256) >> 2;
> +		break;
> +	case 3:
> +		clk_adc = (nau8821->fs * 256) >> 3;
> +		break;
> +	}

Just do:
clk_adc = (nau8821->fs * 256) >> clk_adc_src;
instead of whole switch?

> +
> +	for (i = 0 ; i < 4 ; i++) {
> +		if ((clk_adc >> dmic_speed_sel[i].param) <=
> +			nau8821->dmic_clk_threshold) {
> +			speed_selection = dmic_speed_sel[i].val;
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(nau8821->dev,
> +		"clk_adc=%d, dmic_clk_threshold = %d, param=%d, val = %d\n",
> +		clk_adc, nau8821->dmic_clk_threshold,
> +		dmic_speed_sel[i].param, dmic_speed_sel[i].val);
> +	regmap_update_bits(nau8821->regmap, NAU8821_R13_DMIC_CTRL,
> +		NAU8821_DMIC_SRC_MASK,
> +		(speed_selection << NAU8821_DMIC_SRC_SFT));
> +
> +	return 0;
> +}
> +

...

> +
> +static int nau8821_clock_check(struct nau8821 *nau8821,
> +	int stream, int rate, int osr)
> +{
> +	int osrate = 0;
> +
> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		if (osr >= ARRAY_SIZE(osr_dac_sel))
> +			return -EINVAL;
> +		osrate = osr_dac_sel[osr].osr;
> +	} else {
> +		if (osr >= ARRAY_SIZE(osr_adc_sel))
> +			return -EINVAL;
> +		osrate = osr_adc_sel[osr].osr;
> +	}
true and false cases seem to be the same here, you can remove the "if 
else" and just leave one of them.

> +
> +	if (!osrate || rate * osrate > CLK_DA_AD_MAX) {
> +		dev_err(nau8821->dev,
> +		"exceed the maximum frequency of CLK_ADC or CLK_DAC\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

...