[alsa-devel] [PATCH v2 5/9] ASoC: sti: Add platform driver

[Mark Brown]

On Mon, May 18, 2015 at 02:12:52PM +0200, Arnaud Pouliquen wrote:

> Asoc Platform driver that manages uniperipheral DAIs and associated

ASoC.

> +struct sti_platform_dai {
> +	int stream;
> +	int (*init)(struct platform_device *pdev, struct device_node *node,
> +		    struct uniperif **uni, int idx);
> +	int (*remove)(struct platform_device *pdev);
> +	struct uniperif *uni;
> +	struct snd_dmaengine_dai_dma_data dma_data;
> +};

Again a driver internal abstraction layer...

> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	if (uni->ops->trigger)
> +		return uni->ops->trigger(uni, SNDRV_PCM_TRIGGER_START);

Coding style.

> +	return snd_dmaengine_pcm_prepare_slave_config(substream, params,
> +						     &dma_params->slave_config);

You're using dmaengine but at least one of the earlier patches supplied
a snd_pcm_hardware - the dmaengine code should be able to figure out
constraints from the dmaengine driver in current code, is that causing
you problems?

> +static int sti_pcm_trigger(struct snd_pcm_substream *substream,
> +			   int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct sti_pcm_dma_params *params = runtime->private_data;
> +	enum dma_transfer_direction direction;
> +	struct dma_async_tx_descriptor *desc;
> +	unsigned long flags = DMA_CTRL_ACK;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		direction = snd_pcm_substream_to_dma_direction(substream);
> +
> +		desc = dmaengine_prep_dma_cyclic(
> +				params->dma_channel,	runtime->dma_addr,
> +				snd_pcm_lib_buffer_bytes(substream),
> +				snd_pcm_lib_period_bytes(substream),
> +				direction, flags);

Can you use snd_dmaengine_pcm_trigger?

> +static snd_pcm_uframes_t sti_pcm_pointer(struct snd_pcm_substream *substream)
> +{

snd_dmaengine_pcm_pointer()?

> +static int sti_platform_engine_probe(struct platform_device *pdev)
> +{
> +	/*
> +	 * Driver can not use snd_dmaengine_pcm_register.
> +	 * Reason is that DMA needs to load a firmware.
> +	 * This firmware is loaded on request_channel from file system.
> +	 * So can not be done in probe while alsa card enumerated before
> +	 * file system is mounted
> +	 */

This is not the only driver with similar needs, the Intel drivers use
firmwares as well (all the DMA is done by the DSP which needs firmware)
and even if it were we should be able to arrange for this to work rather
than having to open code things.  That might mean having the dmaengine
driver requesting firmware with _nowait() and then waiting until the
firmware appears before registering as a DMA controller for example.  We
may also be able to have the DMA engine only load the firmware when it's
used rather than at probe (that might allow us to keep the DMA
controller powered off for longer which would be a power win if possible).

It's not just restricted to audio either.

> +static const struct of_device_id snd_soc_sti_match[] = {
> +	{ .compatible = "st,sti-audio-platform", },
> +	{},
> +};

This doesn't seem to represent actual hardware, it seems to be an
abstraction layer between dmaengine and your DAI drivers.  This is fine
and normal but it means the driver shouldn't appear in DT.  Take a look
at how other drivers like Tegra or the Freescale ones handle this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150525/2657f617/attachment.sig>