[alsa-devel] [PATCH 1/2] ALSA: hdspm - Refactor serial number to avoid code duplication
The serial number is used multiple times in hdspm.c. Since it belongs to the card, let's store it in struct hdspm and refer to it whenever necessary.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index d623451..1609253 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -941,6 +941,8 @@ struct hdspm {
cycles_t last_interrupt;
+ unsigned int serial; + struct hdspm_peak_rms peak_rms; };
@@ -4694,7 +4696,7 @@ snd_hdspm_proc_read_madi(struct snd_info_entry * entry,
snd_iprintf(buffer, "HW Serial: 0x%06x%06x\n", (hdspm_read(hdspm, HDSPM_midiStatusIn1)>>8) & 0xFFFFFF, - (hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF); + hdspm->serial);
snd_iprintf(buffer, "IRQ: %d Registers bus: 0x%lx VM: 0x%lx\n", hdspm->irq, hdspm->port, (unsigned long)hdspm->iobase); @@ -6266,8 +6268,7 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, hdspm_version.card_type = hdspm->io_type; strncpy(hdspm_version.cardname, hdspm->card_name, sizeof(hdspm_version.cardname)); - hdspm_version.serial = (hdspm_read(hdspm, - HDSPM_midiStatusIn0)>>8) & 0xFFFFFF; + hdspm_version.serial = hdspm->serial; hdspm_version.firmware_rev = hdspm->firmware_rev; hdspm_version.addons = 0; if (hdspm->tco) @@ -6866,12 +6867,14 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, }
if (hdspm->io_type != MADIface) { + hdspm->serial = (hdspm_read(hdspm, + HDSPM_midiStatusIn0)>>8) & 0xFFFFFF; sprintf(card->shortname, "%s_%x", hdspm->card_name, - (hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF); + hdspm->serial); sprintf(card->longname, "%s S/N 0x%x at 0x%lx, irq %d", hdspm->card_name, - (hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF, + hdspm->serial, hdspm->port, hdspm->irq); } else { sprintf(card->shortname, "%s", hdspm->card_name);
Before, /proc/asound looked like this:
2 [Default ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In case of a second HDSPM card, its name would be Default_1. This is cumbersome, because the order of the cards isn't stable across reboots.
To help userspace tools referring to the correct card, this commit provides a unique id for each card:
2 [HDSPM_f1cd85 ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In this example, userspace (configuration files) would then use hw:HDSPM_f1cd85 to choose the right card.
The serial is masked to 24bits, so this string is always shorter than sixteen chars.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 1609253..a3121c9 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6872,6 +6872,7 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, sprintf(card->shortname, "%s_%x", hdspm->card_name, hdspm->serial); + sprintf(card->id, "HDSPM_%x", hdspm->serial); sprintf(card->longname, "%s S/N 0x%x at 0x%lx, irq %d", hdspm->card_name, hdspm->serial,
At Wed, 4 Jan 2012 14:31:17 +0100, Adrian Knoth wrote:
Before, /proc/asound looked like this:
2 [Default ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In case of a second HDSPM card, its name would be Default_1. This is cumbersome, because the order of the cards isn't stable across reboots.
To help userspace tools referring to the correct card, this commit provides a unique id for each card:
2 [HDSPM_f1cd85 ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In this example, userspace (configuration files) would then use hw:HDSPM_f1cd85 to choose the right card.
The serial is masked to 24bits, so this string is always shorter than sixteen chars.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
This can't be applied as is, unfortunately. This would break when user passes the card id string via option. Also, if multiple boards have the same serial number (who can trust the hardware? :), it gets -EBUSY at registration. And, MADIface will have no serial number, so it'll look strange. (Also the string with "%s_%04x" would be better, IMO.)
For these, I'd suggest the following changes: - check whether serial is non-zero - check whether card->id is empty; fill the new string only if empty - modify sound core registration part to re-generate the id string with suffix whenever conflicted, only not when the automated id string generation via card->id = NULL.
thanks,
Takashi
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 1609253..a3121c9 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6872,6 +6872,7 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, sprintf(card->shortname, "%s_%x", hdspm->card_name, hdspm->serial);
sprintf(card->id, "HDSPM_%x", hdspm->serial);-- 1.7.7.3
On 01/08/2012 04:15 PM, Takashi Iwai wrote:
Hi!
Before, /proc/asound looked like this:
2 [Default ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
2 [HDSPM_f1cd85 ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
This can't be applied as is, unfortunately. This would break when user passes the card id string via option.
Thanks for spotting, I wasn't even aware of this option. :-/
Also, if multiple boards have the same serial number (who can trust the hardware? :), it gets -EBUSY at registration.
Is that true? I don't have two RME cards here, but I've simply duplicated a string from my mainboard chip. snd_card_register() was just fine:
$ cat /proc/asound/cards 0 [PCH ]: HDA-Intel - HDA Intel PCH HDA Intel PCH at 0xf7420000 irq 55 1 [NVidia ]: HDA-Intel - HDA NVidia HDA NVidia at 0xf7080000 irq 17 2 [PCH ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
Note that snd_card_create() is still called without any serial number magic, so card->id will first be set to "Default", "Default_1" and so on and only later modified before calling snd_card_register().
Since I don't see anything in snd_card_register() that could possibly fail, it would at least still work. Anyway, I'm now using snd_card_set_id(), and this function claims to check for duplicates. ;)
And, MADIface will have no serial number, so it'll look strange.
The code in question is limited to "if (!MADIface)", so no problem.
(Also the string with "%s_%04x" would be better, IMO.)
I chose %06x, since the serial is masked with 0xFFFFFF.
For these, I'd suggest the following changes:
- check whether serial is non-zero
That's actually 0xFFFFFF in case there is no serial. All PCI revisions behave like that.
- modify sound core registration part to re-generate the id string with suffix whenever conflicted, only not when the automated id string generation via card->id = NULL.
I did not address this. Firstly, because snd_card_register() worked well even in case of colliding card->ids, and secondly, because snd_card_set_id would take care now to make it unique.
I'll repost the patch in a second.
Before, /proc/asound looked like this:
2 [Default ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In case of a second HDSPM card, its name would be Default_1. This is cumbersome, because the order of the cards isn't stable across reboots.
To help userspace tools referring to the correct card, this commit provides a unique id for each card:
2 [HDSPMxf1cd85 ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In this example, userspace (configuration files) would then use hw:HDSPMxf1cd85 to choose the right card.
The serial is masked to 24bits, so this string is always shorter than sixteen chars.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 1609253..cc9f6c8 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6783,6 +6783,25 @@ static int __devinit snd_hdspm_create(struct snd_card *card, tasklet_init(&hdspm->midi_tasklet, hdspm_midi_tasklet, (unsigned long) hdspm);
+ + if (hdspm->io_type != MADIface) { + hdspm->serial = (hdspm_read(hdspm, + HDSPM_midiStatusIn0)>>8) & 0xFFFFFF; + /* id contains either a user-provided value or the default + * NULL. If it's the default, we're safe to + * fill card->id with the serial number. + * + * If the serial number is 0xFFFFFF, then we're dealing with + * an old PCI revision that comes without a sane number. In + * this case, we don't set card->id to avoid collisions + * when running with multiple cards. + */ + if (NULL == id[hdspm->dev] && hdspm->serial != 0xFFFFFF) { + sprintf(card->id, "HDSPMx%06x", hdspm->serial); + snd_card_set_id(card, card->id); + } + } + snd_printdd("create alsa devices.\n"); err = snd_hdspm_create_alsa_devices(card, hdspm); if (err < 0) @@ -6867,8 +6886,6 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, }
if (hdspm->io_type != MADIface) { - hdspm->serial = (hdspm_read(hdspm, - HDSPM_midiStatusIn0)>>8) & 0xFFFFFF; sprintf(card->shortname, "%s_%x", hdspm->card_name, hdspm->serial);
At Tue, 10 Jan 2012 20:58:40 +0100, Adrian Knoth wrote:
Before, /proc/asound looked like this:
2 [Default ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In case of a second HDSPM card, its name would be Default_1. This is cumbersome, because the order of the cards isn't stable across reboots.
To help userspace tools referring to the correct card, this commit provides a unique id for each card:
2 [HDSPMxf1cd85 ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
In this example, userspace (configuration files) would then use hw:HDSPMxf1cd85 to choose the right card.
The serial is masked to 24bits, so this string is always shorter than sixteen chars.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
Applied now. Thanks.
Takashi
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 1609253..cc9f6c8 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6783,6 +6783,25 @@ static int __devinit snd_hdspm_create(struct snd_card *card, tasklet_init(&hdspm->midi_tasklet, hdspm_midi_tasklet, (unsigned long) hdspm);
- if (hdspm->io_type != MADIface) {
hdspm->serial = (hdspm_read(hdspm,HDSPM_midiStatusIn0)>>8) & 0xFFFFFF;/* id contains either a user-provided value or the default* NULL. If it's the default, we're safe to* fill card->id with the serial number.** If the serial number is 0xFFFFFF, then we're dealing with* an old PCI revision that comes without a sane number. In* this case, we don't set card->id to avoid collisions* when running with multiple cards.*/if (NULL == id[hdspm->dev] && hdspm->serial != 0xFFFFFF) {sprintf(card->id, "HDSPMx%06x", hdspm->serial);snd_card_set_id(card, card->id);}- }
- snd_printdd("create alsa devices.\n"); err = snd_hdspm_create_alsa_devices(card, hdspm); if (err < 0)
@@ -6867,8 +6886,6 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, }
if (hdspm->io_type != MADIface) {
hdspm->serial = (hdspm_read(hdspm,HDSPM_midiStatusIn0)>>8) & 0xFFFFFF;-- 1.7.8.2
At Tue, 10 Jan 2012 20:53:23 +0100, Adrian Knoth wrote:
Also, if multiple boards have the same serial number (who can trust the hardware? :), it gets -EBUSY at registration.
Is that true? I don't have two RME cards here, but I've simply duplicated a string from my mainboard chip. snd_card_register() was just fine:
$ cat /proc/asound/cards 0 [PCH ]: HDA-Intel - HDA Intel PCH HDA Intel PCH at 0xf7420000 irq 55 1 [NVidia ]: HDA-Intel - HDA NVidia HDA NVidia at 0xf7080000 irq 17 2 [PCH ]: HDSPM - RME RayDAT_f1cd85 RME RayDAT S/N 0xf1cd85 at 0xf7300000, irq 18
Note that snd_card_create() is still called without any serial number magic, so card->id will first be set to "Default", "Default_1" and so on and only later modified before calling snd_card_register().
These strings are set in snd_card_register(). At the point of snd_card_create(), the string should be still empty.
Since I don't see anything in snd_card_register() that could possibly fail, it would at least still work. Anyway, I'm now using snd_card_set_id(), and this function claims to check for duplicates. ;)
Well, it looks like a bug. Maybe it got broken when the device id handling was changed sometime ago for allowing dynamic renaming.
Anyway, the fix should be done individually from this patch.
And, MADIface will have no serial number, so it'll look strange.
The code in question is limited to "if (!MADIface)", so no problem.
(Also the string with "%s_%04x" would be better, IMO.)
I chose %06x, since the serial is masked with 0xFFFFFF.
For these, I'd suggest the following changes:
- check whether serial is non-zero
That's actually 0xFFFFFF in case there is no serial. All PCI revisions behave like that.
- modify sound core registration part to re-generate the id string with suffix whenever conflicted, only not when the automated id string generation via card->id = NULL.
I did not address this. Firstly, because snd_card_register() worked well even in case of colliding card->ids, and secondly, because snd_card_set_id would take care now to make it unique.
I'll fix snd_card_register() to make the string unique.
thanks,
Takashi
At Wed, 4 Jan 2012 14:31:16 +0100, Adrian Knoth wrote:
The serial number is used multiple times in hdspm.c. Since it belongs to the card, let's store it in struct hdspm and refer to it whenever necessary.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
One thing this would change is that the serial field returned via ioctl might be different in the case of MADIface. But, judging from the code, it's anyway invalid to read the value for MADIface, so it should be OK, I guess.
Applied now. Thanks!
Takashi
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index d623451..1609253 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -941,6 +941,8 @@ struct hdspm {
cycles_t last_interrupt;
- unsigned int serial;
- struct hdspm_peak_rms peak_rms;
};
@@ -4694,7 +4696,7 @@ snd_hdspm_proc_read_madi(struct snd_info_entry * entry,
snd_iprintf(buffer, "HW Serial: 0x%06x%06x\n", (hdspm_read(hdspm, HDSPM_midiStatusIn1)>>8) & 0xFFFFFF,
(hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF);
hdspm->serial);snd_iprintf(buffer, "IRQ: %d Registers bus: 0x%lx VM: 0x%lx\n", hdspm->irq, hdspm->port, (unsigned long)hdspm->iobase);
@@ -6266,8 +6268,7 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, hdspm_version.card_type = hdspm->io_type; strncpy(hdspm_version.cardname, hdspm->card_name, sizeof(hdspm_version.cardname));
hdspm_version.serial = (hdspm_read(hdspm,HDSPM_midiStatusIn0)>>8) & 0xFFFFFF;
hdspm_version.serial = hdspm->serial;@@ -6866,12 +6867,14 @@ static int __devinit snd_hdspm_probe(struct pci_dev *pci, }
if (hdspm->io_type != MADIface) {
hdspm->serial = (hdspm_read(hdspm,HDSPM_midiStatusIn0)>>8) & 0xFFFFFF;
(hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF);
hdspm->serial);
(hdspm_read(hdspm, HDSPM_midiStatusIn0)>>8) & 0xFFFFFF,
hdspm->serial, hdspm->port, hdspm->irq);-- 1.7.7.3
participants (2)
-
Adrian Knoth -
Takashi Iwai