On 2018年05月07日 17:39, Keyon Jie wrote:
On 2018年05月02日 21:43, Pierre-Louis Bossart wrote:
+ /* 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.
I think the intension for spin_lock here was to protect and avoid reentering during status transition of the SSP port.
Just got the point after Seppo's explanation, let me do a patch to fix it.
Thanks, ~Keyon
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware