[alsa-devel] [PATCH] Improved support for different bt87x board configurations
After applying this patch, the bt87x.patch file in alsa-driver doesn't apply. I'm not sure what I'm supposed to do about that?
# HG changeset patch # User Trent Piepho xyzzy@speakeasy.org # Date 1188469025 25200 # Node ID 33d453db23d246dade155a6fc3b91d8437a4b7f5 # Parent 52dfc5244360d2b0b119786596962ff5d0c9f338 snd-bt87x: Improve support for different board types
Different cards have different audio configurations, but the driver didn't support this. The only setting it had was the digital rate.
This patch adds a board configuration list. Currently, configurable items are the digital rate and the digital data format (for cards with an external ADC), a flag for the absence of an external ADC, and a flag for no connection to the Bt87x internal ADC.
This allows cards that don't use the internal ADC to omit the ALSA "Bt87x analog" device and related controls. Cards without an external ADC can omit the "Bt87x digital" device.
In order to support the CS5331A ADC used on the Osprey 440 and 2x0 cards, the digital format needs to be different than the default.
Support could be added for defining: The connections or lack of them to the Bt87x's internal ADC mux Multiple sample rates for an external ADC (e.g. Osprey) Control of an external mux for an external ADC (e.g. Osprey)
The card definitions for cards other than the Ospreys are kept equivalent to their old values. This is likely inaccurate for most cards, as it is doubtful that both an external and the internal ADC would be used. Lacking information on those cards, the behavior is left unchanged.
Signed-off-by: Trent Piepho xyzzy@speakeasy.org
diff -r 52dfc5244360 -r 33d453db23d2 pci/bt87x.c --- a/pci/bt87x.c Thu Aug 30 10:22:35 2007 +0200 +++ b/pci/bt87x.c Thu Aug 30 03:17:05 2007 -0700 @@ -147,14 +147,67 @@ MODULE_PARM_DESC(load_all, "Allow to loa /* SYNC, one WRITE per line, one extra WRITE per page boundary, SYNC, JUMP */ #define MAX_RISC_SIZE ((1 + 255 + (PAGE_ALIGN(255 * 4092) / PAGE_SIZE - 1) + 1 + 1) * 8)
+/* Cards with configuration information */ +enum snd_bt87x_boardid { + SND_BT87X_BOARD_GENERIC, + SND_BT87X_BOARD_ANALOG, /* board with no external A/D */ + SND_BT87X_BOARD_HAUPPAUGE878, + SND_BT87X_BOARD_OSPREY2x0, + SND_BT87X_BOARD_OSPREY440, + SND_BT87X_BOARD_ATI_TVWONDER, + SND_BT87X_BOARD_WINFAST2000, + SND_BT87X_BOARD_VOODOOTV_200, + SND_BT87X_BOARD_AVPHONE98, +}; + +/* Card configuration */ +struct snd_bt87x_board { + int dig_rate; /* Digital input sampling rate */ + u32 digital_fmt; /* Register settings for digital input */ + unsigned no_analog:1; /* No analog input */ + unsigned no_digital:1; /* No digital input */ +}; + +static const __devinitdata struct snd_bt87x_board snd_bt87x_boards[] = { + [SND_BT87X_BOARD_GENERIC] = { + .dig_rate = 32000, + }, + [SND_BT87X_BOARD_ANALOG] = { + .no_digital = 1, + }, + [SND_BT87X_BOARD_HAUPPAUGE878] = { + .dig_rate = 32000, + }, + [SND_BT87X_BOARD_OSPREY2x0] = { + .dig_rate = 44100, + .digital_fmt = CTL_DA_LRI | (1 << CTL_DA_LRD_SHIFT), + }, + [SND_BT87X_BOARD_OSPREY440] = { + .dig_rate = 32000, + .digital_fmt = CTL_DA_LRI | (1 << CTL_DA_LRD_SHIFT), + .no_analog = 1, + }, + [SND_BT87X_BOARD_ATI_TVWONDER] = { + .dig_rate = 32000, + }, + [SND_BT87X_BOARD_WINFAST2000] = { + .dig_rate = 32000, + }, + [SND_BT87X_BOARD_VOODOOTV_200] = { + .dig_rate = 32000, + }, + [SND_BT87X_BOARD_AVPHONE98] = { + .dig_rate = 48000, + }, +}; + struct snd_bt87x { struct snd_card *card; struct pci_dev *pci; + struct snd_bt87x_board board;
void __iomem *mmio; int irq; - - int dig_rate;
spinlock_t reg_lock; long opened; @@ -342,9 +395,9 @@ static int snd_bt87x_set_digital_hw(stru { chip->reg_control |= CTL_DA_IOM_DA; runtime->hw = snd_bt87x_digital_hw; - runtime->hw.rates = snd_pcm_rate_to_rate_bit(chip->dig_rate); - runtime->hw.rate_min = chip->dig_rate; - runtime->hw.rate_max = chip->dig_rate; + runtime->hw.rates = snd_pcm_rate_to_rate_bit(chip->board.dig_rate); + runtime->hw.rate_min = chip->board.dig_rate; + runtime->hw.rate_max = chip->board.dig_rate; return 0; }
@@ -709,9 +762,9 @@ static int __devinit snd_bt87x_create(st chip->mmio = ioremap_nocache(pci_resource_start(pci, 0), pci_resource_len(pci, 0)); if (!chip->mmio) { - snd_bt87x_free(chip); snd_printk(KERN_ERR "cannot remap io memory\n"); - return -ENOMEM; + err = -ENOMEM; + goto fail; }
chip->reg_control = CTL_DA_ES2 | CTL_PKTP_16 | (15 << CTL_DA_SDR_SHIFT); @@ -720,54 +773,57 @@ static int __devinit snd_bt87x_create(st snd_bt87x_writel(chip, REG_INT_MASK, 0); snd_bt87x_writel(chip, REG_INT_STAT, MY_INTERRUPTS);
- if (request_irq(pci->irq, snd_bt87x_interrupt, IRQF_SHARED, - "Bt87x audio", chip)) { - snd_bt87x_free(chip); - snd_printk(KERN_ERR "cannot grab irq\n"); - return -EBUSY; + err = request_irq(pci->irq, snd_bt87x_interrupt, IRQF_SHARED, + "Bt87x audio", chip); + if (err < 0) { + snd_printk(KERN_ERR "cannot grab irq %d\n", pci->irq); + goto fail; } chip->irq = pci->irq; pci_set_master(pci); synchronize_irq(chip->irq);
err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops); - if (err < 0) { - snd_bt87x_free(chip); - return err; - } + if (err < 0) + goto fail; + snd_card_set_dev(card, &pci->dev); *rchip = chip; return 0; -} - -#define BT_DEVICE(chip, subvend, subdev, rate) \ + +fail: + snd_bt87x_free(chip); + return err; +} + +#define BT_DEVICE(chip, subvend, subdev, id) \ { .vendor = PCI_VENDOR_ID_BROOKTREE, \ - .device = chip, \ + .device = PCI_DEVICE_ID_BROOKTREE ## chip, \ .subvendor = subvend, .subdevice = subdev, \ - .driver_data = rate } - -/* driver_data is the default digital_rate value for that device */ + .driver_data = id } +/* driver_data is the card id for that device */ + static struct pci_device_id snd_bt87x_ids[] = { /* Hauppauge WinTV series */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000), + BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878), /* Hauppauge WinTV series */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_879, 0x0070, 0x13eb, 32000), + BT_DEVICE(_879, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878), /* Viewcast Osprey 200 */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0xff01, 44100), + BT_DEVICE(_878, 0x0070, 0xff01, SND_BT87X_BOARD_OSPREY2x0), /* Viewcast Osprey 440 (rate is configurable via gpio) */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0xff07, 32000), + BT_DEVICE(_878, 0x0070, 0xff07, SND_BT87X_BOARD_OSPREY440), /* ATI TV-Wonder */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x1002, 0x0001, 32000), + BT_DEVICE(_878, 0x1002, 0x0001, SND_BT87X_BOARD_ATI_TVWONDER), /* Leadtek Winfast tv 2000xp delux */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x107d, 0x6606, 32000), + BT_DEVICE(_878, 0x107d, 0x6606, SND_BT87X_BOARD_WINFAST2000), /* Voodoo TV 200 */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x121a, 0x3000, 32000), + BT_DEVICE(_878, 0x121a, 0x3000, SND_BT87X_BOARD_VOODOOTV_200), /* AVerMedia Studio No. 103, 203, ...? */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x1461, 0x0003, 48000), + BT_DEVICE(_878, 0x1461, 0x0003, SND_BT87X_BOARD_AVPHONE98), /* Prolink PixelView PV-M4900 */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x1554, 0x4011, 32000), + BT_DEVICE(_878, 0x1554, 0x4011, SND_BT87X_BOARD_GENERIC), /* Pinnacle Studio PCTV rave */ - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0xbd11, 0x1200, 32000), + BT_DEVICE(_878, 0xbd11, 0x1200, SND_BT87X_BOARD_GENERIC), { } }; MODULE_DEVICE_TABLE(pci, snd_bt87x_ids); @@ -792,7 +848,7 @@ static struct {
static struct pci_driver driver;
-/* return the rate of the card, or a negative value if it's blacklisted */ +/* return the id of the card, or a negative value if it's blacklisted */ static int __devinit snd_bt87x_detect_card(struct pci_dev *pci) { int i; @@ -810,12 +866,12 @@ static int __devinit snd_bt87x_detect_ca return -EBUSY; }
- snd_printk(KERN_INFO "unknown card %#04x-%#04x:%#04x, using default rate 32000\n", - pci->device, pci->subsystem_vendor, pci->subsystem_device); + snd_printk(KERN_INFO "unknown card %#04x-%#04x:%#04x\n", + pci->device, pci->subsystem_vendor, pci->subsystem_device); snd_printk(KERN_DEBUG "please mail id, board name, and, " "if it works, the correct digital_rate option to " "alsa-devel@alsa-project.org\n"); - return 32000; /* default rate */ + return SND_BT87X_BOARD_GENERIC; }
static int __devinit snd_bt87x_probe(struct pci_dev *pci, @@ -824,12 +880,15 @@ static int __devinit snd_bt87x_probe(str static int dev; struct snd_card *card; struct snd_bt87x *chip; - int err, rate; - - rate = pci_id->driver_data; - if (! rate) - if ((rate = snd_bt87x_detect_card(pci)) <= 0) + int err; + enum snd_bt87x_boardid boardid; + + if (! pci_id->driver_data) { + if ((err = snd_bt87x_detect_card(pci)) < 0) return -ENODEV; + boardid = err; + } else + boardid = pci_id->driver_data;
if (dev >= SNDRV_CARDS) return -ENODEV; @@ -846,27 +905,36 @@ static int __devinit snd_bt87x_probe(str if (err < 0) goto _error;
- if (digital_rate[dev] > 0) - chip->dig_rate = digital_rate[dev]; - else - chip->dig_rate = rate; - - err = snd_bt87x_pcm(chip, DEVICE_DIGITAL, "Bt87x Digital"); - if (err < 0) - goto _error; - err = snd_bt87x_pcm(chip, DEVICE_ANALOG, "Bt87x Analog"); - if (err < 0) - goto _error; - - err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_volume, chip)); - if (err < 0) - goto _error; - err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_boost, chip)); - if (err < 0) - goto _error; - err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_source, chip)); - if (err < 0) - goto _error; + memcpy(&chip->board, &snd_bt87x_boards[boardid], sizeof(chip->board)); + + if (! chip->board.no_digital) { + if (digital_rate[dev] > 0) + chip->board.dig_rate = digital_rate[dev]; + + chip->reg_control |= chip->board.digital_fmt; + + err = snd_bt87x_pcm(chip, DEVICE_DIGITAL, "Bt87x Digital"); + if (err < 0) + goto _error; + } + if (! chip->board.no_analog) { + err = snd_bt87x_pcm(chip, DEVICE_ANALOG, "Bt87x Analog"); + if (err < 0) + goto _error; + err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_volume, chip)); + if (err < 0) + goto _error; + err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_boost, chip)); + if (err < 0) + goto _error; + err = snd_ctl_add(card, snd_ctl_new1(&snd_bt87x_capture_source, chip)); + if (err < 0) + goto _error; + } + snd_printk(KERN_INFO "bt87x%d: Using board %d, %sanalog, %sdigital (rate %d Hz)\n", + dev, boardid, chip->board.no_analog ? "no " : "", + chip->board.no_digital ? "no " : "", + chip->board.dig_rate);
strcpy(card->driver, "Bt87x"); sprintf(card->shortname, "Brooktree Bt%x", pci->device); @@ -897,8 +965,8 @@ static void __devexit snd_bt87x_remove(s /* default entries for all Bt87x cards - it's not exported */ /* driver_data is set to 0 to call detection */ static struct pci_device_id snd_bt87x_default_ids[] __devinitdata = { - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, PCI_ANY_ID, PCI_ANY_ID, 0), - BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_879, PCI_ANY_ID, PCI_ANY_ID, 0), + BT_DEVICE(_878, PCI_ANY_ID, PCI_ANY_ID, SND_BT87X_BOARD_GENERIC), + BT_DEVICE(_879, PCI_ANY_ID, PCI_ANY_ID, SND_BT87X_BOARD_GENERIC), { } };
At Thu, 30 Aug 2007 03:21:52 -0700 (PDT), Trent Piepho wrote:
After applying this patch, the bt87x.patch file in alsa-driver doesn't apply. I'm not sure what I'm supposed to do about that?
Would be helpful, of course :) But I can do it myself, too. Don't worry about that.
# HG changeset patch # User Trent Piepho xyzzy@speakeasy.org # Date 1188469025 25200 # Node ID 33d453db23d246dade155a6fc3b91d8437a4b7f5 # Parent 52dfc5244360d2b0b119786596962ff5d0c9f338 snd-bt87x: Improve support for different board types
Different cards have different audio configurations, but the driver didn't support this. The only setting it had was the digital rate.
This patch adds a board configuration list. Currently, configurable items are the digital rate and the digital data format (for cards with an external ADC), a flag for the absence of an external ADC, and a flag for no connection to the Bt87x internal ADC.
This allows cards that don't use the internal ADC to omit the ALSA "Bt87x analog" device and related controls. Cards without an external ADC can omit the "Bt87x digital" device.
In order to support the CS5331A ADC used on the Osprey 440 and 2x0 cards, the digital format needs to be different than the default.
Support could be added for defining: The connections or lack of them to the Bt87x's internal ADC mux Multiple sample rates for an external ADC (e.g. Osprey) Control of an external mux for an external ADC (e.g. Osprey)
The card definitions for cards other than the Ospreys are kept equivalent to their old values. This is likely inaccurate for most cards, as it is doubtful that both an external and the internal ADC would be used. Lacking information on those cards, the behavior is left unchanged.
Signed-off-by: Trent Piepho xyzzy@speakeasy.org
The patch looks fine, but checkpatch.pl complains about some coding style issuse.
One thin I don't like so much is:
+#define BT_DEVICE(chip, subvend, subdev, id) \ { .vendor = PCI_VENDOR_ID_BROOKTREE, \
.device = chip, \
.device = PCI_DEVICE_ID_BROOKTREE ## chip, \
.driver_data = rate }-/* driver_data is the default digital_rate value for that device */
.driver_data = id }+/* driver_data is the card id for that device */
static struct pci_device_id snd_bt87x_ids[] = { /* Hauppauge WinTV series */
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878), /* Hauppauge WinTV series */
The word _878 looks strange. PCI_DEVICE_ID_BROOKTREE_878 is more obvious that it's a macro and the readability isn't so bad. I'd say, let it be.
Thanks!
Takashi
On Thu, 30 Aug 2007, Takashi Iwai wrote:
One thin I don't like so much is:
+#define BT_DEVICE(chip, subvend, subdev, id) \ { .vendor = PCI_VENDOR_ID_BROOKTREE, \
.device = chip, \
.device = PCI_DEVICE_ID_BROOKTREE ## chip, \
.driver_data = rate }-/* driver_data is the default digital_rate value for that device */
.driver_data = id }+/* driver_data is the card id for that device */
static struct pci_device_id snd_bt87x_ids[] = { /* Hauppauge WinTV series */
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878), /* Hauppauge WinTV series */
The word _878 looks strange. PCI_DEVICE_ID_BROOKTREE_878 is more obvious that it's a macro and the readability isn't so bad. I'd say, let it be.
The only problem is that PCI_DEVICE_ID_BROOKTREE_878 is so long that the lines become longer than 80 columns. They have to be wrapped, so it's no longer one line, one card and less readable. It's also easier to miss that one line says PCI_DEVICE_ID_BROOKTREE_879, when you have such a long string repeated many times, with only one digit different.
What if I used "BROOKTREE_878" or some suffix of that?
Trent Piepho wrote:
On Thu, 30 Aug 2007, Takashi Iwai wrote:
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878), /* Hauppauge WinTV series */
The word _878 looks strange. PCI_DEVICE_ID_BROOKTREE_878 is more obvious that it's a macro and the readability isn't so bad. I'd say, let it be.
The only problem is that PCI_DEVICE_ID_BROOKTREE_878 is so long that the lines become longer than 80 columns. They have to be wrapped, so it's no longer one line, one card and less readable. It's also easier to miss that one line says PCI_DEVICE_ID_BROOKTREE_879, when you have such a long string repeated many times, with only one digit different.
What if I used "BROOKTREE_878" or some suffix of that?
My earlier version of the driver that did use "BT_DEVICE(878, ...)" was changed to use PCI_DEVICE_ID_BROOKTREE_878 because only the complete symbol allows to search for PCI IDs with grep.
You could drop the SND_BT87X_ prefix from the board symbols.
The 80 column limit isn't that hard when exceeding it really improves readability.
Regards, Clemens
On Mon, 3 Sep 2007, Clemens Ladisch wrote:
Trent Piepho wrote:
What if I used "BROOKTREE_878" or some suffix of that?
My earlier version of the driver that did use "BT_DEVICE(878, ...)" was changed to use PCI_DEVICE_ID_BROOKTREE_878 because only the complete symbol allows to search for PCI IDs with grep.
You could drop the SND_BT87X_ prefix from the board symbols.
Ok, that's what I did. The other checkpatch warnings are fixed too.
Ok, that's what I did. The other checkpatch warnings are fixed too.
+/* Cards with configuration information */ +enum snd_bt87x_boardid {
SND_BT87X_BOARD_GENERIC,SND_BT87X_BOARD_ANALOG, /* board with no external A/D*/
SND_BT87X_BOARD_HAUPPAUGE878,SND_BT87X_BOARD_OSPREY2x0,SND_BT87X_BOARD_OSPREY440,SND_BT87X_BOARD_ATI_TVWONDER,SND_BT87X_BOARD_WINFAST2000,SND_BT87X_BOARD_VOODOOTV_200,SND_BT87X_BOARD_AVPHONE98,
Hmm... Maybe it would be a good idea to move the board numbers from linux/drivers/media/video/bt8xx/bttv.h to a separate header (maybe something like /include/media/bt8xx-cards.h) and use the same board IDs for both -alsa and -v4l drivers.
This will make easier to associate the audio and video parts for troubleshooting and newer developments.
Cheers, Mauro.
At Tue, 04 Sep 2007 14:54:39 +0100, Mauro Carvalho Chehab wrote:
Ok, that's what I did. The other checkpatch warnings are fixed too.
+/* Cards with configuration information */ +enum snd_bt87x_boardid {
SND_BT87X_BOARD_GENERIC,SND_BT87X_BOARD_ANALOG, /* board with no external A/D*/
SND_BT87X_BOARD_HAUPPAUGE878,SND_BT87X_BOARD_OSPREY2x0,SND_BT87X_BOARD_OSPREY440,SND_BT87X_BOARD_ATI_TVWONDER,SND_BT87X_BOARD_WINFAST2000,SND_BT87X_BOARD_VOODOOTV_200,SND_BT87X_BOARD_AVPHONE98,Hmm... Maybe it would be a good idea to move the board numbers from linux/drivers/media/video/bt8xx/bttv.h to a separate header (maybe something like /include/media/bt8xx-cards.h) and use the same board IDs for both -alsa and -v4l drivers.
This will make easier to associate the audio and video parts for troubleshooting and newer developments.
Yes, it sounds like a good idea.
Takashi
On Tue, 4 Sep 2007, Takashi Iwai wrote:
At Tue, 04 Sep 2007 14:54:39 +0100, Mauro Carvalho Chehab wrote:
SND_BT87X_BOARD_HAUPPAUGE878,SND_BT87X_BOARD_OSPREY2x0,SND_BT87X_BOARD_OSPREY440,SND_BT87X_BOARD_ATI_TVWONDER,SND_BT87X_BOARD_WINFAST2000,SND_BT87X_BOARD_VOODOOTV_200,SND_BT87X_BOARD_AVPHONE98,Hmm... Maybe it would be a good idea to move the board numbers from linux/drivers/media/video/bt8xx/bttv.h to a separate header (maybe something like /include/media/bt8xx-cards.h) and use the same board IDs for both -alsa and -v4l drivers.
This will make easier to associate the audio and video parts for troubleshooting and newer developments.
Yes, it sounds like a good idea.
There are few drawbacks.
First, there are like 20x more cards supported by the bttv driver than by the bt87x driver. It's going to produce a very sparse card database in the ALSA driver. Note that for the most part, __initdata doesn't work, so you can't say "it'll just be dropped after the driver loads."
Secondly, before this patch there was virtually no difference in the configuration for the different supported cards. After it, most of the cards are still exactly the same. It possible to re-use the same generic configuration entry for most of the supported cards. If you use the bttv IDs, it's not.
And finally, bt87x is under ALSA but bttv is under linuxtv. What would include/media/bt8xx-cards.h be under, alsa or linuxtv? Will people have to submit patches to both projects (which use different repository formats locally, so you can't just CC the same patch)? I did one patch that touched both linuxtv and alsa files, and it was a PITA, for a very simple patch. Took a month. Emails to different people. Merge conflicts.
And if both linuxtv and alsa use the same header, what happens if they don't push the changes to GIT at the exact same time? You'll have patches in one subsystem that are dependant on the other subsystem. How will they make the same merge window?
Em Ter, 2007-09-04 às 14:04 -0700, Trent Piepho escreveu:
On Tue, 4 Sep 2007, Takashi Iwai wrote:
At Tue, 04 Sep 2007 14:54:39 +0100, Mauro Carvalho Chehab wrote:
SND_BT87X_BOARD_HAUPPAUGE878,SND_BT87X_BOARD_OSPREY2x0,SND_BT87X_BOARD_OSPREY440,SND_BT87X_BOARD_ATI_TVWONDER,SND_BT87X_BOARD_WINFAST2000,SND_BT87X_BOARD_VOODOOTV_200,SND_BT87X_BOARD_AVPHONE98,Hmm... Maybe it would be a good idea to move the board numbers from linux/drivers/media/video/bt8xx/bttv.h to a separate header (maybe something like /include/media/bt8xx-cards.h) and use the same board IDs for both -alsa and -v4l drivers.
This will make easier to associate the audio and video parts for troubleshooting and newer developments.
Yes, it sounds like a good idea.
There are few drawbacks.
First, there are like 20x more cards supported by the bttv driver than by the bt87x driver. It's going to produce a very sparse card database in the ALSA driver. Note that for the most part, __initdata doesn't work, so you can't say "it'll just be dropped after the driver loads."
You should notice that even the way you've mapped is a a little sparse, since just two boards use digital_fmt.
Maybe one solution for that is to use a different code or data structure.
For example, instead of using a data struct, you could use some sort of code like (*) (**):
static void fill_board (struct snd_bt87x_board &board, int board_id) { board->dig_rate = 32000; switch (board_id) { case BTTV_BOARD_AVPHONE98: board.dig_rate = 48000, break; case BTTV_BOARD_OSPREY2xx: board->dig_rate =44100; board->digital_fmt = CTL_DA_LRI | (1 << CTL_DA_LRD_SHIFT); break; case BTTV_BOARD_OSPREY440: board->digital_fmt = CTL_DA_LRI | (1 << CTL_DA_LRD_SHIFT); break; } }
(*) GCC optimizer probably will even merge the two digital_fmt changes into just one line of object code, and even transform this into an inline. I suspect that this code will be smaller than the data struct.
(**) I didn't found where SND_BT87X_BOARD_ANALOG is used. An usage would be just a case for each analog board, like: case BTTV_BOARD_PV_BT878P_PLUS: case BTTV_BOARD_PV_BT878P_9B: board->no_digital=1;
Secondly, before this patch there was virtually no difference in the configuration for the different supported cards. After it, most of the cards are still exactly the same. It possible to re-use the same generic configuration entry for most of the supported cards. If you use the bttv IDs, it's not.
See above.
And finally, bt87x is under ALSA but bttv is under linuxtv. What would include/media/bt8xx-cards.h be under, alsa or linuxtv? Will people have to submit patches to both projects (which use different repository formats locally, so you can't just CC the same patch)? I did one patch that touched both linuxtv and alsa files, and it was a PITA, for a very simple patch. Took a month. Emails to different people. Merge conflicts.
This seems quite simple to me: it makes not much sense to provide ALSA support if the video is not supported. So, adding a newer board will always add a new entry at the video support. The opposite is not true, since most boards have just an analog card to be connected to the video device. Also, as you said, most boards will just use the default config.
So, the file will be touched more frequently when adding the video entries. My suggestion is that this file would be maintained together with the video counterpart.
And if both linuxtv and alsa use the same header, what happens if they don't push the changes to GIT at the exact same time? You'll have patches in one subsystem that are dependant on the other subsystem. How will they make the same merge window?
This should be coordinated between me and Takashi. I have no objections if he pull such patch, if he is willing to send the pull request before myself.
I don't see this as a big trouble. There are other cases like this already (pci_ids.h, i2c_id.h). Also, currently, there are not much bttv board additions anymore.
Cheers, Mauro
On Wed, 5 Sep 2007, Mauro Carvalho Chehab wrote:
Em Ter, 2007-09-04 às 14:04 -0700, Trent Piepho escreveu:
There are few drawbacks.
First, there are like 20x more cards supported by the bttv driver than by the bt87x driver. It's going to produce a very sparse card database in the ALSA driver. Note that for the most part, __initdata doesn't work, so you can't say "it'll just be dropped after the driver loads."
You should notice that even the way you've mapped is a a little sparse, since just two boards use digital_fmt.
Maybe one solution for that is to use a different code or data structure.
For example, instead of using a data struct, you could use some sort of code like (*) (**):
static void fill_board (struct snd_bt87x_board &board, int board_id) { board->dig_rate = 32000; switch (board_id) { case BTTV_BOARD_AVPHONE98: board.dig_rate = 48000, break;
I don't think gcc will be smart enough to a bunch of cases with random numbers into a O(1) lookup, instead it will use an O(log n) branch chain. With all the explicit assignments, I doubt it will be much smaller, if any, since there are just a couple board configurations. Several board configurations are currently duplicates and I think I'll just merge them.
But code wise I think using a switch with manual assignments is ugly. It's a lot more code to write and just doesn't have the regularity and elegance of a table.
And if both linuxtv and alsa use the same header, what happens if they don't push the changes to GIT at the exact same time? You'll have patches in one subsystem that are dependant on the other subsystem. How will they make the same merge window?
This should be coordinated between me and Takashi. I have no objections if he pull such patch, if he is willing to send the pull request before myself.
I don't see this as a big trouble. There are other cases like this already (pci_ids.h, i2c_id.h). Also, currently, there are not much bttv board additions anymore.
And pci_ids and i2d_id are pain to deal with. Both v4l-dvb and ALSA have out of kernel tree build system that try to be backward compatible with older kernels. When the IDs come from somewhere outside the repository, you need to have compat patches to deal with old id files that are missing ids. Look at all the kernel-sync patches for those id files, all the compat code in the various drivers for non-defined PCI ids. There's even something I had to add to compat.h for an i2c ID that went away in newer kernels.
It's a lot of extra work, and I just don't see the point. To reuse a couple defines from a bttv header file? They are just arbitrary numbers, it's not like there is any benefit from using the same value in both drivers.
At Wed, 5 Sep 2007 02:09:45 -0700 (PDT), Trent Piepho wrote:
On Wed, 5 Sep 2007, Mauro Carvalho Chehab wrote:
Em Ter, 2007-09-04 às 14:04 -0700, Trent Piepho escreveu:
There are few drawbacks.
First, there are like 20x more cards supported by the bttv driver than by the bt87x driver. It's going to produce a very sparse card database in the ALSA driver. Note that for the most part, __initdata doesn't work, so you can't say "it'll just be dropped after the driver loads."
You should notice that even the way you've mapped is a a little sparse, since just two boards use digital_fmt.
Maybe one solution for that is to use a different code or data structure.
For example, instead of using a data struct, you could use some sort of code like (*) (**):
static void fill_board (struct snd_bt87x_board &board, int board_id) { board->dig_rate = 32000; switch (board_id) { case BTTV_BOARD_AVPHONE98: board.dig_rate = 48000, break;
I don't think gcc will be smart enough to a bunch of cases with random numbers into a O(1) lookup, instead it will use an O(log n) branch chain. With all the explicit assignments, I doubt it will be much smaller, if any, since there are just a couple board configurations. Several board configurations are currently duplicates and I think I'll just merge them.
But code wise I think using a switch with manual assignments is ugly. It's a lot more code to write and just doesn't have the regularity and elegance of a table.
I second for Trent. The switch is uglier than the array for such a purpose.
And if both linuxtv and alsa use the same header, what happens if they don't push the changes to GIT at the exact same time? You'll have patches in one subsystem that are dependant on the other subsystem. How will they make the same merge window?
This should be coordinated between me and Takashi. I have no objections if he pull such patch, if he is willing to send the pull request before myself.
I don't see this as a big trouble. There are other cases like this already (pci_ids.h, i2c_id.h). Also, currently, there are not much bttv board additions anymore.
And pci_ids and i2d_id are pain to deal with. Both v4l-dvb and ALSA have out of kernel tree build system that try to be backward compatible with older kernels. When the IDs come from somewhere outside the repository, you need to have compat patches to deal with old id files that are missing ids. Look at all the kernel-sync patches for those id files, all the compat code in the various drivers for non-defined PCI ids. There's even something I had to add to compat.h for an i2c ID that went away in newer kernels.
It's a lot of extra work, and I just don't see the point. To reuse a couple defines from a bttv header file? They are just arbitrary numbers, it's not like there is any benefit from using the same value in both drivers.
Well, at first I misunderstood that we would share the PCI SSIDs together with enum items. Sharing only the enum items doesn't give much improvements. Sharing the card database may do, though.
So, I see Trent's point. The merge work wouldn't pay for the benifit by sharing only the enum id, not the card database.
thanks,
Takashi
Well, at first I misunderstood that we would share the PCI SSIDs together with enum items. Sharing only the enum items doesn't give much improvements. Sharing the card database may do, though.
So, I see Trent's point. The merge work wouldn't pay for the benifit by sharing only the enum id, not the card database.
I like the idea of sharing the database. This probably will mean that snd_bt87x would be dependent of bttv driver (or a bttv core driver). This have the advantage of helping also to fix some conflicts with dvb bttv driver.
This change, if agreed by all, will require some extra work. The better is to commit Trent's patch first. Then, we can work on re-designing bttv driver in a way that the database can be shared, without needing to load the entire bttv drive.
Probably, we should first push the bttv v4l2 series of patches before starting this change at bttv.
On Wed, 5 Sep 2007, Mauro Carvalho Chehab wrote:
Well, at first I misunderstood that we would share the PCI SSIDs together with enum items. Sharing only the enum items doesn't give much improvements. Sharing the card database may do, though.
So, I see Trent's point. The merge work wouldn't pay for the benifit by sharing only the enum id, not the card database.
I like the idea of sharing the database. This probably will mean that snd_bt87x would be dependent of bttv driver (or a bttv core driver). This have the advantage of helping also to fix some conflicts with dvb bttv driver.
I see how that could be an improvement. Now the bt87x driver has a whitelist of cards with sound and a blacklist of cards with DVB, while the bt878 DVB driver has the exact opposite lists. Except the two driver's lists aren't in sync of course.
The cx88 driver keeps the database in the core module. One drawback is that the database can't be __initdata, as it has to stick around after the core module is loaded so the other drivers that are loaded later can get at it. Of course, __initdata doesn't do any for most configurations and it's not that big of a drawback in practice.
Em Qua, 2007-09-05 às 15:34 -0700, Trent Piepho escreveu:
On Wed, 5 Sep 2007, Mauro Carvalho Chehab wrote:
Well, at first I misunderstood that we would share the PCI SSIDs together with enum items. Sharing only the enum items doesn't give much improvements. Sharing the card database may do, though.
So, I see Trent's point. The merge work wouldn't pay for the benifit by sharing only the enum id, not the card database.
I like the idea of sharing the database. This probably will mean that snd_bt87x would be dependent of bttv driver (or a bttv core driver). This have the advantage of helping also to fix some conflicts with dvb bttv driver.
I see how that could be an improvement. Now the bt87x driver has a whitelist of cards with sound and a blacklist of cards with DVB, while the bt878 DVB driver has the exact opposite lists. Except the two driver's lists aren't in sync of course.
The cx88 driver keeps the database in the core module. One drawback is that the database can't be __initdata, as it has to stick around after the core module is loaded so the other drivers that are loaded later can get at it. Of course, __initdata doesn't do any for most configurations and it's not that big of a drawback in practice.
Trent,
Could you work on such patches for 2.6.25? For 2.6.24, my suggestion is just to apply your already submitted one.
Takashi,
If you want, you may add my ack to the already-sent Trent's patch:
Reviewed-by: Mauro Carvalho Chehab mchehab@infradead.org
Cheers, Mauro.
At Thu, 06 Sep 2007 21:17:12 +0100, Mauro Carvalho Chehab wrote:
Em Qua, 2007-09-05 às 15:34 -0700, Trent Piepho escreveu:
On Wed, 5 Sep 2007, Mauro Carvalho Chehab wrote:
Well, at first I misunderstood that we would share the PCI SSIDs together with enum items. Sharing only the enum items doesn't give much improvements. Sharing the card database may do, though.
So, I see Trent's point. The merge work wouldn't pay for the benifit by sharing only the enum id, not the card database.
I like the idea of sharing the database. This probably will mean that snd_bt87x would be dependent of bttv driver (or a bttv core driver). This have the advantage of helping also to fix some conflicts with dvb bttv driver.
I see how that could be an improvement. Now the bt87x driver has a whitelist of cards with sound and a blacklist of cards with DVB, while the bt878 DVB driver has the exact opposite lists. Except the two driver's lists aren't in sync of course.
The cx88 driver keeps the database in the core module. One drawback is that the database can't be __initdata, as it has to stick around after the core module is loaded so the other drivers that are loaded later can get at it. Of course, __initdata doesn't do any for most configurations and it's not that big of a drawback in practice.
Trent,
Could you work on such patches for 2.6.25? For 2.6.24, my suggestion is just to apply your already submitted one.
Takashi,
If you want, you may add my ack to the already-sent Trent's patch:
Reviewed-by: Mauro Carvalho Chehab mchehab@infradead.org
Thanks, but I have already committed, and don't want to rollback now :)
(BTW, how is this tag supposed to use? On git or Hg, it's hard to change the already committed stuff, especially after openly published...)
Takashi
Reviewed-by: Mauro Carvalho Chehab mchehab@infradead.org
Thanks, but I have already committed, and don't want to rollback now :)
No problem ;)
BTW, how is this tag supposed to use?
This is a new tag that were agreed to be used informally at KS/2007 :)
The idea behind is to use the following logic:
Signed-off-by: - should be used by the author(s) and the trees for what the patch passed. A maintainer should add this tag to confirm that he received a patch by its author or from a sub-subsystem tree (in fact, sometimes, this tag is abused);
Acked-by: - this simply means that you aren't against some patch;
Reviewed-by: - It means that somebody that are not at the direct forwarding way of a patch has reviewed and/or tested a patch.
So, in this case, as Trent sent the patch directly to you (even I'm being C/C), I should use either acked-by (if I'm just ok with this) or reviewed-by. As I've reviewed the source code, reviewed-by is the better option.
Also, subsystem maintainers are asked to require at least one reviewed-by by patch. The idea is to stimulate the community to review changesets and improve the overall quality of the kernel.
On git or Hg, it's hard to change the already committed stuff, especially after openly published...)
What I do here is: I don't change -hg tree. During my -git conversion, I add such tags that I receive from third parties, even for patches already published at kernel.org. I only stop adding tags after asking a git pull.
Here, I'm still using stgit porcelain to allow me to review changesets, but this is not recommended. Probably, I'll review my procedures. However, git have already some way for you to edit a message for an already-edited changeset.
On possible way would be do to this - supposing that you want to edit the patch that is HEAD - 3 patches from your master branch:
git checkout HEAD~3 git commit -e --amend (edit the comments) git chckout master
I didn't really tested the above procedure, so I would try this on an experimental tree first. .
Takashi
At Tue, 4 Sep 2007 04:49:46 -0700 (PDT), Trent Piepho wrote:
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878),
It's a bad move. As Clemens pointed, it was in a similar expression in the earlier version, but we changed to the form with PCI_DEVICE_ID_ prefix *intentionally* after complains from kernel hackers (and that was reasonable). If the length of a line really matters, we can drop SND_BT87X_ prefix from the board enum items instead.
thanks,
Takashi
On Wed, 5 Sep 2007, Takashi Iwai wrote:
At Tue, 4 Sep 2007 04:49:46 -0700 (PDT), Trent Piepho wrote:
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878),
It's a bad move. As Clemens pointed, it was in a similar expression in the earlier version, but we changed to the form with PCI_DEVICE_ID_ prefix *intentionally* after complains from kernel hackers (and that was reasonable). If the length of a line really matters, we can drop SND_BT87X_ prefix from the board enum items instead.
This is what I've done. I think last time I sent the patch I accidently sent an old version without this change.
I also merged a few of the SND_BT87X boards with the same configuration. Initially I though each board should have a unique entry in the configuration list, but the fact is that there are only a couple different configurations now, and probably won't be many more, if any. It makes more sense to think of it as a "board type id" instead of "board id", since there is several times more boards than there are board types.
At Wed, 5 Sep 2007 15:23:06 -0700 (PDT), Trent Piepho wrote:
On Wed, 5 Sep 2007, Takashi Iwai wrote:
At Tue, 4 Sep 2007 04:49:46 -0700 (PDT), Trent Piepho wrote:
- BT_DEVICE(PCI_DEVICE_ID_BROOKTREE_878, 0x0070, 0x13eb, 32000),
- BT_DEVICE(_878, 0x0070, 0x13eb, SND_BT87X_BOARD_HAUPPAUGE878),
It's a bad move. As Clemens pointed, it was in a similar expression in the earlier version, but we changed to the form with PCI_DEVICE_ID_ prefix *intentionally* after complains from kernel hackers (and that was reasonable). If the length of a line really matters, we can drop SND_BT87X_ prefix from the board enum items instead.
This is what I've done. I think last time I sent the patch I accidently sent an old version without this change.
I also merged a few of the SND_BT87X boards with the same configuration. Initially I though each board should have a unique entry in the configuration list, but the fact is that there are only a couple different configurations now, and probably won't be many more, if any. It makes more sense to think of it as a "board type id" instead of "board id", since there is several times more boards than there are board types.
OK, then that's fine. I merged your patch to HG tree now. Thanks!
Takashi
participants (4)
-
Clemens Ladisch -
Mauro Carvalho Chehab -
Takashi Iwai -
Trent Piepho