Skip to contentSkip to conversation

Fix some minor issues in simple card driver

PATCH Applied Nicolin Chen 3 participants 8 messages Apr 24, 2014

Subsystems (3)
Sound ASoC Sound - generic sound card (Simple-Audio-Card, Audio-Graph-Card)
lore
Share & tools
lore

Nicolin Chen

author
#0 lore raw

cover Fix some minor issues in simple card driver

The series fixes one name limitation and code style problems.

Nicolin Chen (3):
ASoC: simple-card: Drop node->name checking
ASoC: simple-card: Simplify error msg in simple_card_dai_link_of()
ASoC: simple-card: Improve coding style

 sound/soc/generic/simple-card.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)
Signature · 1 line
1.8.4
author
#1 lore raw

PATCH 1/3 ASoC: simple-card: Drop node->name checking

The current simple-card driver limits the DT node name to "sound". Any of other names is forbidden while actually we should allow DT to pass other node names.

And if this function is being called, the node must already have the compatible "simple-audio-card" in DTB. So there should be no need to check the name here.

Signed-off-by Nicolin Chen <[email protected]>
---
 sound/soc/generic/simple-card.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/soc/generic/simple-card.c +1 -2
index 3f2e580..383a4a1 100644
@@ -156,8 +156,7 @@ static int simple_card_dai_link_of(struct device_node *node,
 	char *prefix = "";
 	int ret;
 
-	if (!strcmp("sound", node->name))
-		prefix = "simple-audio-card,";
+	prefix = "simple-audio-card,";
 
 	daifmt = snd_soc_of_parse_daifmt(node, prefix,
 					 &bitclkmaster, &framemaster);
Signature · 1 line
1.8.4
#2 lore raw

Re: [PATCH 1/3] ASoC: simple-card: Drop node->name checking

Quoted 19 lines · Nicolin Chen

The current simple-card driver limits the DT node name to "sound". Any of other names is forbidden while actually we should allow DT to pass other node names.

And if this function is being called, the node must already have the compatible "simple-audio-card" in DTB. So there should be no need to check the name here.

Signed-off-by Nicolin Chen <[email protected]>
---
  sound/soc/generic/simple-card.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
sound/soc/generic/simple-card.c +0 -0
index 3f2e580..383a4a1 100644

sound/soc/generic/simple-card.c static int simple_card_dai_link_of(struct device_node *node, · line 162

Quoted 9 lines
sound/soc/generic/simple-card.c +1 -2
@@ -156,8 +156,7 @@ static int simple_card_dai_link_of(struct device_node *node,
  	char *prefix = "";
  	int ret;
 
-	if (!strcmp("sound", node->name))
-		prefix = "simple-audio-card,";
+	prefix = "simple-audio-card,";
 
  	daifmt = snd_soc_of_parse_daifmt(node, prefix,
  					 &bitclkmaster, &framemaster);

I think you have missed the point of selecting the prefix based on the node name.

Before the change the "simple-audio-card,"-prefix was only needed for dai-link properties and subnodes if the dai-link node was omitted in a single dai-link setup.

After your change the prefix is also needed for the properties and subnodes inside dai-link subnodes.

See the details in: Documentation/devicetree/bindings/sound/simple-card.txt

Maybe the implementation could have been more explicit, but I think the old behavior is more convenient. If we anyway decide to go with this change then at least the DT binding document should be updated.

Best regards,
Jyri

maintainer
#3 lore raw

Re: [PATCH 1/3] ASoC: simple-card: Drop node->name checking

Quoted 1 line · Jyri Sarha

On 04/24/2014 02:13 PM, Nicolin Chen wrote:

Please delete unneeded context from your mails...

Quoted 6 lines

sound/soc/generic/simple-card.c static int simple_card_dai_link_of(struct device_node *node, · line 159

Quoted 3 lines
sound/soc/generic/simple-card.c +1 -2
-	if (!strcmp("sound", node->name))
-		prefix = "simple-audio-card,";
+	prefix = "simple-audio-card,";

I think you have missed the point of selecting the prefix based on the node name.

...

Quoted 3 lines

Maybe the implementation could have been more explicit, but I think the old behavior is more convenient. If we anyway decide to go with this change then at least the DT binding document should be updated.

Yes, the implementation needs to be *way* more explicit - I'd expect a parameter/variable saying which level we're at depending on where we're at in the parse rather than guessing based on the node name.

#4 lore raw

Re: [PATCH 1/3] ASoC: simple-card: Drop node->name checking

Quoted 14 lines · Mark Brown
Quoted 1 line · Jyri Sarha

On 04/24/2014 02:13 PM, Nicolin Chen wrote:

Please delete unneeded context from your mails...

Quoted 6 lines

sound/soc/generic/simple-card.c static int simple_card_dai_link_of(struct device_node *node, · line 159

Quoted 3 lines
sound/soc/generic/simple-card.c +1 -2
-	if (!strcmp("sound", node->name))
-		prefix = "simple-audio-card,";
+	prefix = "simple-audio-card,";

I think you have missed the point of selecting the prefix based on the node name.

...

Quoted 8 lines
Quoted 3 lines

Maybe the implementation could have been more explicit, but I think the old behavior is more convenient. If we anyway decide to go with this change then at least the DT binding document should be updated.

Yes, the implementation needs to be *way* more explicit - I'd expect a parameter/variable saying which level we're at depending on where we're at in the parse rather than guessing based on the node name.

Ok, I'll come up with a patch soon.

Best regards,
Jyri

author
#5 lore raw

PATCH 2/3 ASoC: simple-card: Simplify error msg in simple_card_dai_link_of()

In reply to Nicolin Chen

It would look better to use prop instead.

Signed-off-by Nicolin Chen <[email protected]>
---
 sound/soc/generic/simple-card.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
sound/soc/generic/simple-card.c +2 -4
index 383a4a1..c091557 100644
@@ -166,8 +166,7 @@ static int simple_card_dai_link_of(struct device_node *node,
 	np = of_get_child_by_name(node, prop);
 	if (!np) {
 		ret = -EINVAL;
-		dev_err(dev, "%s: Can't find simple-audio-card,cpu DT node\n",
-			__func__);
+		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
 		goto dai_link_of_err;
 	}
 

@@ -198,8 +197,7 @@ static int simple_card_dai_link_of(struct device_node *node,
 	np = of_get_child_by_name(node, prop);
 	if (!np) {
 		ret = -EINVAL;
-		dev_err(dev, "%s: Can't find simple-audio-card,codec DT node\n",
-			__func__);
+		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
 		goto dai_link_of_err;
 	}
Signature · 1 line
1.8.4
author
#6 lore raw

PATCH 3/3 ASoC: simple-card: Improve coding style

In reply to Nicolin Chen

Improve indentation and space.

Signed-off-by Nicolin Chen <[email protected]>
---
 sound/soc/generic/simple-card.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)
sound/soc/generic/simple-card.c +11 -12
index c091557..98f97e5 100644
@@ -66,8 +66,7 @@ err:
 
 static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
 {
-	struct simple_card_data *priv =
-				snd_soc_card_get_drvdata(rtd->card);
+	struct simple_card_data *priv =	snd_soc_card_get_drvdata(rtd->card);
 	struct snd_soc_dai *codec = rtd->codec_dai;
 	struct snd_soc_dai *cpu = rtd->cpu_dai;
 	struct simple_dai_props *dai_props;

@@ -177,7 +176,7 @@ static int simple_card_dai_link_of(struct device_node *node,
 		goto dai_link_of_err;
 
 	dai_props->cpu_dai.fmt = daifmt;
-	switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
+	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
 	case 0x11:
 		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
 		break;

@@ -218,7 +217,7 @@ static int simple_card_dai_link_of(struct device_node *node,
 			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
 	} else {
 		dai_props->codec_dai.fmt = daifmt;
-		switch (((np == bitclkmaster)<<4)|(np == framemaster)) {
+		switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
 		case 0x11:
 			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
 			break;

@@ -235,8 +234,8 @@ static int simple_card_dai_link_of(struct device_node *node,
 	}
 
 	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
-			ret = -EINVAL;
-			goto dai_link_of_err;
+		ret = -EINVAL;
+		goto dai_link_of_err;
 	}
 
 	/* simple-card assumes platform == cpu */

@@ -417,10 +416,10 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 
-		if (!cinfo->name	||
-		    !cinfo->codec_dai.name	||
-		    !cinfo->codec	||
-		    !cinfo->platform	||
+		if (!cinfo->name ||
+		    !cinfo->codec_dai.name ||
+		    !cinfo->codec ||
+		    !cinfo->platform ||
 		    !cinfo->cpu_dai.name) {
 			dev_err(dev, "insufficient asoc_simple_card_info settings\n");
 			return -EINVAL;

@@ -464,11 +463,11 @@ MODULE_DEVICE_TABLE(of, asoc_simple_of_match);
 
 static struct platform_driver asoc_simple_card = {
 	.driver = {
-		.name	= "asoc-simple-card",
+		.name = "asoc-simple-card",
 		.owner = THIS_MODULE,
 		.of_match_table = asoc_simple_of_match,
 	},
-	.probe		= asoc_simple_card_probe,
+	.probe = asoc_simple_card_probe,
 };
 
 module_platform_driver(asoc_simple_card);
Signature · 1 line
1.8.4