Re: [PATCH] ASoC: nau8821: new driver

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

On 8/18/2021 5:30 AM, John Hsu wrote:
> 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.

Right, I somehow did misread dac and adc, and thought that they are the 
same table.

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