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

[Seppo Ingalsuo]

On 02.05.2018 16:43, Pierre-Louis Bossart wrote:
>>>> +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.

Your comments would be useful. This is an open that I didn't yet address 
in RFC v2 patch.

Thanks,
Seppo