[alsa-devel] [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 shengjiu.wang@nxp.com --- 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;
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.
Signed-off-by: Shengjiu Wang shengjiu.wang@nxp.com
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;
Also, you should use a simple assignment operator "=" instead of "|=" in both cases.
/* fall through */
break;
Thanks -- Gustavo
Hi Gustavo
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.
Ok, this will be updated.
Signed-off-by: Shengjiu Wang shengjiu.wang@nxp.com
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;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.
/* 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
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.
Ok, this will be updated.
Signed-off-by: Shengjiu Wang shengjiu.wang@nxp.com
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;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
/* 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:
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;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:
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;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
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 shengjiu.wang@nxp.com
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:
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 shengjiu.wang@nxp.com
Acked-by: Nicolin Chen nicoleotsuka@gmail.com
Thanks
Oops. Acked the older version...should have gong for v2.
Please ignore it.
participants (3)
-
Gustavo A. R. Silva -
Nicolin Chen -
S.j. Wang