| Message ID | 66c117128f300b0ec5f278f538412789c78b60a6.1554715589.git.shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1] ASoC: fsl_esai: replace fall-through with break | expand |
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; > 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: > > 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.
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;
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(-)