[PATCH 1/2] ALSA: hdspm - Refactor serial number to avoid code duplication

[PATCH 1/2] ALSA: hdspm - Refactor serial number to avoid code duplication

[Adrian Knoth]

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);
-- 
1.7.7.3

[PATCH 2/2] ALSA: hdspm - Provide unique driver id based on card serial

[Adrian Knoth]

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,
-- 
1.7.7.3

Re: [PATCH 1/2] ALSA: hdspm - Refactor serial number to avoid code duplication

[Takashi Iwai]

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;
>  		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);
> -- 
> 1.7.7.3
> 

Re: [PATCH 2/2] ALSA: hdspm - Provide unique driver id based on card serial

[Takashi Iwai]

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);
>  		sprintf(card->longname, "%s S/N 0x%x at 0x%lx, irq %d",
>  			hdspm->card_name,
>  			hdspm->serial,
> -- 
> 1.7.7.3
> 

Re: [PATCH 2/2] ALSA: hdspm - Provide unique driver id based on card serial

[Adrian Knoth]

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.

[PATCH 1/1] ALSA: hdspm - Provide unique driver id based on card serial

[Adrian Knoth]

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);
-- 
1.7.8.2

Re: [PATCH 2/2] ALSA: hdspm - Provide unique driver id based on card serial

[Takashi Iwai]

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

Re: [PATCH 1/1] ALSA: hdspm - Provide unique driver id based on card serial

[Takashi Iwai]

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;
>  		sprintf(card->shortname, "%s_%x",
>  			hdspm->card_name,
>  			hdspm->serial);
> -- 
> 1.7.8.2
>