alsa-lib: if card driver name is empty string, use card name instead

PATCH In review Scott Jiang 3 participants 13 messages Sep 20, 2011

Share & tools

Scott Jiang

author

2011-09-20 05:17 UTC #0 lore raw

PATCH 1/1 alsa-lib: if card driver name is empty string, use card name instead

Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.

Signed-off-by Scott Jiang <[email protected]>

---
 src/confmisc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

src/confmisc.c +7 -1

index 80b0027..3c4b5aa 100644
@@ -666,6 +666,7 @@ int snd_determine_driver(int card, char **driver)
 	snd_ctl_t *ctl = NULL;
 	snd_ctl_card_info_t *info;
 	char *res = NULL;
+	const char *name = NULL;
 	int err;
 
 	assert(card >= 0 && card <= 32);

@@ -680,7 +681,12 @@ int snd_determine_driver(int card, char **driver)
 		SNDERR("snd_ctl_card_info error: %s", snd_strerror(err));
 		goto __error;
 	}
-	res = strdup(snd_ctl_card_info_get_driver(info));
+	name = snd_ctl_card_info_get_driver(info);
+	/* if card driver name is empty string, try to use card name */
+	if (name[0] == '\0')
+		name = snd_ctl_card_info_get_name(info);
+
+	res = strdup(name);
 	if (res == NULL)
 		err = -ENOMEM;
 	else {

Signature · 1 line

1.7.0.4

Takashi Iwai

2011-09-20 06:24 UTC #1 lore raw

At Tue, 20 Sep 2011 13:15:44 -0400,

Quoted 5 lines · Scott Jiang

Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
Signed-off-by Scott Jiang <[email protected]>

It's rather a bug of driver, no?
I don't think we need to work around it.

thanks,

Takashi

Quoted 34 lines

---
 src/confmisc.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

src/confmisc.c +7 -1

index 80b0027..3c4b5aa 100644
@@ -666,6 +666,7 @@ int snd_determine_driver(int card, char **driver)
 	snd_ctl_t *ctl = NULL;
 	snd_ctl_card_info_t *info;
 	char *res = NULL;
+	const char *name = NULL;
 	int err;
 
 	assert(card >= 0 && card <= 32);

@@ -680,7 +681,12 @@ int snd_determine_driver(int card, char **driver)
 		SNDERR("snd_ctl_card_info error: %s", snd_strerror(err));
 		goto __error;
 	}
-	res = strdup(snd_ctl_card_info_get_driver(info));
+	name = snd_ctl_card_info_get_driver(info);
+	/* if card driver name is empty string, try to use card name */
+	if (name[0] == '\0')
+		name = snd_ctl_card_info_get_name(info);
+
+	res = strdup(name);
 	if (res == NULL)
 		err = -ENOMEM;
 	else {

Signature · 1 line

1.7.0.4

Scott Jiang

author

2011-09-20 06:37 UTC #2 lore raw

2011/9/20 Takashi Iwai <[email protected]>:

Quoted 11 lines

At Tue, 20 Sep 2011 13:15:44 -0400,
Quoted 5 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
Signed-off-by Scott Jiang <[email protected]>
It's rather a bug of driver, no?
I don't think we need to work around it.

Mark said it's a bug of app..., he believes driver has provided enough info in card name.

Takashi Iwai

2011-09-20 06:39 UTC #3 lore raw

At Tue, 20 Sep 2011 14:37:04 +0800,

Quoted 8 lines · Scott Jiang

2011/9/20 Takashi Iwai <[email protected]>:
Quoted 6 lines
At Tue, 20 Sep 2011 13:15:44 -0400,
Quoted 4 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Scott Jiang <[email protected]>
It's rather a bug of driver, no?
I don't think we need to work around it.
Mark said it's a bug of app..., he believes driver has provided enough info in card name.

No, the driver name should be set, too.

Takashi

Scott Jiang

author

2011-09-20 07:06 UTC #4 lore raw

2011/9/20 Takashi Iwai <[email protected]>:

Quoted 12 lines

At Tue, 20 Sep 2011 14:37:04 +0800,
Quoted 10 lines · Scott Jiang
2011/9/20 Takashi Iwai <[email protected]>:
Quoted 8 lines
At Tue, 20 Sep 2011 13:15:44 -0400,
Quoted 5 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
Signed-off-by Scott Jiang <[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
It's rather a bug of driver, no?
I don't think we need to work around it.
Mark said it's a bug of app..., he believes driver has provided enough info in card name.
No, the driver name should be set, too.

I agree with you, but my driver patch has been rejected by Mark.

Quoted 8 lines · Scott Jiang

2011/9/15 Mark Brown <[email protected]>:
Quoted 6 lines · Scott Jiang
Quoted 5 lines
in alsa lib snd_config_hook_load_for_all_cards(), use sndrv_ctl_card_info->driver to determine card config,
not sndrv_ctl_card_info->name, though card name contains enough info.

Quoted 6 lines

Quoted 2 lines
So the issue isn't that the driver doesn't have a name, it's that you don't like the name it was given.
Do you mean alsa lib needs to modify its way to determine which card config to load?
I don't think alsa lib guys will agree.

Well, that's one option.

Quoted 5 lines

Quoted 2 lines
The changelog needs to explain this,
and also explain why this is an issue in this one driver.
Not only for this one driver, it's a problem for all drivers under asoc. Because sndrv_ctl_card_info->driver is an empty string now.

So if that's the case then it seems very clear that a driver specific change isn't a good way of addressing the issue. If the driver name is null it actually seems like modifying userspace should be very easy, we simply need to fall back to reading the card name if the driver name is empty.

Takashi Iwai

2011-09-20 07:12 UTC #5 lore raw

At Tue, 20 Sep 2011 14:55:14 +0800,

Quoted 17 lines · Scott Jiang

2011/9/20 Takashi Iwai <[email protected]>:
Quoted 15 lines
At Tue, 20 Sep 2011 14:37:04 +0800,
Quoted 13 lines · Scott Jiang
2011/9/20 Takashi Iwai <[email protected]>:
Quoted 11 lines
At Tue, 20 Sep 2011 13:15:44 -0400,
Quoted 5 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
Signed-off-by Scott Jiang <[email protected]>
It's rather a bug of driver, no?
I don't think we need to work around it.

Quoted 6 lines

Quoted 5 lines
Quoted 2 lines
Mark said it's a bug of app..., he believes driver has provided enough info in card name.
No, the driver name should be set, too.
I agree with you, but my driver patch has been rejected by Mark.

That's bad. But it's a rule since over 10 years ago: the driver should set driver field.

Takashi

Mark Brown

2011-09-20 10:03 UTC #6 lore raw

Quoted 6 lines · Takashi Iwai

Quoted 1 line · Scott Jiang
I agree with you, but my driver patch has been rejected by Mark.
That's bad. But it's a rule since over 10 years ago: the driver should set driver field.

Scott, as I've said every time you've posted your patch there's several problems with your patch:

- The name you've chosen is really not at all suitable, it doesn't identify the system at all.
- This clearly isn't something that affects only one driver - no ASoC driver sets a driver name - so a change to a single driver is clearly not a good solution even for just Blackfin.

You need to address these issues, they're both blocking.

Scott Jiang

author

2011-09-20 10:36 UTC #7 lore raw

2011/9/20 Mark Brown <[email protected]>:

Quoted 5 lines · Takashi Iwai

Quoted 3 lines
Quoted 1 line · Scott Jiang
I agree with you, but my driver patch has been rejected by Mark.

Quoted 8 lines

Quoted 2 lines
That's bad. But it's a rule since over 10 years ago: the driver should set driver field.
Scott, as I've said every time you've posted your patch there's several problems with your patch:
- The name you've chosen is really not at all suitable, it doesn't identify the system at all.

I said I wound modify the name. But I want to know how to fix this problem before I send a new patch.

Quoted 4 lines

- This clearly isn't something that affects only one driver - no ASoC driver sets a driver name - so a change to a single driver is clearly not a good solution even for just Blackfin.

I thought about this. I wanted to copy codec name to driver name as before in asoc but I don't think it's a good idea.
Now we can have multicodecs in one card, they may not be the same, right? If yes, we should add this name in each snd_soc_card.

Mark Brown

2011-09-20 10:53 UTC #8 lore raw

Quoted 10 lines · Scott Jiang

2011/9/20 Mark Brown <[email protected]>:
Quoted 3 lines
- This clearly isn't something that affects only one driver - no ASoC driver sets a driver name - so a change to a single driver is clearly not a good solution even for just Blackfin.
I thought about this. I wanted to copy codec name to driver name as before in asoc but I don't think it's a good idea.
Now we can have multicodecs in one card, they may not be the same, right? If yes, we should add this name in each snd_soc_card.

If we want to do that then we need to go round every single machine driver and add a driver name, plus add code in the core which flags cards that don't set one.

Mark Brown

2011-09-20 10:14 UTC #9 lore raw

In reply to Takashi Iwai

Quoted 7 lines · Takashi Iwai

Quoted 2 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
It's rather a bug of driver, no?
I don't think we need to work around it.

This was introduced by your commit 873bd4 (ASoC: don't set invalid name string to snd_card->driver field) which means we no longer provide a driver name for any ASoC cards as they're all relying on using the card name as the driver name. We should be doing something like replacing ' ' by '_' in the driver name we generate rather than just not generating a name.

I have to confess I didn't review the patch at the time as it was applied too quickly after it was mailed out for me to have a chance to see it. Sorry about that.

Takashi Iwai

2011-09-20 10:33 UTC #10 lore raw

At Tue, 20 Sep 2011 11:14:34 +0100,

Quoted 17 lines · Mark Brown

Quoted 7 lines · Takashi Iwai
Quoted 2 lines · Scott Jiang
Some drivers don't have a card driver name, but they have a nice card name containing enough info. So we can use card name to search card config if we found driver name is not valid.
It's rather a bug of driver, no?
I don't think we need to work around it.
This was introduced by your commit 873bd4 (ASoC: don't set invalid name string to snd_card->driver field) which means we no longer provide a driver name for any ASoC cards as they're all relying on using the card name as the driver name. We should be doing something like replacing ' ' by '_' in the driver name we generate rather than just not generating a name.

Quoted 3 lines

I have to confess I didn't review the patch at the time as it was applied too quickly after it was mailed out for me to have a chance to see it. Sorry about that.

Well, you can follow a short history from the commit: the commit above was really a fix to get back to the old good behavior. Until 3.0, ASoC had no way to set card->driver field. The method to set card->driver was firstly introduced by Liam's patch in 3.0-rc2, 22de71ba03311cdc1063757c50a1488cb90a1fca
ASoC: core - allow ASoC more flexible machine name

Then it turned out this gives a string "(null)", so some workaround was added, 2b39535b9e54888649923beaab443af212b6c0fd
ASoC: core: Don't set "(null)" as a driver name

But, this was no good move, too. The card->driver field is to be a concise string without special letters while card->name contains more flexible string. So, I changed the way back to the state before 3.0 there, the commit 873bd4.

All this happened in 3.0 development cycle, so it's no longer history than NULL driver name :)

Of course, it'd be nice to implement a logic in ASoC core to automatically generate some valid driver-name string. But, the driver name string is at most 15 letters, and card->name is an arbitrary string, so you'd need to do it a bit carefully.

thanks,

Takashi

Mark Brown

2011-09-20 10:51 UTC #11 lore raw

Quoted 3 lines · Takashi Iwai

Well, you can follow a short history from the commit: the commit above was really a fix to get back to the old good behavior. Until 3.0, ASoC had no way to set card->driver field. The method to set

No - older versions of ASoC always automatically generated a driver name (badly but that's a separate story). I guess that got broken with multi-component but didn't bother checking.

Quoted 4 lines

But, this was no good move, too. The card->driver field is to be a concise string without special letters while card->name contains more flexible string. So, I changed the way back to the state before 3.0 there, the commit 873bd4.

Which unfortunately restored the original problem which was being fixed by Jarkko.

Quoted 4 lines

Of course, it'd be nice to implement a logic in ASoC core to automatically generate some valid driver-name string. But, the driver name string is at most 15 letters, and card->name is an arbitrary string, so you'd need to do it a bit carefully.

I think the card name is fine, people don't tend to write anything terribly long there and very little actually cares about the driver name
- alsa-lib's config loading thing is the only thing I'm aware of (and that's not being terribly useful really, one driver can easily support multiple cards). If it's a problem people can always explicitly set something.

Takashi Iwai

2011-09-20 10:58 UTC #12 lore raw

At Tue, 20 Sep 2011 11:51:57 +0100,

Quoted 10 lines · Mark Brown

Quoted 3 lines · Takashi Iwai
Well, you can follow a short history from the commit: the commit above was really a fix to get back to the old good behavior. Until 3.0, ASoC had no way to set card->driver field. The method to set
No - older versions of ASoC always automatically generated a driver name (badly but that's a separate story). I guess that got broken with multi-component but didn't bother checking.

Ah, OK, so the problem existed since long time ago...

Quoted 13 lines

Quoted 4 lines
But, this was no good move, too. The card->driver field is to be a concise string without special letters while card->name contains more flexible string. So, I changed the way back to the state before 3.0 there, the commit 873bd4.
Which unfortunately restored the original problem which was being fixed by Jarkko.
Quoted 4 lines
Of course, it'd be nice to implement a logic in ASoC core to automatically generate some valid driver-name string. But, the driver name string is at most 15 letters, and card->name is an arbitrary string, so you'd need to do it a bit carefully.

Quoted 6 lines

I think the card name is fine, people don't tend to write anything terribly long there and very little actually cares about the driver name
- alsa-lib's config loading thing is the only thing I'm aware of (and that's not being terribly useful really, one driver can easily support multiple cards). If it's a problem people can always explicitly set something.

Sounds reasonable.

thanks,

Takashi