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

[Jyri Sarha <jsarha@ti.com>]

On 06/02/15 13:31, Peter Ujfalusi wrote:
> On 06/02/2015 11:58 AM, Jyri Sarha wrote:
>> Find the configured DMA controller by asking for a DMA channel in the
>> probe phase and releasing it right after. The controller device can be
>> found via the dma_chan struct and the controller is recognized from
>> the compatible property of its device node. The patch assumes EDMA if
>> there is no device node.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>   sound/soc/davinci/davinci-mcasp.c | 61 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 51 insertions(+), 10 deletions(-)
>>
>> diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
>> index d793494..5bc2712 100644
>> --- a/sound/soc/davinci/davinci-mcasp.c
>> +++ b/sound/soc/davinci/davinci-mcasp.c
>> @@ -1565,6 +1565,46 @@ static int davinci_mcasp_init_ch_constraints(struct davinci_mcasp *mcasp)
>>   	return ret;
>>   }
>>
>> +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.

> 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.

>
>> +	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...

>> +
>> +	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;
>>   	}
>>
>>
>
>