Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Mark Brown]

On Tue, Sep 13, 2011 at 02:59:26PM -0400, Scott Jiang wrote:

> Add driver_name in snd_soc_card, asoc will copy this name to driver field of snd_card.
> ALSA lib need this info to load the proper card config.

>  static struct snd_soc_card bf5xx_ad73311 = {
>  	.name = "bfin-ad73311",
> +	.driver_name = "AD73311",

All my comments about the previous revision of the patch continue to
apply here.  The changlog doesn't explain the issue well (the driver
should already end up with a unique name) and the name you've picked
is less distinctive than the existing name.

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Liam Girdwood]

On Tue, 2011-09-13 at 14:59 -0400, Scott Jiang wrote:
> Add driver_name in snd_soc_card, asoc will copy this name to driver field of snd_card.
> ALSA lib need this info to load the proper card config.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
> ---
>  sound/soc/blackfin/bf5xx-ad73311.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/blackfin/bf5xx-ad73311.c b/sound/soc/blackfin/bf5xx-ad73311.c
> index 732a247..975c6e8 100644
> --- a/sound/soc/blackfin/bf5xx-ad73311.c
> +++ b/sound/soc/blackfin/bf5xx-ad73311.c
> @@ -192,6 +192,7 @@ static struct snd_soc_dai_link bf5xx_ad73311_dai[] = {
>  
>  static struct snd_soc_card bf5xx_ad73311 = {
>  	.name = "bfin-ad73311",
> +	.driver_name = "AD73311",
>  	.probe = bf5xx_probe,
>  	.dai_link = &bf5xx_ad73311_dai[CONFIG_SND_BF5XX_SPORT_NUM],
>  	.num_links = 1,

Acked-by: Liam Girdwood <lrg@ti.com>

[PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Scott Jiang]

Add driver_name in snd_soc_card, asoc will copy this name to driver field of snd_card.
ALSA lib need this info to load the proper card config.

Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
---
 sound/soc/blackfin/bf5xx-ad73311.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-ad73311.c b/sound/soc/blackfin/bf5xx-ad73311.c
index 732a247..975c6e8 100644
--- a/sound/soc/blackfin/bf5xx-ad73311.c
+++ b/sound/soc/blackfin/bf5xx-ad73311.c
@@ -192,6 +192,7 @@ static struct snd_soc_dai_link bf5xx_ad73311_dai[] = {
 
 static struct snd_soc_card bf5xx_ad73311 = {
 	.name = "bfin-ad73311",
+	.driver_name = "AD73311",
 	.probe = bf5xx_probe,
 	.dai_link = &bf5xx_ad73311_dai[CONFIG_SND_BF5XX_SPORT_NUM],
 	.num_links = 1,
-- 
1.7.0.4

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Scott Jiang]

2011/9/13 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Tue, Sep 13, 2011 at 02:59:26PM -0400, Scott Jiang wrote:
>
>> Add driver_name in snd_soc_card, asoc will copy this name to driver field of snd_card.
>> ALSA lib need this info to load the proper card config.
>
>>  static struct snd_soc_card bf5xx_ad73311 = {
>>       .name = "bfin-ad73311",
>> +     .driver_name = "AD73311",
>
> All my comments about the previous revision of the patch continue to
> apply here.  The changlog doesn't explain the issue well (the driver
> should already end up with a unique name)
>
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.

> and the name you've picked
> is less distinctive than the existing name.
>
I know the name is not good enough, but it is a historical result.
Asoc copy codec name to card->driver before, cause all cards config
file in alsa lib conf dir have this kind of name.

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Mark Brown]

On Wed, Sep 14, 2011 at 11:27:28AM +0800, Scott Jiang wrote:
> 2011/9/13 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> > On Tue, Sep 13, 2011 at 02:59:26PM -0400, Scott Jiang wrote:

> > All my comments about the previous revision of the patch continue to
> > apply here.  The changlog doesn't explain the issue well (the driver
> > should already end up with a unique name)

> 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.  The changelog needs to explain this,
and also explain why this is an issue in this one driver.

> > and the name you've picked
> > is less distinctive than the existing name.

> I know the name is not good enough, but it is a historical result.
> Asoc copy codec name to card->driver before, cause all cards config
> file in alsa lib conf dir have this kind of name.

This sounds like you should fix userspace.  It does go back to the issue
I regularly raise with the Blackfin code constantly assuming that there
can be a single driver which covers all combinations of a Blackfin CPU
with a given CODEC.

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Scott Jiang]

2011/9/15 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Wed, Sep 14, 2011 at 11:27:28AM +0800, Scott Jiang wrote:
>> 2011/9/13 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>> > On Tue, Sep 13, 2011 at 02:59:26PM -0400, Scott Jiang wrote:
>
>> > All my comments about the previous revision of the patch continue to
>> > apply here.  The changlog doesn't explain the issue well (the driver
>> > should already end up with a unique name)
>
>> 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.

>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.

>> > and the name you've picked
>> > is less distinctive than the existing name.
>
>> I know the name is not good enough, but it is a historical result.
>> Asoc copy codec name to card->driver before, cause all cards config
>> file in alsa lib conf dir have this kind of name.
>
> This sounds like you should fix userspace.  It does go back to the issue
> I regularly raise with the Blackfin code constantly assuming that there
> can be a single driver which covers all combinations of a Blackfin CPU
> with a given CODEC.
>
As I have said it was caused by ASoC before. It is not a guilt of blackfin.
All drivers under ASoC must name their card config the same as their
codec driver's name because asoc copy codec name to snd_card->driver
and alsa lib get it through sndrv_ctl_card_info->driver. They must
match.
Now ASoC doesn't copy codec name to snd_card->driver so we can rename
it to bfin-ad73311 if you like this kind of name style.

Re: [PATCH v2]ASoC:blackfin:add ad73311 card driver name

[Mark Brown]

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.

> > This sounds like you should fix userspace.  It does go back to the issue
> > I regularly raise with the Blackfin code constantly assuming that there
> > can be a single driver which covers all combinations of a Blackfin CPU
> > with a given CODEC.

> As I have said it was caused by ASoC before. It is not a guilt of blackfin.
> All drivers under ASoC must name their card config the same as their
> codec driver's name because asoc copy codec name to snd_card->driver
> and alsa lib get it through sndrv_ctl_card_info->driver. They must
> match.

Right, but if the Blackfin community had been alert to the idea that not
all machines using a given CODEC are the same it should have been
obvious that this was a poor identifier to use for this and something
was wrong.

> Now ASoC doesn't copy codec name to snd_card->driver so we can rename
> it to bfin-ad73311 if you like this kind of name style.

No, doing something that requires updates to every single machine driver
isn't good (unless you're going to go through and update every one of
them I guess, but even then it's not great).