* [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
@ 2008-07-18 19:51 Krzysztof Helt
2008-07-20 16:09 ` Rene Herman
0 siblings, 1 reply; 22+ messages in thread
From: Krzysztof Helt @ 2008-07-18 19:51 UTC (permalink / raw)
To: Alsa-devel; +Cc: Takashi Iwai, Rene Herman
From: Krzysztof Helt <krzysztof.h1@wp.pl>
Use the wss detection code and kill the ad1848 library.
The library is fully assimilated into the new wss library.
This patch fixes also the issue with AD1848 codec
MCE timeout - a waiting loop is now 10 times longer.
I have tested it on following cards:
Gallant SC-6600 (codec: AD1848, driver: snd-sc6600]
SoundScape VIVO/90 (codec: AD1845, driver: snd-sscape]
SG Waverider (codec: CS4231A, driver: Rene Herman's snd-galaxy]
Opti930 (codec: built-in - CS4231 compatible, driver: snd-opti93x]
Opti931 (codec: built-in - CS4231 compatible, driver: snd-opti93x]
Gallant SC-70P (chip/codec: CS4237B, driver: snd-cs4236]
Sound playback and recording works on all these cards.
Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
---
This is the last patch which completely removes
the ad1848_lib.c and the ad1848.h header.
The detection code was modified comparing to
the original ad1848 code to correctly recognize
pre cs4231 chips (16 codec registers) and
post cs4231 chips (32 codec registers).
One issue for Rene: Opti93x cards supports mu/a-law playback
but no recording. If these formats are selected only noise is
recorded (I suspect ADPCM and big endian formats cannot
be recorded as well). The mu-law recording works ok on
CS4231 and AD1845 chips. I tested only on them to check
if it is a hardware or driver problem.
Some wss code improvement patches may follow.
Regards,
Krzysztof
diff -urpN linux-alsa/include/sound/ad1848.h linux-mm/include/sound/ad1848.h
--- linux-alsa/include/sound/ad1848.h 2008-07-18 07:51:49.694754585 +0200
+++ linux-mm/include/sound/ad1848.h 1970-01-01 01:00:00.000000000 +0100
@@ -1,111 +0,0 @@
-#ifndef __SOUND_AD1848_H
-#define __SOUND_AD1848_H
-
-/*
- * Copyright (c) by Jaroslav Kysela <perex@perex.cz>
- * Definitions for AD1847/AD1848/CS4248 chips
- *
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- *
- */
-
-#include "pcm.h"
-#include <linux/interrupt.h>
-
-/* codec registers */
-
-#define AD1848_LEFT_INPUT 0x00 /* left input control */
-#define AD1848_RIGHT_INPUT 0x01 /* right input control */
-#define AD1848_AUX1_LEFT_INPUT 0x02 /* left AUX1 input control */
-#define AD1848_AUX1_RIGHT_INPUT 0x03 /* right AUX1 input control */
-#define AD1848_AUX2_LEFT_INPUT 0x04 /* left AUX2 input control */
-#define AD1848_AUX2_RIGHT_INPUT 0x05 /* right AUX2 input control */
-#define AD1848_LEFT_OUTPUT 0x06 /* left output control register */
-#define AD1848_RIGHT_OUTPUT 0x07 /* right output control register */
-#define AD1848_DATA_FORMAT 0x08 /* clock and data format - playback/capture - bits 7-0 MCE */
-#define AD1848_IFACE_CTRL 0x09 /* interface control - bits 7-2 MCE */
-#define AD1848_PIN_CTRL 0x0a /* pin control */
-#define AD1848_TEST_INIT 0x0b /* test and initialization */
-#define AD1848_MISC_INFO 0x0c /* miscellaneous information */
-#define AD1848_LOOPBACK 0x0d /* loopback control */
-#define AD1848_DATA_UPR_CNT 0x0e /* playback/capture upper base count */
-#define AD1848_DATA_LWR_CNT 0x0f /* playback/capture lower base count */
-
-/* definitions for codec register select port - CODECP( REGSEL ) */
-
-#define AD1848_INIT 0x80 /* CODEC is initializing */
-#define AD1848_MCE 0x40 /* mode change enable */
-#define AD1848_TRD 0x20 /* transfer request disable */
-
-/* definitions for codec status register - CODECP( STATUS ) */
-
-#define AD1848_GLOBALIRQ 0x01 /* IRQ is active */
-
-/* definitions for AD1848_LEFT_INPUT and AD1848_RIGHT_INPUT registers */
-
-#define AD1848_ENABLE_MIC_GAIN 0x20
-
-#define AD1848_MIXS_LINE1 0x00
-#define AD1848_MIXS_AUX1 0x40
-#define AD1848_MIXS_LINE2 0x80
-#define AD1848_MIXS_ALL 0xc0
-
-/* definitions for clock and data format register - AD1848_PLAYBK_FORMAT */
-
-#define AD1848_LINEAR_8 0x00 /* 8-bit unsigned data */
-#define AD1848_ALAW_8 0x60 /* 8-bit A-law companded */
-#define AD1848_ULAW_8 0x20 /* 8-bit U-law companded */
-#define AD1848_LINEAR_16 0x40 /* 16-bit twos complement data - little endian */
-#define AD1848_STEREO 0x10 /* stereo mode */
-/* bits 3-1 define frequency divisor */
-#define AD1848_XTAL1 0x00 /* 24.576 crystal */
-#define AD1848_XTAL2 0x01 /* 16.9344 crystal */
-
-/* definitions for interface control register - AD1848_IFACE_CTRL */
-
-#define AD1848_CAPTURE_PIO 0x80 /* capture PIO enable */
-#define AD1848_PLAYBACK_PIO 0x40 /* playback PIO enable */
-#define AD1848_CALIB_MODE 0x18 /* calibration mode bits */
-#define AD1848_AUTOCALIB 0x08 /* auto calibrate */
-#define AD1848_SINGLE_DMA 0x04 /* use single DMA channel */
-#define AD1848_CAPTURE_ENABLE 0x02 /* capture enable */
-#define AD1848_PLAYBACK_ENABLE 0x01 /* playback enable */
-
-/* definitions for pin control register - AD1848_PIN_CTRL */
-
-#define AD1848_IRQ_ENABLE 0x02 /* enable IRQ */
-#define AD1848_XCTL1 0x40 /* external control #1 */
-#define AD1848_XCTL0 0x80 /* external control #0 */
-
-/* definitions for test and init register - AD1848_TEST_INIT */
-
-#define AD1848_CALIB_IN_PROGRESS 0x20 /* auto calibrate in progress */
-#define AD1848_DMA_REQUEST 0x10 /* DMA request in progress */
-
-#include "wss.h"
-
-/* exported functions */
-
-void snd_ad1848_out(struct snd_wss *chip, unsigned char reg,
- unsigned char value);
-
-int snd_ad1848_create(struct snd_card *card,
- unsigned long port,
- int irq, int dma,
- unsigned short hardware,
- struct snd_wss **chip);
-
-#endif /* __SOUND_AD1848_H */
diff -urpN linux-alsa/sound/isa/Kconfig linux-mm/sound/isa/Kconfig
--- linux-alsa/sound/isa/Kconfig 2008-07-18 07:51:49.750768622 +0200
+++ linux-mm/sound/isa/Kconfig 2008-07-18 13:34:54.698303387 +0200
@@ -4,11 +4,6 @@ config SND_WSS_LIB
tristate
select SND_PCM
-config SND_AD1848_LIB
- tristate
- select SND_PCM
- select SND_WSS_LIB
-
config SND_SB_COMMON
tristate
@@ -56,7 +51,7 @@ config SND_AD1816A
config SND_AD1848
tristate "Generic AD1848/CS4248 driver"
- select SND_AD1848_LIB
+ select SND_WSS_LIB
help
Say Y here to include support for AD1848 (Analog Devices) or
CS4248 (Cirrus Logic - Crystal Semiconductors) chips.
@@ -97,7 +92,7 @@ config SND_AZT2320
config SND_CMI8330
tristate "C-Media CMI8330"
- select SND_AD1848_LIB
+ select SND_WSS_LIB
select SND_SB16_DSP
help
Say Y here to include support for soundcards based on the
@@ -193,7 +188,7 @@ config SND_ES18XX
config SND_SC6000
tristate "Gallant SC-6000, Audio Excel DSP 16"
depends on HAS_IOPORT
- select SND_AD1848_LIB
+ select SND_WSS_LIB
select SND_OPL3_LIB
select SND_MPU401_UART
help
@@ -280,7 +275,7 @@ config SND_OPTI92X_AD1848
select SND_OPL3_LIB
select SND_OPL4_LIB
select SND_MPU401_UART
- select SND_AD1848_LIB
+ select SND_WSS_LIB
help
Say Y here to include support for soundcards based on Opti
82C92x or OTI-601 chips and using an AD1848 codec.
@@ -382,7 +377,7 @@ config SND_SB16_CSP_FIRMWARE_IN_KERNEL
config SND_SGALAXY
tristate "Aztech Sound Galaxy"
- select SND_AD1848_LIB
+ select SND_WSS_LIB
help
Say Y here to include support for Aztech Sound Galaxy
soundcards.
diff -urpN linux-alsa/sound/isa/ad1848/Makefile linux-mm/sound/isa/ad1848/Makefile
--- linux-alsa/sound/isa/ad1848/Makefile 2008-04-17 04:49:44.000000000 +0200
+++ linux-mm/sound/isa/ad1848/Makefile 2008-07-18 13:34:54.698303387 +0200
@@ -3,10 +3,8 @@
# Copyright (c) 2001 by Jaroslav Kysela <perex@perex.cz>
#
-snd-ad1848-lib-objs := ad1848_lib.o
snd-ad1848-objs := ad1848.o
# Toplevel Module Dependency
obj-$(CONFIG_SND_AD1848) += snd-ad1848.o
-obj-$(CONFIG_SND_AD1848_LIB) += snd-ad1848-lib.o
diff -urpN linux-alsa/sound/isa/ad1848/ad1848.c linux-mm/sound/isa/ad1848/ad1848.c
--- linux-alsa/sound/isa/ad1848/ad1848.c 2008-07-18 07:51:49.786753998 +0200
+++ linux-mm/sound/isa/ad1848/ad1848.c 2008-07-18 13:34:54.702304924 +0200
@@ -28,7 +28,7 @@
#include <linux/wait.h>
#include <linux/moduleparam.h>
#include <sound/core.h>
-#include <sound/ad1848.h>
+#include <sound/wss.h>
#include <sound/initval.h>
#define CRD_NAME "Generic AD1848/AD1847/CS4248"
@@ -95,8 +95,9 @@ static int __devinit snd_ad1848_probe(st
if (!card)
return -EINVAL;
- error = snd_ad1848_create(card, port[n], irq[n], dma1[n],
- thinkpad[n] ? WSS_HW_THINKPAD : WSS_HW_DETECT, &chip);
+ error = snd_wss_create(card, port[n], -1, irq[n], dma1[n], -1,
+ thinkpad[n] ? WSS_HW_THINKPAD : WSS_HW_DETECT,
+ 0, &chip);
if (error < 0)
goto out;
diff -urpN linux-alsa/sound/isa/ad1848/ad1848_lib.c linux-mm/sound/isa/ad1848/ad1848_lib.c
--- linux-alsa/sound/isa/ad1848/ad1848_lib.c 2008-07-18 07:51:49.802793124 +0200
+++ linux-mm/sound/isa/ad1848/ad1848_lib.c 1970-01-01 01:00:00.000000000 +0100
@@ -1,505 +0,0 @@
-/*
- * Copyright (c) by Jaroslav Kysela <perex@perex.cz>
- * Routines for control of AD1848/AD1847/CS4248
- *
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
- *
- */
-
-#define SNDRV_MAIN_OBJECT_FILE
-#include <linux/delay.h>
-#include <linux/init.h>
-#include <linux/interrupt.h>
-#include <linux/slab.h>
-#include <linux/ioport.h>
-#include <sound/core.h>
-#include <sound/ad1848.h>
-#include <sound/control.h>
-#include <sound/tlv.h>
-#include <sound/pcm_params.h>
-
-#include <asm/io.h>
-#include <asm/dma.h>
-
-MODULE_AUTHOR("Jaroslav Kysela <perex@perex.cz>");
-MODULE_DESCRIPTION("Routines for control of AD1848/AD1847/CS4248");
-MODULE_LICENSE("GPL");
-
-#if 0
-#define SNDRV_DEBUG_MCE
-#endif
-
-/*
- * Some variables
- */
-
-static unsigned char snd_ad1848_original_image[16] =
-{
- 0x00, /* 00 - lic */
- 0x00, /* 01 - ric */
- 0x9f, /* 02 - la1ic */
- 0x9f, /* 03 - ra1ic */
- 0x9f, /* 04 - la2ic */
- 0x9f, /* 05 - ra2ic */
- 0xbf, /* 06 - loc */
- 0xbf, /* 07 - roc */
- 0x20, /* 08 - dfr */
- AD1848_AUTOCALIB, /* 09 - ic */
- 0x00, /* 0a - pc */
- 0x00, /* 0b - ti */
- 0x00, /* 0c - mi */
- 0x00, /* 0d - lbc */
- 0x00, /* 0e - dru */
- 0x00, /* 0f - drl */
-};
-
-/*
- * Basic I/O functions
- */
-
-static void snd_ad1848_wait(struct snd_wss *chip)
-{
- int timeout;
-
- for (timeout = 250; timeout > 0; timeout--) {
- if ((inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT) == 0)
- break;
- udelay(100);
- }
-}
-
-void snd_ad1848_out(struct snd_wss *chip,
- unsigned char reg,
- unsigned char value)
-{
- snd_ad1848_wait(chip);
-#ifdef CONFIG_SND_DEBUG
- if (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT)
- snd_printk(KERN_WARNING "auto calibration time out - "
- "reg = 0x%x, value = 0x%x\n", reg, value);
-#endif
- outb(chip->mce_bit | reg, chip->port + CS4231P(REGSEL));
- outb(chip->image[reg] = value, chip->port + CS4231P(REG));
- mb();
- snd_printdd("codec out - reg 0x%x = 0x%x\n",
- chip->mce_bit | reg, value);
-}
-
-EXPORT_SYMBOL(snd_ad1848_out);
-
-static unsigned char snd_ad1848_in(struct snd_wss *chip, unsigned char reg)
-{
- snd_ad1848_wait(chip);
-#ifdef CONFIG_SND_DEBUG
- if (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT)
- snd_printk(KERN_WARNING "auto calibration time out - "
- "reg = 0x%x\n", reg);
-#endif
- outb(chip->mce_bit | reg, chip->port + CS4231P(REGSEL));
- mb();
- return inb(chip->port + CS4231P(REG));
-}
-
-#if 0
-
-static void snd_ad1848_debug(struct snd_wss *chip)
-{
- printk(KERN_DEBUG "AD1848 REGS: INDEX = 0x%02x ",
- inb(chip->port + CS4231P(REGSEL)));
- printk(KERN_DEBUG " STATUS = 0x%02x\n",
- inb(chip->port + CS4231P(STATUS)));
- printk(KERN_DEBUG " 0x00: left input = 0x%02x ",
- snd_ad1848_in(chip, 0x00));
- printk(KERN_DEBUG " 0x08: playback format = 0x%02x\n",
- snd_ad1848_in(chip, 0x08));
- printk(KERN_DEBUG " 0x01: right input = 0x%02x ",
- snd_ad1848_in(chip, 0x01));
- printk(KERN_DEBUG " 0x09: iface (CFIG 1) = 0x%02x\n",
- snd_ad1848_in(chip, 0x09));
- printk(KERN_DEBUG " 0x02: AUXA left = 0x%02x ",
- snd_ad1848_in(chip, 0x02));
- printk(KERN_DEBUG " 0x0a: pin control = 0x%02x\n",
- snd_ad1848_in(chip, 0x0a));
- printk(KERN_DEBUG " 0x03: AUXA right = 0x%02x ",
- snd_ad1848_in(chip, 0x03));
- printk(KERN_DEBUG " 0x0b: init & status = 0x%02x\n",
- snd_ad1848_in(chip, 0x0b));
- printk(KERN_DEBUG " 0x04: AUXB left = 0x%02x ",
- snd_ad1848_in(chip, 0x04));
- printk(KERN_DEBUG " 0x0c: revision & mode = 0x%02x\n",
- snd_ad1848_in(chip, 0x0c));
- printk(KERN_DEBUG " 0x05: AUXB right = 0x%02x ",
- snd_ad1848_in(chip, 0x05));
- printk(KERN_DEBUG " 0x0d: loopback = 0x%02x\n",
- snd_ad1848_in(chip, 0x0d));
- printk(KERN_DEBUG " 0x06: left output = 0x%02x ",
- snd_ad1848_in(chip, 0x06));
- printk(KERN_DEBUG " 0x0e: data upr count = 0x%02x\n",
- snd_ad1848_in(chip, 0x0e));
- printk(KERN_DEBUG " 0x07: right output = 0x%02x ",
- snd_ad1848_in(chip, 0x07));
- printk(KERN_DEBUG " 0x0f: data lwr count = 0x%02x\n",
- snd_ad1848_in(chip, 0x0f));
-}
-
-#endif
-
-/*
- * AD1848 detection / MCE routines
- */
-
-static void snd_ad1848_mce_up(struct snd_wss *chip)
-{
- unsigned long flags;
- int timeout;
-
- snd_ad1848_wait(chip);
-#ifdef CONFIG_SND_DEBUG
- if (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT)
- snd_printk(KERN_WARNING "mce_up - auto calibration time out (0)\n");
-#endif
- spin_lock_irqsave(&chip->reg_lock, flags);
- chip->mce_bit |= AD1848_MCE;
- timeout = inb(chip->port + CS4231P(REGSEL));
- if (timeout == 0x80)
- snd_printk(KERN_WARNING "mce_up [0x%lx]: serious init problem - codec still busy\n", chip->port);
- if (!(timeout & AD1848_MCE))
- outb(chip->mce_bit | (timeout & 0x1f),
- chip->port + CS4231P(REGSEL));
- spin_unlock_irqrestore(&chip->reg_lock, flags);
-}
-
-static void snd_ad1848_mce_down(struct snd_wss *chip)
-{
- unsigned long flags, timeout;
- int reg;
-
- spin_lock_irqsave(&chip->reg_lock, flags);
- for (timeout = 5; timeout > 0; timeout--)
- inb(chip->port + CS4231P(REGSEL));
- /* end of cleanup sequence */
- for (timeout = 12000;
- timeout > 0 && (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT);
- timeout--)
- udelay(100);
-
- snd_printdd("(1) timeout = %ld\n", timeout);
-
-#ifdef CONFIG_SND_DEBUG
- if (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT)
- snd_printk(KERN_WARNING
- "mce_down [0x%lx] - auto calibration time out (0)\n",
- chip->port + CS4231P(REGSEL));
-#endif
-
- chip->mce_bit &= ~AD1848_MCE;
- reg = inb(chip->port + CS4231P(REGSEL));
- outb(chip->mce_bit | (reg & 0x1f), chip->port + CS4231P(REGSEL));
- if (reg == 0x80)
- snd_printk(KERN_WARNING "mce_down [0x%lx]: serious init problem - codec still busy\n", chip->port);
- if ((reg & AD1848_MCE) == 0) {
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- return;
- }
-
- /*
- * Wait for auto-calibration (AC) process to finish, i.e. ACI to go low.
- * It may take up to 5 sample periods (at most 907 us @ 5.5125 kHz) for
- * the process to _start_, so it is important to wait at least that long
- * before checking. Otherwise we might think AC has finished when it
- * has in fact not begun. It could take 128 (no AC) or 384 (AC) cycles
- * for ACI to drop. This gives a wait of at most 70 ms with a more
- * typical value of 3-9 ms.
- */
- timeout = jiffies + msecs_to_jiffies(250);
- do {
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- msleep(1);
- spin_lock_irqsave(&chip->reg_lock, flags);
- reg = snd_ad1848_in(chip, AD1848_TEST_INIT) &
- AD1848_CALIB_IN_PROGRESS;
- } while (reg && time_before(jiffies, timeout));
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- if (reg)
- snd_printk(KERN_ERR
- "mce_down - auto calibration time out (2)\n");
-
- snd_printdd("(4) jiffies = %lu\n", jiffies);
- snd_printd("mce_down - exit = 0x%x\n",
- inb(chip->port + CS4231P(REGSEL)));
-}
-
-static irqreturn_t snd_ad1848_interrupt(int irq, void *dev_id)
-{
- struct snd_wss *chip = dev_id;
-
- if ((chip->mode & WSS_MODE_PLAY) && chip->playback_substream)
- snd_pcm_period_elapsed(chip->playback_substream);
- if ((chip->mode & WSS_MODE_RECORD) && chip->capture_substream)
- snd_pcm_period_elapsed(chip->capture_substream);
- outb(0, chip->port + CS4231P(STATUS)); /* clear global interrupt bit */
- return IRQ_HANDLED;
-}
-
-/*
-
- */
-
-static void snd_ad1848_thinkpad_twiddle(struct snd_wss *chip, int on)
-{
- int tmp;
-
- if (!chip->thinkpad_flag) return;
-
- outb(0x1c, AD1848_THINKPAD_CTL_PORT1);
- tmp = inb(AD1848_THINKPAD_CTL_PORT2);
-
- if (on)
- /* turn it on */
- tmp |= AD1848_THINKPAD_CS4248_ENABLE_BIT;
- else
- /* turn it off */
- tmp &= ~AD1848_THINKPAD_CS4248_ENABLE_BIT;
-
- outb(tmp, AD1848_THINKPAD_CTL_PORT2);
-
-}
-
-#ifdef CONFIG_PM
-static void snd_ad1848_suspend(struct snd_wss *chip)
-{
- snd_pcm_suspend_all(chip->pcm);
- if (chip->thinkpad_flag)
- snd_ad1848_thinkpad_twiddle(chip, 0);
-}
-
-static void snd_ad1848_resume(struct snd_wss *chip)
-{
- int i;
-
- if (chip->thinkpad_flag)
- snd_ad1848_thinkpad_twiddle(chip, 1);
-
- /* clear any pendings IRQ */
- inb(chip->port + CS4231P(STATUS));
- outb(0, chip->port + CS4231P(STATUS));
- mb();
-
- snd_ad1848_mce_down(chip);
- for (i = 0; i < 16; i++)
- snd_ad1848_out(chip, i, chip->image[i]);
- snd_ad1848_mce_up(chip);
- snd_ad1848_mce_down(chip);
-}
-#endif /* CONFIG_PM */
-
-static int snd_ad1848_probe(struct snd_wss *chip)
-{
- unsigned long flags;
- int i, id, rev, ad1847;
- unsigned char *ptr;
-
-#if 0
- snd_ad1848_debug(chip);
-#endif
- id = ad1847 = 0;
- for (i = 0; i < 1000; i++) {
- mb();
- if (inb(chip->port + CS4231P(REGSEL)) & AD1848_INIT)
- udelay(500);
- else {
- spin_lock_irqsave(&chip->reg_lock, flags);
- snd_ad1848_out(chip, AD1848_MISC_INFO, 0x00);
- snd_ad1848_out(chip, AD1848_LEFT_INPUT, 0xaa);
- snd_ad1848_out(chip, AD1848_RIGHT_INPUT, 0x45);
- rev = snd_ad1848_in(chip, AD1848_RIGHT_INPUT);
- if (rev == 0x65) {
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- id = 1;
- ad1847 = 1;
- break;
- }
- if (snd_ad1848_in(chip, AD1848_LEFT_INPUT) == 0xaa && rev == 0x45) {
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- id = 1;
- break;
- }
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- }
- }
- if (id != 1)
- return -ENODEV; /* no valid device found */
- if (chip->hardware == WSS_HW_DETECT) {
- if (ad1847) {
- chip->hardware = WSS_HW_AD1847;
- } else {
- chip->hardware = WSS_HW_AD1848;
- rev = snd_ad1848_in(chip, AD1848_MISC_INFO);
- if (rev & 0x80) {
- chip->hardware = WSS_HW_CS4248;
- } else if ((rev & 0x0f) == 0x0a) {
- snd_ad1848_out(chip, AD1848_MISC_INFO, 0x40);
- for (i = 0; i < 16; ++i) {
- if (snd_ad1848_in(chip, i) != snd_ad1848_in(chip, i + 16)) {
- chip->hardware = WSS_HW_CMI8330;
- break;
- }
- }
- snd_ad1848_out(chip, AD1848_MISC_INFO, 0x00);
- }
- }
- }
- spin_lock_irqsave(&chip->reg_lock, flags);
- inb(chip->port + CS4231P(STATUS)); /* clear any pendings IRQ */
- outb(0, chip->port + CS4231P(STATUS));
- mb();
- spin_unlock_irqrestore(&chip->reg_lock, flags);
-
- chip->image[AD1848_MISC_INFO] = 0x00;
- chip->image[AD1848_IFACE_CTRL] =
- (chip->image[AD1848_IFACE_CTRL] & ~AD1848_SINGLE_DMA) | AD1848_SINGLE_DMA;
- ptr = (unsigned char *) &chip->image;
- snd_ad1848_mce_down(chip);
- spin_lock_irqsave(&chip->reg_lock, flags);
- for (i = 0; i < 16; i++) /* ok.. fill all AD1848 registers */
- snd_ad1848_out(chip, i, *ptr++);
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- snd_ad1848_mce_up(chip);
- /* init needed for WSS pcm */
- spin_lock_irqsave(&chip->reg_lock, flags);
- chip->image[AD1848_IFACE_CTRL] &= ~(AD1848_PLAYBACK_ENABLE |
- AD1848_PLAYBACK_PIO |
- AD1848_CAPTURE_ENABLE |
- AD1848_CAPTURE_PIO |
- AD1848_CALIB_MODE);
- chip->image[AD1848_IFACE_CTRL] |= AD1848_AUTOCALIB;
- snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL]);
- spin_unlock_irqrestore(&chip->reg_lock, flags);
- snd_ad1848_mce_down(chip);
- return 0; /* all things are ok.. */
-}
-
-/*
-
- */
-
-static int snd_ad1848_free(struct snd_wss *chip)
-{
- release_and_free_resource(chip->res_port);
- if (chip->irq >= 0)
- free_irq(chip->irq, (void *) chip);
- if (chip->dma1 >= 0) {
- snd_dma_disable(chip->dma1);
- free_dma(chip->dma1);
- }
- kfree(chip);
- return 0;
-}
-
-static int snd_ad1848_dev_free(struct snd_device *device)
-{
- struct snd_wss *chip = device->device_data;
- return snd_ad1848_free(chip);
-}
-
-int snd_ad1848_create(struct snd_card *card,
- unsigned long port,
- int irq, int dma,
- unsigned short hardware,
- struct snd_wss **rchip)
-{
- static struct snd_device_ops ops = {
- .dev_free = snd_ad1848_dev_free,
- };
- struct snd_wss *chip;
- int err;
-
- *rchip = NULL;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL);
- if (chip == NULL)
- return -ENOMEM;
- spin_lock_init(&chip->reg_lock);
- chip->card = card;
- chip->port = port;
- chip->irq = -1;
- chip->dma1 = -1;
- chip->dma2 = -1;
- chip->single_dma = 1;
- chip->hardware = hardware;
- memcpy(&chip->image, &snd_ad1848_original_image, sizeof(snd_ad1848_original_image));
-
- if ((chip->res_port = request_region(port, 4, "AD1848")) == NULL) {
- snd_printk(KERN_ERR "ad1848: can't grab port 0x%lx\n", port);
- snd_ad1848_free(chip);
- return -EBUSY;
- }
- if (request_irq(irq, snd_ad1848_interrupt, IRQF_DISABLED, "AD1848", (void *) chip)) {
- snd_printk(KERN_ERR "ad1848: can't grab IRQ %d\n", irq);
- snd_ad1848_free(chip);
- return -EBUSY;
- }
- chip->irq = irq;
- if (request_dma(dma, "AD1848")) {
- snd_printk(KERN_ERR "ad1848: can't grab DMA %d\n", dma);
- snd_ad1848_free(chip);
- return -EBUSY;
- }
- chip->dma1 = dma;
- chip->dma2 = dma;
-
- if (hardware == WSS_HW_THINKPAD) {
- chip->thinkpad_flag = 1;
- chip->hardware = WSS_HW_DETECT; /* reset */
- snd_ad1848_thinkpad_twiddle(chip, 1);
- }
-
- if (snd_ad1848_probe(chip) < 0) {
- snd_ad1848_free(chip);
- return -ENODEV;
- }
-
- /* Register device */
- if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) {
- snd_ad1848_free(chip);
- return err;
- }
-
-#ifdef CONFIG_PM
- chip->suspend = snd_ad1848_suspend;
- chip->resume = snd_ad1848_resume;
-#endif
-
- *rchip = chip;
- return 0;
-}
-
-EXPORT_SYMBOL(snd_ad1848_create);
-
-/*
- * INIT part
- */
-
-static int __init alsa_ad1848_init(void)
-{
- return 0;
-}
-
-static void __exit alsa_ad1848_exit(void)
-{
-}
-
-module_init(alsa_ad1848_init)
-module_exit(alsa_ad1848_exit)
diff -urpN linux-alsa/sound/isa/sc6000.c linux-mm/sound/isa/sc6000.c
--- linux-alsa/sound/isa/sc6000.c 2008-07-18 07:51:49.886753130 +0200
+++ linux-mm/sound/isa/sc6000.c 2008-07-18 13:34:54.738307314 +0200
@@ -29,7 +29,7 @@
#include <linux/io.h>
#include <asm/dma.h>
#include <sound/core.h>
-#include <sound/ad1848.h>
+#include <sound/wss.h>
#include <sound/opl3.h>
#include <sound/mpu401.h>
#include <sound/control.h>
@@ -548,8 +548,8 @@ static int __devinit snd_sc6000_probe(st
if (err < 0)
goto err_unmap2;
- err = snd_ad1848_create(card, mss_port[dev] + 4, xirq, xdma,
- WSS_HW_DETECT, &chip);
+ err = snd_wss_create(card, mss_port[dev] + 4, -1, xirq, xdma, -1,
+ WSS_HW_DETECT, 0, &chip);
if (err < 0)
goto err_unmap2;
card->private_data = chip;
diff -urpN linux-alsa/sound/isa/sgalaxy.c linux-mm/sound/isa/sgalaxy.c
--- linux-alsa/sound/isa/sgalaxy.c 2008-07-18 07:51:49.894756141 +0200
+++ linux-mm/sound/isa/sgalaxy.c 2008-07-18 13:34:54.742304370 +0200
@@ -31,7 +31,7 @@
#include <asm/dma.h>
#include <sound/core.h>
#include <sound/sb.h>
-#include <sound/ad1848.h>
+#include <sound/wss.h>
#include <sound/control.h>
#define SNDRV_LEGACY_FIND_FREE_IRQ
#define SNDRV_LEGACY_FIND_FREE_DMA
@@ -265,9 +265,10 @@ static int __devinit snd_sgalaxy_probe(s
if ((err = snd_sgalaxy_detect(dev, xirq, xdma1)) < 0)
goto _err;
- if ((err = snd_ad1848_create(card, wssport[dev] + 4,
- xirq, xdma1,
- WSS_HW_DETECT, &chip)) < 0)
+ err = snd_wss_create(card, wssport[dev] + 4, -1,
+ xirq, xdma1, -1,
+ WSS_HW_DETECT, 0, &chip);
+ if (err < 0)
goto _err;
card->private_data = chip;
@@ -329,8 +330,8 @@ static int snd_sgalaxy_resume(struct dev
struct snd_wss *chip = card->private_data;
chip->resume(chip);
- snd_ad1848_out(chip, SGALAXY_AUXC_LEFT, chip->image[SGALAXY_AUXC_LEFT]);
- snd_ad1848_out(chip, SGALAXY_AUXC_RIGHT, chip->image[SGALAXY_AUXC_RIGHT]);
+ snd_wss_out(chip, SGALAXY_AUXC_LEFT, chip->image[SGALAXY_AUXC_LEFT]);
+ snd_wss_out(chip, SGALAXY_AUXC_RIGHT, chip->image[SGALAXY_AUXC_RIGHT]);
snd_power_change_state(card, SNDRV_CTL_POWER_D0);
return 0;
diff -urp linux-alsa/sound/isa/cmi8330.c linux-mm/sound/isa/cmi8330.c
--- linux-alsa/sound/isa/cmi8330.c 2008-07-18 07:51:49.850762517 +0200
+++ linux-mm/sound/isa/cmi8330.c 2008-07-18 16:39:28.461170191 +0200
@@ -50,7 +50,7 @@
#include <linux/pnp.h>
#include <linux/moduleparam.h>
#include <sound/core.h>
-#include <sound/ad1848.h>
+#include <sound/wss.h>
#include <sound/sb.h>
#include <sound/initval.h>
@@ -178,9 +178,9 @@ static struct snd_kcontrol_new snd_cmi83
WSS_DOUBLE("Master Playback Volume", 0, CMI8330_MASTVOL, CMI8330_MASTVOL,
4, 0, 15, 0),
WSS_SINGLE("Loud Playback Switch", 0, CMI8330_MUTEMUX, 6, 1, 1),
-WSS_DOUBLE("PCM Playback Switch", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT,
+WSS_DOUBLE("PCM Playback Switch", 0, CS4231_LEFT_OUTPUT, CS4231_RIGHT_OUTPUT,
7, 7, 1, 1),
-WSS_DOUBLE("PCM Playback Volume", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT,
+WSS_DOUBLE("PCM Playback Volume", 0, CS4231_LEFT_OUTPUT, CS4231_RIGHT_OUTPUT,
0, 0, 63, 1),
WSS_DOUBLE("Line Playback Switch", 0, CMI8330_MUTEMUX, CMI8330_MUTEMUX,
4, 3, 1, 0),
@@ -480,12 +480,11 @@ static int __devinit snd_cmi8330_probe(s
int i, err;
acard = card->private_data;
- if ((err = snd_ad1848_create(card,
- wssport[dev] + 4,
- wssirq[dev],
- wssdma[dev],
- WSS_HW_DETECT,
- &acard->wss)) < 0) {
+ err = snd_wss_create(card, wssport[dev] + 4, -1,
+ wssirq[dev],
+ wssdma[dev], -1,
+ WSS_HW_DETECT, 0, &acard->wss);
+ if (err < 0) {
snd_printk(KERN_ERR PFX "(AD1848) device busy??\n");
return err;
}
@@ -508,9 +507,10 @@ static int __devinit snd_cmi8330_probe(s
return err;
}
- snd_ad1848_out(acard->wss, AD1848_MISC_INFO, 0x40); /* switch on MODE2 */
+ snd_wss_out(acard->wss, CS4231_MISC_INFO, 0x40); /* switch on MODE2 */
for (i = CMI8330_RMUX3D; i <= CMI8330_CDINGAIN; i++)
- snd_ad1848_out(acard->wss, i, snd_cmi8330_image[i - CMI8330_RMUX3D]);
+ snd_wss_out(acard->wss, i,
+ snd_cmi8330_image[i - CMI8330_RMUX3D]);
if ((err = snd_cmi8330_mixer(card, acard)) < 0) {
snd_printk(KERN_ERR PFX "failed to create mixers\n");
diff -urp linux-alsa/sound/isa/wss/wss_lib.c linux-mm/sound/isa/wss/wss_lib.c
--- linux-alsa/sound/isa/wss/wss_lib.c 2008-07-18 07:51:49.990751242 +0200
+++ linux-mm/sound/isa/wss/wss_lib.c 2008-07-18 18:34:34.992298787 +0200
@@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd
for (timeout = 5; timeout > 0; timeout--)
wss_inb(chip, CS4231P(REGSEL));
/* end of cleanup sequence */
- for (timeout = 2500;
+ for (timeout = 25000;
timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
timeout--)
udelay(10);
@@ -1050,7 +1050,11 @@ irqreturn_t snd_wss_interrupt(int irq, v
struct snd_wss *chip = dev_id;
unsigned char status;
- status = snd_wss_in(chip, CS4231_IRQ_STATUS);
+ if (chip->hardware & WSS_HW_AD1848_MASK)
+ /* pretend it was the only possible irq for AD1848 */
+ status = CS4231_PLAYBACK_IRQ;
+ else
+ status = snd_wss_in(chip, CS4231_IRQ_STATUS);
if (status & CS4231_TIMER_IRQ) {
if (chip->timer)
snd_timer_interrupt(chip->timer, chip->timer->sticks);
@@ -1082,7 +1086,11 @@ irqreturn_t snd_wss_interrupt(int irq, v
}
spin_lock(&chip->reg_lock);
- snd_wss_outm(chip, CS4231_IRQ_STATUS, ~CS4231_ALL_IRQS | ~status, 0);
+ status = ~CS4231_ALL_IRQS | ~status;
+ if (chip->hardware & WSS_HW_AD1848_MASK)
+ wss_outb(chip, CS4231P(STATUS), 0);
+ else
+ snd_wss_outm(chip, CS4231_IRQ_STATUS, status, 0);
spin_unlock(&chip->reg_lock);
return IRQ_HANDLED;
}
@@ -1116,36 +1124,112 @@ static snd_pcm_uframes_t snd_wss_capture
*/
-static int snd_wss_probe(struct snd_wss *chip)
+static int snd_ad1848_probe(struct snd_wss *chip)
{
unsigned long flags;
- int i, id, rev;
- unsigned char *ptr;
- unsigned int hw;
+ int i, id, rev, ad1847;
-#if 0
- snd_wss_debug(chip);
-#endif
id = 0;
- for (i = 0; i < 50; i++) {
+ ad1847 = 0;
+ for (i = 0; i < 1000; i++) {
mb();
- if (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT)
- udelay(2000);
+ if (inb(chip->port + CS4231P(REGSEL)) & CS4231_INIT)
+ msleep(1);
else {
spin_lock_irqsave(&chip->reg_lock, flags);
- snd_wss_out(chip, CS4231_MISC_INFO, CS4231_MODE2);
- id = snd_wss_in(chip, CS4231_MISC_INFO) & 0x0f;
+ snd_wss_out(chip, CS4231_MISC_INFO, 0x00);
+ snd_wss_out(chip, CS4231_LEFT_INPUT, 0xaa);
+ snd_wss_out(chip, CS4231_RIGHT_INPUT, 0x45);
+ rev = snd_wss_in(chip, CS4231_RIGHT_INPUT);
+ if (rev == 0x65) {
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ id = 1;
+ ad1847 = 1;
+ break;
+ }
+ if (snd_wss_in(chip, CS4231_LEFT_INPUT) == 0xaa &&
+ rev == 0x45) {
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ id = 1;
+ break;
+ }
spin_unlock_irqrestore(&chip->reg_lock, flags);
- if (id == 0x0a)
- break; /* this is valid value */
}
}
- snd_printdd("wss: port = 0x%lx, id = 0x%x\n", chip->port, id);
- if (id != 0x0a)
+ if (id != 1)
return -ENODEV; /* no valid device found */
+ id = 0;
+ if (chip->hardware == WSS_HW_DETECT)
+ id = ad1847 ? WSS_HW_AD1847 : WSS_HW_AD1848;
+
+ spin_lock_irqsave(&chip->reg_lock, flags);
+ inb(chip->port + CS4231P(STATUS)); /* clear any pendings IRQ */
+ outb(0, chip->port + CS4231P(STATUS));
+ mb();
+ if (id == WSS_HW_AD1848) {
+ /* check if there are more than 16 registers */
+ rev = snd_wss_in(chip, CS4231_MISC_INFO);
+ snd_wss_out(chip, CS4231_MISC_INFO, 0x40);
+ for (i = 0; i < 16; ++i) {
+ if (snd_wss_in(chip, i) != snd_wss_in(chip, i + 16)) {
+ id = WSS_HW_CMI8330;
+ break;
+ }
+ }
+ snd_wss_out(chip, CS4231_MISC_INFO, 0x00);
+ if (id != WSS_HW_CMI8330 && (rev & 0x80))
+ id = WSS_HW_CS4248;
+ if (id == WSS_HW_CMI8330 && (rev & 0x0f) != 0x0a)
+ id = 0;
+ }
+ if (id == WSS_HW_CMI8330) {
+ /* verify it is not CS4231 by changing the version register */
+ /* on CMI8330 it is volume control register and can be set 0 */
+ snd_wss_out(chip, CS4231_MISC_INFO, CS4231_MODE2);
+ snd_wss_dout(chip, CS4231_VERSION, 0x00);
+ rev = snd_wss_in(chip, CS4231_VERSION) & 0xe7;
+ if (rev)
+ id = 0;
+ snd_wss_out(chip, CS4231_MISC_INFO, 0);
+ }
+ if (id)
+ chip->hardware = id;
+
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ return 0; /* all things are ok.. */
+}
+
+static int snd_wss_probe(struct snd_wss *chip)
+{
+ unsigned long flags;
+ int i, id, rev, regnum;
+ unsigned char *ptr;
+ unsigned int hw;
+
+ id = snd_ad1848_probe(chip);
+ if (id < 0)
+ return id;
hw = chip->hardware;
if ((hw & WSS_HW_TYPE_MASK) == WSS_HW_DETECT) {
+ for (i = 0; i < 50; i++) {
+ mb();
+ if (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT)
+ msleep(2);
+ else {
+ spin_lock_irqsave(&chip->reg_lock, flags);
+ snd_wss_out(chip, CS4231_MISC_INFO,
+ CS4231_MODE2);
+ id = snd_wss_in(chip, CS4231_MISC_INFO) & 0x0f;
+ spin_unlock_irqrestore(&chip->reg_lock, flags);
+ if (id == 0x0a)
+ break; /* this is valid value */
+ }
+ }
+ snd_printdd("wss: port = 0x%lx, id = 0x%x\n", chip->port, id);
+ if (id != 0x0a)
+ return -ENODEV; /* no valid device found */
+
rev = snd_wss_in(chip, CS4231_VERSION) & 0xe7;
snd_printdd("CS4231: VERSION (I25) = 0x%x\n", rev);
if (rev == 0x80) {
@@ -1176,7 +1260,8 @@ static int snd_wss_probe(struct snd_wss
mb();
spin_unlock_irqrestore(&chip->reg_lock, flags);
- chip->image[CS4231_MISC_INFO] = CS4231_MODE2;
+ if (!(chip->hardware & WSS_HW_AD1848_MASK))
+ chip->image[CS4231_MISC_INFO] = CS4231_MODE2;
switch (chip->hardware) {
case WSS_HW_INTERWAVE:
chip->image[CS4231_MISC_INFO] = CS4231_IW_MODE3;
@@ -1202,9 +1287,10 @@ static int snd_wss_probe(struct snd_wss
chip->hardware == WSS_HW_INTERWAVE ? 0xc2 : 0x01;
}
ptr = (unsigned char *) &chip->image;
+ regnum = (chip->hardware & WSS_HW_AD1848_MASK) ? 16 : 32;
snd_wss_mce_down(chip);
spin_lock_irqsave(&chip->reg_lock, flags);
- for (i = 0; i < 32; i++) /* ok.. fill all CS4231 registers */
+ for (i = 0; i < regnum; i++) /* ok.. fill all registers */
snd_wss_out(chip, i, *ptr++);
spin_unlock_irqrestore(&chip->reg_lock, flags);
snd_wss_mce_up(chip);
@@ -1614,6 +1700,10 @@ static int snd_wss_new(struct snd_card *
else
memcpy(&chip->image, &snd_wss_original_image,
sizeof(snd_wss_original_image));
+ if (chip->hardware & WSS_HW_AD1848_MASK) {
+ chip->image[CS4231_PIN_CTRL] = 0;
+ chip->image[CS4231_TEST_INIT] = 0;
+ }
*rchip = chip;
return 0;
@@ -1641,7 +1731,8 @@ int snd_wss_create(struct snd_card *card
chip->dma1 = -1;
chip->dma2 = -1;
- if ((chip->res_port = request_region(port, 4, "CS4231")) == NULL) {
+ chip->res_port = request_region(port, 4, "WSS");
+ if (chip->res_port == NULL) {
snd_printk(KERN_ERR "wss: can't grab port 0x%lx\n", port);
snd_wss_free(chip);
return -EBUSY;
@@ -1656,20 +1747,20 @@ int snd_wss_create(struct snd_card *card
chip->cport = cport;
if (!(hwshare & WSS_HWSHARE_IRQ))
if (request_irq(irq, snd_wss_interrupt, IRQF_DISABLED,
- "CS4231", (void *) chip)) {
+ "WSS", (void *) chip)) {
snd_printk(KERN_ERR "wss: can't grab IRQ %d\n", irq);
snd_wss_free(chip);
return -EBUSY;
}
chip->irq = irq;
- if (!(hwshare & WSS_HWSHARE_DMA1) && request_dma(dma1, "CS4231 - 1")) {
+ if (!(hwshare & WSS_HWSHARE_DMA1) && request_dma(dma1, "WSS - 1")) {
snd_printk(KERN_ERR "wss: can't grab DMA1 %d\n", dma1);
snd_wss_free(chip);
return -EBUSY;
}
chip->dma1 = dma1;
if (!(hwshare & WSS_HWSHARE_DMA2) && dma1 != dma2 &&
- dma2 >= 0 && request_dma(dma2, "CS4231 - 2")) {
+ dma2 >= 0 && request_dma(dma2, "WSS - 2")) {
snd_printk(KERN_ERR "wss: can't grab DMA2 %d\n", dma2);
snd_wss_free(chip);
return -EBUSY;
@@ -1680,6 +1771,12 @@ int snd_wss_create(struct snd_card *card
} else
chip->dma2 = dma2;
+ if (hardware == WSS_HW_THINKPAD) {
+ chip->thinkpad_flag = 1;
+ chip->hardware = WSS_HW_DETECT; /* reset */
+ snd_wss_thinkpad_twiddle(chip, 1);
+ }
+
/* global setup */
if (snd_wss_probe(chip) < 0) {
snd_wss_free(chip);
@@ -1749,12 +1846,6 @@ int snd_wss_pcm(struct snd_wss *chip, in
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_wss_playback_ops);
snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &snd_wss_capture_ops);
- /* temporary */
- if (chip->hardware & WSS_HW_AD1848_MASK) {
- chip->rate_constraint = snd_wss_xrate;
- chip->set_playback_format = snd_wss_playback_format;
- chip->set_capture_format = snd_wss_capture_format;
- }
/* global setup */
pcm->private_data = chip;
pcm->info_flags = 0;
diff -urp linux-alsa/sound/isa/opti9xx/opti92x-ad1848.c linux-mm/sound/isa/opti9xx/opti92x-ad1848.c
--- linux-alsa/sound/isa/opti9xx/opti92x-ad1848.c 2008-07-18 07:51:49.874759721 +0200
+++ linux-mm/sound/isa/opti9xx/opti92x-ad1848.c 2008-07-18 16:54:21.131152444 +0200
@@ -33,11 +33,7 @@
#include <asm/io.h>
#include <asm/dma.h>
#include <sound/core.h>
-#if defined(CS4231) || defined(OPTi93X)
#include <sound/wss.h>
-#else
-#include <sound/ad1848.h>
-#endif /* CS4231 */
#include <sound/mpu401.h>
#include <sound/opl3.h>
#ifndef OPTi93X
@@ -53,16 +49,10 @@ MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("OPTi93X");
MODULE_SUPPORTED_DEVICE("{{OPTi,82C931/3}}");
#else /* OPTi93X */
-#ifdef CS4231
-MODULE_DESCRIPTION("OPTi92X - CS4231");
-MODULE_SUPPORTED_DEVICE("{{OPTi,82C924 (CS4231)},"
- "{OPTi,82C925 (CS4231)}}");
-#else /* CS4231 */
-MODULE_DESCRIPTION("OPTi92X - AD1848");
-MODULE_SUPPORTED_DEVICE("{{OPTi,82C924 (AD1848)},"
- "{OPTi,82C925 (AD1848)},"
+MODULE_DESCRIPTION("OPTi92X - WSS");
+MODULE_SUPPORTED_DEVICE("{{OPTi,82C924 (WSS)},"
+ "{OPTi,82C925 (WSS)}},"
"{OAK,Mozart}}");
-#endif /* CS4231 */
#endif /* OPTi93X */
static int index = SNDRV_DEFAULT_IDX1; /* Index 0-MAX */
@@ -144,9 +134,7 @@ struct snd_opti9xx {
long wss_base;
int irq;
int dma1;
-#if defined(CS4231) || defined(OPTi93X)
int dma2;
-#endif /* CS4231 || OPTi93X */
long fm_port;
@@ -221,9 +209,7 @@ static int __devinit snd_opti9xx_init(st
chip->wss_base = -1;
chip->irq = -1;
chip->dma1 = -1;
-#if defined(CS4231) || defined (OPTi93X)
chip->dma2 = -1;
-#endif /* CS4231 || OPTi93X */
chip->fm_port = -1;
chip->mpu_port = -1;
chip->mpu_irq = -1;
@@ -688,7 +674,7 @@ static void snd_card_opti9xx_free(struct
if (chip) {
#ifdef OPTi93X
struct snd_wss *codec = chip->codec;
- if (codec->irq > 0) {
+ if (codec && codec->irq > 0) {
disable_irq(codec->irq);
free_irq(codec->irq, codec);
}
@@ -736,7 +722,6 @@ static int __devinit snd_opti9xx_probe(s
if (error)
return error;
-#if defined(CS4231) || defined(OPTi93X)
error = snd_wss_create(card, chip->wss_base + 4, -1,
chip->irq, chip->dma1, chip->dma2,
#ifdef CS4231
@@ -750,12 +735,6 @@ static int __devinit snd_opti9xx_probe(s
#ifdef OPTi93X
chip->codec = codec;
#endif
-#else
- error = snd_ad1848_create(card, chip->wss_base + 4, chip->irq,
- chip->dma1, WSS_HW_DETECT, &codec);
- if (error < 0)
- return error;
-#endif
error = snd_wss_pcm(codec, 0, &pcm);
if (error < 0)
return error;
----------------------------------------------------------------------
Zobacz cala prawde o Lukaszu Podolskim!
kliknij >>> http://link.interia.pl/f1e57
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-18 19:51 [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one Krzysztof Helt
@ 2008-07-20 16:09 ` Rene Herman
2008-07-23 21:28 ` Rene Herman
0 siblings, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-20 16:09 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 18-07-08 21:51, Krzysztof Helt wrote:
> From: Krzysztof Helt <krzysztof.h1@wp.pl>
>
> Use the wss detection code and kill the ad1848 library.
> The library is fully assimilated into the new wss library.
>
> This patch fixes also the issue with AD1848 codec
> MCE timeout - a waiting loop is now 10 times longer.
>
> I have tested it on following cards:
> Gallant SC-6600 (codec: AD1848, driver: snd-sc6600]
> SoundScape VIVO/90 (codec: AD1845, driver: snd-sscape]
> SG Waverider (codec: CS4231A, driver: Rene Herman's snd-galaxy]
Now that you mention it... I should revive and submit that thing.
> Opti930 (codec: built-in - CS4231 compatible, driver: snd-opti93x]
> Opti931 (codec: built-in - CS4231 compatible, driver: snd-opti93x]
> Gallant SC-70P (chip/codec: CS4237B, driver: snd-cs4236]
>
> Sound playback and recording works on all these cards.
>
> Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
Just a quick note to say I'm now running this series (and am at the
moment listening to a snd-cs4236 card) on top of Takashi's sound-2.6
tree (and including the PNP stuff that's going into 2.6.27 but that's
unrelated).
For anyone interested, I have placed the complete series as I have it
here at:
http://members.home.nl/rene.herman/wss/
It's against current ALSA. I'll give this a proper review early this
coming week. Takashi was away for the week so that should get things
looked at before he returns.
Thanks!
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-20 16:09 ` Rene Herman
@ 2008-07-23 21:28 ` Rene Herman
2008-07-24 5:26 ` Krzysztof Helt
0 siblings, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-23 21:28 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 20-07-08 18:09, Rene Herman wrote:
> For anyone interested, I have placed the complete series as I have it
> here at:
>
> http://members.home.nl/rene.herman/wss/
>
> It's against current ALSA. I'll give this a proper review early this
> coming week. Takashi was away for the week so that should get things
> looked at before he returns.
*** 0001-wss_lib-move-cs4231_lib-into-wss_lib.patch
Acked-by: Rene Herman <rene.herman@gmail.com>
*** 0002-wss_lib-rename-cs4231.h-into-wss.h.patch
Acked-by: Rene Herman <rene.herman@gmail.com>
*** 0003-wss-rename-cs4321_foo-to-wss_foo.patch
> -#define CS4231_HW_DETECT 0x0000 /* let CS4231 driver detect chip */
[ ... ]
> +#define WSS_HW_DETECT 0x0000 /* let CS4231 driver detect chip */
The comment likes to be "let WSS driver [ ... ]". But please don't even
think of actually changing that. Only pointed out to dazzle you with my
unrelenting eye for detail.
> diff --git a/sound/isa/ad1848/ad1848.c b/sound/isa/ad1848/ad1848.c
> index 5f5271e..3500548 100644
> --- a/sound/isa/ad1848/ad1848.c
> +++ b/sound/isa/ad1848/ad1848.c
> @@ -70,15 +70,15 @@ static int __devinit snd_ad1848_match(struct device *dev, unsigned int n)
> return 0;
>
> if (port[n] == SNDRV_AUTO_PORT) {
> - snd_printk(KERN_ERR "%s: please specify port\n", dev->bus_id);
> + snd_printk(KERN_ERR "%s: please specify port\n", dev_name(dev));
> return 0;
> }
The comment in dev_name() makes me wonder a bit but I guess we'll deal
with it then if there's anything to deal with.
> -CS4236_DOUBLE("Master Digital Playback Switch", 0, CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> -CS4236_DOUBLE("Master Digital Capture Switch", 0, CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
> +CS4236_DOUBLE("Master Digital Playback Switch", 0,
> + CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> +CS4236_DOUBLE("Master Digital Capture Switch", 0,
> + CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
I can't say I'm personally a fan of these kinds of changes. The point of
them would supposedly be to make the code more readable but as far as I
am concerned it does the reverse.
I know that Takashi can be an 80-column fundamentalist so I'll not
object I guess. I'd personally like these (all) restored to a single
line but if he doesn't, so be it.
> --- a/sound/isa/wss/wss_lib.c
> +++ b/sound/isa/wss/wss_lib.c
[ ... ]
> -static inline void cs4231_outb(struct snd_cs4231 *chip, u8 offset, u8 val)
> +static inline void wss_outb(struct snd_wss *chip, u8 offset, u8 val)
> {
> outb(val, chip->port + offset);
> }
> +EXPORT_SYMBOL(snd_wss_out);
This EXPORT_SYMBOL() is in the wrong spot (ie, not after snd_wss_out).
> +static void snd_wss_debug(struct snd_wss *chip)
> +{
> + printk(KERN_DEBUG "CS4231 REGS: INDEX = 0x%02x ",
> + wss_inb(chip, CS4231P(REGSEL)));
> + printk(KERN_DEBUG " STATUS = 0x%02x\n",
> + wss_inb(chip, CS4231P(STATUS)));
This is an even worse example of the 80-column change here. Just makes
it worse as it destroys the What You Write Is What You Get format.
> +static void snd_wss_playback_format(struct snd_wss *chip,
[ ... ]
> - snd_cs4231_out(chip, CS4231_ALT_FEATURE_1, chip->image[CS4231_ALT_FEATURE_1] | 0x10);
> - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT, chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> - snd_cs4231_out(chip, CS4231_ALT_FEATURE_1, chip->image[CS4231_ALT_FEATURE_1] &= ~0x10);
> + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
> + chip->image[CS4231_ALT_FEATURE_1] | 0x10);
> + chip->image[CS4231_PLAYBK_FORMAT] = pdfr;
> + snd_wss_out(chip, CS4231_PLAYBK_FORMAT,
> + chip->image[CS4231_PLAYBK_FORMAT]);
> + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
> + chip->image[CS4231_ALT_FEATURE_1] &= ~0x10);
[ .. ]
> if (full_calib) {
> - snd_cs4231_mce_up(chip);
> + snd_wss_mce_up(chip);
> spin_lock_irqsave(&chip->reg_lock, flags);
> - if (chip->hardware != CS4231_HW_INTERWAVE && !chip->single_dma) {
> - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT,
> + if (chip->hardware != WSS_HW_INTERWAVE && !chip->single_dma) {
> + snd_wss_out(chip, CS4231_PLAYBK_FORMAT,
> (chip->image[CS4231_IFACE_CTRL] & CS4231_RECORD_ENABLE) ?
> (pdfr & 0xf0) | (chip->image[CS4231_REC_FORMAT] & 0x0f) :
> - pdfr);
> + pdfr);
> } else {
> - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT, chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> + snd_wss_out(chip, CS4231_PLAYBK_FORMAT,
> + chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> }
As long as you're at it... could you split this assignment same as you
did above?
> +static int snd_wss_timer_stop(struct snd_timer *timer)
> {
> unsigned long flags;
> - struct snd_cs4231 *chip = snd_timer_chip(timer);
> + struct snd_wss *chip = snd_timer_chip(timer);
> spin_lock_irqsave(&chip->reg_lock, flags);
> - snd_cs4231_out(chip, CS4231_ALT_FEATURE_1, chip->image[CS4231_ALT_FEATURE_1] &= ~CS4231_TIMER_ENABLE);
> + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
> + chip->image[CS4231_ALT_FEATURE_1] &= ~CS4231_TIMER_ENABLE);
> spin_unlock_irqrestore(&chip->reg_lock, flags);
> return 0;
> }
Same.
> -static snd_pcm_uframes_t snd_cs4231_playback_pointer(struct snd_pcm_substream *substream)
> +static snd_pcm_uframes_t snd_wss_playback_pointer
> + (struct snd_pcm_substream *substream)
>
Please don't split function names quite like that. If the 80-column
thing must be at least keep the opening brace on the same line.
Same thing for snd_wss_capture_pointer() just below that one.
> @@ -1475,32 +1568,36 @@ int snd_cs4231_create(struct snd_card *card,
> chip->dma2 = -1;
>
> if ((chip->res_port = request_region(port, 4, "CS4231")) == NULL) {
> - snd_printk(KERN_ERR "cs4231: can't grab port 0x%lx\n", port);
> - snd_cs4231_free(chip);
> + snd_printk(KERN_ERR "wss: can't grab port 0x%lx\n", port);
> + snd_wss_free(chip);
> return -EBUSY;
> }
> chip->port = port;
> if ((long)cport >= 0 && (chip->res_cport = request_region(cport, 8, "CS4232 Control")) == NULL) {
You've left these two if assignments in. You do all the others so I
assume you forgot.
> +static struct snd_kcontrol_new snd_wss_controls[] = {
> +WSS_DOUBLE("PCM Playback Switch", 0,
> + CS4231_LEFT_OUTPUT, CS4231_RIGHT_OUTPUT, 7, 7, 1, 1),
Same comment about the split.
> +module_init(alsa_wss_init)
> +module_exit(alsa_wss_exit)
Can you add a ";" after these? These macros should've not included them.
Just makes for an odd special case to remember (yes, I know there are
more of them in the tree, but I myself fix it when I get close also).
...
Right-o. That was a 4000+ line patch and I ran out of evening. Rest will
have to wait for tomorrow then. If you make changes, could you very
specifically indicate those changes so that I might be able to get away
with not reading the entire thing again? :-/
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-23 21:28 ` Rene Herman
@ 2008-07-24 5:26 ` Krzysztof Helt
2008-07-24 9:31 ` Rene Herman
2008-07-24 17:19 ` Rene Herman
0 siblings, 2 replies; 22+ messages in thread
From: Krzysztof Helt @ 2008-07-24 5:26 UTC (permalink / raw)
To: Rene Herman; +Cc: Takashi Iwai, Alsa-devel
On Wed, 23 Jul 2008 23:28:47 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:
> On 20-07-08 18:09, Rene Herman wrote:
>
> > For anyone interested, I have placed the complete series as I have it
> > here at:
> >
> > http://members.home.nl/rene.herman/wss/
> >
> > It's against current ALSA. I'll give this a proper review early this
> > coming week. Takashi was away for the week so that should get things
> > looked at before he returns.
>
> *** 0001-wss_lib-move-cs4231_lib-into-wss_lib.patch
>
> Acked-by: Rene Herman <rene.herman@gmail.com>
>
> *** 0002-wss_lib-rename-cs4231.h-into-wss.h.patch
>
> Acked-by: Rene Herman <rene.herman@gmail.com>
>
If you have done so detailed review of patches as below
you can add: Reviewed-by which gives you credits
for your effort (and is considered stronger by Andrew
Morton at least).
> *** 0003-wss-rename-cs4321_foo-to-wss_foo.patch
>
> > -#define CS4231_HW_DETECT 0x0000 /* let CS4231 driver detect chip */
> [ ... ]
> > +#define WSS_HW_DETECT 0x0000 /* let CS4231 driver detect chip */
>
> The comment likes to be "let WSS driver [ ... ]". But please don't even
> think of actually changing that. Only pointed out to dazzle you with my
> unrelenting eye for detail.
>
This series of patches gives unification. I found few issues in the wss_lib
which can be improved but I didn't want to mix them up with
these patches.
> > diff --git a/sound/isa/ad1848/ad1848.c b/sound/isa/ad1848/ad1848.c
> > index 5f5271e..3500548 100644
> > --- a/sound/isa/ad1848/ad1848.c
> > +++ b/sound/isa/ad1848/ad1848.c
> > @@ -70,15 +70,15 @@ static int __devinit snd_ad1848_match(struct device *dev, unsigned int n)
> > return 0;
> >
> > if (port[n] == SNDRV_AUTO_PORT) {
> > - snd_printk(KERN_ERR "%s: please specify port\n", dev->bus_id);
> > + snd_printk(KERN_ERR "%s: please specify port\n", dev_name(dev));
> > return 0;
> > }
>
> The comment in dev_name() makes me wonder a bit but I guess we'll deal
> with it then if there's anything to deal with.
>
This is not my change. It is probably diff between some -mm kernel version and alsa-driver.
It should not be there as the ad1848.c is going to be deleted.
> > -CS4236_DOUBLE("Master Digital Playback Switch", 0, CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> > -CS4236_DOUBLE("Master Digital Capture Switch", 0, CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
> > +CS4236_DOUBLE("Master Digital Playback Switch", 0,
> > + CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> > +CS4236_DOUBLE("Master Digital Capture Switch", 0,
> > + CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
>
> I can't say I'm personally a fan of these kinds of changes. The point of
> them would supposedly be to make the code more readable but as far as I
> am concerned it does the reverse.
>
> I know that Takashi can be an 80-column fundamentalist so I'll not
> object I guess. I'd personally like these (all) restored to a single
> line but if he doesn't, so be it.
>
Exactly. It was done for Takashi.
> > --- a/sound/isa/wss/wss_lib.c
> > +++ b/sound/isa/wss/wss_lib.c
>
> [ ... ]
>
> > -static inline void cs4231_outb(struct snd_cs4231 *chip, u8 offset, u8 val)
> > +static inline void wss_outb(struct snd_wss *chip, u8 offset, u8 val)
> > {
> > outb(val, chip->port + offset);
> > }
> > +EXPORT_SYMBOL(snd_wss_out);
>
> This EXPORT_SYMBOL() is in the wrong spot (ie, not after snd_wss_out).
>
Ok.
> > +static void snd_wss_debug(struct snd_wss *chip)
> > +{
> > + printk(KERN_DEBUG "CS4231 REGS: INDEX = 0x%02x ",
> > + wss_inb(chip, CS4231P(REGSEL)));
> > + printk(KERN_DEBUG " STATUS = 0x%02x\n",
> > + wss_inb(chip, CS4231P(STATUS)));
>
> This is an even worse example of the 80-column change here. Just makes
> it worse as it destroys the What You Write Is What You Get format.
>
Again. It was done for Takashi. I was unsure how to proceed in case of
renaming constants in lines like these. They could be left one-line
because it was not regression.
> > +static void snd_wss_playback_format(struct snd_wss *chip,
>
> [ ... ]
>
> > - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT, chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> > + chip->image[CS4231_PLAYBK_FORMAT] = pdfr;
> > + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
> > + chip->image[CS4231_ALT_FEATURE_1] &= ~0x10);
>
> [ .. ]
>
> > if (full_calib) {
> > - snd_cs4231_mce_up(chip);
> > + snd_wss_mce_up(chip);
> > spin_lock_irqsave(&chip->reg_lock, flags);
> > - if (chip->hardware != CS4231_HW_INTERWAVE && !chip->single_dma) {
> > - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT,
> > + if (chip->hardware != WSS_HW_INTERWAVE && !chip->single_dma) {
> > + snd_wss_out(chip, CS4231_PLAYBK_FORMAT,
> > (chip->image[CS4231_IFACE_CTRL] & CS4231_RECORD_ENABLE) ?
> > (pdfr & 0xf0) | (chip->image[CS4231_REC_FORMAT] & 0x0f) :
> > - pdfr);
> > + pdfr);
> > } else {
> > - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT, chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> > + snd_wss_out(chip, CS4231_PLAYBK_FORMAT,
> > + chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
> > }
>
> As long as you're at it... could you split this assignment same as you
> did above?
>
Ok. I did it above because otherwise the line was too long ...
> > +static int snd_wss_timer_stop(struct snd_timer *timer)
> > {
> > unsigned long flags;
> > - struct snd_cs4231 *chip = snd_timer_chip(timer);
> > + struct snd_wss *chip = snd_timer_chip(timer);
> > spin_lock_irqsave(&chip->reg_lock, flags);
> > - snd_cs4231_out(chip, CS4231_ALT_FEATURE_1, chip->image[CS4231_ALT_FEATURE_1] &= ~CS4231_TIMER_ENABLE);
> > + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
> > + chip->image[CS4231_ALT_FEATURE_1] &= ~CS4231_TIMER_ENABLE);
> > spin_unlock_irqrestore(&chip->reg_lock, flags);
> > return 0;
> > }
>
> Same.
Ok.
>
> > -static snd_pcm_uframes_t snd_cs4231_playback_pointer(struct snd_pcm_substream *substream)
> > +static snd_pcm_uframes_t snd_wss_playback_pointer
> > + (struct snd_pcm_substream *substream)
> >
>
> Please don't split function names quite like that. If the 80-column
> thing must be at least keep the opening brace on the same line.
>
> Same thing for snd_wss_capture_pointer() just below that one.
Ok.
>
> > @@ -1475,32 +1568,36 @@ int snd_cs4231_create(struct snd_card *card,
> > chip->dma2 = -1;
> >
> > if ((chip->res_port = request_region(port, 4, "CS4231")) == NULL) {
> > - snd_printk(KERN_ERR "cs4231: can't grab port 0x%lx\n", port);
> > - snd_cs4231_free(chip);
> > + snd_printk(KERN_ERR "wss: can't grab port 0x%lx\n", port);
> > + snd_wss_free(chip);
> > return -EBUSY;
> > }
> > chip->port = port;
> > if ((long)cport >= 0 && (chip->res_cport = request_region(cport, 8, "CS4232 Control")) == NULL) {
>
> You've left these two if assignments in. You do all the others so I
> assume you forgot.
>
I could miss them. I used the checkpatch script and it is not checking context lines
as new lines.
> > +static struct snd_kcontrol_new snd_wss_controls[] = {
> > +WSS_DOUBLE("PCM Playback Switch", 0,
> > + CS4231_LEFT_OUTPUT, CS4231_RIGHT_OUTPUT, 7, 7, 1, 1),
>
> Same comment about the split.
>
> > +module_init(alsa_wss_init)
> > +module_exit(alsa_wss_exit)
>
> Can you add a ";" after these? These macros should've not included them.
> Just makes for an odd special case to remember (yes, I know there are
> more of them in the tree, but I myself fix it when I get close also).
>
Ok.
> ...
>
> Right-o. That was a 4000+ line patch and I ran out of evening. Rest will
> have to wait for tomorrow then. If you make changes, could you very
> specifically indicate those changes so that I might be able to get away
> with not reading the entire thing again? :-/
>
Yes.
> Rene.
>
Thank you for your review and time.
Krzysztof
----------------------------------------------------------------------
Rowerem po Roztoczu.
Zobacz relacje >>> http://link.interia.pl/f1e65
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 5:26 ` Krzysztof Helt
@ 2008-07-24 9:31 ` Rene Herman
2008-07-28 15:37 ` Takashi Iwai
2008-07-24 17:19 ` Rene Herman
1 sibling, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-24 9:31 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 24-07-08 07:26, Krzysztof Helt wrote:
>>> diff --git a/sound/isa/ad1848/ad1848.c b/sound/isa/ad1848/ad1848.c
>>> index 5f5271e..3500548 100644
>>> --- a/sound/isa/ad1848/ad1848.c
>>> +++ b/sound/isa/ad1848/ad1848.c
>>> @@ -70,15 +70,15 @@ static int __devinit snd_ad1848_match(struct device *dev, unsigned int n)
>>> return 0;
>>>
>>> if (port[n] == SNDRV_AUTO_PORT) {
>>> - snd_printk(KERN_ERR "%s: please specify port\n", dev->bus_id);
>>> + snd_printk(KERN_ERR "%s: please specify port\n", dev_name(dev));
>>> return 0;
>>> }
>> The comment in dev_name() makes me wonder a bit but I guess we'll deal
>> with it then if there's anything to deal with.
>
> This is not my change. It is probably diff between some -mm kernel
> version and alsa-driver. It should not be there as the ad1848.c is
> going to be deleted.
I see. It's definitely from the patch you sent though:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/008978.html
>>> -CS4236_DOUBLE("Master Digital Playback Switch", 0, CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
>>> -CS4236_DOUBLE("Master Digital Capture Switch", 0, CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
>>> +CS4236_DOUBLE("Master Digital Playback Switch", 0,
>>> + CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
>>> +CS4236_DOUBLE("Master Digital Capture Switch", 0,
>>> + CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
>> I can't say I'm personally a fan of these kinds of changes. The point of
>> them would supposedly be to make the code more readable but as far as I
>> am concerned it does the reverse.
>>
>> I know that Takashi can be an 80-column fundamentalist so I'll not
>> object I guess. I'd personally like these (all) restored to a single
>> line but if he doesn't, so be it.
>
> Exactly. It was done for Takashi.
Yes, he overrides. I'd try to get away with just saying no though. That
checkpatch thing desperately needs a clue.
>>> +static void snd_wss_playback_format(struct snd_wss *chip,
>> [ ... ]
>>
>>> - snd_cs4231_out(chip, CS4231_PLAYBK_FORMAT, chip->image[CS4231_PLAYBK_FORMAT] = pdfr);
>>> + chip->image[CS4231_PLAYBK_FORMAT] = pdfr;
>>> + snd_wss_out(chip, CS4231_ALT_FEATURE_1,
>>> + chip->image[CS4231_ALT_FEATURE_1] &= ~0x10);
Oh, missed commenting on this assignment-in-argument due to commenting
on the other one...
>> As long as you're at it... could you split this assignment same as you
>> did above?
>>
>
> Ok. I did it above because otherwise the line was too long ...
Okay. Style-consistency really is fairly important. Otherwise you each
time have to "retrain" while reading code. In that sense, even leaving
them all in would/can be better than some in, some out although in this
case "all out" is quite preffered.
(I only looked at the patch itself though -- if there are many more of
these than leaving them in and (perhaps) fixing it in a follow-up might
be preferred).
>> Right-o. That was a 4000+ line patch and I ran out of evening. Rest will
>> have to wait for tomorrow then. If you make changes, could you very
>> specifically indicate those changes so that I might be able to get away
>> with not reading the entire thing again? :-/
>
> Yes.
Thank you :-)
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 5:26 ` Krzysztof Helt
2008-07-24 9:31 ` Rene Herman
@ 2008-07-24 17:19 ` Rene Herman
2008-07-24 18:47 ` Krzysztof Helt
1 sibling, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-24 17:19 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
*** 0001-wss_lib-move-cs4231_lib-into-wss_lib.patch
Reviewed-by: Rene Herman <rene.herman@gmail.ccom>
*** 0002-wss_lib-rename-cs4231.h-into-wss.h.patch
Reviewed-by: Rene Herman <rene.herman@gmail.ccom>
*** 0003-wss-rename-cs4321_foo-to-wss_foo.patch
Outstanding comments.
*** 0004-wss-use-stuct-snd_wss-instead-of-snd_ad1848.patch
> +++ b/include/sound/ad1848.h
>
> [ ... ]
>
> +#include "wss.h"
Bad (not at top and "") but given that it's going anyway...
*** 0005-wss_lib-use-wss-constants-instead-of-ad1848-ones.patch
> --- a/sound/isa/ad1848/ad1848_lib.c
> +++ b/sound/isa/ad1848/ad1848_lib.c
> @@ -283,14 +283,12 @@ static int snd_ad1848_trigger(struct snd_wss *chip, unsigned char what,
> return 0;
> }
> snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL] |= what);
> - chip->mode |= AD1848_MODE_RUNNING;
Is this now done in generic code? Not necessary anymore? Was no comment
in the changelog.
> static const char *snd_ad1848_chip_id(struct snd_wss *chip)
> {
> switch (chip->hardware) {
> - case AD1848_HW_AD1847: return "AD1847";
> - case AD1848_HW_AD1848: return "AD1848";
> - case AD1848_HW_CS4248: return "CS4248";
> - case AD1848_HW_CMI8330: return "CMI8330/C3D";
> - default: return "???";
> + case WSS_HW_AD1847:
> + return "AD1847";
> + case WSS_HW_AD1848:
> + return "AD1848";
> + case WSS_HW_CS4248:
> + return "CS4248";
> + case WSS_HW_CMI8330:
> + return "CMI8330/C3D";
> + default:
> + return "???";
> }
> }
These kinds of switches are just about the only time you get to
knowingly ignore coding style and you forego the opportunity? Tsssk...
> --- a/sound/isa/opti9xx/opti92x-ad1848.c
> +++ b/sound/isa/opti9xx/opti92x-ad1848.c
> @@ -775,7 +775,7 @@ static int __devinit snd_opti9xx_probe(struct snd_card *card)
> #else
> if ((error = snd_ad1848_create(card, chip->wss_base + 4,
> chip->irq, chip->dma1,
> - AD1848_HW_DETECT, &codec)) < 0)
> + WSS_HW_DETECT, &codec)) < 0)
> return error;
> if ((error = snd_ad1848_pcm(codec, 0, &pcm)) < 0)
> return error;
and
> --- a/sound/isa/sgalaxy.c
> +++ b/sound/isa/sgalaxy.c
> @@ -265,7 +265,7 @@ static int __devinit snd_sgalaxy_probe(struct device *devptr, unsigned int dev)
>
> if ((err = snd_ad1848_create(card, wssport[dev] + 4,
> xirq, xdma1,
> - AD1848_HW_DETECT, &chip)) < 0)
> + WSS_HW_DETECT, &chip)) < 0)
> goto _err;
> card->private_data = chip;
For those that come after -- those error ifs are split when they are
turned into snd_wss_create() later.
*** 0006-wss_lib-replace-ad1848-mixer-element-macros-with-ws.patch
> #define AD1848_SINGLE_TLV(xname, xindex, reg, shift, mask, invert, xtlv) \
> -{ .name = xname, \
> - .index = xindex, \
> - .type = AD1848_MIX_SINGLE, \
> - .private_value = AD1848_MIXVAL_SINGLE(reg, shift, mask, invert), \
> - .tlv = xtlv }
> +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> + .name = xname, .index = xindex, \
> + .info = snd_ad1848_info_single, \
> + .get = snd_ad1848_get_single, .put = snd_ad1848_put_single, \
> + .private_value = reg | (shift << 8) | (mask << 16) | (invert << 24), \
> + .tlv = { .p = (xtlv) } }
Please just one per line (aligned is nice...)
> #define AD1848_DOUBLE_TLV(xname, xindex, left_reg, right_reg, shift_left, shift_right, mask, invert, xtlv) \
> -{ .name = xname, \
> - .index = xindex, \
> - .type = AD1848_MIX_DOUBLE, \
> - .private_value = AD1848_MIXVAL_DOUBLE(left_reg, right_reg, shift_left, shift_right, mask, invert), \
> - .tlv = xtlv }
> -
> -static struct ad1848_mix_elem snd_ad1848_controls[] = {
> -AD1848_DOUBLE("PCM Playback Switch", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT, 7, 7, 1, 1),
> -AD1848_DOUBLE_TLV("PCM Playback Volume", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT, 0, 0, 63, 1,
> +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> + .name = xname, .index = xindex, \
> + .info = snd_ad1848_info_double, \
> + .get = snd_ad1848_get_double, .put = snd_ad1848_put_double, \
> + .private_value = left_reg | (right_reg << 8) | (shift_left << 16) | \
> + (shift_right << 19) | (mask << 24) | (invert << 22), \
> + .tlv = { .p = (xtlv) } }
Same.
Throughout this patch there's also still the comment about ignoring the
80-column thing with these macros. The cmi8330.c ones are a wonderful
example of how much worse it gets. It's horrible.
*** 0007-wss_lib-use-CS4231P-instead-of-AD1848P-kill-the-AD.patch
> static void snd_ad1848_debug(struct snd_wss *chip)
Same. Otherwise
Reviewed-by: Rene Herman <rene.herman@gmail.com>
One other comment:
> diff --git a/sound/isa/wss/wss_lib.c b/sound/isa/wss/wss_lib.c
> index 08997d0..f695c85 100644
> --- a/sound/isa/wss/wss_lib.c
> +++ b/sound/isa/wss/wss_lib.c
> @@ -164,7 +164,6 @@ static inline void wss_outb(struct snd_wss *chip, u8 offset, u8 val)
> {
> outb(val, chip->port + offset);
> }
> -EXPORT_SYMBOL(snd_wss_out);
>
> static inline u8 wss_inb(struct snd_wss *chip, u8 offset)
> {
> @@ -228,6 +227,7 @@ void snd_wss_out(struct snd_wss *chip, unsigned char reg, unsigned char value)
> snd_printdd("codec out - reg 0x%x = 0x%x\n",
> chip->mce_bit | reg, value);
> }
> +EXPORT_SYMBOL(snd_wss_out);
Ah. This fixes up one of my comments from yesterday already. If this
finds its way into 3, it should ofcourse be gone here.
*** 0008-wss_lib-use-wss-mixer-code-instead-of-ad1848-one.patch
> +#define WSS_SINGLE_TLV(xname, xindex, reg, shift, mask, invert, xtlv) \
> +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> + .name = xname, .index = xindex, \
> + .info = snd_wss_info_single, \
> + .get = snd_wss_get_single, .put = snd_wss_put_single, \
> + .private_value = reg | (shift << 8) | (mask << 16) | (invert << 24), \
> + .tlv = { .p = (xtlv) } }
> +
> +#define WSS_DOUBLE_TLV(xname, xindex, left_reg, right_reg, \
> + shift_left, shift_right, mask, invert, xtlv) \
> +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> + .name = xname, .index = xindex, \
> + .info = snd_wss_info_double, \
> + .get = snd_wss_get_double, .put = snd_wss_put_double, \
> + .private_value = left_reg | (right_reg << 8) | (shift_left << 16) | \
> + (shift_right << 19) | (mask << 24) | (invert << 22), \
> + .tlv = { .p = (xtlv) } }
Ofcourse same with the please one member per line.
*** 0009-wss_lib-use-wss-pcm-code-instead-of-ad1848-one.patch
> --- a/sound/isa/wss/wss_lib.c
> +++ b/sound/isa/wss/wss_lib.c
> @@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd_wss *chip)
> for (timeout = 5; timeout > 0; timeout--)
> wss_inb(chip, CS4231P(REGSEL));
> /* end of cleanup sequence */
> - for (timeout = 250;
> + for (timeout = 2500;
> timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
> timeout--)
> udelay(10);
Was this intentional? You comment on this in 10/10...
> @@ -1360,6 +1381,11 @@ static int snd_wss_capture_open(struct s
>
> runtime->hw = snd_wss_capture;
>
> + /* hardware limitation of older chipsets */
> + if (chip->hardware == WSS_HW_INTERWAVE && chip->dma1 > 3)
> + runtime->hw.formats &= ~(SNDRV_PCM_FMTBIT_IMA_ADPCM |
> + SNDRV_PCM_FMTBIT_S16_BE);
> +
If you'll be posting a V2 series, might as well fold 11/10 in here.
> const char *snd_wss_chip_id(struct snd_wss *chip)
Comment about the switch getting uglier again.
*** 0010-wss_lib-use-wss-detection-code-instead-of-ad1848-on.patch
> +++ b/sound/isa/wss/wss_lib.c
> @@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd_wss *chip)
> for (timeout = 5; timeout > 0; timeout--)
> wss_inb(chip, CS4231P(REGSEL));
> /* end of cleanup sequence */
> - for (timeout = 2500;
> + for (timeout = 25000;
> timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
> timeout--)
> udelay(10);
So now it's 100x total. Is this as planned?
> -static int snd_wss_probe(struct snd_wss *chip)
> +static int snd_ad1848_probe(struct snd_wss *chip)
> {
> unsigned long flags;
> - int i, id, rev;
> - unsigned char *ptr;
> - unsigned int hw;
> + int i, id, rev, ad1847;
>
> -#if 0
> - snd_wss_debug(chip);
> -#endif
> id = 0;
> - for (i = 0; i < 50; i++) {
> + ad1847 = 0;
> + for (i = 0; i < 1000; i++) {
> mb();
> - if (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT)
> - udelay(2000);
> + if (inb(chip->port + CS4231P(REGSEL)) & CS4231_INIT)
> + msleep(1);
Mmm. This was changed from a udelay(500) in the ad1848 case. It would be
better to keep these kinds of really functional changes seperated out.
[ ... ]
> + id = 0;
> + if (chip->hardware == WSS_HW_DETECT)
> + id = ad1847 ? WSS_HW_AD1847 : WSS_HW_AD1848;
> +
> + spin_lock_irqsave(&chip->reg_lock, flags);
> + inb(chip->port + CS4231P(STATUS)); /* clear any pendings IRQ */
> + outb(0, chip->port + CS4231P(STATUS));
The original snd_ad1848_probe() had this below the below tests (which
were also outside the spinlock)
> + mb();
> + if (id == WSS_HW_AD1848) {
> + /* check if there are more than 16 registers */
> + rev = snd_wss_in(chip, CS4231_MISC_INFO);
> + snd_wss_out(chip, CS4231_MISC_INFO, 0x40);
> + for (i = 0; i < 16; ++i) {
> + if (snd_wss_in(chip, i) != snd_wss_in(chip, i + 16)) {
> + id = WSS_HW_CMI8330;
> + break;
> + }
> + }
> + snd_wss_out(chip, CS4231_MISC_INFO, 0x00);
> + if (id != WSS_HW_CMI8330 && (rev & 0x80))
> + id = WSS_HW_CS4248;
This one could also wind up different. Originally, snd_ad1848_probe()
would conclude CS4248 immediately upon (rev & 0x80) without doing that
register test. It's probably all fine, but snd_ad1848 function changed
very significantly in the move and I'd rather it not do that. A patch
12 alone is much easier reviewable and any possible difficulty much
easier bisectable. Could you do that?
*** 0011-wss-fix-capture-formats-limitations.patch
Can be folded, but otherwise no comments...
Will also put a next series through some tests here locally. I'l test at
least a few OPTi 92x cards since I don't see these in your report. Any
specific hardware you'd want tested more than others? I might have it.
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 17:19 ` Rene Herman
@ 2008-07-24 18:47 ` Krzysztof Helt
2008-07-24 19:30 ` Rene Herman
0 siblings, 1 reply; 22+ messages in thread
From: Krzysztof Helt @ 2008-07-24 18:47 UTC (permalink / raw)
To: Rene Herman; +Cc: Takashi Iwai, Alsa-devel
On Thu, 24 Jul 2008 19:19:15 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:
> *** 0004-wss-use-stuct-snd_wss-instead-of-snd_ad1848.patch
>
> > +++ b/include/sound/ad1848.h
> >
> > [ ... ]
> >
> > +#include "wss.h"
>
> Bad (not at top and "") but given that it's going anyway...
>
During changes like renaming I got into "dummy" mode so I changed without deep thinking.
>
> *** 0005-wss_lib-use-wss-constants-instead-of-ad1848-ones.patch
>
> > --- a/sound/isa/ad1848/ad1848_lib.c
> > +++ b/sound/isa/ad1848/ad1848_lib.c
> > @@ -283,14 +283,12 @@ static int snd_ad1848_trigger(struct snd_wss *chip, unsigned char what,
> > return 0;
> > }
> > snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL] |= what);
> > - chip->mode |= AD1848_MODE_RUNNING;
>
> Is this now done in generic code? Not necessary anymore? Was no comment
> in the changelog.
>
The MODE_RUNNING is not used at all in the cs4231 code. I wonder what the purpose of it.
> > static const char *snd_ad1848_chip_id(struct snd_wss *chip)
> > {
> > switch (chip->hardware) {
> > - case AD1848_HW_AD1847: return "AD1847";
> > - case AD1848_HW_AD1848: return "AD1848";
> > - case AD1848_HW_CS4248: return "CS4248";
> > - case AD1848_HW_CMI8330: return "CMI8330/C3D";
> > - default: return "???";
> > + case WSS_HW_AD1847:
> > + return "AD1847";
> > + case WSS_HW_AD1848:
> > + return "AD1848";
> > + case WSS_HW_CS4248:
> > + return "CS4248";
> > + case WSS_HW_CMI8330:
> > + return "CMI8330/C3D";
> > + default:
> > + return "???";
> > }
> > }
>
> These kinds of switches are just about the only time you get to
> knowingly ignore coding style and you forego the opportunity? Tsssk...
>
If switches like these generates more than one warnings
by the checkpatch script I change them to be on safer side
with "checkpatch fundamentalists". Ok?
> > --- a/sound/isa/opti9xx/opti92x-ad1848.c
> > +++ b/sound/isa/opti9xx/opti92x-ad1848.c
> > @@ -775,7 +775,7 @@ static int __devinit snd_opti9xx_probe(struct snd_card *card)
> > #else
> > if ((error = snd_ad1848_create(card, chip->wss_base + 4,
> > chip->irq, chip->dma1,
> > - AD1848_HW_DETECT, &codec)) < 0)
> > + WSS_HW_DETECT, &codec)) < 0)
> > return error;
> > if ((error = snd_ad1848_pcm(codec, 0, &pcm)) < 0)
> > return error;
>
> and
>
> > --- a/sound/isa/sgalaxy.c
> > +++ b/sound/isa/sgalaxy.c
> > @@ -265,7 +265,7 @@ static int __devinit snd_sgalaxy_probe(struct device *devptr, unsigned int dev)
> >
> > if ((err = snd_ad1848_create(card, wssport[dev] + 4,
> > xirq, xdma1,
> > - AD1848_HW_DETECT, &chip)) < 0)
> > + WSS_HW_DETECT, &chip)) < 0)
> > goto _err;
> > card->private_data = chip;
>
> For those that come after -- those error ifs are split when they are
> turned into snd_wss_create() later.
>
I tried to do as few as possible changes not related to what the patch did.
So I left ifs like these for now.
>
> *** 0006-wss_lib-replace-ad1848-mixer-element-macros-with-ws.patch
>
> > #define AD1848_SINGLE_TLV(xname, xindex, reg, shift, mask, invert, xtlv) \
> > -{ .name = xname, \
> > - .index = xindex, \
> > - .type = AD1848_MIX_SINGLE, \
> > - .private_value = AD1848_MIXVAL_SINGLE(reg, shift, mask, invert), \
> > - .tlv = xtlv }
> > +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> > + .name = xname, .index = xindex, \
> > + .info = snd_ad1848_info_single, \
> > + .get = snd_ad1848_get_single, .put = snd_ad1848_put_single, \
> > + .private_value = reg | (shift << 8) | (mask << 16) | (invert << 24), \
> > + .tlv = { .p = (xtlv) } }
>
> Please just one per line (aligned is nice...)
Ok.
>
> > #define AD1848_DOUBLE_TLV(xname, xindex, left_reg, right_reg, shift_left, shift_right, mask, invert, xtlv) \
> > -{ .name = xname, \
> > - .index = xindex, \
> > - .type = AD1848_MIX_DOUBLE, \
> > - .private_value = AD1848_MIXVAL_DOUBLE(left_reg, right_reg, shift_left, shift_right, mask, invert), \
> > - .tlv = xtlv }
> > -
> > -static struct ad1848_mix_elem snd_ad1848_controls[] = {
> > -AD1848_DOUBLE("PCM Playback Switch", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT, 7, 7, 1, 1),
> > -AD1848_DOUBLE_TLV("PCM Playback Volume", 0, AD1848_LEFT_OUTPUT, AD1848_RIGHT_OUTPUT, 0, 0, 63, 1,
> > +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> > + .name = xname, .index = xindex, \
> > + .info = snd_ad1848_info_double, \
> > + .get = snd_ad1848_get_double, .put = snd_ad1848_put_double, \
> > + .private_value = left_reg | (right_reg << 8) | (shift_left << 16) | \
> > + (shift_right << 19) | (mask << 24) | (invert << 22), \
> > + .tlv = { .p = (xtlv) } }
>
> Same.
>
> Throughout this patch there's also still the comment about ignoring the
> 80-column thing with these macros. The cmi8330.c ones are a wonderful
> example of how much worse it gets. It's horrible.
>
I wrote that changing it all into nice checkpatch code was probably not
sane. I left long macros in few places they were already.
>
> *** 0007-wss_lib-use-CS4231P-instead-of-AD1848P-kill-the-AD.patch
>
> > static void snd_ad1848_debug(struct snd_wss *chip)
>
> Same. Otherwise
It is done to have fewer checkpatch warning. As the file was going
to be killed completely it does not matter.
>
> Reviewed-by: Rene Herman <rene.herman@gmail.com>
>
> One other comment:
>
> > diff --git a/sound/isa/wss/wss_lib.c b/sound/isa/wss/wss_lib.c
> > index 08997d0..f695c85 100644
> > --- a/sound/isa/wss/wss_lib.c
> > +++ b/sound/isa/wss/wss_lib.c
> > @@ -164,7 +164,6 @@ static inline void wss_outb(struct snd_wss *chip, u8 offset, u8 val)
> > {
> > outb(val, chip->port + offset);
> > }
> > -EXPORT_SYMBOL(snd_wss_out);
> >
> > static inline u8 wss_inb(struct snd_wss *chip, u8 offset)
> > {
> > @@ -228,6 +227,7 @@ void snd_wss_out(struct snd_wss *chip, unsigned char reg, unsigned char value)
> > snd_printdd("codec out - reg 0x%x = 0x%x\n",
> > chip->mce_bit | reg, value);
> > }
> > +EXPORT_SYMBOL(snd_wss_out);
>
> Ah. This fixes up one of my comments from yesterday already. If this
> finds its way into 3, it should ofcourse be gone here.
>
This is a proof that code can become alive and does thing programer
is not aware of ;-)
>
> *** 0008-wss_lib-use-wss-mixer-code-instead-of-ad1848-one.patch
>
> > +#define WSS_SINGLE_TLV(xname, xindex, reg, shift, mask, invert, xtlv) \
> > +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> > + .name = xname, .index = xindex, \
> > + .info = snd_wss_info_single, \
> > + .get = snd_wss_get_single, .put = snd_wss_put_single, \
> > + .private_value = reg | (shift << 8) | (mask << 16) | (invert << 24), \
> > + .tlv = { .p = (xtlv) } }
> > +
> > +#define WSS_DOUBLE_TLV(xname, xindex, left_reg, right_reg, \
> > + shift_left, shift_right, mask, invert, xtlv) \
> > +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
> > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_READ, \
> > + .name = xname, .index = xindex, \
> > + .info = snd_wss_info_double, \
> > + .get = snd_wss_get_double, .put = snd_wss_put_double, \
> > + .private_value = left_reg | (right_reg << 8) | (shift_left << 16) | \
> > + (shift_right << 19) | (mask << 24) | (invert << 22), \
> > + .tlv = { .p = (xtlv) } }
>
> Ofcourse same with the please one member per line.
>
Ok.
>
> *** 0009-wss_lib-use-wss-pcm-code-instead-of-ad1848-one.patch
>
> > --- a/sound/isa/wss/wss_lib.c
> > +++ b/sound/isa/wss/wss_lib.c
> > @@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd_wss *chip)
> > for (timeout = 5; timeout > 0; timeout--)
> > wss_inb(chip, CS4231P(REGSEL));
> > /* end of cleanup sequence */
> > - for (timeout = 250;
> > + for (timeout = 2500;
> > timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
> > timeout--)
> > udelay(10);
>
> Was this intentional? You comment on this in 10/10...
Yes. It was. The original ad1848 loop is 12000 x 100us. The cs4231 loop
was 250 x 10us. The code I tested had 2500 x 100us but I lowered the udelay()
argument for cs4231chips which were much faster (only few if any 10us
during tests). I forgot to retest it again on the original ad1848 so the patch
went in with the error.
If this kind of change should be in a separate patch it must be a patch
before this patch otherwise we end up with perfectly bisectable error
the author was aware of while sending patches (so there is no point
in applying only one and no bisecting then).
>
> > @@ -1360,6 +1381,11 @@ static int snd_wss_capture_open(struct s
> >
> > runtime->hw = snd_wss_capture;
> >
> > + /* hardware limitation of older chipsets */
> > + if (chip->hardware == WSS_HW_INTERWAVE && chip->dma1 > 3)
> > + runtime->hw.formats &= ~(SNDRV_PCM_FMTBIT_IMA_ADPCM |
> > + SNDRV_PCM_FMTBIT_S16_BE);
> > +
>
> If you'll be posting a V2 series, might as well fold 11/10 in here.
>
I'll do fold the fix, but new limits for opti cards I would like to keep
as a separate one (missing it is not a regression).
> > const char *snd_wss_chip_id(struct snd_wss *chip)
>
> Comment about the switch getting uglier again.
>
The checkpatch likes the new switch more.
>
> *** 0010-wss_lib-use-wss-detection-code-instead-of-ad1848-on.patch
>
> > +++ b/sound/isa/wss/wss_lib.c
> > @@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd_wss *chip)
> > for (timeout = 5; timeout > 0; timeout--)
> > wss_inb(chip, CS4231P(REGSEL));
> > /* end of cleanup sequence */
> > - for (timeout = 2500;
> > + for (timeout = 25000;
> > timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
> > timeout--)
> > udelay(10);
>
> So now it's 100x total. Is this as planned?
>
Yes. I have described it above.
> > -static int snd_wss_probe(struct snd_wss *chip)
> > +static int snd_ad1848_probe(struct snd_wss *chip)
> > {
> > unsigned long flags;
> > - int i, id, rev;
> > - unsigned char *ptr;
> > - unsigned int hw;
> > + int i, id, rev, ad1847;
> >
> > -#if 0
> > - snd_wss_debug(chip);
> > -#endif
> > id = 0;
> > - for (i = 0; i < 50; i++) {
> > + ad1847 = 0;
> > + for (i = 0; i < 1000; i++) {
> > mb();
> > - if (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT)
> > - udelay(2000);
> > + if (inb(chip->port + CS4231P(REGSEL)) & CS4231_INIT)
> > + msleep(1);
>
> Mmm. This was changed from a udelay(500) in the ad1848 case. It would be
> better to keep these kinds of really functional changes seperated out.
>
Again, without the change it may provide two patches but working
only if both applied.
> [ ... ]
>
> > + id = 0;
> > + if (chip->hardware == WSS_HW_DETECT)
> > + id = ad1847 ? WSS_HW_AD1847 : WSS_HW_AD1848;
> > +
> > + spin_lock_irqsave(&chip->reg_lock, flags);
> > + inb(chip->port + CS4231P(STATUS)); /* clear any pendings IRQ */
> > + outb(0, chip->port + CS4231P(STATUS));
>
> The original snd_ad1848_probe() had this below the below tests (which
> were also outside the spinlock)
>
> > + mb();
> > + if (id == WSS_HW_AD1848) {
> > + /* check if there are more than 16 registers */
> > + rev = snd_wss_in(chip, CS4231_MISC_INFO);
> > + snd_wss_out(chip, CS4231_MISC_INFO, 0x40);
> > + for (i = 0; i < 16; ++i) {
> > + if (snd_wss_in(chip, i) != snd_wss_in(chip, i + 16)) {
> > + id = WSS_HW_CMI8330;
> > + break;
> > + }
> > + }
> > + snd_wss_out(chip, CS4231_MISC_INFO, 0x00);
> > + if (id != WSS_HW_CMI8330 && (rev & 0x80))
> > + id = WSS_HW_CS4248;
>
> This one could also wind up different. Originally, snd_ad1848_probe()
> would conclude CS4248 immediately upon (rev & 0x80) without doing that
> register test.
It did not work. It hit the Galaxy Waverider as it misdetected
the cs4231 as the cs4248. A smarter way was needed if ad1848 and
cs4231 families were present.
> It's probably all fine, but snd_ad1848 function changed
> very significantly in the move and I'd rather it not do that. A patch
> 12 alone is much easier reviewable and any possible difficulty much
> easier bisectable. Could you do that?
>
It can be done but the patch which merges the code will incorrectly
detect chips (tested that it does). So both must be applied together.
>
> *** 0011-wss-fix-capture-formats-limitations.patch
>
> Can be folded, but otherwise no comments...
>
> Will also put a next series through some tests here locally. I'l test at
> least a few OPTi 92x cards since I don't see these in your report.
You may try merging opti92x-ad1848 and opti92x-cs4231 drivers now.
There are only few lines of difference (e.g. timer creation). I haven't
done this as I do not have these chips.
> Any
> specific hardware you'd want tested more than others? I might have it.
>
Yes. Try if older codecs (not yet tested) are correctly detected. Especially,
these with special code paths in the detection function:
ad1847,
cmi8330,
cs4248,
Also try if they are working as they may require even longer delays.
The cs4231 chips are much simpler as you have the revision register
so I do not have problem here. I would like to have tested the
integrated or compatible cs4231 codecs:
yamaha opl3sa2
interwave
If you have any of these please post results of your tests.
I did tests on cs4232 chip inside the Dell Laptitue CP250 and it works.
It is found by the PNP bios and configured automatically (the anti-crash
patch I posted is needed).
The chip is detected as CS4236, however. The cs4231 revision is 0x03 which
code detects as CS4236B. I cannot open the laptop (it is not mine)
so I assume the Dell replaced older chip with newer one but left the same
BIOS.
Thank you for your effort,
Regards,
Krzysztof
----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>> http://link.interia.pl/f1e91
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 18:47 ` Krzysztof Helt
@ 2008-07-24 19:30 ` Rene Herman
2008-07-24 20:05 ` Rene Herman
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Rene Herman @ 2008-07-24 19:30 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 24-07-08 20:47, Krzysztof Helt wrote:
>> *** 0005-wss_lib-use-wss-constants-instead-of-ad1848-ones.patch
>>
>>> --- a/sound/isa/ad1848/ad1848_lib.c
>>> +++ b/sound/isa/ad1848/ad1848_lib.c
>>> @@ -283,14 +283,12 @@ static int snd_ad1848_trigger(struct snd_wss *chip, unsigned char what,
>>> return 0;
>>> }
>>> snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL] |= what);
>>> - chip->mode |= AD1848_MODE_RUNNING;
>> Is this now done in generic code? Not necessary anymore? Was no comment
>> in the changelog.
>
> The MODE_RUNNING is not used at all in the cs4231 code. I wonder what the purpose of it.
It was used by the AD1848 code though; snd_ad1848_trigger() set/reset it
on start/stop and it was then tested by snd_ad1848_interrupt() to decide
whether or not to call snd_pcm_period_elapsed().
> If switches like these generates more than one warnings by the
> checkpatch script I change them to be on safer side with "checkpatch
> fundamentalists". Ok?
Yeah, sure. Just hoping to convince Takashi that checkpatch makes some
things worse instead of better.
>> *** 0009-wss_lib-use-wss-pcm-code-instead-of-ad1848-one.patch
>>
>>> --- a/sound/isa/wss/wss_lib.c
>>> +++ b/sound/isa/wss/wss_lib.c
>>> @@ -363,7 +363,7 @@ static void snd_wss_busy_wait(struct snd_wss *chip)
>>> for (timeout = 5; timeout > 0; timeout--)
>>> wss_inb(chip, CS4231P(REGSEL));
>>> /* end of cleanup sequence */
>>> - for (timeout = 250;
>>> + for (timeout = 2500;
>>> timeout > 0 && (wss_inb(chip, CS4231P(REGSEL)) & CS4231_INIT);
>>> timeout--)
>>> udelay(10);
>> Was this intentional? You comment on this in 10/10...
>
> Yes. It was. The original ad1848 loop is 12000 x 100us. The cs4231 loop
> was 250 x 10us. The code I tested had 2500 x 100us but I lowered the udelay()
> argument for cs4231chips which were much faster (only few if any 10us
> during tests). I forgot to retest it again on the original ad1848 so the patch
> went in with the error.
>
> If this kind of change should be in a separate patch it must be a patch
> before this patch otherwise we end up with perfectly bisectable error
> the author was aware of while sending patches (so there is no point
> in applying only one and no bisecting then).
Okay...
> I'll do fold the fix, but new limits for opti cards I would like to keep
> as a separate one (missing it is not a regression).
Yep.
>> It's probably all fine, but snd_ad1848 function changed
>> very significantly in the move and I'd rather it not do that. A patch
>> 12 alone is much easier reviewable and any possible difficulty much
>> easier bisectable. Could you do that?
>
> It can be done but the patch which merges the code will incorrectly
> detect chips (tested that it does). So both must be applied together.
Okay, I see this was specifically tested and all. Try and see if you can
put something sensible in the changelog about it. It's _very_ hard to be
too verbose in changelogs...
> You may try merging opti92x-ad1848 and opti92x-cs4231 drivers now.
> There are only few lines of difference (e.g. timer creation). I haven't
> done this as I do not have these chips.
Have a ton of them. Yes, I may try for merging opti92x and opti93x as
well in fact. Also have all 93x cards to test. Don't yet know if it'll
end up clumsy, not started.
> Yes. Try if older codecs (not yet tested) are correctly detected. Especially,
> these with special code paths in the detection function:
> ad1847,
This one I don't think I have... (damnit!)
> cmi8330,
Yup.
> cs4248,
Yup.
> Also try if they are working as they may require even longer delays.
>
> The cs4231 chips are much simpler as you have the revision register
> so I do not have problem here. I would like to have tested the
> integrated or compatible cs4231 codecs:
> yamaha opl3sa2
Yup.
> interwave
Nope. Many people are _still_ deluded by the GUS name and believe that
(even) a GUS PnP is worth something, so I've upto now told people where
to stick the GUS PnPs they tried to sell me (but I do have classic, max
and extreme).
> If you have any of these please post results of your tests.
Will wait for V2 and will do; make take some time. I hope to not have
time over the coming weekend...
> I did tests on cs4232 chip inside the Dell Laptitue CP250 and it
> works. It is found by the PNP bios and configured automatically (the
> anti-crash patch I posted is needed). The chip is detected as CS4236,
> however. The cs4231 revision is 0x03 which code detects as CS4236B. I
> cannot open the laptop (it is not mine) so I assume the Dell replaced
> older chip with newer one but left the same BIOS.
I'm not surprised. The CS4232 part of your chip will advertise itself
with ID CSC0000 or CSC0100 in /sys/devices/pnp0/<foo>/id and you
probably have a CSC0010 or CSC0110 as one of the other devices in there.
I noticed this before when debugging someone else's system on alsa-user
a while ago. That latter ID is the CS4236 control port. These things
should really just be driven as CS4236.
If you have this laptop for long enough, we'll get that going...
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 19:30 ` Rene Herman
@ 2008-07-24 20:05 ` Rene Herman
2008-07-24 21:41 ` Krzysztof Helt
2008-08-04 1:47 ` Rene Herman
2 siblings, 0 replies; 22+ messages in thread
From: Rene Herman @ 2008-07-24 20:05 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 24-07-08 21:30, Rene Herman wrote:
> I'm not surprised. The CS4232 part of your chip will advertise itself
> with ID CSC0000 or CSC0100 in /sys/devices/pnp0/<foo>/id and you
> probably have a CSC0010 or CSC0110 as one of the other devices in
> there. I noticed this before when debugging someone else's system on
> alsa-user a while ago. That latter ID is the CS4236 control port.
> These things should really just be driven as CS4236.
>
> If you have this laptop for long enough, we'll get that going...
That's confused. The CSC0{0,1}10 is the CS4232 control port, the PNPBIOS
driver just isn't using it. But yes, see the PNPID tables in there. From
the device IDs you don't know cs4232 from cs4236.
Were you in fact planning on merging cs4236_lib as well? Then we can
just do runtime switching.
Rene
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 19:30 ` Rene Herman
2008-07-24 20:05 ` Rene Herman
@ 2008-07-24 21:41 ` Krzysztof Helt
2008-07-25 9:59 ` Rene Herman
2008-08-04 1:47 ` Rene Herman
2 siblings, 1 reply; 22+ messages in thread
From: Krzysztof Helt @ 2008-07-24 21:41 UTC (permalink / raw)
To: Rene Herman; +Cc: Takashi Iwai, Alsa-devel
On Thu, 24 Jul 2008 21:30:28 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:
> On 24-07-08 20:47, Krzysztof Helt wrote:
>
> >> *** 0005-wss_lib-use-wss-constants-instead-of-ad1848-ones.patch
> >>
> >>> --- a/sound/isa/ad1848/ad1848_lib.c
> >>> +++ b/sound/isa/ad1848/ad1848_lib.c
> >>> @@ -283,14 +283,12 @@ static int snd_ad1848_trigger(struct snd_wss *chip, unsigned char what,
> >>> return 0;
> >>> }
> >>> snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL] |= what);
> >>> - chip->mode |= AD1848_MODE_RUNNING;
> >> Is this now done in generic code? Not necessary anymore? Was no comment
> >> in the changelog.
> >
> > The MODE_RUNNING is not used at all in the cs4231 code. I wonder what the purpose of it.
>
> It was used by the AD1848 code though; snd_ad1848_trigger() set/reset it
> on start/stop and it was then tested by snd_ad1848_interrupt() to decide
> whether or not to call snd_pcm_period_elapsed().
>
It is not used by the cs4231. The only difference is that ad1848 does not
call the snd_pcm_period_elapsed() after calling the snd_ad1848_open()
but before calling the snd_ad1848_trigger() (and similar restriction
after the snd_ad1848_trigger() and snd_ad1848_close().
The cs4231 does not use such restriction. I decided it does not really matter.
The interrupts are not enabled before and after the snd_ad1848_trigger().
If the cs4231 driver needed this it would be already causing problems.
If you see any possible problem, the MODE_RUNNING can be added
to the wss_lib.
> >> It's probably all fine, but snd_ad1848 function changed
> >> very significantly in the move and I'd rather it not do that. A patch
> >> 12 alone is much easier reviewable and any possible difficulty much
> >> easier bisectable. Could you do that?
> >
> > It can be done but the patch which merges the code will incorrectly
> > detect chips (tested that it does). So both must be applied together.
>
> Okay, I see this was specifically tested and all. Try and see if you can
> put something sensible in the changelog about it. It's _very_ hard to be
> too verbose in changelogs...
>
OK.
>
> > If you have any of these please post results of your tests.
>
> Will wait for V2 and will do; make take some time. I hope to not have
> time over the coming weekend...
>
Please test at least these older chips you have (cmi8330 and cs4248)
so we know that patches are working on them. They carry higher risk
for the patches then newer ones (from the cs4231 family).
> I'm not surprised. The CS4232 part of your chip will advertise itself
> with ID CSC0000 or CSC0100 in /sys/devices/pnp0/<foo>/id and you
> probably have a CSC0010 or CSC0110 as one of the other devices in there.
> I noticed this before when debugging someone else's system on alsa-user
> a while ago. That latter ID is the CS4236 control port. These things
> should really just be driven as CS4236.
>
> If you have this laptop for long enough, we'll get that going...
>
I can keep it quite long (few weeks).
I am not planning merging the cs4236_lib at the moment. I will
start after the wss_lib is merged. I see few things to improve
in the wss_lib which I leave untouched (they do not belong to the
patch set I sent): drop mutex_open as it is done by higher layer,
simplify mute function, fix recording settings after driver loading
- the detection leave it in stupid state.
I want to polish the wss_lib some more before doing something else.
Regards,
Krzysztof
----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>> http://link.interia.pl/f1e91
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 21:41 ` Krzysztof Helt
@ 2008-07-25 9:59 ` Rene Herman
0 siblings, 0 replies; 22+ messages in thread
From: Rene Herman @ 2008-07-25 9:59 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 24-07-08 23:41, Krzysztof Helt wrote:
> On Thu, 24 Jul 2008 21:30:28 +0200
> Rene Herman <rene.herman@keyaccess.nl> wrote:
>>>> *** 0005-wss_lib-use-wss-constants-instead-of-ad1848-ones.patch
>>>>
>>>>> --- a/sound/isa/ad1848/ad1848_lib.c
>>>>> +++ b/sound/isa/ad1848/ad1848_lib.c
>>>>> @@ -283,14 +283,12 @@ static int snd_ad1848_trigger(struct snd_wss *chip, unsigned char what,
>>>>> return 0;
>>>>> }
>>>>> snd_ad1848_out(chip, AD1848_IFACE_CTRL, chip->image[AD1848_IFACE_CTRL] |= what);
>>>>> - chip->mode |= AD1848_MODE_RUNNING;
>>>> Is this now done in generic code? Not necessary anymore? Was no comment
>>>> in the changelog.
>>> The MODE_RUNNING is not used at all in the cs4231 code. I wonder what the purpose of it.
>> It was used by the AD1848 code though; snd_ad1848_trigger() set/reset it
>> on start/stop and it was then tested by snd_ad1848_interrupt() to decide
>> whether or not to call snd_pcm_period_elapsed().
>
> It is not used by the cs4231. The only difference is that ad1848 does not
> call the snd_pcm_period_elapsed() after calling the snd_ad1848_open()
> but before calling the snd_ad1848_trigger() (and similar restriction
> after the snd_ad1848_trigger() and snd_ad1848_close().
>
> The cs4231 does not use such restriction. I decided it does not really matter.
> The interrupts are not enabled before and after the snd_ad1848_trigger().
> If the cs4231 driver needed this it would be already causing problems.
It seems we are talking at cross purposes. I'm not talking about cs4231.
I see this MODE_RUNNING thing disappear from ad1848_lib and, unless I've
missed it, not resurface in wss_lib -- that library that after these
patches is used to drive AD1848 chips that used to be driven by ad1848_lib.
The MODE_RUNNING looks as something someone will have once added upon
seeing spurious IRQs and as such, testing that it isn't needed on
chip/card A, B and C doesn't tell us much. The problem may have been
observed on type D, E or F and/or under condition foo or bar.
Looking at snd_wss_interrupt() I dom't seem to be seeing a similar guard
after these patches.
If it's not needed, all for slashing it but please be convincing. Note
that by now you should know a lot more about the innards of this code
than I do, so please be as verbose as needed.
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
@ 2008-07-25 13:39 krzysztof.h1
0 siblings, 0 replies; 22+ messages in thread
From: krzysztof.h1 @ 2008-07-25 13:39 UTC (permalink / raw)
To: Rene Herman; +Cc: Takashi Iwai, Alsa-devel, Krzysztof Helt
Rene Herman napisał(a):
> >>>> Is this now done in generic code? Not necessary anymore? Was no
> comment
> >>>> in the changelog.
> >>> The MODE_RUNNING is not used at all in the cs4231 code. I wonder what
> the purpose of it.
> >> It was used by the AD1848 code though; snd_ad1848_trigger() set/reset
> it
> >> on start/stop and it was then tested by snd_ad1848_interrupt() to
> decide
> >> whether or not to call snd_pcm_period_elapsed().
> >
> > It is not used by the cs4231. The only difference is that ad1848 does
> not
> > call the snd_pcm_period_elapsed() after calling the snd_ad1848_open()
> > but before calling the snd_ad1848_trigger() (and similar restriction
> > after the snd_ad1848_trigger() and snd_ad1848_close().
> >
> > The cs4231 does not use such restriction. I decided it does not really
> matter.
> > The interrupts are not enabled before and after the
> snd_ad1848_trigger().
> > If the cs4231 driver needed this it would be already causing problems.
>
> It seems we are talking at cross purposes. I'm not talking about cs4231.
> I see this MODE_RUNNING thing disappear from ad1848_lib and, unless I've
> missed it, not resurface in wss_lib -- that library that after these
> patches is used to drive AD1848 chips that used to be driven by
> ad1848_lib.
>
> The MODE_RUNNING looks as something someone will have once added upon
> seeing spurious IRQs and as such, testing that it isn't needed on
> chip/card A, B and C doesn't tell us much. The problem may have been
> observed on type D, E or F and/or under condition foo or bar.
>
> Looking at snd_wss_interrupt() I dom't seem to be seeing a similar guard
> after these patches.
>
> If it's not needed, all for slashing it but please be convincing. Note
> that by now you should know a lot more about the innards of this code
> than I do, so please be as verbose as needed.
>
I cannot be convincing as I am not convinced myself. I understand your reasoning.
My situation was that I saw the guard in the ad1848 which was missing in the cs4231
while they should look like twins (in the area when MODE_RUNNING existed).
I had two choices:
1. Add MODE_RUNNING to the cs4231
2. Remove MODE_RUNNING
The first approach is a bloat to me as I tested on ad1848 and it was not needed.
I don't know the conditions you called the "condition foo or bar" and I don't know
if it ever exists. I have checked git history on the file, but this one was added
earlier. Maybe there was a kind of approach in the old days then dropped/replaced
later (like mutex_open which is not needed now).
The second approach is more dangerous but with enough testing should be justified
and provides us with cleaner code.
Regards,
Krzysztof
----------------------------------------------------------------------
Te newsy nakreca Cie na caly dzien!
Sprawdz >>> http://link.interia.pl/f1e94
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 9:31 ` Rene Herman
@ 2008-07-28 15:37 ` Takashi Iwai
2008-07-28 18:39 ` Rene Herman
0 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2008-07-28 15:37 UTC (permalink / raw)
To: Rene Herman; +Cc: Alsa-devel, Krzysztof Helt
At Thu, 24 Jul 2008 11:31:00 +0200,
Rene Herman wrote:
>
> >>> -CS4236_DOUBLE("Master Digital Playback Switch", 0, CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> >>> -CS4236_DOUBLE("Master Digital Capture Switch", 0, CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
> >>> +CS4236_DOUBLE("Master Digital Playback Switch", 0,
> >>> + CS4236_LEFT_MASTER, CS4236_RIGHT_MASTER, 7, 7, 1, 1),
> >>> +CS4236_DOUBLE("Master Digital Capture Switch", 0,
> >>> + CS4236_DAC_MUTE, CS4236_DAC_MUTE, 7, 6, 1, 1),
> >> I can't say I'm personally a fan of these kinds of changes. The point of
> >> them would supposedly be to make the code more readable but as far as I
> >> am concerned it does the reverse.
> >>
> >> I know that Takashi can be an 80-column fundamentalist so I'll not
> >> object I guess. I'd personally like these (all) restored to a single
> >> line but if he doesn't, so be it.
> >
> > Exactly. It was done for Takashi.
>
> Yes, he overrides. I'd try to get away with just saying no though. That
> checkpatch thing desperately needs a clue.
Well, I still prefer folding lines to fit 80-column - of course
only if the result is somewhat reasonable and more readable.
Usually, you set the ter
minal with 80-column, an
d, longer lines are diff
icult to read.
With appropriate line-
breaks, it becomes far
easier to read.
Takashi (love 80's)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-28 15:37 ` Takashi Iwai
@ 2008-07-28 18:39 ` Rene Herman
2008-07-29 13:02 ` Takashi Iwai
0 siblings, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-28 18:39 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Alsa-devel, Krzysztof Helt
On 28-07-08 17:37, Takashi Iwai wrote:
> Well, I still prefer folding lines to fit 80-column - of course
> only if the result is somewhat reasonable and more readable.
Which it absolutely never is, because if it were, the original
programmer would've already formatted it that way.
> Usually, you set the ter
> minal with 80-column
For a definition of "you" which includes... well, "you" (and ofcourse
other than that everyone who will now take the opportunity to show us
their True b@dazz h@ck0r terminal skilzzzz).
> Takashi (love 80's)
/me forcefeeds Rick Astley's collected works down Takashi's throat.
Bah.
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-28 18:39 ` Rene Herman
@ 2008-07-29 13:02 ` Takashi Iwai
2008-07-29 14:15 ` Rene Herman
0 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2008-07-29 13:02 UTC (permalink / raw)
To: Rene Herman; +Cc: Alsa-devel, Krzysztof Helt
At Mon, 28 Jul 2008 20:39:05 +0200,
Rene Herman wrote:
>
> On 28-07-08 17:37, Takashi Iwai wrote:
>
> > Well, I still prefer folding lines to fit 80-column - of course
> > only if the result is somewhat reasonable and more readable.
>
> Which it absolutely never is, because if it were, the original
> programmer would've already formatted it that way.
... only if the original author respected the standard CodingStyle.
Many old ALSA codes are not in that category.
Honestly, I don't mind much to keep them as they are now, even though
checkpatch grumbles, if the author (or the heir) wants to keep it
intentionally even after reading the CodingStyle text carefully...
Takashi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-29 13:02 ` Takashi Iwai
@ 2008-07-29 14:15 ` Rene Herman
2008-07-29 14:31 ` Takashi Iwai
0 siblings, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-07-29 14:15 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Alsa-devel, Krzysztof Helt
On 29-07-08 15:02, Takashi Iwai wrote:
> At Mon, 28 Jul 2008 20:39:05 +0200,
> Rene Herman wrote:
>> On 28-07-08 17:37, Takashi Iwai wrote:
>>
>>> Well, I still prefer folding lines to fit 80-column - of course
>>> only if the result is somewhat reasonable and more readable.
>> Which it absolutely never is, because if it were, the original
>> programmer would've already formatted it that way.
>
> ... only if the original author respected the standard CodingStyle.
> Many old ALSA codes are not in that category.
>
> Honestly, I don't mind much to keep them as they are now, even though
> checkpatch grumbles, if the author (or the heir) wants to keep it
> intentionally even after reading the CodingStyle text carefully...
I'm also definitely not speaking about things such as function headers
which needlessly walk of to the far right, but specifically about stuff
where the formatting _not_ inside 80 cols made things much easier to
read. In this case, my specific comments were about:
1) mixer element macros
Many spots in this patchset, but for IMO most clearly bad example:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009272.html
See the cmi8330 ones.
Not only do these kind of changes muddy up a patch, they muddy up the
result as well. Hate it...
2) debug printks
For one example here, see:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/008978.html
/snd_wss_debug
Bad, bad, triply bad.
3) trivial switches, although I don't feel hugely strongly about those.
Example:
http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009314.html
/snd_wss_chip_id
...
All of these, I strongly feel, are examples where checkpatch needs and
deserves to be fully ignored.
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-29 14:15 ` Rene Herman
@ 2008-07-29 14:31 ` Takashi Iwai
2008-07-29 14:37 ` Rene Herman
2008-07-29 18:53 ` Krzysztof Helt
0 siblings, 2 replies; 22+ messages in thread
From: Takashi Iwai @ 2008-07-29 14:31 UTC (permalink / raw)
To: Rene Herman; +Cc: Alsa-devel, Krzysztof Helt
At Tue, 29 Jul 2008 16:15:10 +0200,
Rene Herman wrote:
>
> > At Mon, 28 Jul 2008 20:39:05 +0200,
> > Rene Herman wrote:
> >> On 28-07-08 17:37, Takashi Iwai wrote:
> >>
> >>> Well, I still prefer folding lines to fit 80-column - of course
> >>> only if the result is somewhat reasonable and more readable.
> >> Which it absolutely never is, because if it were, the original
> >> programmer would've already formatted it that way.
> >
> > ... only if the original author respected the standard CodingStyle.
> > Many old ALSA codes are not in that category.
> >
> > Honestly, I don't mind much to keep them as they are now, even though
> > checkpatch grumbles, if the author (or the heir) wants to keep it
> > intentionally even after reading the CodingStyle text carefully...
>
> I'm also definitely not speaking about things such as function headers
> which needlessly walk of to the far right, but specifically about stuff
> where the formatting _not_ inside 80 cols made things much easier to
> read. In this case, my specific comments were about:
>
> 1) mixer element macros
>
> Many spots in this patchset, but for IMO most clearly bad example:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009272.html
>
> See the cmi8330 ones.
This kind of change isn't always bad. The problem is that the
expressions aren't consistent through the whole list. If the same
style is used for each element, e.g.
WSS_DOUBLE("LONG NAME HERE", 0,
LEFT_REG, RIGHT_REG,
0, 1, 2, 3),
then it'll be easier to compare each item than before.
A general problem of such macros or functions with many arguments is
that you can loose easily the relationship of each argument, because
they are listed just plainly. Breaking lines properly could help a
bit.
> Not only do these kind of changes muddy up a patch, they muddy up the
> result as well. Hate it...
>
> 2) debug printks
>
> For one example here, see:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/008978.html
>
> /snd_wss_debug
>
> Bad, bad, triply bad.
This change is actually buggy. Each second printk should have no
KERN_* prefix. KERN_* prefix is only for the beginning of the line.
A more better fix would be to rewrite this to use a loop, BTW.
> 3) trivial switches, although I don't feel hugely strongly about those.
>
> Example:
>
> http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009314.html
>
> /snd_wss_chip_id
This is really trivial and fine to keep in the old way.
thanks,
Takashi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-29 14:31 ` Takashi Iwai
@ 2008-07-29 14:37 ` Rene Herman
2008-07-29 18:53 ` Krzysztof Helt
1 sibling, 0 replies; 22+ messages in thread
From: Rene Herman @ 2008-07-29 14:37 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Alsa-devel, Krzysztof Helt
On 29-07-08 16:31, Takashi Iwai wrote:
>> 2) debug printks
>>
>> For one example here, see:
>>
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/008978.html
>>
>> /snd_wss_debug
>>
>> Bad, bad, triply bad.
>
> This change is actually buggy. Each second printk should have no
> KERN_* prefix. KERN_* prefix is only for the beginning of the line.
See how these formatting "fixes" made me miss that? ;-)
> A more better fix would be to rewrite this to use a loop, BTW.
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-29 14:31 ` Takashi Iwai
2008-07-29 14:37 ` Rene Herman
@ 2008-07-29 18:53 ` Krzysztof Helt
2008-07-29 19:00 ` Rene Herman
1 sibling, 1 reply; 22+ messages in thread
From: Krzysztof Helt @ 2008-07-29 18:53 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Rene Herman, Alsa-devel
On Tue, 29 Jul 2008 16:31:16 +0200
Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 29 Jul 2008 16:15:10 +0200,
> Rene Herman wrote:
> >
> > > At Mon, 28 Jul 2008 20:39:05 +0200,
> > > Rene Herman wrote:
> > >> On 28-07-08 17:37, Takashi Iwai wrote:
> > >>
> > >>> Well, I still prefer folding lines to fit 80-column - of course
> > >>> only if the result is somewhat reasonable and more readable.
> > >> Which it absolutely never is, because if it were, the original
> > >> programmer would've already formatted it that way.
> > >
> > > ... only if the original author respected the standard CodingStyle.
> > > Many old ALSA codes are not in that category.
> > >
> > > Honestly, I don't mind much to keep them as they are now, even though
> > > checkpatch grumbles, if the author (or the heir) wants to keep it
> > > intentionally even after reading the CodingStyle text carefully...
> >
> > I'm also definitely not speaking about things such as function headers
> > which needlessly walk of to the far right, but specifically about stuff
> > where the formatting _not_ inside 80 cols made things much easier to
> > read. In this case, my specific comments were about:
> >
> > 1) mixer element macros
> >
> > Many spots in this patchset, but for IMO most clearly bad example:
> >
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009272.html
> >
> > See the cmi8330 ones.
>
> This kind of change isn't always bad. The problem is that the
> expressions aren't consistent through the whole list. If the same
> style is used for each element, e.g.
>
> WSS_DOUBLE("LONG NAME HERE", 0,
> LEFT_REG, RIGHT_REG,
> 0, 1, 2, 3),
>
> then it'll be easier to compare each item than before.
>
> A general problem of such macros or functions with many arguments is
> that you can loose easily the relationship of each argument, because
> they are listed just plainly. Breaking lines properly could help a
> bit.
>
> > Not only do these kind of changes muddy up a patch, they muddy up the
> > result as well. Hate it...
> >
> > 2) debug printks
> >
> > For one example here, see:
> >
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/008978.html
> >
> > /snd_wss_debug
> >
> > Bad, bad, triply bad.
>
> This change is actually buggy. Each second printk should have no
> KERN_* prefix. KERN_* prefix is only for the beginning of the line.
>
> A more better fix would be to rewrite this to use a loop, BTW.
>
I'll undo some damage I have done.
> > 3) trivial switches, although I don't feel hugely strongly about those.
> >
> > Example:
> >
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2008-July/009314.html
> >
> > /snd_wss_chip_id
>
> This is really trivial and fine to keep in the old way.
>
I like the new one - when case lines stand out from the code.
BTW, I am one of people who use only 80 character terminals. I just use xterm
and vim even in X11.
This set of patches has a gray area: the ad1848-lib and ad1848.h. All changes into
these files are killed by the end of set as the files are deleted. It is not worth
to be picky about changes into them.
Regards,
Krzysztof
----------------------------------------------------------------------
Nie dla mieczakow!
Sprawdz >>> http://link.interia.pl/f1e93
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-29 18:53 ` Krzysztof Helt
@ 2008-07-29 19:00 ` Rene Herman
0 siblings, 0 replies; 22+ messages in thread
From: Rene Herman @ 2008-07-29 19:00 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 29-07-08 20:53, Krzysztof Helt wrote:
> BTW, I am one of people who use only 80 character terminals. I just
> use xterm and vim even in X11.
As do I, but definitely not in an 80-char terminal. Isn't the entire
point of X having wider terminals, alongside your browser?
<shrug>
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-07-24 19:30 ` Rene Herman
2008-07-24 20:05 ` Rene Herman
2008-07-24 21:41 ` Krzysztof Helt
@ 2008-08-04 1:47 ` Rene Herman
2008-08-04 4:31 ` Krzysztof Helt
2 siblings, 1 reply; 22+ messages in thread
From: Rene Herman @ 2008-08-04 1:47 UTC (permalink / raw)
To: Krzysztof Helt; +Cc: Takashi Iwai, Alsa-devel
On 24-07-08 21:30, Rene Herman wrote:
>> If you have any of these please post results of your tests.
>
> Will wait for V2 and will do; make take some time. I hope to not have
> time over the coming weekend...
Testing results. Only tested playback for now, not capture:
--- OPTi 92x (snd-opti92x-ad1848, snd-opti92x-cs4231)
Firstly, snd-opti92x-ad1848 doesn't work for anything anymore. The chip
is each time detected as "OPTi 93x" instead of the actual AD/CS chip.
Probably minor buglet -- I did not look, just mechanically switched
cards and tested.
snd-opti92x-cs4231 works better:
OPTi WSS /proc/asund/cards Result
---------------------------------------------------------------------
924 CS4231A-KL "OPTi 82C924, CS4231" Hangs
924 AD1845JP "OPTi 82C924, AD1845" Hangs
925 AD1845JP "OPTi 82C925, AD1845" Works
928 CS4248-KL "OPTi 82C928, CS4231" Works
929A CS4248-KL "OPTi 82C929, CS4231A" Works
929A AD1848 "OPTi 82C929, AD1848" Works
929A AD1845JP "OPTi 82C929, AD1845" Works
929A AD1846JP" "OPTi 82C929, AD1848" Works
I've been running with the WSS code for some time locally and will need
to recompile a vanilla kernel onto there to check 924. I expect it's not
a regression.
As to your specific question -- CS4248 seems to be detected as CS4231(A)
--- OPTi 93x (snd-opti93x)
Chip /proc/asound/cards Result
------------------------------------------------------
930 "OPTi 82C930, OPTi 93x" Works
931 "OPTi 82C931, OPTi 93x" Works
933 "OPTi 82C933, OPTi 93x" Works
--- Yamaha OPL3-SA2 (snd-opl3sa2)
Chip /proc/asound/cards Result
------------------------------------------------------
YMF719-S "Yamaha OPL3-SA23" Works
YMF719E-S "Yamaha OPL3-SA23" Works
--- CMI 8330 (snd-cmi8330)
Chip /proc/asound/cards Result
------------------------------------------------------
CMI8330A "C-Media CMI8330/C3D" Works
--- Crystal CS423x (snd-cs4232, snd-cs4236)
Chip /proc/asound/cards Result
------------------------------------------------------
CS4232 "CS4232" Works
CX4235-XQ3 "CS4235" Silent
CS4236B-KQ "CS4236B" Works
CX4236B-XQ3 "CS4236B" Works
CX4237B-XQ3 "CS4237B" Works
CS4239-KQ "CS4239" Works
I believe the CX4235 is simply broken -- everything looks in order, just
no signal. Not expected to have anything to do with things.
--- Gravis Ultrasound MAX (snd-gusmax)
Gravis WSS "Chip" Result
--------------------------------------------------------------
GF1 CS4231A-KL "CS4231A" Hangs
Definitely unrelated. Need to sit down one day and figure out what's
wrong with the Gravis drivers.
--- Ensoniq SoundScape VIVO90 (snd-vivo local minimal driver)
Ensoniq WSS "Chip" Result
--------------------------------------------------------------
OTTOR2C AD1845JP "AD1845" Works
Also hve an older non-vivo SoundScape, but I haven't had that working
yet (also not tried, but will at some point). It has an AD1848KP.
--- Aztech Sound Galaxy AZT2320 (snd-azt2320)
Chip: AZT2320
Ha, finally... this one looks to be a regression. I have an large stream of:
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0xc = 0x0
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x0 = 0xaa
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x1 = 0x45
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0xc = 0x0
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x0 = 0xaa
ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x1 = 0x45
before it craps out (doesn't load). I'll look into it unless you beat me
to it.
--- Aztech Sound Galaxy AZT1605/AZT2316 (local snd-galaxy)
Aztech WSS "Chip" Result
--------------------------------------------------------------
AZT1605 CS4231-KL To be tested To be tested
AZT1605 CS4231A-KL To be tested To be tested
AZT1605 AD1845JP To be tested To be tested
AZT2316A CS4248-KL To be tested To be tested
AZT2316A CS4231A-KL To be tested To be tested
AZT2316A AD1845XP To be tested To be tested
AZT2316R CS4231A-KL To be tested To be tested
AZT2316-S CS4231A-KL To be tested To be tested
I'll test these after reviving snd-galaxy. Saw you already tested one as
well though.
--- Aztech Sound Galaxy NX Pro 16 (no driver yet)
AZTSSPT0592/AZT-NXPMIX0592 + AD1848KP
I believe those are all the WSS chips that I have. In the context of the
WSS testing, the AZT2320 result is most interesting...
Rene.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
2008-08-04 1:47 ` Rene Herman
@ 2008-08-04 4:31 ` Krzysztof Helt
0 siblings, 0 replies; 22+ messages in thread
From: Krzysztof Helt @ 2008-08-04 4:31 UTC (permalink / raw)
To: Rene Herman; +Cc: Takashi Iwai, Alsa-devel
On Mon, 04 Aug 2008 03:47:11 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:
> On 24-07-08 21:30, Rene Herman wrote:
>
> >> If you have any of these please post results of your tests.
> >
> > Will wait for V2 and will do; make take some time. I hope to not have
> > time over the coming weekend...
>
> Testing results. Only tested playback for now, not capture:
>
Thank you very much for testing...
>
> --- OPTi 92x (snd-opti92x-ad1848, snd-opti92x-cs4231)
>
> Firstly, snd-opti92x-ad1848 doesn't work for anything anymore. The chip
> is each time detected as "OPTi 93x" instead of the actual AD/CS chip.
> Probably minor buglet -- I did not look, just mechanically switched
> cards and tested.
>
> snd-opti92x-cs4231 works better:
>
> OPTi WSS /proc/asund/cards Result
> ---------------------------------------------------------------------
> 924 CS4231A-KL "OPTi 82C924, CS4231" Hangs
> 924 AD1845JP "OPTi 82C924, AD1845" Hangs
> 925 AD1845JP "OPTi 82C925, AD1845" Works
> 928 CS4248-KL "OPTi 82C928, CS4231" Works
> 929A CS4248-KL "OPTi 82C929, CS4231A" Works
> 929A AD1848 "OPTi 82C929, AD1848" Works
> 929A AD1845JP "OPTi 82C929, AD1845" Works
> 929A AD1846JP" "OPTi 82C929, AD1848" Works
>
> I've been running with the WSS code for some time locally and will need
> to recompile a vanilla kernel onto there to check 924. I expect it's not
> a regression.
>
I do not have hardware to test this.
> As to your specific question -- CS4248 seems to be detected as CS4231(A)
>
I'll look into detection code again. I will try to find another detection procedure (as was done for cmi8330).
>
> --- Crystal CS423x (snd-cs4232, snd-cs4236)
>
> Chip /proc/asound/cards Result
> ------------------------------------------------------
> CS4232 "CS4232" Works
> CX4235-XQ3 "CS4235" Silent
> CS4236B-KQ "CS4236B" Works
> CX4236B-XQ3 "CS4236B" Works
> CX4237B-XQ3 "CS4237B" Works
> CS4239-KQ "CS4239" Works
>
>
> I believe the CX4235 is simply broken -- everything looks in order, just
> no signal. Not expected to have anything to do with things.
>
>
> --- Aztech Sound Galaxy AZT2320 (snd-azt2320)
>
> Chip: AZT2320
>
> Ha, finally... this one looks to be a regression. I have an large stream of:
>
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0xc = 0x0
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x0 = 0xaa
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x1 = 0x45
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0xc = 0x0
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x0 = 0xaa
> ALSA sound/isa/wss/wss_lib.c:229: codec out - reg 0x1 = 0x45
>
> before it craps out (doesn't load). I'll look into it unless you beat me
> to it.
>
I do not have such a card. The debug code shows that it comes from
the wss detection procedure. You may check if the azt2320 driver
correctly enables access to the wss codec (it looks like the codec
is not accessible or it is very slowww).
> --- Aztech Sound Galaxy AZT1605/AZT2316 (local snd-galaxy)
>
> Aztech WSS "Chip" Result
> --------------------------------------------------------------
> AZT1605 CS4231-KL To be tested To be tested
> AZT1605 CS4231A-KL To be tested To be tested
> AZT1605 AD1845JP To be tested To be tested
> AZT2316A CS4248-KL To be tested To be tested
> AZT2316A CS4231A-KL To be tested To be tested
> AZT2316A AD1845XP To be tested To be tested
> AZT2316R CS4231A-KL To be tested To be tested
> AZT2316-S CS4231A-KL To be tested To be tested
>
> I'll test these after reviving snd-galaxy. Saw you already tested one as
> well though.
>
I tested: AZT2316R + CS4231A-KL (card I38-MMSN852)
Regards,
Krzysztof
----------------------------------------------------------------------
Tanie rozmowy!
Sprawdz >>> http://link.interia.pl/f1e91
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2008-08-04 4:26 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-07-18 19:51 [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one Krzysztof Helt
2008-07-20 16:09 ` Rene Herman
2008-07-23 21:28 ` Rene Herman
2008-07-24 5:26 ` Krzysztof Helt
2008-07-24 9:31 ` Rene Herman
2008-07-28 15:37 ` Takashi Iwai
2008-07-28 18:39 ` Rene Herman
2008-07-29 13:02 ` Takashi Iwai
2008-07-29 14:15 ` Rene Herman
2008-07-29 14:31 ` Takashi Iwai
2008-07-29 14:37 ` Rene Herman
2008-07-29 18:53 ` Krzysztof Helt
2008-07-29 19:00 ` Rene Herman
2008-07-24 17:19 ` Rene Herman
2008-07-24 18:47 ` Krzysztof Helt
2008-07-24 19:30 ` Rene Herman
2008-07-24 20:05 ` Rene Herman
2008-07-24 21:41 ` Krzysztof Helt
2008-07-25 9:59 ` Rene Herman
2008-08-04 1:47 ` Rene Herman
2008-08-04 4:31 ` Krzysztof Helt
-- strict thread matches above, loose matches on Subject: below --
2008-07-25 13:39 krzysztof.h1
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.