Re: [PATCH] ASoC drivers for the Freescale MPC8610 SoC

[Olof Johansson <olof@lixom.net>]

Hi,

This is a fairly substantial driver to get through, but here are some
initial comments on some of the simpler stuff:


On Wed, Dec 19, 2007 at 06:03:09PM -0600, Timur Tabi wrote:
> This patch adds ALSA SoC device drivers for the Freescale MPC8610 SoC
> and the MPC8610-HPCD reference board.

[...]

> diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
> index 6390895..6e1bde3 100644
> --- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
> +++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c
> @@ -34,9 +34,27 @@
>  
>  #include <asm/mpic.h>
>  
> +#include <asm/of_platform.h>
>  #include <sysdev/fsl_pci.h>
>  #include <sysdev/fsl_soc.h>
>  
> +static struct of_device_id mpc8610_ids[] = {
> +	{ .type = "soc", },
> +	{}

Please scan based on compatible instead of device_type.

> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> new file mode 100644
> index 0000000..4a5bbd2
> --- /dev/null
> +++ b/sound/soc/fsl/Kconfig
> @@ -0,0 +1,21 @@
> +menu "ALSA SoC audio for Freescale SOCs"
> +
> +config SND_SOC_MPC8610
> +	bool "ALSA SoC support for the MPC8610 SOC"
> +	depends on SND_SOC #&& MPC8610_HPCD
> +	default y #if MPC8610
> +	help
> +	  Say Y if you want to add support for codecs attached to the SSI
> +          device on an MPC8610.

Don't default configs to 'y'. Also, what's with the commented-out
dependencies and if?

> +config SND_SOC_MPC8610_HPCD
> +	# ALSA SoC support for Freescale MPC8610HPCD
> +	bool "ALSA SoC support for the Freescale MPC8610 HPCD board"
> +	depends on SND_SOC_MPC8610
> +	select SND_SOC_CS4270
> +	select SND_SOC_CS4270_VD33_ERRATA
> +	default y #if MPC8610_HPCD
> +	help
> +	  Say Y if you want to enable audio on the Freescale MPC8610 HPCD.

Same here w.r.t. defaults and dependencies.

> diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c
> new file mode 100644
> index 0000000..6b86be0
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_dma.c
> @@ -0,0 +1,819 @@
> +/*
> + * Freescale DMA ALSA SoC PCM driver
> + *
> + * Author: Timur Tabi <timur@freescale.com>
> + *
> + * Copyright 2007 Freescale Semiconductor, Inc.  This file is licensed under
> + * the terms of the GNU General Public License version 2.  This program
> + * is licensed "as is" without any warranty of any kind, whether express
> + * or implied.
> + *
> + * This driver implements ASoC support for the Elo DMA controller, which is
> + * the DMA controller on Freescale 83xx, 85xx, and 86xx SOCs. In ALSA terms,
> + * the PCM driver is what handles the DMA buffer.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +
> +#include <sound/driver.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +
> +#include <asm/io.h>
> +
> +#include "fsl_dma.h"
> +
> +/*
> + * The formats that the DMA controller supports, which is anything
> + * that is 8, 16, or 32 bits.
> + */
> +#define FSLDMA_PCM_FORMATS (SNDRV_PCM_FMTBIT_S8 	| \
> +			    SNDRV_PCM_FMTBIT_U8 	| \
> +			    SNDRV_PCM_FMTBIT_S16_LE     | \
> +			    SNDRV_PCM_FMTBIT_S16_BE     | \
> +			    SNDRV_PCM_FMTBIT_U16_LE     | \
> +			    SNDRV_PCM_FMTBIT_U16_BE     | \
> +			    SNDRV_PCM_FMTBIT_S24_LE     | \
> +			    SNDRV_PCM_FMTBIT_S24_BE     | \
> +			    SNDRV_PCM_FMTBIT_U24_LE     | \
> +			    SNDRV_PCM_FMTBIT_U24_BE     | \
> +			    SNDRV_PCM_FMTBIT_S32_LE     | \
> +			    SNDRV_PCM_FMTBIT_S32_BE     | \
> +			    SNDRV_PCM_FMTBIT_U32_LE     | \
> +			    SNDRV_PCM_FMTBIT_U32_BE)
> +
> +#define FSLDMA_PCM_RATES (SNDRV_PCM_RATE_5512 | SNDRV_PCM_RATE_8000_192000 | \
> +			  SNDRV_PCM_RATE_CONTINUOUS)
> +
> +/* DMA global data.  This structure is used by fsl_dma_open() to determine
> + * which DMA channels to assign to a substream.  Unfortunately, ASoC V1 does
> + * not allow the machine driver to provide this information to the PCM
> + * driver in advance, and there's no way to differentiate between the two
> + * DMA controllers.  So for now, this driver only supports one SSI device
> + * using two DMA channels.  We cannot support multiple DMA devices.
> + *
> + * ssi_stx_phys: bus address of SSI STX register
> + * ssi_srx_phys: bus address of SSI SRX register
> + * dma_channel: pointer to the DMA channel's registers
> + * irq: IRQ for this DMA channel
> + * assigned: set to 1 if that DMA channel is assigned to a substream
> + */

This goes for the whole patch: You've got good documentation, but it's
not in docbook format. Please reformat it since it should be a pretty
simple thing to do.

> +/*
> + * Initialize this PCM driver.
> + *
> + * This function is called when the codec driver calls snd_soc_new_pcms(),
> + * once for each .dai_link in the machine driver's snd_soc_machine
> + * structure.
> + */
> +static int fsl_dma_new(struct snd_card *card, struct snd_soc_codec_dai *dai,
> +	struct snd_pcm *pcm)
> +{
> +	static u64 fsl_dma_dmamask = 0xffffffff;
> +	int ret;
> +
> +	if (!card->dev->dma_mask)
> +		card->dev->dma_mask = &fsl_dma_dmamask;

I haven't read how your channel allocation works, but providing a
pointer to a local static variable is a bit fishy no matter what.

> +	/* Find the DMA channels to use.  For now, we always use the first DMA
> +	   controller. */
> +	for_each_compatible_node(dma_np, NULL, "fsl,mpc8610-dma") {
> +		iprop = of_get_property(dma_np, "cell-index", NULL);
> +		if (iprop && (*iprop == 0)) {
> +			of_node_put(dma_np);
> +			break;
> +		}
> +	}

Do you ever anticipate having other dma users on the system, such as
memcpy offload? You'll probably need allocation support for channels
when that day comes (I ended up writing a simple library for dma channel
management for that very reason on my platform).


-Olof