[PATCH V1] ASoC: fsl_esai: replace fall-through with break

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")

Signed-off-by: Shengjiu Wang <redacted>
---
 sound/soc/fsl/fsl_esai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index c7410bbfd2af..bad0dfed6b68 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c

@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
 		break;
 	case ESAI_HCKT_EXTAL:
 		ecr |= ESAI_ECR_ETI;
-		/* fall through */
+		break;
 	case ESAI_HCKR_EXTAL:
 		ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
 		break;

-- 
1.9.1

On 4/8/19 4:28 AM, S.j. Wang wrote:

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.

If this is correct, then you should use the following "Fixes" tag instead,
which is the one that introduced the bug:

Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")

        ^^^^
because this didn't change any functionality.

Also, you should use a simple assignment operator "=" instead of "|=" in both cases.

-		/* fall through */
+		break;
 	case ESAI_HCKR_EXTAL:
 		ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
 		break;

Thanks
--
Gustavo

Hi Gustavo

On 4/8/19 4:28 AM, S.j. Wang wrote:

quoted

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be

independent of

quoted

each other, so replace fall-through with break.

If this is correct, then you should use the following "Fixes" tag instead,
which is the one that introduced the bug:

Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")

quoted

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch
fall-through")

        ^^^^
because this didn't change any functionality.

Ok, this will be updated.

quoted

Signed-off-by: Shengjiu Wang <redacted>
---
 sound/soc/fsl/fsl_esai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index

c7410bbfd2af..bad0dfed6b68 100644

--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c

@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct

snd_soc_dai *dai, int clk_id,

quoted

              break;
      case ESAI_HCKT_EXTAL:
              ecr |= ESAI_ECR_ETI;

Also, you should use a simple assignment operator "=" instead of "|=" in
both cases.

The result is same for "=" and "|=", because there is "ecr = 0" in beginning of
This function. 

quoted

-             /* fall through */
+             break;
      case ESAI_HCKR_EXTAL:
              ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
              break;

Thanks
--
Gustavo

Hi Shengjiu,

On 4/8/19 9:54 PM, S.j. Wang wrote:

Hi Gustavo

quoted

On 4/8/19 4:28 AM, S.j. Wang wrote:

quoted

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be

independent of

quoted

each other, so replace fall-through with break.

If this is correct, then you should use the following "Fixes" tag instead,
which is the one that introduced the bug:

Fixes: 43d24e76b698 ("ASoC: fsl_esai: Add ESAI CPU DAI driver")

quoted

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch
fall-through")

        ^^^^
because this didn't change any functionality.

Ok, this will be updated.

quoted

quoted

Signed-off-by: Shengjiu Wang <redacted>
---
 sound/soc/fsl/fsl_esai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index

c7410bbfd2af..bad0dfed6b68 100644

--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c

@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct

snd_soc_dai *dai, int clk_id,

quoted

              break;
      case ESAI_HCKT_EXTAL:
              ecr |= ESAI_ECR_ETI;

Also, you should use a simple assignment operator "=" instead of "|=" in
both cases.

The result is same for "=" and "|=", because there is "ecr = 0" in beginning of
This function. 

Following that same logic, then why not use "+=" instead?

The point is: is "|=" or any other assignment operator other than "=" necessary?
The answer in this case is: no, it is not.  So, go for the simple one and avoid
any unnecessary confusion.

Also, there is no need for versioning a patch for it's first revision.  If you
receive feedback on a patch and are asked to update it, then you do need to
version the patches that you re-send.

Thanks
--
Gustavo

quoted

quoted

-             /* fall through */
+             break;
      case ESAI_HCKR_EXTAL:
              ecr |= esai_priv->synchronous ? ESAI_ECR_ETI : ESAI_ECR_ERI;
              break;

Thanks
--
Gustavo

Hi Gustavo,

On Mon, Apr 08, 2019 at 10:20:25PM -0500, Gustavo A. R. Silva wrote:

quoted

quoted

quoted

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c index

c7410bbfd2af..bad0dfed6b68 100644

--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c

@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct

snd_soc_dai *dai, int clk_id,

quoted

              break;
      case ESAI_HCKT_EXTAL:
              ecr |= ESAI_ECR_ETI;

Also, you should use a simple assignment operator "=" instead of "|=" in
both cases.

The result is same for "=" and "|=", because there is "ecr = 0" in beginning of
This function. 

Following that same logic, then why not use "+=" instead?

The point is: is "|=" or any other assignment operator other than "=" necessary?
The answer in this case is: no, it is not.  So, go for the simple one and avoid
any unnecessary confusion.

I would like to keep "|=" here, just in case that someday it'd be easier
to insert something to ecr before this chunk. So please get easy on this
one.

Thanks
Nicolin

Hi

Hi Gustavo,

On Mon, Apr 08, 2019 at 10:20:25PM -0500, Gustavo A. R. Silva wrote:

quoted

quoted

quoted

quoted

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c

index
c7410bbfd2af..bad0dfed6b68 100644

--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c

@@ -251,7 +251,7 @@ static int fsl_esai_set_dai_sysclk(struct

snd_soc_dai *dai, int clk_id,

quoted

              break;
      case ESAI_HCKT_EXTAL:
              ecr |= ESAI_ECR_ETI;

Also, you should use a simple assignment operator "=" instead of
"|=" in both cases.

The result is same for "=" and "|=", because there is "ecr = 0" in
beginning of This function.

Following that same logic, then why not use "+=" instead?

The point is: is "|=" or any other assignment operator other than "="

necessary?

quoted

The answer in this case is: no, it is not.  So, go for the simple one
and avoid any unnecessary confusion.

I would like to keep "|=" here, just in case that someday it'd be easier to
insert something to ecr before this chunk. So please get easy on this one.

Thanks
Nicolin

Thanks for reviewing,  I will send v2.

Best regards
Wang shengjiu

On Mon, Apr 08, 2019 at 09:28:06AM +0000, S.j. Wang wrote:

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")

Signed-off-by: Shengjiu Wang <redacted>

Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

Thanks

On Tue, Apr 09, 2019 at 08:50:59PM -0700, Nicolin Chen wrote:

On Mon, Apr 08, 2019 at 09:28:06AM +0000, S.j. Wang wrote:

quoted

case ESAI_HCKT_EXTAL and case ESAI_HCKR_EXTAL should be independent of
each other, so replace fall-through with break.

Fixes: 16bbeb2b43c3 ("ASoC: fsl_esai: Mark expected switch fall-through")

Signed-off-by: Shengjiu Wang <redacted>

Acked-by: Nicolin Chen <nicoleotsuka@gmail.com>

Thanks

Oops. Acked the older version...should have gong for v2.

Please ignore it.