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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
