[Sound-open-firmware] [PATCH 1/4] [RFC]DMIC: Add PDM microphones (DMIC) support to DAI
Keyon Jie
yang.jie at linux.intel.com
Mon May 7 11:39:37 CEST 2018
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.
Thanks,
~Keyon
>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list