[Sound-open-firmware] [PATCH 01/15] [RFC][v2]DMIC: Add PDM microphones (DMIC) support to DAI

[Seppo Ingalsuo]

On 07.05.2018 14:37, Liam Girdwood wrote:
> On Fri, 2018-05-04 at 18:01 +0300, Seppo Ingalsuo wrote:
>> This patch adds the DMIC audio capture driver for SOF DAI component use.
>> The DMIC feature allows to directly attach one to (typically) four PDM
>> digital microphones into Intel SoC without a separate codec IC. This is
>> supported by APL and most successor platforms.
>>
>> Corresponding patches are needed for kernel driver and topology to enable
>> this feature.
>>
>> Tested in
>> APL UP squared board without connected microphones
>> SOF git master 3ad69eb715a09de9a0b91c56c9cca8a79ead00a9
>>
>> Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo at linux.intel.com>
>> ---
>>   src/drivers/dmic.c | 1383 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 1383 insertions(+)
>>   create mode 100644 src/drivers/dmic.c
>>
>>
> I've applied 2,5,7,8,9,10,11,14 and 15. The rest are good but needs

Great, it was RFC but no objections! It saves me a lot of effort for 
next patch version.

>   
>
>
>> +
>> +#if defined MODULE_TEST
>> +#include <stdio.h>
>> +#define QTOF(x, q) ((float)(x) / ((int64_t)1 << (q)))
>> +#endif
> Does this need to go in a maths header ?

Yep, it will be useful since the new platforms have float capability and 
there will be more float <-> fractional conversions needed when we have 
float pipelines support. Also, this is not optimized at all but can work 
as reference.


>
> +
> +#if DMIC_HW_VERSION == 1
> +       ipm_helper(&ipm, stereo, swap, dmic);
> +       val = OUTCONTROL0_TIE(0) |
> +               OUTCONTROL0_SIP(0) |
> +               OUTCONTROL0_FINIT(1) |
> +               OUTCONTROL0_FCI(0) |
> +               OUTCONTROL0_BFTH(bfth) |
> +               OUTCONTROL0_OF(of0) |
> +               OUTCONTROL0_IPM(ipm) |
> +               OUTCONTROL0_TH(th);
> +       dmic_write(dai, OUTCONTROL0, val);
> +       trace_value(val);
> +
> +       val = OUTCONTROL1_TIE(0) |
> +               OUTCONTROL1_SIP(0) |
> +               OUTCONTROL1_FINIT(1) |
> +               OUTCONTROL1_FCI(0) |
> +               OUTCONTROL1_BFTH(bfth) |
> +               OUTCONTROL1_OF(of1) |
> +               OUTCONTROL1_IPM(ipm) |
> +               OUTCONTROL1_TH(th);
> +       dmic_write(dai, OUTCONTROL1, val);
> +       trace_value(val);
> +#endif
> +
> +#if DMIC_HW_VERSION == 2
> +       source_ipm_helper(source, &ipm, stereo, swap, dmic);
>
>
> Best to use #elif
>
>
>
> +/* start the DMIC for capture */
> +static void dmic_start(struct dai *dai, int direction)
> +{
> +       struct dmic_pdata *dmic = dai_get_drvdata(dai);
> +       int i;
> +       int mic_a;
> +       int mic_b;
> +       int fir_a;
> +       int fir_b;
> +
> +       if (direction != DAI_DIR_CAPTURE)
> +               return;
> +
> +       /* enable port */
> +       spin_lock(&dmic->lock);
> +       trace_dmic("sta");
> +       dmic->state = COMP_STATE_ACTIVE;
>
>
> Regarding the spinlock, was this to be used to guarantee that both mic A and B
> would use a compatible configuration at start ?

I only copied the spinlock practice from SSP so not sure about need. 
Microphones A (left) and B (right) control for each PDM controller 
should be OK since the HW re-samples the signals to not mess up internal 
processing pipeline. Concerning FIFOs A and B this may not be sufficient.

I'd assume the code could work 1. capture via FIFO A. 2. stop capture, 
3. capture via FIFO B so no support for simultaneous or overlapping 
usage. Some next driver version should address that.

What is your proposal?

>   
>
>> +	if (direction != DAI_DIR_CAPTURE) {
>> +		trace_dmic_error("cap");
>> +		return -EINVAL;
>> +	}
>
> You need to do this here :-
>
> 	ret = comp_set_state(dev, cmd);
> 	if (ret < 0)
> 		return ret;
>
> This does all state checking/setting so no need to do it below.

OK.  Keyon is also is checking the same with SSP so I can follow how 
it's done for SSP.


>
>> +
>> +	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);
>> +		break;
>> +	case COMP_TRIGGER_RESUME:
>> +		dmic_context_restore(dai);
>> +		break;
>> +	case COMP_TRIGGER_SUSPEND:
>> +		dmic_context_store(dai);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* TODO: No idea what should be done here. Currently only trace the
>> + * status register that contains a few status and error bit fields.
>> + */
> The intention here is to emit any DMIC errors via trace errors to help with
> debug.

OK, sounds then just removing the TODO comment is enough. When I had DMA 
mis-programmed I saw these DMIC irq events.

Thanks,
Seppo


>
>> +static void dmic_irq_handler(void *data)
>> +{
>> +	struct dai *dai = data;
>> +	uint32_t val;
>> +
>> +	/* Trace OUTSTAT0 register */
>> +	val = dmic_read(dai, OUTSTAT0);
>> +	trace_dmic("irq");
>> +
>> +	if (val & OUTSTAT0_ROR_BIT)
>> +		trace_dmic_error("eor"); /* Full fifo or PDM overrrun */
>> +
>> +	trace_value(dmic_read(dai, OUTSTAT0));
>> +
>> +	/* clear IRQ */
>> +	platform_interrupt_clear(dmic_irq(dai), 1);
>> +}
>>
> Liam
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>