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@xxxxxxxxxxxxxxxx>
> ---
>
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
