Mark Brown <broonie@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> 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 on
>> + Lyrtech 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@xxxxxxxxxxxxx>
>> + * Copyright: (C) 2007 MontaVista Software, Inc., <source@xxxxxxxxxx>
>
> Are you sure? Looks like cut'n'paste - MODULE_AUTHOR() claims you wrote
> this driver.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
