Mark Brown broonie@sirena.org.uk writes:
On Mon, Nov 17, 2008 at 07:27:59PM -0500, Hugo Villeneuve wrote:
The PCM3008 is used on the Lyrtech SFFSDR board, in conjunction with an FPGA that generates the bit clock and the master clock
Looks fairly good overall.
Signed-off-by: Hugo Villeneuve hugo.villeneuve@lyrtech.com
Normally codec drivers and machine drivers should be submitted as separate patches, though it's not critical.
I agree. In addition, I believe the board-specific init (sound/soc/davinci/davinci-sffsdr.c) should not be under sound/*, but rather belongs in the board init code in arch/arm/mach-davinci/board*.
Kevin
index 5df7402..dc58ce2 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -80,6 +80,10 @@ config SND_SOC_TWL4030 tristate depends on TWL4030_CORE
+config SND_SOC_PCM3008
- tristate
- depends on SFFSDR_FPGA
The codec driver shouldn't depend on the FPGA driver. Once that dependency has been removed the codec should be added to SND_SOC_ALL_CODECS.
+static int pcm3008_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- int fs;
- /* Fsref can be 32000, 44100 or 48000. */
- fs = params_rate(params);
- printk(KERN_INFO "pcm3008_hw_params: rate = %d Hz\n", fs);
- return sffsdr_fpga_set_codec_fs(fs);
+}
This should be being done by the machine driver rather than by the codec driver - this will allow the codec driver to be used on another machine that has some other method for generating the clocks. As far as I can see that's the only dependency on the sffsdr?
- /* DEM1 DEM0 DE-EMPHASIS_MODE
* Low Low De-emphasis 44.1 kHz ON* Low High De-emphasis OFF* High Low De-emphasis 48 kHz ON* High High De-emphasis 32 kHz ON*/Looks like this should be exported as a control? It's not important for merging but it'd be nice.
+static int pcm3008_soc_suspend(struct platform_device *pdev, pm_message_t msg) +{
- struct snd_soc_device *socdev = platform_get_drvdata(pdev);
- struct pcm3008_setup_data *setup = socdev->codec_data;
- printk(KERN_INFO "pcm3008_soc_suspend(): TODO\n");
This should be a comment, though it looks like you may actually have implemented this already?
+struct pcm3008_setup_data {
- u8 dem0_pin;
- u8 dem1_pin;
- u8 pdad_pin;
- u8 pdda_pin;
+};
GPIO numbers should be unsigned to match the gpiolib API (some systems do support an awful lot of GPIOs).
Also, ideally pdad and pdad would be used to implement DAPM controls so that they can be powered down when not in use but again that's not essential for merging - so long as they're passed in someone could do that later.
+config SND_DAVINCI_SOC_SFFSDR
- tristate "SoC Audio support for SFFSDR"
- depends on SND_DAVINCI_SOC && MACH_DAVINCI_SFFSDR
- select SND_DAVINCI_SOC_I2S
- select SND_SOC_PCM3008
- help
Say Y if you want to add support for SoC audio onLyrtech SFFSDR board.This should also select SSFDR_FPGA - select ignores dependencies so the select from the codec driver won't be picked up (though the codec driver ought not to be selecting it in the first place).
--- /dev/null +++ b/sound/soc/davinci/davinci-sffsdr.c @@ -0,0 +1,145 @@ +/*
- ASoC driver for Lyrtech SFFSDR board.
- Author: Vladimir Barinov, vbarinov@ru.mvista.com
- Copyright: (C) 2007 MontaVista Software, Inc., source@mvista.com
Are you sure? Looks like cut'n'paste - MODULE_AUTHOR() claims you wrote this driver. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel