Re: [PATCH] ARM DaVinci: Add PCM3008 ALSA SoC driver for the Lyrtech SFFSDR board

[Kevin Hilman <khilman@deeprootsystems.com>]

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 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@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