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

[Takashi Iwai]

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