[alsa-devel] [PATCH] ASoC: davinci-mcasp: Choose PCM driver based on configured DMA controller

[Peter Ujfalusi]

On 06/02/2015 01:40 PM, Jyri Sarha wrote:
>>> +enum {
>>> +    MCASP_EDMA = 1,
>>
>> Why start from 1?
> 
> This is just an implementation detail, for "if" statement below. Maybe I
> should restucture for better readability.

No need to change if it makes the code flow better. I was just asking.

>> I'm not sure about the name. It somehow implies that McASP has something to do
>> with the dma engine.
>> MCASP_SERVICED_BY_E/SDMA
>> PLATFORM_USES_E/SDMA
>>
>> I can not come up with a decent name :(
>>
> 
> What about PCM_EDMA/PCM_SDMA? This is a local definition after all.

Sounds much better!

>>> +    MCASP_SDMA,
>>> +};
>>> +static const char *sdma_prefix = "ti,omap";
>>> +
>>> +static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp)
>>> +{
>>> +    struct dma_chan *chan;
>>> +    const char *tmp;
>>> +    int ret = MCASP_EDMA;
>>> +
>>> +    tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data;
>>> +    chan = dma_request_slave_channel_reason(mcasp->dev, tmp);
>>
>> this will return with error in case of !dev->of_node.
>> The first check should be:
>> if (!mcasp->dev->of_node)
>>     return MCASP_EDMA;
>>
>> since we can only boot in legacy mode with daVinci devices and they are using
>> eDMA.
>>
>>> +    if (IS_ERR(chan)) {
>>> +        if (PTR_ERR(chan) != -EPROBE_DEFER)
>>> +            dev_err(mcasp->dev,
>>> +                "Can't verify DMA configuration (%ld)\n",
>>> +                PTR_ERR(chan));
>>> +        return PTR_ERR(chan);
>>> +    }
>>> +    BUG_ON(!chan->device || !chan->device->dev);
>>> +
>>> +    if (chan->device->dev->of_node)
>>> +        ret = of_property_read_string(chan->device->dev->of_node,
>>> +                          "compatible", &tmp);
>>> +    else
>>> +        dev_dbg(mcasp->dev, "DMA controller has no of-node\n");
>>
>> Now this is not likely to happen...
>>
> 
> Not having of_node happens on am335x, but not having compatible property is
> unlikely...

Yes, you are right. The edma dmaengine driver is not probed via DT, but the
arch/arm/common/edma.c will create a platform device for it to load, so it
does not have of_node.
On the other hand omap-dma is probed via DT.

> 
>>> +
>>> +    dma_release_channel(chan);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    dev_dbg(mcasp->dev, "DMA controller compatible = \"%s\"\n", tmp);
>>> +    if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix)))
>>> +        return MCASP_SDMA;
>>> +
>>> +    return MCASP_EDMA;
>>> +}
>>> +
>>>   static int davinci_mcasp_probe(struct platform_device *pdev)
>>>   {
>>>       struct snd_dmaengine_dai_dma_data *dma_data;
>>> @@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct
>>> platform_device *pdev)
>>>       if (ret != 0)
>>>           goto err;
>>>
>>> -    switch (mcasp->version) {
>>> +    ret = -EINVAL;
>>
>> Why? What was the problem with setting the ret in the default case?
>>
>>> +    switch (davinci_mcasp_get_dma_type(mcasp)) {
>>> +    case MCASP_EDMA:
>>>   #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \
>>>       (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \
>>>        IS_MODULE(CONFIG_SND_EDMA_SOC))
>>> -    case MCASP_VERSION_1:
>>> -    case MCASP_VERSION_2:
>>> -    case MCASP_VERSION_3:
>>>           ret = edma_pcm_platform_register(&pdev->dev);
>>> -        break;
>>> +#else
>>> +        dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");
>>>   #endif
>>> +        break;
>>> +    case MCASP_SDMA:
>>>   #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \
>>>       (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \
>>>        IS_MODULE(CONFIG_SND_OMAP_SOC))
>>> -    case MCASP_VERSION_4:
>>>           ret = omap_pcm_platform_register(&pdev->dev);
>>> -        break;
>>> +#else
>>> +        dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");
>>>   #endif
>>> +        break;
>>>       default:
>>> -        dev_err(&pdev->dev, "Invalid McASP version: %d\n",
>>> -            mcasp->version);
>>> -        ret = -EINVAL;
>>> +        dev_err(&pdev->dev, "No DMA controller found\n");
>>>           break;
>>>       }
>>>
>>>
>>
>>
> 


-- 
Péter