[alsa-devel] [PATCH] 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 scott.jiang.linux@gmail.com --- src/confmisc.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/confmisc.c b/src/confmisc.c index 80b0027..3c4b5aa 100644 --- a/src/confmisc.c +++ b/src/confmisc.c @@ -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 {
At Tue, 20 Sep 2011 13:15:44 -0400, Scott Jiang wrote:
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 scott.jiang.linux@gmail.com
It's rather a bug of driver, no? I don't think we need to work around it.
thanks,
Takashi
src/confmisc.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/confmisc.c b/src/confmisc.c index 80b0027..3c4b5aa 100644 --- a/src/confmisc.c +++ b/src/confmisc.c @@ -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 {
-- 1.7.0.4
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 13:15:44 -0400, Scott Jiang wrote:
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 scott.jiang.linux@gmail.com
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.
At Tue, 20 Sep 2011 14:37:04 +0800, Scott Jiang wrote:
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 13:15:44 -0400, Scott Jiang wrote:
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 scott.jiang.linux@gmail.com
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
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 14:37:04 +0800, Scott Jiang wrote:
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 13:15:44 -0400, Scott Jiang wrote:
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 scott.jiang.linux@gmail.com
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.
On Fri, Sep 16, 2011 at 11:53:16AM +0800, Scott Jiang wrote:
2011/9/15 Mark Brown broonie@opensource.wolfsonmicro.com:
On Wed, Sep 14, 2011 at 11:27:28AM +0800, Scott Jiang wrote:
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.
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.
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.
At Tue, 20 Sep 2011 14:55:14 +0800, Scott Jiang wrote:
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 14:37:04 +0800, Scott Jiang wrote:
2011/9/20 Takashi Iwai tiwai@suse.de:
At Tue, 20 Sep 2011 13:15:44 -0400, Scott Jiang wrote:
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 scott.jiang.linux@gmail.com
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.
That's bad. But it's a rule since over 10 years ago: the driver should set driver field.
Takashi
On Tue, Sep 20, 2011 at 09:12:19AM +0200, Takashi Iwai wrote:
Scott Jiang wrote:
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.
2011/9/20 Mark Brown broonie@opensource.wolfsonmicro.com:
On Tue, Sep 20, 2011 at 09:12:19AM +0200, Takashi Iwai wrote:
Scott Jiang wrote:
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.
I said I wound modify the name. But I want to know how to fix this problem before I send a new patch.
- 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.
On Tue, Sep 20, 2011 at 06:36:15PM +0800, Scott Jiang wrote:
2011/9/20 Mark Brown broonie@opensource.wolfsonmicro.com:
- 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.
On Tue, Sep 20, 2011 at 08:24:52AM +0200, Takashi Iwai wrote:
Scott Jiang wrote:
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.
At Tue, 20 Sep 2011 11:14:34 +0100, Mark Brown wrote:
On Tue, Sep 20, 2011 at 08:24:52AM +0200, Takashi Iwai wrote:
Scott Jiang wrote:
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.
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
On Tue, Sep 20, 2011 at 12:33:41PM +0200, Takashi Iwai wrote:
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.
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.
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.
At Tue, 20 Sep 2011 11:51:57 +0100, Mark Brown wrote:
On Tue, Sep 20, 2011 at 12:33:41PM +0200, Takashi Iwai wrote:
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...
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.
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.
Sounds reasonable.
thanks,
Takashi
participants (3)
-
Mark Brown -
Scott Jiang -
Takashi Iwai