Re: [alsa-devel] [PATCH] ALSA driver for SGI O2 audio board

At Sat, 5 Jul 2008 01:45:59 +0200 (CEST), Thomas Bogendoerfer wrote:

This patch adds a new ALSA driver for the audio device found inside most of the SGI O2 workstation. The hardware uses a SGI custom chip, which feeds a AD codec chip.

Signed-off-by: Thomas Bogendoerfer tsbogend@alpha.franken.de

Please apply for 2.6.27.

Thanks for the patch. Below is a quick review.

diff --git a/sound/mips/Kconfig b/sound/mips/Kconfig index 531f8ba..a3e202e 100644 --- a/sound/mips/Kconfig +++ b/sound/mips/Kconfig @@ -11,5 +11,11 @@ config SND_AU1X00 help ALSA Sound driver for the Au1x00's AC97 port.

+config SND_SGI_O2

• tristate "SGI O2 Audio"
• depends on SND && SGI_IP32

•    help
  

•            Sound support for the SGI O2 Workstation. 
  

endmenu

The recent version doesn't require "depends on SND" here because of changes to menuconfig. Check the latest ALSA tree or linux-next tree.

--- /dev/null +++ b/sound/mips/ad1843.c +const struct ad1843_gain ad1843_gain_RECLEV = {

• 0, &ad1843_LIG, &ad1843_RIG

+}; +const struct ad1843_gain ad1843_gain_LINE = {

• 1, &ad1843_LX1M, &ad1843_RX1M, &ad1843_LX1MM, &ad1843_RX1MM

+}; +const struct ad1843_gain ad1843_gain_LINE_2 = {

• 1, &ad1843_LDA2G, &ad1843_RDA2G, &ad1843_LDA2GM, &ad1843_RDA2GM

+}; +const struct ad1843_gain ad1843_gain_MIC = {

• 1, &ad1843_LMCM, &ad1843_RMCM, &ad1843_LMCMM, &ad1843_RMCMM

+}; +const struct ad1843_gain ad1843_gain_PCM_0 = {

• 1, &ad1843_LDA1G, &ad1843_RDA1G, &ad1843_LDA1GM, &ad1843_RDA1GM

+}; +const struct ad1843_gain ad1843_gain_PCM_1 = {

• 1, &ad1843_LD2M, &ad1843_RD2M, &ad1843_LD2MM, &ad1843_RD2MM

+};

Missing static?

+const struct ad1843_gain *ad1843_gain[AD1843_GAIN_SIZE] = +{

• &ad1843_gain_RECLEV,
• &ad1843_gain_LINE,
• &ad1843_gain_LINE_2,
• &ad1843_gain_MIC,
• &ad1843_gain_PCM_0,
• &ad1843_gain_PCM_1,

+};

Ditto. Also, better to use C99 style initialization.

+int ad1843_init(struct snd_ad1843 *ad1843) +{

• unsigned long later;
• if (ad1843_read_bits(ad1843, &ad1843_INIT) != 0) {

• printk(KERN_ERR "ad1843: AD1843 won't initialize\n");
  

• return -EIO;
  

• }
• ad1843_write_bits(ad1843, &ad1843_SCF, 1);
• /* 4. Put the conversion resources into standby. */
• ad1843_write_bits(ad1843, &ad1843_PDNI, 0);
• later = jiffies + HZ / 2; /* roughly half a second */

Use msecs_to_jiffies().

• while (ad1843_read_bits(ad1843, &ad1843_PDNO)) {

• if (time_after(jiffies, later)) {
  

• 	printk(KERN_ERR
  

• 	       "ad1843: AD1843 won't power up\n");
  

• 	return -EIO;
  

• }
  

• schedule();
  

No set_current_state() ?

--- /dev/null +++ b/sound/mips/sgio2audio.c +static int read_ad1843_reg(void *priv, int reg) +{

• struct snd_sgio2audio *chip = priv;
• int val;
• unsigned long flags;
• spin_lock_irqsave(&chip->ad1843_lock, flags);
• writeq((reg << CODEC_CONTROL_ADDRESS_SHIFT) |

•       CODEC_CONTROL_READ, &mace->perif.audio.codec_control);
  

• wmb();
• val = readq(&mace->perif.audio.codec_control); /* flush bus */
• udelay(200);

It'd be nicer if this long delay can be avoided. Or, could it be mutex?

This isn't a fatal problem, so it's OK as is, though.

+static int __devinit snd_sgio2audio_create(struct snd_card *card,

• 			   struct snd_sgio2audio **rchip)
  

+{

• struct snd_sgio2audio *chip;
• int i, err;
• *rchip = NULL;
• /* check if a codec is attached to the interface */
• /* (Audio or Audio/Video board present) */
• if (!(readq(&mace->perif.audio.control) & AUDIO_CONTROL_CODEC_PRESENT))

• return -ENOENT;
  

• chip = kzalloc(sizeof(struct snd_sgio2audio), GFP_KERNEL);
• if (chip == NULL)

• return -ENOMEM;
  

• chip->card = card;
• chip->ring_base = dma_alloc_coherent(NULL, MACEISA_RINGBUFFERS_SIZE,

• 			     &chip->ring_base_dma, GFP_USER);
  

• if (chip->ring_base == NULL) {

• printk(KERN_ERR
  

•        "sgio2audio: could not allocate ring buffers\n");
  

• kfree(chip);
  

• return -ENOMEM;
  

• }
• spin_lock_init(&chip->ad1843_lock);
• chip->volume = SGIO2AUDIO_MAX_VOLUME;
• /* initialize channels */
• for (i = 0; i < 3; i++) {

• spin_lock_init(&chip->channel[i].lock);
  

• chip->channel[i].idx = i;
  

• }
• /* allocate IRQs */
• for (i = 0; i < ARRAY_SIZE(snd_sgio2_isr_table); i++) {

• if (request_irq(snd_sgio2_isr_table[i].irq,
  

• 		snd_sgio2_isr_table[i].isr,
  

• 		IRQF_SHARED,
  

• 		snd_sgio2_isr_table[i].desc,
  

• 		&chip->channel[snd_sgio2_isr_table[i].idx])) {
  

• 	snd_sgio2audio_free(chip);
  

• 	printk(KERN_ERR "sgio2audio: cannot allocate irq %d\n",
  

• 	       snd_sgio2_isr_table[i].irq);
  

• 	return -EBUSY;
  

• }
  

• }

If there are shared interrupts, they could be called before the initialization below. So, safer to move after the init.

thanks,

Takashi