Re: [alsa-devel] [PATCH] ASoC Platform Driver for AT32AP7000 (AVR32)

On Tue, May 27, 2008 at 05:32:00PM -0500, Geoffrey Wossum wrote:

Add ASoC platform driver for AT32AP7000 (AVR32). This is basically a port of the at91 ASoC platform code to the

Thanks - I've added this to the ASoC git tree.

The driver looks good but there are quite a few issues that should be fixed up before submission to mainline but these are coding style issues rather than anything substantial - most of them can be picked up by running the script scripts/checkpatch.pl in the kernel source against your patch (it also has a --file argument which can be used to run it on regular files rather than patches).

One thing that's missing is the build system updates in sound/soc to hook this in.

AVR32. This is currently against the Linux 2.6.24.3.atmel.3 kernel, the latest kernel Atmel has released for the AVR32.

Will this driver work with a current mainline kernel or does it require other things from the Atmel kernel?

playpaq_wm8510.c isn't necessarily intended for inclusion into the mainline kernel or linux-2.6-asoc repository. It's provided more as a reference machine driver for an AT32AP7000 system.

It's generally useful to have reference machine drivers - apart from anything else, it helps from the point of view of doing build tests even if you don't have the hardware to actually run them.

+#define ssc_readx(base, reg) (__raw_readl((base) + (reg))) +#define ssc_writex(base, reg, value) __raw_writel((value), (base) + (reg))

It might be a bit better to provide these as inline

+#include <sound/driver.h>

driver.h shouldn't be required with current ALSA versions, though I can't remember if it's needed on the older kernel you're running with.

+/*

• • SSC register values that Atmel left out of <linux/atmel-ssc.h>. These
• • are expected to be used with SSC_BF
• */

+/* START bit field values */ +#define SSC_START_CONTINUOUS 0 +#define SSC_START_TX_RX 1 +#define SSC_START_LOW_RF 2 +#define SSC_START_HIGH_RF 3 +#define SSC_START_FALLING_RF 4 +#define SSC_START_RISING_RF 5 +#define SSC_START_LEVEL_RF 6 +#define SSC_START_EDGE_RF 7 +#define SSS_START_COMPARE_0 8

I guess ideally these should be merged in there? Shouldn't be a blocker for merging this, though.

+config SND_AT32_SOC_PLAYPAQ

•    tristate "SoC Audio support for PlayPaq with WM8510"
  

•    depends on SND_AT32_SOC
  

•    select SND_AT32_SOC_SSC
  

•    select SND_SOC_WM8510
  

•    help
  

•      Say Y or M here if you want to add support for SoC audio
  

•      on the LRS PlayPaq.
  

Should this not also depend on having base support for the system?

+config SND_AT32_SOC_PLAYPAQ_SLAVE

•    bool "Run CODEC on PlayPaq in slave mode"
  

•    depends on SND_AT32_SOC_PLAYPAQ
  

•    default n
  

•    help
  

•      Say Y if you want to run with the AT32 SSC generating the BCLK
  

•      and FRAME signals on the PlayPaq
  

Probably best to include a note here explaining why you don't want to select this unless you want to test the AT32 clock generation or something?

+static int playpaq_wm8510_startup(

• struct snd_pcm_substream *substream)

+{

• return 0;

+}

You shouldn't need to provide empty functions for things like this.

• /* always connected endpoints */
• snd_soc_dapm_set_endpoint(codec, "Int Mic", 1);
• snd_soc_dapm_set_endpoint(codec, "Ext Spk", 1);

It's also a good idea to explicitly disconnect any unused connections on the codec, although it's not in any way essential to do so.