[alsa-devel] [PATCH 10/10] wss_lib: use wss detection code instead of ad1848 one
Rene Herman
rene.herman at keyaccess.nl
Thu Jul 24 19:19:15 CEST 2008
*** 0001-wss_lib-move-cs4231_lib-into-wss_lib.patch
Reviewed-by: Rene Herman <rene.herman at gmail.ccom>
*** 0002-wss_lib-rename-cs4231.h-into-wss.h.patch
Reviewed-by: Rene Herman <rene.herman at 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 at 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.
More information about the Alsa-devel
mailing list