Re: [PATCH] ASoC: nau8821: new driver

[John Hsu <supercraig0719@gmail.com>]

Hi,

On 8/9/2021 6:44 PM, Amadeusz Sławiński wrote:
> 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?
>

Yes, you are right. I will fix it.
>> +
>> +    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.
>

The OSR of DAC and ADC can be different. And the ratio corresponding
to the bit is also different. They are two different tables for
osr_dac_sel and osr_adc_sel. Then it should be noted that the OSR and
Fs must be selected carefully so that the max frequency of CLK_ADC or
CLK_DAC are less than or equal to 6.144MHz.
>> +
>> +    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;
>> +}
>> +
>
> ...