| Message ID | 1437665015.20606.6.camel@ingics.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 48f403be3eb9b603cfaf946ca7a0c76272750469 |
| Headers | show |
Hi Axel, On 23.07.2015 17:23, Axel Lin wrote: > 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 <axel.lin@ingics.com> > --- > sound/soc/codecs/pcm1681.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c > index 1011142..5832523 100644 > --- a/sound/soc/codecs/pcm1681.c > +++ b/sound/soc/codecs/pcm1681.c > @@ -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; > + } > + } > + } ^^^^^^^ I think we don't need those brackets only for if statement (where you add break) > > if (val != -1) { > regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL, > PCM1681_DEEMPH_RATE_MASK, val << 3); > enable = 1; > - } else > + } else { > enable = 0; > + } ^^^ same here > > /* enable/disable deemphasis functionality */ > return regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL, > BR, marek
2015-07-24 3:48 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: > Hi Axel, > > On 23.07.2015 17:23, Axel Lin wrote: >> >> 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 <axel.lin@ingics.com> >> --- >> sound/soc/codecs/pcm1681.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c >> index 1011142..5832523 100644 >> --- a/sound/soc/codecs/pcm1681.c >> +++ b/sound/soc/codecs/pcm1681.c >> @@ -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; >> + } >> + } >> + } > > ^^^^^^^ > 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. >> >> >> if (val != -1) { >> regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL, >> PCM1681_DEEMPH_RATE_MASK, val << 3); >> enable = 1; >> - } else >> + } else { >> enable = 0; >> + } > > ^^^ 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
On 07/24/2015 03:17 AM, Axel Lin wrote: > 2015-07-24 3:48 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: >> Hi Axel, >> >> On 23.07.2015 17:23, Axel Lin wrote: >>> >>> 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 <axel.lin@ingics.com> >>> --- >>> sound/soc/codecs/pcm1681.c | 13 +++++++++---- >>> 1 file changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c >>> index 1011142..5832523 100644 >>> --- a/sound/soc/codecs/pcm1681.c >>> +++ b/sound/soc/codecs/pcm1681.c >>> @@ -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; >>> + } >>> + } >>> + } >> >> ^^^^^^^ >> 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. If we follow CodingStyle then it says no brackets around single statement. > >>> >>> >>> if (val != -1) { >>> regmap_update_bits(priv->regmap, PCM1681_DEEMPH_CONTROL, >>> PCM1681_DEEMPH_RATE_MASK, val << 3); >>> enable = 1; >>> - } else >>> + } else { >>> enable = 0; >>> + } >> >> ^^^ 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 ;) > > Regards, > Axel > BR, marek
2015-07-24 13:22 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: > On 07/24/2015 03:17 AM, Axel Lin wrote: >> 2015-07-24 3:48 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: >>> Hi Axel, >>> >>> On 23.07.2015 17:23, Axel Lin wrote: >>>> >>>> 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 <axel.lin@ingics.com> >>>> --- >>>> sound/soc/codecs/pcm1681.c | 13 +++++++++---- >>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c >>>> index 1011142..5832523 100644 >>>> --- a/sound/soc/codecs/pcm1681.c >>>> +++ b/sound/soc/codecs/pcm1681.c >>>> @@ -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; >>>> + } >>>> + } >>>> + } >>> >>> ^^^^^^^ >>> 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. > 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
On 07/24/2015 07:48 AM, Axel Lin wrote: > 2015-07-24 13:22 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: >> On 07/24/2015 03:17 AM, Axel Lin wrote: >>> 2015-07-24 3:48 GMT+08:00 Marek Belisko <marek.belisko@streamunlimited.com>: >>>> Hi Axel, >>>> >>>> On 23.07.2015 17:23, Axel Lin wrote: >>>>> >>>>> 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 <axel.lin@ingics.com> >>>>> --- >>>>> sound/soc/codecs/pcm1681.c | 13 +++++++++---- >>>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c >>>>> index 1011142..5832523 100644 >>>>> --- a/sound/soc/codecs/pcm1681.c >>>>> +++ b/sound/soc/codecs/pcm1681.c >>>>> @@ -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; >>>>> + } >>>>> + } >>>>> + } >>>> >>>> ^^^^^^^ >>>> 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. >> 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. I have no strong preference about it. I'll keep it on maintainers. You can add my Acked-by: Marek Belisko <marek.belisko@streamunlimited.com> > > Regards, > Axel > BR, marek
diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c index 1011142..5832523 100644 --- a/sound/soc/codecs/pcm1681.c +++ b/sound/soc/codecs/pcm1681.c @@ -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,
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 <axel.lin@ingics.com> --- sound/soc/codecs/pcm1681.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)