[Sound-open-firmware] [PATCH 1/4] [RFC]DMIC: Add PDM microphones (DMIC) support to DAI

[Pierre-Louis Bossart]

>>> +    /* Loop possible clock dividers and check based on resulting
>>> +     * oversampling ratio that CIC and FIR decimation ratios are
>>> +     * feasible. The ratios need to be integers. Also the mic clock
>>> +     * duty cycle need to be within limits.
>>> +     */
>>> +    for (clkdiv = clkdiv_min; clkdiv <= clkdiv_max; clkdiv++) {
>>> +        c1 = clkdiv >> 1;
>>> +        c2 = clkdiv - c1;
>>> +        du_min = 100 * c1 / clkdiv;
>>
>> du_min = 100 * (clkdiv/2)/clkdiv  -> 50?
>>
>>> +        du_max = 100 * c2 / clkdiv;
>>
>> c2 = clkdiv - c1 = clkdiv/2, so du_min == du_max ?
>> what am I missing?
> 
> The duty cycle becomes non-50% when the divider is odd, e.g. clkdiv = 5 
> case:
> 
> c1 = clkdiv >> 1 = 2
> c2 = clkdiv - c1 = 5 - 2 = 3
> du_min = 100*c1/clkdiv = 100*2/5 = 200/5 = 40
> du_max = 100*c2/clkdif = 100*3/5 = 300/5 = 60
> 
> The latter equation can be though simplified to "c2 = 100 - c1;" that I 
> didn't realize earlier.

And you may want to add a comment that the ratio is not 50% for odd 
dividers, I missed the non-linear result.


>>> +    /* Check for request only for FIFO A or B. In such case pass 
>>> list for
>>> +     * A or B as such.
>>> +     */
>>> +    if (b->num_of_modes == 0) {
>>> +        c->num_of_modes = a->num_of_modes;
>>> +        for (i = 0; i < a->num_of_modes; i++) {
>>> +            c->clkdiv[i] = a->clkdiv[i];
>>> +            c->mcic[i] = a->mcic[i];
>>> +            c->mfir_a[i] = a->mfir[i];
>>> +            c->mfir_b[i] = 0;
>>
>> is this initialization to zero needed?
> 
> Yes, It indicates the FIFO B path won't be used.

yes, but you are only using the first element as an indication. you want 
to add a comment here to help the reader follow the flow.

> 
>>
>>> +        }
>>> +        return;
>>> +    }
>>> +
>>> +    if (a->num_of_modes == 0) {
>>> +        c->num_of_modes = b->num_of_modes;
>>> +        for (i = 0; i < b->num_of_modes; i++) {
>>> +            c->clkdiv[i] = b->clkdiv[i];
>>> +            c->mcic[i] = b->mcic[i];
>>> +            c->mfir_b[i] = b->mfir[i];
>>> +            c->mfir_a[i] = 0;
>>
>> same here, is this initialization needed?
> 
> And vice versa no FIFO A path used.

same here.


>>> +
>>> +    shift = 0;
>>> +    while ((new_scale << shift) < (1 << 20))
>>> +        shift++;
>>
>> don't we have some sort of macro for this (find number of leading 
>> zeroes)?
> 
> Not yet I think but such would be useful. I'll add it to math/numbers. 
> Since DSPs have an instruction for that it would be then easier to 
> optimize into single location.

ok

>>> +    /* Calculate CIC shift from the decimation factor specific gain.
>>> +     */
>>> +    mcic = cfg->mcic;
>>> +    g_cic = mcic * mcic * mcic * mcic * mcic;
>>
>> yikes, what is this mcic^5 ?
> 
> Yep, it's the gain for +1 / -1 PDM bits in CIC decimator part. It's 
> needed to set the scaling HW shifter. It also gives some idea of 
> oversampling ratios to use for a microphone to get some # of bits of 
> resolution as PCM code.

I meant: where does this ^5 come from? First time i see this sort of 
formula in filter design.


>>> +    for (i = 0; i < dmic->number_of_pdm_controllers; i++) {
>>> +        if (dmic->pdm[i].enable_mic_a > 0 ||
>>> +            dmic->pdm[i].enable_mic_b > 0)
>>> +            pdm[i] = 1;
>>> +        else
>>> +            pdm[i] = 0;
>>> +
>>> +        if (dmic->pdm[i].enable_mic_a > 0 &&
>>> +            dmic->pdm[i].enable_mic_b > 0) {
>>> +            stereo[i] = 1;
>>> +            swap[i] = 0;
>>> +        } else {
>>> +            stereo[i] = 0;
>>> +            if (dmic->pdm[i].enable_mic_a == 0)
>>> +                swap[i] = 1;
>>> +            else
>>> +                swap[i] = 0;
>>> +        }
>>
>> the logic is weird here.
>>
>> if (a || b)
>>     ...
>> else
>>     ...
>>
>> if (a && b)
>>     ...
>> else
>>     // this could be a || b or !a && !b??
> 
> I'm not sure this if spaghetti can be simplified. If the PDM bits 
> processing is not stereo the mono-left mic (a) HW mode is used. If the 
> the right mic (b) needs to be used as mono input then need to swap the 
> PDM bit inputs to process it as mono-left.
> 
> The FIFO packer packer has a mono mode so the use or left channel 
> processing for right microphone is only a HW internal thing.

may something like:

cnt=0;
for (i = 0; i < dmic->number_of_pdm_controllers; i++) {
         if (dmic->pdm[i].enable_mic_a > 0)
		cnt++;
	if (dmic->pdm[i].enable_mic_b > 0)
		cnt++;

         pdm[i] = !!cnt;
	cnt >>= 1;
	stereo[i] = cnt;
	swap[i] = dmic->pdm[i].enable_mic_b & !cnt;
}

>>> +        /* Mono right channel mic usage requires swap of PDM channels
>>> +         * since the mono decimation is done with only left channel
>>> +         * processing active.
>>> +         */
>>> +        edge = dmic->pdm[i].clk_edge;
>>> +        if (swap[i])
>>> +            edge = edge ^ 1;
>>
>> ! edge?
> 
> If C standard has it defined that !0 gives 1 and !1 gives 0 it's OK to 
> change. I used bitwise XOR to make it sure.

yes, that's ok. The xor is not usually not used for this.


>>> +static int dmic_trigger(struct dai *dai, int cmd, int direction)
>>> +{
>>> +    struct dmic_pdata *dmic = dai_get_drvdata(dai);
>>> +
>>> +    trace_dmic("tri");
>>> +
>>> +    /* dai private is set in dmic_probe(), error if not set */
>>> +    if (!dmic) {
>>> +        trace_dmic_error("trn");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (direction != DAI_DIR_CAPTURE) {
>>> +        trace_dmic_error("cap");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    switch (cmd) {
>>> +    case COMP_TRIGGER_START:
>>> +        if (dmic->state == COMP_STATE_PREPARE ||
>>> +            dmic->state == COMP_STATE_PAUSED) {
>>> +            dmic_start(dai, direction);
>>> +        } else {
>>> +            trace_dmic_error("cst");
>>> +            trace_value(dmic->state);
>>> +        }
>>> +        break;
>>> +    case COMP_TRIGGER_RELEASE:
>>> +        if (dmic->state == COMP_STATE_PREPARE) {
>>> +            dmic_start(dai, direction);
>>> +        } else {
>>> +            trace_dmic_error("crl");
>>> +            trace_value(dmic->state);
>>> +        }
>>> +        break;
>>> +    case COMP_TRIGGER_STOP:
>>> +    case COMP_TRIGGER_PAUSE:
>>> +        dmic->state = COMP_STATE_PAUSED;
>>> +        dmic_stop(dai);
>>
>> shouldn't the state assignment be protected by a spinlock (as done for 
>> the start case)?
>> Also is this really PAUSED?
> 
> I followed in this part the ssp driver. I have no plans to implement 
> anything like RAM buffer for recording pause so I don't know what should 
> be done with it. Could COMP_TRIGGER_PAUSE be removed and return error if 
> attempted?

Liam or Keyon, can you look at this? The spinlock usage is odd and I 
can't remember why we would use PAUSE even for SSP.