Quantcast

[PATCH] ASoC: Clean up coding style issues in GTA02

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] ASoC: Clean up coding style issues in GTA02

Mark Brown-3
No substantial changes, this just tidies up a bunch of coding style
issues that ought to be fixed up before merge (which I'll do when the
GTA02 machine support is queued for merge).

Signed-off-by: Mark Brown <[hidden email]>
---
 sound/soc/s3c24xx/neo1973_gta02_wm8753.c |   32 +++++++++++++----------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/sound/soc/s3c24xx/neo1973_gta02_wm8753.c b/sound/soc/s3c24xx/neo1973_gta02_wm8753.c
index d9fe7d1..1358f6f 100644
--- a/sound/soc/s3c24xx/neo1973_gta02_wm8753.c
+++ b/sound/soc/s3c24xx/neo1973_gta02_wm8753.c
@@ -104,7 +104,7 @@ static int neo1973_gta02_hifi_hw_params(struct snd_pcm_substream *substream,
 
  /* set MCLK division for sample rate */
  ret = snd_soc_dai_set_clkdiv(cpu_dai, S3C24XX_DIV_MCLK,
- S3C2410_IISMOD_32FS );
+ S3C2410_IISMOD_32FS);
  if (ret < 0)
  return ret;
 
@@ -116,7 +116,7 @@ static int neo1973_gta02_hifi_hw_params(struct snd_pcm_substream *substream,
 
  /* set prescaler division for sample rate */
  ret = snd_soc_dai_set_clkdiv(cpu_dai, S3C24XX_DIV_PRESCALER,
- S3C24XX_PRESCALE(4,4));
+ S3C24XX_PRESCALE(4, 4));
  if (ret < 0)
  return ret;
 
@@ -210,7 +210,7 @@ static struct snd_soc_ops neo1973_gta02_voice_ops = {
 #define LM4853_AMP 1
 #define LM4853_SPK 2
 
-static u8 lm4853_state=0;
+static u8 lm4853_state;
 
 /* This has no effect, it exists only to maintain compatibility with
  * existing ALSA state files.
@@ -220,11 +220,10 @@ static int lm4853_set_state(struct snd_kcontrol *kcontrol,
 {
  int val = ucontrol->value.integer.value[0];
 
- if(val) {
+ if (val)
  lm4853_state |= LM4853_AMP;
- } else {
+ else
  lm4853_state &= ~LM4853_AMP;
- }
 
  return 0;
 }
@@ -242,12 +241,12 @@ static int lm4853_set_spk(struct snd_kcontrol *kcontrol,
 {
  int val = ucontrol->value.integer.value[0];
 
- if(val) {
+ if (val) {
  lm4853_state |= LM4853_SPK;
- s3c2410_gpio_setpin(GTA02_GPIO_HP_IN,0);
+ s3c2410_gpio_setpin(GTA02_GPIO_HP_IN, 0);
  } else {
  lm4853_state &= ~LM4853_SPK;
- s3c2410_gpio_setpin(GTA02_GPIO_HP_IN,1);
+ s3c2410_gpio_setpin(GTA02_GPIO_HP_IN, 1);
  }
 
  return 0;
@@ -348,7 +347,8 @@ static int neo1973_gta02_wm8753_init(struct snd_soc_codec *codec)
  snd_soc_dapm_nc_pin(codec, "LINE2");
 
  /* Add neo1973 gta02 specific widgets */
- snd_soc_dapm_new_controls(codec, wm8753_dapm_widgets, ARRAY_SIZE(wm8753_dapm_widgets));
+ snd_soc_dapm_new_controls(codec, wm8753_dapm_widgets,
+  ARRAY_SIZE(wm8753_dapm_widgets));
 
  /* add neo1973 gta02 specific controls */
  for (i = 0; i < ARRAY_SIZE(wm8753_neo1973_gta02_controls); i++) {
@@ -378,8 +378,8 @@ static int neo1973_gta02_wm8753_init(struct snd_soc_codec *codec)
 /*
  * BT Codec DAI
  */
-static struct snd_soc_dai bt_dai =
-{ .name = "Bluetooth",
+static struct snd_soc_dai bt_dai = {
+ .name = "Bluetooth",
  .id = 0,
  .playback = {
  .channels_min = 1,
@@ -423,8 +423,6 @@ static struct snd_soc_device neo1973_gta02_snd_devdata = {
  .codec_dev = &soc_codec_dev_wm8753,
 };
 
-
-
 static struct platform_device *neo1973_gta02_snd_device;
 
 static int __init neo1973_gta02_init(void)
@@ -433,7 +431,7 @@ static int __init neo1973_gta02_init(void)
 
  if (!machine_is_neo1973_gta02()) {
  printk(KERN_INFO
-       "Only GTA02 hardware supported by ASoc driver\n");
+       "Only GTA02 is supported by this ASoC driver\n");
  return -ENODEV;
  }
 
@@ -468,18 +466,16 @@ static int __init neo1973_gta02_init(void)
 
  return ret;
 }
+module_init(neo1973_gta02_init);
 
 static void __exit neo1973_gta02_exit(void)
 {
  snd_soc_unregister_dai(&bt_dai);
  platform_device_unregister(neo1973_gta02_snd_device);
 }
-
-module_init(neo1973_gta02_init);
 module_exit(neo1973_gta02_exit);
 
 /* Module information */
 MODULE_AUTHOR("Graeme Gregory, [hidden email]");
 MODULE_DESCRIPTION("ALSA SoC WM8753 Neo1973 GTA02");
 MODULE_LICENSE("GPL");
-
--
1.5.6.3


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Werner Almesberger
Mark Brown wrote:
> No substantial changes, this just tidies up a bunch of coding style
> issues that ought to be fixed up before merge (which I'll do when the
> GTA02 machine support is queued for merge).

Very nice, thanks ! One question, though:

> - if(val) {
> + if (val)
>   lm4853_state |= LM4853_AMP;
> - } else {
> + else
>   lm4853_state &= ~LM4853_AMP;
> - }

This appears to be in contradiction with the following passage of
Documentation/CodingStyle:

| Do not unnecessarily use braces where a single statement will do.
|
| if (condition)
|         action();
|
| This does not apply if one branch of a conditional statement is a single
| statement. Use braces in both branches.
|
| if (condition) {
|         do_this();
|         do_that();
| } else {
|         otherwise();
| }

I must say that I don't particularly like this rule. I'd rather write

        if (foo)
                then_this();
        else
                otherwise_that();

(the else is easy to spot) and even

        if (foo)
                then_this();
        else {
                otherwise();
                that();
        }

(still no problem spotting the else) but never

        if (foo) {
                then();
                this();
        } else
                otherwise_that();

In the latter case, I'd try to change the logic such that it becomes
either a one-statement if-true branch, or something else entirely.

What's your interpretation of the scripture ?

- Werner

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Mark Brown-3
On Fri, Apr 03, 2009 at 10:21:34AM -0300, Werner Almesberger wrote:

> > - if(val) {
> > + if (val)

> This appears to be in contradiction with the following passage of
> Documentation/CodingStyle:

I think you're reading the patch the wrong way round :)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Werner Almesberger
Mark Brown wrote:
> I think you're reading the patch the wrong way round :)

Am I ? Before:

|        if(val) {
|                lm4853_state |= LM4853_AMP;
|        } else {
|                lm4853_state &= ~LM4853_AMP;
|        }

after

|        if (val)
|                lm4853_state |= LM4853_AMP;
|        else
|                lm4853_state &= ~LM4853_AMP;

yet line 172 of Documentation/CodingStyle says

| Use braces in both branches.

I agree with you preferring the style used in your patch, but I
wonder how to resolve the apparent disagree with CodingStyle.

K&R 2nd ed. also seems to agree with us and even condones braceless
single-statement else, e.g., on page 125.

- Werner

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

pHilipp Zabel
On Fri, Apr 3, 2009 at 6:30 PM, Werner Almesberger <[hidden email]> wrote:

> Mark Brown wrote:
>> I think you're reading the patch the wrong way round :)
>
> Am I ? Before:
>
> |        if(val) {
> |                lm4853_state |= LM4853_AMP;
> |        } else {
> |                lm4853_state &= ~LM4853_AMP;
> |        }
>
> after
>
> |        if (val)
> |                lm4853_state |= LM4853_AMP;
> |        else
> |                lm4853_state &= ~LM4853_AMP;
>
> yet line 172 of Documentation/CodingStyle says
>
> | Use braces in both branches.

Context :) Previous sentence, emphasis mine:

| This does not apply if ONE branch of a conditional statement is a single
| statement. Use braces in both branches.

I read this as meaning that if BOTH branches of a conditional
statement are a single statement, not using braces is still preferred.

> I agree with you preferring the style used in your patch, but I
> wonder how to resolve the apparent disagree with CodingStyle.
>
> K&R 2nd ed. also seems to agree with us and even condones braceless
> single-statement else, e.g., on page 125.
>
> - Werner
>
>

regards
Philipp

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Nicolas Dufresne
What it means is that you should not mix like:

        if (val) {
        do_stuff();
        do_more_stuff();
        }
        else
        do_other_suff();

All the rest is user choice, Not changing it for fun will reduce noise
into you patches and reviewer will better manage to focus on what you
really are trying to fix.

But that's my opinion,
Nicolas

Le vendredi 03 avril 2009 à 18:43 +0200, pHilipp Zabel a écrit :
> | This does not apply if ONE branch of a conditional statement is a
> single
> | statement. Use braces in both branches.
>
> I read this as meaning that if BOTH branches of a conditional
> statement are a single statement, not using braces is still preferred.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Mark Brown-3
On Sat, Apr 04, 2009 at 10:04:53AM -0400, Nicolas Dufresne wrote:

> All the rest is user choice, Not changing it for fun will reduce noise
> into you patches and reviewer will better manage to focus on what you
> really are trying to fix.

No, there's a very clear policy against not including braces around
single statements in the kernel.  In this case the changes are all about
bringing the code in line with the standards required to merge with
mainline.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Nicolas Dufresne
Le samedi 04 avril 2009 à 15:26 +0100, Mark Brown a écrit :
> No, there's a very clear policy against not including braces around
> single statements in the kernel.

I agree in this case. There still exist one exception which is badly
phrased in CodingStyle. We should add this word (between *) to make it
say the right thing.

"This does not apply if *only* one branch of a conditional statement is
a single statement. Use braces in both branches."




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Werner Almesberger
Nicolas Dufresne wrote:
> I agree in this case. There still exist one exception which is badly
> phrased in CodingStyle. We should add this word (between *) to make it
> say the right thing.

I'd be very happy with this :-) Who sends the patch ?

- Werner

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Nelson Castillo-2
In reply to this post by Mark Brown-3
On Fri, Apr 3, 2009 at 6:36 AM, Mark Brown
<[hidden email]> wrote:
> No substantial changes, this just tidies up a bunch of coding style
> issues that ought to be fixed up before merge (which I'll do when the
> GTA02 machine support is queued for merge).
>
> Signed-off-by: Mark Brown <[hidden email]>

Applied. Thanks.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ASoC: Clean up coding style issues in GTA02

Nicolas Dufresne
In reply to this post by Werner Almesberger
Le dimanche 05 avril 2009 à 12:39 -0300, Werner Almesberger a écrit :
> I'd be very happy with this :-) Who sends the patch ?

As I don't track (lake of time) the LKM, I guess you can take it.

Thanks,
Nicolas


Loading...