[alsa-devel] [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
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
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.
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.
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
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.
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)
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.
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
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.
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:
- 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...
- 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.
- 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
On 29-07-08 16:31, Takashi Iwai wrote:
- 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.
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:
- 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...
- 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.
- 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
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.
*** 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.
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
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.
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
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
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.
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.
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
participants (3)
-
Krzysztof Helt -
Rene Herman -
Takashi Iwai