Re: [PATCH 7/9] ASoC: tegra: move to generic DMA DT binding

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 07/30/2013 04:12 AM, Richard Zhao wrote:
> On Sat, Jul 27, 2013 at 03:58:21AM +0800, Stephen Warren wrote:
>> On 07/23/2013 10:10 PM, Richard Zhao wrote:
>>> - add tegra_dma_filter_data to specify dma info
>>>   DMA DT binding needs the device that raise dma request and dma name
>>>   to request a dma channel. tegra30_i2s is a special case. It should be ahub
>>>   device and it also has dma name that cannot handled by ASoC dmaengine code.
>>>   So we pass the info using filter data in snd_dmaengine_dai_dma_data.
>>> - change i2s/ac97 drivers to use generic DT binding
>>> - tegra30_i2s: alloc ahub tx/rx fifo at driver probe time
>>
>>> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
>>
>> I don't understand why the FIFO alloc/free had to move from
>> startup()/shutdown() to probe()/remove().
> 
> The call path is:
> [<c0520e38>] (dump_stack+0x78/0xbc) from [<c03bb854>] (tegra_pcm_request_chan+0x10/0x34)
> [<c03bb854>] (tegra_pcm_request_chan+0x10/0x34) from [<c03b6c2c>] (dmaengine_pcm_new+0xe0/0x134)
> [<c03b6c2c>] (dmaengine_pcm_new+0xe0/0x134) from [<c03b5824>] (soc_new_pcm+0x2f0/0x3cc)
> [<c03b5824>] (soc_new_pcm+0x2f0/0x3cc) from [<c03aa1a0>] (soc_probe_link_dais+0x358/0x39c)
> [<c03aa1a0>] (soc_probe_link_dais+0x358/0x39c) from [<c03ab334>] (snd_soc_instantiate_card+0x35c/0x7a0)
> [<c03ab334>] (snd_soc_instantiate_card+0x35c/0x7a0) from [<c03ab9dc>] (snd_soc_register_card+0x264/0x330)
> [<c03ab9dc>] (snd_soc_register_card+0x264/0x330) from [<c03be350>] (tegra_rt5640_probe+0xf4/0x17c)
> [<c03be350>] (tegra_rt5640_probe+0xf4/0x17c) from [<c0294290>] (platform_drv_probe+0x18/0x1c)
> 
> So I need to request dma channel at probe time.

The AHUB driver should look up and cache the DMA channel information
during its probe() call, and defer probe() there if necessary.

The FIFO alloc/free calls should simply use the cached information for
the allocated channel.

That way, the changes are isolated in the AHUB driver and shouldn't
require any changes in the I2S driver.

The DT parsing and DMA request lookup will then happen well before the
snd_soc_instantiate_card() backtrace you mention above, since it will
prevent the card from being instantiated until all the ASoC components
are fully probed.

>> This will prevent later
>> changes from making the connectivity between the AHUB FIFOs and I2S
>> modules more dynamic, which is kind of the whole point of the AHUB HW
>> module.
> 
> That's because it's i2s driver to register pcm device rather not ahub.
> Ideally dma channel is only bound to ahub.

Long-term, this should change. However, I believe if you make the
changes I mention above, this won't be an issue.

>>> diff --git a/sound/soc/tegra/tegra_pcm.h b/sound/soc/tegra/tegra_pcm.h
>>
>>> +#define TEGRA_DMA_NAME_MAX_LEN	10
>>> +
>>> +/*
>>> + * Generic DMA DT binding needs the device that raise dma request and dma name
>>> + * to request a dma channel. tegra30_i2s is a special case. It should be ahub
>>> + * device and it also has dma name that cannot handled by ASoC dmaengine code.
>>> + * So we pass the info using filter data in snd_dmaengine_dai_dma_data.
>>> + */
>>> +struct tegra_dma_filter_data {
>>> +	struct device *dma_dev;
>>> +	char dma_name[TEGRA_DMA_NAME_MAX_LEN];
>>> +};
>>
>> I agree with Lars-Peter's comment that it'd be better to enhance the
>> ASoC dmaengine support to allow the channel name to be specified so we
>> don't need to much custom code.
> 
> Not only dma-name but also dma client device for this case.
> 
>> And that should use "char *" not a
>> fixed-length array.
>
> Ok, then there' two solutions.
> 
> struct tegra30_i2s {
> ...
>         struct snd_dmaengine_dai_dma_data capture_dma_data;
>         char dma_rx_name[10];
>         struct tegra_dma_filter_data filter_data_rx;
> ...
>         struct snd_dmaengine_dai_dma_data playback_dma_data;
>         char dma_tx_name[10];
>         struct tegra_dma_filter_data filter_data_tx;
> };
> 
> probe: i2s->filter_data_rx.dma_name = i2s.dma_rx_name
> 
> or
> 
> probe: i2s->filter_data_rx.dma_name = kasprintf(GFP_KERNEL, "channel%d" ...);
> remove: kfree(i2s->filter_data_rx.dma.name)
> 
> Which one do you like?

Either is fine, although the first option has less dynamic allocation,
so is probably better. The AHUB driver should be constructing the
channel names though.