ASoC: pcm1681: Fix setting de-emphasis sampling rate selection

PATCH In review Axel Lin 3 participants 10 messages Jul 23, 2015 – Jul 24, 2015

Subsystems (4)

Stable branch Sound ASoC Texas Instruments audio (ASoC/HDA) drivers

lore

Share & tools

lore

Axel Lin

author

2015-07-23 15:22 UTC #0 lore raw

PATCH 1/2 ASoC: pcm1681: Fix setting de-emphasis sampling rate selection

The de-emphasis sampling rate selection is controlled by BIT[3:4] of PCM1681_DEEMPH_CONTROL register. Do proper left shift to set it.

Signed-off-by Axel Lin <[email protected]>

---
 sound/soc/codecs/pcm1681.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/codecs/pcm1681.c +1 -1

index b2c990f..1011142 100644
@@ -102,7 +102,7 @@ static int pcm1681_set_deemph(struct snd_soc_codec *codec)
 
 	if (val != -1) {
 		regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
-					PCM1681_DEEMPH_RATE_MASK, val);
+				   PCM1681_DEEMPH_RATE_MASK, val << 3);
 		enable = 1;
 	} else
 		enable = 0;

Signature · 1 line

2.1.0

Axel Lin

author

2015-07-23 15:23 UTC #1 lore raw

PATCH 2/2 ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

Slightly improve the logic for de-emphasis sampling rate selection by break out the loop if the rate is matched.

Signed-off-by Axel Lin <[email protected]>

---
 sound/soc/codecs/pcm1681.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

sound/soc/codecs/pcm1681.c +9 -4

index 1011142..5832523 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec *codec)
 	struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
 	int i = 0, val = -1, enable = 0;
 
-	if (priv->deemph)
-		for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-			if (pcm1681_deemph[i] == priv->rate)
+	if (priv->deemph) {
+		for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+			if (pcm1681_deemph[i] == priv->rate) {
 				val = i;
+				break;
+			}
+		}
+	}
 
 	if (val != -1) {
 		regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
 				   PCM1681_DEEMPH_RATE_MASK, val << 3);
 		enable = 1;
-	} else
+	} else {
 		enable = 0;
+	}
 
 	/* enable/disable deemphasis functionality */
 	return regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,

Signature · 1 line

2.1.0

Axel Lin

author

2015-07-24 01:17 UTC #3 lore raw

Re: [PATCH 2/2] ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

2015-07-24 3:48 GMT+08:00 Marek Belisko <[email protected]>:

Quoted 22 lines

Hi Axel,
Quoted 19 lines · Axel Lin
Slightly improve the logic for de-emphasis sampling rate selection by break
out the loop if the rate is matched.
Signed-off-by Axel Lin <[email protected]>
---
  sound/soc/codecs/pcm1681.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)
sound/soc/codecs/pcm1681.c +0 -0
index 1011142..5832523 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec
 *codec)
        struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
        int i = 0, val = -1, enable = 0;
 

Quoted 15 lines

sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec *codec) · line 105

Quoted 11 lines

sound/soc/codecs/pcm1681.c +7 -3

-       if (priv->deemph)
-               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-                       if (pcm1681_deemph[i] == priv->rate)
+       if (priv->deemph) {
+               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+                       if (pcm1681_deemph[i] == priv->rate) {
                                val = i;
+                               break;
+                       }
+               }
+       }

^^^^^^^
I think we don't need those brackets only for if statement (where you add break)

Because I think having the brackets here makes the code looks better.

Quoted 11 lines

sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec *codec) · line 113

Quoted 10 lines

sound/soc/codecs/pcm1681.c +2 -1

        if (val != -1) {
                regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
                                   PCM1681_DEEMPH_RATE_MASK, val << 3);
                enable = 1;
-       } else
+       } else {
                enable = 0;
+       }

Quoted 1 line

^^^ same here

This is also a common pattern in kernel coding style that if we have brackets around the if statement, also add the brackets for the else part.

Regards,
Axel

Marek Belisko

2015-07-24 05:22 UTC #4 lore raw

Re: [PATCH 2/2] ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

Quoted 39 lines · Axel Lin

2015-07-24 3:48 GMT+08:00 Marek Belisko <[email protected]>:
Quoted 22 lines
Hi Axel,
Quoted 19 lines · Axel Lin
Slightly improve the logic for de-emphasis sampling rate selection by break
out the loop if the rate is matched.
Signed-off-by Axel Lin <[email protected]>
---
  sound/soc/codecs/pcm1681.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)
sound/soc/codecs/pcm1681.c +0 -0
index 1011142..5832523 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec
 *codec)
        struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
        int i = 0, val = -1, enable = 0;
 
Quoted 15 lines
sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec *codec) · line 105
Quoted 11 lines
sound/soc/codecs/pcm1681.c +7 -3
-       if (priv->deemph)
-               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-                       if (pcm1681_deemph[i] == priv->rate)
+       if (priv->deemph) {
+               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+                       if (pcm1681_deemph[i] == priv->rate) {
                                val = i;
+                               break;
+                       }
+               }
+       }
^^^^^^^
I think we don't need those brackets only for if statement (where you add break)

Quoted 1 line

Because I think having the brackets here makes the code looks better.

If we follow CodingStyle then it says no brackets around single statement.

Quoted 12 lines

Quoted 11 lines

sound/soc/codecs/pcm1681.c +2 -1

        if (val != -1) {
                regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
                                   PCM1681_DEEMPH_RATE_MASK, val << 3);
                enable = 1;
-       } else
+       } else {
                enable = 0;
+       }

Quoted 5 lines

Quoted 1 line
^^^ same here
This is also a common pattern in kernel coding style that if we have brackets around the if statement, also add the brackets for the else part.

OK agree. Sorry about that. I have to read again CodingStyle ;)

Quoted 4 lines

Regards,
Axel

BR,

marek

Axel Lin

author

2015-07-24 05:48 UTC #5 lore raw

Re: [PATCH 2/2] ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

2015-07-24 13:22 GMT+08:00 Marek Belisko <[email protected]>:

Quoted 40 lines · Axel Lin

Quoted 39 lines

2015-07-24 3:48 GMT+08:00 Marek Belisko <[email protected]>:

Quoted 37 lines

Hi Axel,

Quoted 20 lines · Axel Lin

Slightly improve the logic for de-emphasis sampling rate selection by break
out the loop if the rate is matched.

Signed-off-by Axel Lin <[email protected]>

---
  sound/soc/codecs/pcm1681.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)

sound/soc/codecs/pcm1681.c +0 -1

index 1011142..5832523 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec
 *codec)
        struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
        int i = 0, val = -1, enable = 0;
 
-       if (priv->deemph)

sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec · line 106

Quoted 10 lines

sound/soc/codecs/pcm1681.c +7 -2

-               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-                       if (pcm1681_deemph[i] == priv->rate)
+       if (priv->deemph) {
+               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+                       if (pcm1681_deemph[i] == priv->rate) {
                                val = i;
+                               break;
+                       }
+               }
+       }

^^^^^^^
I think we don't need those brackets only for if statement (where you add break)

Quoted 2 lines

Quoted 1 line
Because I think having the brackets here makes the code looks better.
If we follow CodingStyle then it says no brackets around single statement.

Yes, but it's not always good if the "single statement" is actually cross multiple lines. And checkpatch does not complain this patch.

Well, this is probably a bit personal preference because I feel it has better readability with the brackets here.

If you insist on removing the brackets, I can send v2 to do that.

Regards,
Axel

Marek Belisko

2015-07-24 12:07 UTC #6 lore raw

Re: [PATCH 2/2] ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

Quoted 44 lines · Axel Lin

2015-07-24 13:22 GMT+08:00 Marek Belisko <[email protected]>:
Quoted 40 lines · Axel Lin
Quoted 39 lines
2015-07-24 3:48 GMT+08:00 Marek Belisko <[email protected]>:
Quoted 37 lines
Hi Axel,
Quoted 20 lines · Axel Lin
Slightly improve the logic for de-emphasis sampling rate selection by break
out the loop if the rate is matched.
Signed-off-by Axel Lin <[email protected]>
---
  sound/soc/codecs/pcm1681.c | 13 +++++++++----
  1 file changed, 9 insertions(+), 4 deletions(-)
sound/soc/codecs/pcm1681.c +0 -1
index 1011142..5832523 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec
 *codec)
        struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
        int i = 0, val = -1, enable = 0;
 
-       if (priv->deemph)
sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec · line 106
Quoted 10 lines
sound/soc/codecs/pcm1681.c +7 -2
-               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-                       if (pcm1681_deemph[i] == priv->rate)
+       if (priv->deemph) {
+               for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+                       if (pcm1681_deemph[i] == priv->rate) {
                                val = i;
+                               break;
+                       }
+               }
+       }
^^^^^^^
I think we don't need those brackets only for if statement (where you add break)
Quoted 2 lines
Quoted 1 line
Because I think having the brackets here makes the code looks better.
If we follow CodingStyle then it says no brackets around single statement.

Quoted 7 lines

Yes, but it's not always good if the "single statement" is actually cross multiple lines. And checkpatch does not complain this patch.
Well, this is probably a bit personal preference because I feel it has better readability with the brackets here.
If you insist on removing the brackets, I can send v2 to do that.

I have no strong preference about it. I'll keep it on maintainers. You can add my Acked-by: Marek Belisko <[email protected]>

Quoted 4 lines

Regards,
Axel

BR,

marek

Mark Brown

maintainer

2015-07-24 11:31 UTC #7 lore raw

Applied "ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection" to the asoc tree

In reply to Axel Lin

The patch

ASoC: pcm1681: Improve the logic for de-emphasis sampling rate selection

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 48f403be3eb9b603cfaf946ca7a0c76272750469 Mon Sep 17 00:00:00 2001

From: Axel Lin <[email protected]>
Date: Thu, 23 Jul 2015 23:23:35 +0800
Subject: [PATCH] ASoC: pcm1681: Improve the logic for de-emphasis sampling
rate selection

Slightly improve the logic for de-emphasis sampling rate selection by break out the loop if the rate is matched.

Signed-off-by Axel Lin <[email protected]>

Signed-off-by Mark Brown <[email protected]>

---
 sound/soc/codecs/pcm1681.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

sound/soc/codecs/pcm1681.c +9 -4

index e7ba557979cb..490970e5ab8c 100644
@@ -95,17 +95,22 @@ static int pcm1681_set_deemph(struct snd_soc_codec *codec)
 	struct pcm1681_private *priv = snd_soc_codec_get_drvdata(codec);
 	int i = 0, val = -1, enable = 0;
 
-	if (priv->deemph)
-		for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++)
-			if (pcm1681_deemph[i] == priv->rate)
+	if (priv->deemph) {
+		for (i = 0; i < ARRAY_SIZE(pcm1681_deemph); i++) {
+			if (pcm1681_deemph[i] == priv->rate) {
 				val = i;
+				break;
+			}
+		}
+	}
 
 	if (val != -1) {
 		regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
 				   PCM1681_DEEMPH_RATE_MASK, val << 3);
 		enable = 1;
-	} else
+	} else {
 		enable = 0;
+	}
 
 	/* enable/disable deemphasis functionality */
 	return regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,

Signature · 1 line

2.1.4

Marek Belisko

2015-07-23 19:46 UTC #8 lore raw

In reply to Axel Lin

Quoted 13 lines · Axel Lin

The de-emphasis sampling rate selection is controlled by BIT[3:4] of PCM1681_DEEMPH_CONTROL register. Do proper left shift to set it.
Signed-off-by Axel Lin <[email protected]>
---
  sound/soc/codecs/pcm1681.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
sound/soc/codecs/pcm1681.c +0 -0
index b2c990f..1011142 100644

sound/soc/codecs/pcm1681.c static int pcm1681_set_deemph(struct snd_soc_codec *codec) · line 105

Quoted 5 lines

sound/soc/codecs/pcm1681.c +1 -1

@@ -102,7 +102,7 @@ static int pcm1681_set_deemph(struct snd_soc_codec *codec)
 
  	if (val != -1) {
  		regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
-					PCM1681_DEEMPH_RATE_MASK, val);
+				   PCM1681_DEEMPH_RATE_MASK, val << 3);

Good catch. Acked-by: Marek Belisko <[email protected]>

Quoted 4 lines

sound/soc/codecs/pcm1681.c +0 -0

  		enable = 1;
  	} else
  		enable = 0;

Mark Brown

maintainer

2015-07-24 11:31 UTC #9 lore raw

Applied "ASoC: pcm1681: Fix setting de-emphasis sampling rate selection" to the asoc tree

In reply to Axel Lin

The patch

ASoC: pcm1681: Fix setting de-emphasis sampling rate selection

has been applied to the asoc tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From fa8173a3ef0570affde7da352de202190b3786c2 Mon Sep 17 00:00:00 2001

From: Axel Lin <[email protected]>
Date: Thu, 23 Jul 2015 23:22:26 +0800
Subject: [PATCH] ASoC: pcm1681: Fix setting de-emphasis sampling rate
selection

The de-emphasis sampling rate selection is controlled by BIT[3:4] of PCM1681_DEEMPH_CONTROL register. Do proper left shift to set it.

Signed-off-by Axel Lin <[email protected]>

Acked-by Marek Belisko <[email protected]>

Signed-off-by Mark Brown <[email protected]>

Cc [email protected]

---
 sound/soc/codecs/pcm1681.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

sound/soc/codecs/pcm1681.c +1 -1

index 477e13d30971..e7ba557979cb 100644
@@ -102,7 +102,7 @@ static int pcm1681_set_deemph(struct snd_soc_codec *codec)
 
 	if (val != -1) {
 		regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL,
-					PCM1681_DEEMPH_RATE_MASK, val);
+				   PCM1681_DEEMPH_RATE_MASK, val << 3);
 		enable = 1;
 	} else
 		enable = 0;

Signature · 1 line

2.1.4