diff mbox

[21/30] ALSA: oxfw: Change the way to name card

Message ID 1417190379-4172-22-git-send-email-o-takashi@sakamocchi.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Sakamoto Nov. 28, 2014, 3:59 p.m. UTC
The 'struct snd_card' has 4 members for name. They're 'driver', 'shortname',
'longname' and 'mixername'. This commit names these 4 members according to
two members in model-specific structure and reduce rest of members in the
structure.

The reason that alias names are still used is that currently supported devices,
Griffin FireWave and LaCie Firewire Speaker have too long names for model and
vendor field in their config rom to be over allocated buffer for the names.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/oxfw/oxfw.c | 51 +++++++++++++++++++++++++++-------------------
 sound/firewire/oxfw/oxfw.h |  3 +--
 2 files changed, 31 insertions(+), 23 deletions(-)

Comments

Takashi Iwai Nov. 29, 2014, 7:37 p.m. UTC | #1
At Sat, 29 Nov 2014 00:59:30 +0900,
Takashi Sakamoto wrote:
> 
> The 'struct snd_card' has 4 members for name. They're 'driver', 'shortname',
> 'longname' and 'mixername'. This commit names these 4 members according to
> two members in model-specific structure and reduce rest of members in the
> structure.
> 
> The reason that alias names are still used is that currently supported devices,
> Griffin FireWave and LaCie Firewire Speaker have too long names for model and
> vendor field in their config rom to be over allocated buffer for the names.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/oxfw/oxfw.c | 51 +++++++++++++++++++++++++++-------------------
>  sound/firewire/oxfw/oxfw.h |  3 +--
>  2 files changed, 31 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
> index 951d9a4..3308253 100644
> --- a/sound/firewire/oxfw/oxfw.c
> +++ b/sound/firewire/oxfw/oxfw.c
> @@ -25,14 +25,34 @@ MODULE_AUTHOR("Clemens Ladisch <clemens@ladisch.de>");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("snd-firewire-speakers");
>  
> -static u32 oxfw_read_firmware_version(struct fw_unit *unit)
> +static int name_card(struct snd_oxfw *oxfw)
>  {
> -	__be32 data;
> +	struct fw_device *fw_dev = fw_parent_device(oxfw->unit);
> +	const char *d, *v, *m;
> +	u32 firmware;
>  	int err;
>  
> -	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
> -				 OXFORD_FIRMWARE_ID_ADDRESS, &data, 4, 0);
> -	return err >= 0 ? be32_to_cpu(data) : 0;
> +	err = snd_fw_transaction(oxfw->unit, TCODE_READ_QUADLET_REQUEST,
> +				 OXFORD_FIRMWARE_ID_ADDRESS, &firmware, 4, 0);
> +	if (err < 0)
> +		goto end;
> +	be32_to_cpus(&firmware);
> +
> +	d = oxfw->device_info->driver_name;
> +	v = oxfw->device_info->vendor_name;
> +	m = oxfw->device_info->driver_name;
> +
> +	strcpy(oxfw->card->driver, d);
> +	strcpy(oxfw->card->shortname, m);
> +	strcpy(oxfw->card->mixername, m);

So, this ends up with the identical strings for both driver and
shortname?  This doesn't sound like an improvement.


thanks,

Takashi
Takashi Sakamoto Nov. 30, 2014, 4:33 a.m. UTC | #2
On Nov 30 2014 04:37, Takashi Iwai wrote:
>> diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
>> index 951d9a4..3308253 100644
>> --- a/sound/firewire/oxfw/oxfw.c
>> +++ b/sound/firewire/oxfw/oxfw.c
>> @@ -25,14 +25,34 @@ MODULE_AUTHOR("Clemens Ladisch <clemens@ladisch.de>");
>>  MODULE_LICENSE("GPL v2");
>>  MODULE_ALIAS("snd-firewire-speakers");
>>  
>> -static u32 oxfw_read_firmware_version(struct fw_unit *unit)
>> +static int name_card(struct snd_oxfw *oxfw)
>>  {
>> -	__be32 data;
>> +	struct fw_device *fw_dev = fw_parent_device(oxfw->unit);
>> +	const char *d, *v, *m;
>> +	u32 firmware;
>>  	int err;
>>  
>> -	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
>> -				 OXFORD_FIRMWARE_ID_ADDRESS, &data, 4, 0);
>> -	return err >= 0 ? be32_to_cpu(data) : 0;
>> +	err = snd_fw_transaction(oxfw->unit, TCODE_READ_QUADLET_REQUEST,
>> +				 OXFORD_FIRMWARE_ID_ADDRESS, &firmware, 4, 0);
>> +	if (err < 0)
>> +		goto end;
>> +	be32_to_cpus(&firmware);
>> +
>> +	d = oxfw->device_info->driver_name;
>> +	v = oxfw->device_info->vendor_name;
>> +	m = oxfw->device_info->driver_name;
>> +
>> +	strcpy(oxfw->card->driver, d);
>> +	strcpy(oxfw->card->shortname, m);
>> +	strcpy(oxfw->card->mixername, m);
> 
> So, this ends up with the identical strings for both driver and
> shortname?  This doesn't sound like an improvement.

Basically, hard-coded strings can be replaced by strings read from
config-rom on the devices. My original intension of this commit is
following to this.

But FWSpeakers/FireWave has long strings for vendor/model strings on
config-rom, therefore his commit can obsolete long name only.

Actually this patch is a help for later patch:
[PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Iwai Nov. 30, 2014, 8:31 a.m. UTC | #3
At Sun, 30 Nov 2014 13:33:31 +0900,
Takashi Sakamoto wrote:
> 
> On Nov 30 2014 04:37, Takashi Iwai wrote:
> >> diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
> >> index 951d9a4..3308253 100644
> >> --- a/sound/firewire/oxfw/oxfw.c
> >> +++ b/sound/firewire/oxfw/oxfw.c
> >> @@ -25,14 +25,34 @@ MODULE_AUTHOR("Clemens Ladisch <clemens@ladisch.de>");
> >>  MODULE_LICENSE("GPL v2");
> >>  MODULE_ALIAS("snd-firewire-speakers");
> >>  
> >> -static u32 oxfw_read_firmware_version(struct fw_unit *unit)
> >> +static int name_card(struct snd_oxfw *oxfw)
> >>  {
> >> -	__be32 data;
> >> +	struct fw_device *fw_dev = fw_parent_device(oxfw->unit);
> >> +	const char *d, *v, *m;
> >> +	u32 firmware;
> >>  	int err;
> >>  
> >> -	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
> >> -				 OXFORD_FIRMWARE_ID_ADDRESS, &data, 4, 0);
> >> -	return err >= 0 ? be32_to_cpu(data) : 0;
> >> +	err = snd_fw_transaction(oxfw->unit, TCODE_READ_QUADLET_REQUEST,
> >> +				 OXFORD_FIRMWARE_ID_ADDRESS, &firmware, 4, 0);
> >> +	if (err < 0)
> >> +		goto end;
> >> +	be32_to_cpus(&firmware);
> >> +
> >> +	d = oxfw->device_info->driver_name;
> >> +	v = oxfw->device_info->vendor_name;
> >> +	m = oxfw->device_info->driver_name;
> >> +
> >> +	strcpy(oxfw->card->driver, d);
> >> +	strcpy(oxfw->card->shortname, m);
> >> +	strcpy(oxfw->card->mixername, m);
> > 
> > So, this ends up with the identical strings for both driver and
> > shortname?  This doesn't sound like an improvement.
> 
> Basically, hard-coded strings can be replaced by strings read from
> config-rom on the devices. My original intension of this commit is
> following to this.
> 
> But FWSpeakers/FireWave has long strings for vendor/model strings on
> config-rom, therefore his commit can obsolete long name only.
> 
> Actually this patch is a help for later patch:
> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html

But still it's not applied to LaCie and other that have device_info,
no?  So they'll end up with the even shorter names than now.

I'd be better to create a short_name from "Vendor Model" where
driver_name can be a fallback of model name.


Takashi
Takashi Sakamoto Nov. 30, 2014, 11:19 p.m. UTC | #4
On Nov 30 2013 17:31, Takashi Iwai wrote:
>>> So, this ends up with the identical strings for both driver and
>>> shortname?  This doesn't sound like an improvement.
>>
>> Basically, hard-coded strings can be replaced by strings read from
>> config-rom on the devices. My original intension of this commit is
>> following to this.
>>
>> But FWSpeakers/FireWave has long strings for vendor/model strings on
>> config-rom, therefore his commit can obsolete long name only.
>>
>> Actually this patch is a help for later patch:
>> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
>> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html
> 
> But still it's not applied to LaCie and other that have device_info,
> no?  So they'll end up with the even shorter names than now.
>
> I'd be better to create a short_name from "Vendor Model" where
> driver_name can be a fallback of model name.

Any reasons?


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Iwai Dec. 1, 2014, 8:41 a.m. UTC | #5
At Mon, 01 Dec 2014 08:19:14 +0900,
Takashi Sakamoto wrote:
> 
> On Nov 30 2013 17:31, Takashi Iwai wrote:
> >>> So, this ends up with the identical strings for both driver and
> >>> shortname?  This doesn't sound like an improvement.
> >>
> >> Basically, hard-coded strings can be replaced by strings read from
> >> config-rom on the devices. My original intension of this commit is
> >> following to this.
> >>
> >> But FWSpeakers/FireWave has long strings for vendor/model strings on
> >> config-rom, therefore his commit can obsolete long name only.
> >>
> >> Actually this patch is a help for later patch:
> >> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
> >> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html
> > 
> > But still it's not applied to LaCie and other that have device_info,
> > no?  So they'll end up with the even shorter names than now.
> >
> > I'd be better to create a short_name from "Vendor Model" where
> > driver_name can be a fallback of model name.
> 
> Any reasons?

Read again: it'll be "shorter name than now".

Basically it's strange to have a very same string for both driver and
shrotname.  The shortname is a more detailed description usually
including both vendor and model names while the driver is the unique
identifier string.

OK?


Takashi
Clemens Ladisch Dec. 1, 2014, 11:21 a.m. UTC | #6
Takashi Iwai wrote:
> Takashi Sakamoto wrote:
>> On Nov 30 2013 17:31, Takashi Iwai wrote:
>>>>> So, this ends up with the identical strings for both driver and
>>>>> shortname?  This doesn't sound like an improvement.
>>>>
>>>> Basically, hard-coded strings can be replaced by strings read from
>>>> config-rom on the devices. My original intension of this commit is
>>>> following to this.
>>>>
>>>> But FWSpeakers/FireWave has long strings for vendor/model strings on
>>>> config-rom, therefore his commit can obsolete long name only.
>>>>
>>>> Actually this patch is a help for later patch:
>>>> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
>>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html
>>>
>>> But still it's not applied to LaCie and other that have device_info,
>>> no?  So they'll end up with the even shorter names than now.
>>>
>>> I'd be better to create a short_name from "Vendor Model" where
>>> driver_name can be a fallback of model name.
>>
>> Any reasons?
>
> Read again: it'll be "shorter name than now".
>
> Basically it's strange to have a very same string for both driver and
> shrotname.  The shortname is a more detailed description usually
> including both vendor and model names while the driver is the unique
> identifier string.

(While the driver name often is name of the driver, it is also used to
select one of the configuration files in /usr/share/alsa/cards/.  The
FireWave and the LaCie speakers are often used for 'normal' 5.1 and 2.0
playback, and need different configurations, so I used different driver
names for these.

For these two devices, the driver name should stay the same.  All the
other snd-oxfw device are (semi)pro devices where it does not really
make sense to define devices like "surround51", so the driver name does
not matter and might as well be "OXFW".)

As for the shortname, it is not an internal identifier but typically
displayed to identify the device for the user.  Something like
"FWSpeakers" might be recognizable, but I'd prefer the old "FireWire
Speakers".


Regards,
Clemens
Takashi Sakamoto Dec. 1, 2014, 2:58 p.m. UTC | #7
On Dec 1 2014 17:41, Takashi Iwai wrote:
> At Mon, 01 Dec 2014 08:19:14 +0900,
> Takashi Sakamoto wrote:
>>
>> On Nov 30 2013 17:31, Takashi Iwai wrote:
>>>>> So, this ends up with the identical strings for both driver and
>>>>> shortname?  This doesn't sound like an improvement.
>>>>
>>>> Basically, hard-coded strings can be replaced by strings read from
>>>> config-rom on the devices. My original intension of this commit is
>>>> following to this.
>>>>
>>>> But FWSpeakers/FireWave has long strings for vendor/model strings on
>>>> config-rom, therefore his commit can obsolete long name only.
>>>>
>>>> Actually this patch is a help for later patch:
>>>> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
>>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html
>>>
>>> But still it's not applied to LaCie and other that have device_info,
>>> no?  So they'll end up with the even shorter names than now.
>>>
>>> I'd be better to create a short_name from "Vendor Model" where
>>> driver_name can be a fallback of model name.
>>
>> Any reasons?
> 
> Read again: it'll be "shorter name than now".

I think this is a description, and

> Basically it's strange to have a very same string for both driver and
> shrotname.  The shortname is a more detailed description usually
> including both vendor and model names while the driver is the unique
> identifier string.

this is a reason, your intension. This is what I required because I
didn't understand why you insist.

There is just a comment, '/* short name of this soundcard */' in
include/sound/core.h or '/* Short name of soundcard */' in
include/uapi/sound/asound.h. There is no hint for how I should define
the shortname.

Well, I don't mind to use 'Vendor Model' style for shortname. This is
also good for identical names such as 'hw:FireWave' because the last
word of shortname is used for the identical name. Therefore I don't like
to use reverse combination, 'Model Vendor' style, such as 'HDA Intel'.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Sakamoto Dec. 1, 2014, 3:05 p.m. UTC | #8
On Dec 1 2014 20:21, Clemens Ladisch wrote:
> (While the driver name often is name of the driver, it is also used to
> select one of the configuration files in /usr/share/alsa/cards/.  The
> FireWave and the LaCie speakers are often used for 'normal' 5.1 and 2.0
> playback, and need different configurations, so I used different driver
> names for these.
> 
> For these two devices, the driver name should stay the same.  All the
> other snd-oxfw device are (semi)pro devices where it does not really
> make sense to define devices like "surround51", so the driver name does
> not matter and might as well be "OXFW".)

Exactly. If userspace didn't have the configuration files, I were
willing to use 'OXFW' as driver name for all of models.

> As for the shortname, it is not an internal identifier but typically
> displayed to identify the device for the user.  Something like
> "FWSpeakers" might be recognizable, but I'd prefer the old "FireWire
> Speakers".

In this case (the old name), 'Speakers' is an identical name such as
'hw:Speakers'. IMHO, this is a bit strange because it's usual, not
identical.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Iwai Dec. 1, 2014, 3:21 p.m. UTC | #9
At Tue, 02 Dec 2014 00:05:31 +0900,
Takashi Sakamoto wrote:
> 
> On Dec 1 2014 20:21, Clemens Ladisch wrote:
> > (While the driver name often is name of the driver, it is also used to
> > select one of the configuration files in /usr/share/alsa/cards/.  The
> > FireWave and the LaCie speakers are often used for 'normal' 5.1 and 2.0
> > playback, and need different configurations, so I used different driver
> > names for these.
> > 
> > For these two devices, the driver name should stay the same.  All the
> > other snd-oxfw device are (semi)pro devices where it does not really
> > make sense to define devices like "surround51", so the driver name does
> > not matter and might as well be "OXFW".)
> 
> Exactly. If userspace didn't have the configuration files, I were
> willing to use 'OXFW' as driver name for all of models.
> 
> > As for the shortname, it is not an internal identifier but typically
> > displayed to identify the device for the user.  Something like
> > "FWSpeakers" might be recognizable, but I'd prefer the old "FireWire
> > Speakers".
> 
> In this case (the old name), 'Speakers' is an identical name such as
> 'hw:Speakers'. IMHO, this is a bit strange because it's usual, not
> identical.

You see now that you're breaking user's setup, don't you?
This is another reason why you shouldn't change the short name
string.  Keeping things running is more valuable than cleanup.


Takashi
Takashi Iwai Dec. 1, 2014, 3:22 p.m. UTC | #10
At Mon, 01 Dec 2014 23:58:43 +0900,
Takashi Sakamoto wrote:
> 
> On Dec 1 2014 17:41, Takashi Iwai wrote:
> > At Mon, 01 Dec 2014 08:19:14 +0900,
> > Takashi Sakamoto wrote:
> >>
> >> On Nov 30 2013 17:31, Takashi Iwai wrote:
> >>>>> So, this ends up with the identical strings for both driver and
> >>>>> shortname?  This doesn't sound like an improvement.
> >>>>
> >>>> Basically, hard-coded strings can be replaced by strings read from
> >>>> config-rom on the devices. My original intension of this commit is
> >>>> following to this.
> >>>>
> >>>> But FWSpeakers/FireWave has long strings for vendor/model strings on
> >>>> config-rom, therefore his commit can obsolete long name only.
> >>>>
> >>>> Actually this patch is a help for later patch:
> >>>> [PATCH 26/30] ALSA: oxfw: Add support for Behringer/Mackie devices
> >>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-November/084818.html
> >>>
> >>> But still it's not applied to LaCie and other that have device_info,
> >>> no?  So they'll end up with the even shorter names than now.
> >>>
> >>> I'd be better to create a short_name from "Vendor Model" where
> >>> driver_name can be a fallback of model name.
> >>
> >> Any reasons?
> > 
> > Read again: it'll be "shorter name than now".
> 
> I think this is a description, and
> 
> > Basically it's strange to have a very same string for both driver and
> > shrotname.  The shortname is a more detailed description usually
> > including both vendor and model names while the driver is the unique
> > identifier string.
> 
> this is a reason, your intension. This is what I required because I
> didn't understand why you insist.
> 
> There is just a comment, '/* short name of this soundcard */' in
> include/sound/core.h or '/* Short name of soundcard */' in
> include/uapi/sound/asound.h. There is no hint for how I should define
> the shortname.

You learned now.


Takashi
Takashi Sakamoto Dec. 2, 2014, 6:17 a.m. UTC | #11
On Dec 2 2014 00:21, Takashi Iwai wrote:
>>> As for the shortname, it is not an internal identifier but typically
>>> displayed to identify the device for the user.  Something like
>>> "FWSpeakers" might be recognizable, but I'd prefer the old "FireWire
>>> Speakers".
>>
>> In this case (the old name), 'Speakers' is an identical name such as
>> 'hw:Speakers'. IMHO, this is a bit strange because it's usual, not
>> identical.
>
> You see now that you're breaking user's setup, don't you?
> This is another reason why you shouldn't change the short name
> string.

In this case, it depends on the estimated number of actual users for 
this device. At least, snd-firewire-speakers cannot handle this device 
correctly, while there're no bug reports, for 4 years. I think this is 
an evidence of few users.

...although havind said so, it's not my favorite to spend more time to 
such a small things, instead of more important things. I'll keep the 
naming rule what it was in next patchset.


Thanks

Takashi Sakamoto
o-takashi@sakamocchi.jp
Takashi Iwai Dec. 2, 2014, 6:41 a.m. UTC | #12
At Tue, 02 Dec 2014 15:17:25 +0900,
Takashi Sakamoto wrote:
> 
> On Dec 2 2014 00:21, Takashi Iwai wrote:
> >>> As for the shortname, it is not an internal identifier but typically
> >>> displayed to identify the device for the user.  Something like
> >>> "FWSpeakers" might be recognizable, but I'd prefer the old "FireWire
> >>> Speakers".
> >>
> >> In this case (the old name), 'Speakers' is an identical name such as
> >> 'hw:Speakers'. IMHO, this is a bit strange because it's usual, not
> >> identical.
> >
> > You see now that you're breaking user's setup, don't you?
> > This is another reason why you shouldn't change the short name
> > string.
> 
> In this case, it depends on the estimated number of actual users for 
> this device.

No.  The number doesn't matter unless it's proved to be really no
regression.

> At least, snd-firewire-speakers cannot handle this device 
> correctly, while there're no bug reports, for 4 years. I think this is 
> an evidence of few users.

Did you get reports that the name hw:Speaker is bad, too?
If not, why do you insist to "fix" this at all?  It's just because you
feel so -- which is no objective measurement, either.

> ...although havind said so, it's not my favorite to spend more time to 
> such a small things, instead of more important things. I'll keep the 
> naming rule what it was in next patchset.

There is no more important things than fixing a regression.

Just fix it for the next patchset, or patches won't be accepted.
That's so simple.

I'm giving a strong word here because you might miss the point: in
such a rewrite, the primary item you have to care is to avoid
regressions.  Supporting other devices, the cleanness of the code, or
even the correctness of the code, is hence in the second place.


thanks,

Takashi
Takashi Sakamoto Dec. 3, 2014, 2:19 p.m. UTC | #13
Sorry to be late.

On Dec 2 2014 15:41, Takashi Iwai wrote:
>>> You see now that you're breaking user's setup, don't you?
>>> This is another reason why you shouldn't change the short name
>>> string.
>>
>> In this case, it depends on the estimated number of actual users for 
>> this device.
> 
> No.  The number doesn't matter unless it's proved to be really no
> regression.
> 
>> At least, snd-firewire-speakers cannot handle this device 
>> correctly, while there're no bug reports, for 4 years. I think this is 
>> an evidence of few users.
> 
> Did you get reports that the name hw:Speaker is bad, too?
> If not, why do you insist to "fix" this at all?  It's just because you
> feel so -- which is no objective measurement, either.

Not. Just depending on my feeling.

>> ...although havind said so, it's not my favorite to spend more time to 
>> such a small things, instead of more important things. I'll keep the 
>> naming rule what it was in next patchset.
> 
> There is no more important things than fixing a regression.
> 
> Just fix it for the next patchset, or patches won't be accepted.
> That's so simple.
> 
> I'm giving a strong word here because you might miss the point: in
> such a rewrite, the primary item you have to care is to avoid
> regressions.  Supporting other devices, the cleanness of the code, or
> even the correctness of the code, is hence in the second place.

I'm sure.

This weekend, I'll post the next patchset. I hope it's the last one for
OXFW 970/971.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp
diff mbox

Patch

diff --git a/sound/firewire/oxfw/oxfw.c b/sound/firewire/oxfw/oxfw.c
index 951d9a4..3308253 100644
--- a/sound/firewire/oxfw/oxfw.c
+++ b/sound/firewire/oxfw/oxfw.c
@@ -25,14 +25,34 @@  MODULE_AUTHOR("Clemens Ladisch <clemens@ladisch.de>");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("snd-firewire-speakers");
 
-static u32 oxfw_read_firmware_version(struct fw_unit *unit)
+static int name_card(struct snd_oxfw *oxfw)
 {
-	__be32 data;
+	struct fw_device *fw_dev = fw_parent_device(oxfw->unit);
+	const char *d, *v, *m;
+	u32 firmware;
 	int err;
 
-	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
-				 OXFORD_FIRMWARE_ID_ADDRESS, &data, 4, 0);
-	return err >= 0 ? be32_to_cpu(data) : 0;
+	err = snd_fw_transaction(oxfw->unit, TCODE_READ_QUADLET_REQUEST,
+				 OXFORD_FIRMWARE_ID_ADDRESS, &firmware, 4, 0);
+	if (err < 0)
+		goto end;
+	be32_to_cpus(&firmware);
+
+	d = oxfw->device_info->driver_name;
+	v = oxfw->device_info->vendor_name;
+	m = oxfw->device_info->driver_name;
+
+	strcpy(oxfw->card->driver, d);
+	strcpy(oxfw->card->shortname, m);
+	strcpy(oxfw->card->mixername, m);
+
+	snprintf(oxfw->card->longname, sizeof(oxfw->card->longname),
+		 "%s %s (OXFW%x %04x), GUID %08x%08x at %s, S%d",
+		 v, m, firmware >> 20, firmware & 0xffff,
+		 fw_dev->config_rom[3], fw_dev->config_rom[4],
+		 dev_name(&oxfw->unit->device), 100 << fw_dev->max_speed);
+end:
+	return err;
 }
 
 static void oxfw_card_free(struct snd_card *card)
@@ -45,10 +65,8 @@  static void oxfw_card_free(struct snd_card *card)
 static int oxfw_probe(struct fw_unit *unit,
 		       const struct ieee1394_device_id *id)
 {
-	struct fw_device *fw_dev = fw_parent_device(unit);
 	struct snd_card *card;
 	struct snd_oxfw *oxfw;
-	u32 firmware;
 	int err;
 
 	err = snd_card_new(&unit->device, -1, NULL, THIS_MODULE,
@@ -63,16 +81,9 @@  static int oxfw_probe(struct fw_unit *unit,
 	oxfw->unit = unit;
 	oxfw->device_info = (const struct device_info *)id->driver_data;
 
-	strcpy(card->driver, oxfw->device_info->driver_name);
-	strcpy(card->shortname, oxfw->device_info->short_name);
-	firmware = oxfw_read_firmware_version(unit);
-	snprintf(card->longname, sizeof(card->longname),
-		 "%s (OXFW%x %04x), GUID %08x%08x at %s, S%d",
-		 oxfw->device_info->long_name,
-		 firmware >> 20, firmware & 0xffff,
-		 fw_dev->config_rom[3], fw_dev->config_rom[4],
-		 dev_name(&unit->device), 100 << fw_dev->max_speed);
-	strcpy(card->mixername, "OXFW");
+	err = name_card(oxfw);
+	if (err < 0)
+		goto error;
 
 	err = snd_oxfw_create_pcm(oxfw);
 	if (err < 0)
@@ -122,9 +133,8 @@  static void oxfw_remove(struct fw_unit *unit)
 }
 
 static const struct device_info griffin_firewave = {
+	.vendor_name = "Griffin",
 	.driver_name = "FireWave",
-	.short_name  = "FireWave",
-	.long_name   = "Griffin FireWave Surround",
 	.pcm_constraints = firewave_constraints,
 	.mixer_channels = 6,
 	.mute_fb_id   = 0x01,
@@ -132,9 +142,8 @@  static const struct device_info griffin_firewave = {
 };
 
 static const struct device_info lacie_speakers = {
+	.vendor_name = "LaCie",
 	.driver_name = "FWSpeakers",
-	.short_name  = "FireWire Speakers",
-	.long_name   = "LaCie FireWire Speakers",
 	.pcm_constraints = lacie_speakers_constraints,
 	.mixer_channels = 1,
 	.mute_fb_id   = 0x01,
diff --git a/sound/firewire/oxfw/oxfw.h b/sound/firewire/oxfw/oxfw.h
index 6164bf3..b60e91d 100644
--- a/sound/firewire/oxfw/oxfw.h
+++ b/sound/firewire/oxfw/oxfw.h
@@ -27,9 +27,8 @@ 
 #include "../cmp.h"
 
 struct device_info {
+	const char *vendor_name;
 	const char *driver_name;
-	const char *short_name;
-	const char *long_name;
 	int (*pcm_constraints)(struct snd_pcm_runtime *runtime);
 	unsigned int mixer_channels;
 	u8 mute_fb_id;