[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 12:02:54 CEST 2018
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 at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
> _______________________________________________
> 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