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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
