Re: [alsa-devel] ALSA:AT91SAM9G20: Add support(DAI) for wolfson8731

On Wed, Sep 17, 2008 at 11:51:09AM +0200, Sedji Gaouaou wrote:

So here is a patch to add support for the wolfson 8731 on the at91sam9g20 evaluation board. Any comments will be welcomed!!

Thanks! Overall in terms of code quality this patch looks very good - see below for some comments but they're all very minor.

The only major issue I see with the patch is a documentation one: it's not clear to me reading the code how the atmel_ssc DAI driver relates to the existing at91_ssc driver. It may be that this is something that's obvious to someone familiar with the at91 hardware but just looking at the code it's not clear to me what the difference is between the two and when each should be used.

Looking at the code they appear to be similar to the point where they should be the same driver but it's entirely possible that I'm missing something here. I've CCed in Frank Mandarino who did the existing AT91 support. If they should be separate drivers then some comments should be added in the driver and the Kconfig help text explaning the situation.

Please include a changelog entry and a Signed-off-by - see Documentation/SubmittingPatches for details of how things should be formatted. It's also helpful to check your patches with scripts/checkpatch.pl which will pick up things like that and also coding style issues (there's a small number of them in your patch).

--- a/sound/soc/at91/Kconfig +++ b/sound/soc/at91/Kconfig @@ -7,7 +7,15 @@ config SND_AT91_SOC to select the audio interfaces to support below.

config SND_AT91_SOC_SSC

• tristate

• tristate "at91 soc ssc"

If this is intended to be user visible (rather than selected by the machine drivers) it'd be nice to provide some help text for it - it's not terribly clear at the minute.

+config SND_ATMEL_SOC_SSC

• tristate "ATMEL SoC ssc dai"
• depends on ATMEL_SSC
• help

•  Say Y or M if you want to add support for codecs the
  

•  ATMEL SSC interface(interface between at91sam9g20 and
  

•  WM8731 for instance).
  

Just a non-native English thing but how about:

Say Y or M to enable support for codecs attached to the ATMEL SSC interface. You will also need to select the individual machine drivers to support below.

+snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o

Very minor but please don't add these extra blank lines.

diff --git a/sound/soc/at91/atmel_ssc_dai.c b/sound/soc/at91/atmel_ssc_dai.c new file mode 100644 index 0000000..a3888c0 --- /dev/null +++ b/sound/soc/at91/atmel_ssc_dai.c @@ -0,0 +1,715 @@ +/*

• • atmel_ssc_dai.c -- ALSA SoC ATMEL SSC Audio Layer Platform driver
• • Copyright (C) 2005 SAN People
• • Copyright (C) 2008 Atmel
• • Author: Sedji Gaouaou sedji.gaouaou@atmel.com

• •     ATMEL CORP.
    

• • Based on at91-ssc.c by
• • Frank Mandarino fmandarino@endrelia.com
• • Based on pxa2xx Platform drivers by
• • Liam Girdwood liam.girdwood@wolfsonmicro.com

+#if 0 +#define DBG(x...) printk(KERN_DEBUG "atmel_ssc_dai:" x) +#else +#define DBG(x...) +#endif

Please use the standard pr_dbg() (or, if possible, dev_dbg()) for this. Quite a lot of the existing drivers do this but they're gradually being converted away and it'd be good to avoid adding any more.

+/*

• • Record SSC clock dividers for use in hw_params().
• */

+static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,

• int div_id, int div)

+{

• struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id];
• switch (div_id) {
• case AT91SSC_CMR_DIV:

• /*
  

•  * The same master clock divider is used for both
  

•  * transmit and receive, so if a value has already
  

•  * been set, it must match this value.
  

•  */
  

• if (ssc_p->cmr_div == 0)
  

• 	ssc_p->cmr_div = div;
  

• else
  

• 	if (div != ssc_p->cmr_div)
  

• 		return -EBUSY;
  

• break;
  

What happens if the user wants to change the master clock divider at runtime - for example, when changing sample rates?

• start_event = channels == 1
  

• 		? 4
  

• 		: 7;
  

This would be much clearer if it were expanded into multiple statements.

+#ifdef CONFIG_PM +#define atmel_ssc_suspend NULL +#define atmel_ssc_resume NULL +#else +#define atmel_ssc_suspend NULL +#define atmel_ssc_resume NULL +#endif

These may as well be removed - if someone implements suspend/resume support they can add them then then.

+#if 0 +#define DBG(x...) printk(KERN_INFO "at91sam9g20ek_wm8731: " x) +#else +#define DBG(x...) +#endif

Again, please replace with the standard pr_dbg() macro.

+static struct wm8731_setup_data at91sam9g20ek_wm8731_setup = {

• .i2c_address = 0x1b,

+};

Is the codec on I2C bus zero? If not then the i2c_bus field should also be initialised here. This was added as part of the recent patches converting the codec drivers to the new I2C registration interface - see the topic/asoc branch of Takashi's tree:

git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

for the latest code queued for merge in 2.6.28.

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 1db04a2..1595b04 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -12,7 +12,7 @@ config SND_SOC_WM8510 tristate

config SND_SOC_WM8731

• tristate

• tristate "WM8731"

This shouldn't be a user-visible config option - codec drivers are only useful in conjunction with a machine driver describing how they are connected to the system so there's no need to expose the configuration to users (there is an all codecs config option which can be used for test builds).