sound/pci/hda/intel_hda: small cleanups

sound/pci/hda/intel_hda: small cleanups

[Pavel Machek]

Cleanup whitespace and warn about wrong volatile usage. This code
loves to use deprecated if ((err = foo())), but I did not have enough
strength to fix all of it. Ouch also maintainers item would be handy.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 79d63c9..86064a8 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -252,6 +252,7 @@ enum {
 struct azx_dev {
 	u32 *bdl;			/* virtual address of the BDL */
 	dma_addr_t bdl_addr;		/* physical address of the BDL */
+	/* FIXME: volatile does not provide needed locking on SMP systems */
 	volatile u32 *posbuf;			/* position buffer pointer */
 
 	unsigned int bufsize;		/* size of the play buffer in bytes */
@@ -271,8 +272,8 @@ struct azx_dev {
 	/* for sanity check of position buffer */
 	unsigned int period_intr;
 
-	unsigned int opened: 1;
-	unsigned int running: 1;
+	unsigned int opened :1;
+	unsigned int running :1;
 };
 
 /* CORB/RIRB */
@@ -330,8 +331,8 @@ struct azx {
 
 	/* flags */
 	int position_fix;
-	unsigned int initialized: 1;
-	unsigned int single_cmd: 1;
+	unsigned int initialized :1;
+	unsigned int single_cmd :1;
 };
 
 /* driver types */
@@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
 	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
 
 	count = 50;
-	while (! azx_readb(chip, GCTL) && --count)
+	while (!azx_readb(chip, GCTL) && --count)
 		msleep(1);
 
-	/* Brent Chartrand said to wait >= 540us for codecs to intialize */
+	/* Brent Chartrand said to wait >= 540us for codecs to initialize */
 	msleep(1);
 
 	/* check to see if controller is ready */
-	if (! azx_readb(chip, GCTL)) {
+	if (!azx_readb(chip, GCTL)) {
 		snd_printd("azx_reset: controller not ready!\n");
 		return -EBUSY;
 	}
@@ -658,7 +659,7 @@ static int azx_reset(struct azx *chip)
 	azx_writel(chip, GCTL, azx_readl(chip, GCTL) | ICH6_GCTL_UREN);
 
 	/* detect codecs */
-	if (! chip->codec_mask) {
+	if (!chip->codec_mask) {
 		chip->codec_mask = azx_readw(chip, STATESTS);
 		snd_printdd("codec_mask = 0x%x\n", chip->codec_mask);
 	}
@@ -766,7 +767,7 @@ static void azx_init_chip(struct azx *ch
 	azx_int_enable(chip);
 
 	/* initialize the codec command I/O */
-	if (! chip->single_cmd)
+	if (!chip->single_cmd)
 		azx_init_cmd_io(chip);
 
 	/* program the position buffer */
@@ -794,7 +795,7 @@ static void azx_init_chip(struct azx *ch
 /*
  * interrupt handler
  */
-static irqreturn_t azx_interrupt(int irq, void* dev_id, struct pt_regs *regs)
+static irqreturn_t azx_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
 	struct azx *chip = dev_id;
 	struct azx_dev *azx_dev;
@@ -999,8 +1000,7 @@ static struct snd_pcm_hardware azx_pcm_h
 	.info =			(SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED |
 				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				 SNDRV_PCM_INFO_MMAP_VALID |
-				 SNDRV_PCM_INFO_PAUSE /*|*/
-				 /*SNDRV_PCM_INFO_RESUME*/),
+				 SNDRV_PCM_INFO_PAUSE),
 	.formats =		SNDRV_PCM_FMTBIT_S16_LE,
 	.rates =		SNDRV_PCM_RATE_48000,
 	.rate_min =		48000,
@@ -1322,6 +1322,8 @@ static int __devinit azx_init_stream(str
 	 * assign the starting bdl address to each stream (device) and initialize
 	 */
 	for (i = 0; i < chip->num_streams; i++) {
+		/* FIXME: this should probably use readl or something.
+		   hand-doing volatiles is wrong. */
 		unsigned int off = sizeof(u32) * (i * AZX_MAX_FRAG * 4);
 		struct azx_dev *azx_dev = &chip->azx_dev[i];
 		azx_dev->bdl = (u32 *)(chip->bdl.area + off);
@@ -1399,6 +1401,7 @@ static int azx_free(struct azx *chip)
 		azx_writel(chip, DPUBASE, 0);
 
 		/* wait a little for interrupts to finish */
+		/* FIXME: delay is not right way to wait for interrupts */
 		msleep(1);
 	}
 
@@ -1434,19 +1437,19 @@ static int __devinit azx_create(struct s
 				struct azx **rchip)
 {
 	struct azx *chip;
-	int err = 0;
+	int err;
 	static struct snd_device_ops ops = {
 		.dev_free = azx_dev_free,
 	};
 
 	*rchip = NULL;
 	
-	if ((err = pci_enable_device(pci)) < 0)
+	err = pci_enable_device(pci);
+	if (err < 0)
 		return err;
 
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
-	
-	if (NULL == chip) {
+	if (!chip) {
 		snd_printk(KERN_ERR SFX "cannot allocate chip\n");
 		pci_disable_device(pci);
 		return -ENOMEM;
@@ -1472,13 +1475,14 @@ static int __devinit azx_create(struct s
 	}
 #endif
 
-	if ((err = pci_request_regions(pci, "ICH HD audio")) < 0) {
+	err = pci_request_regions(pci, "ICH HD audio");
+	if (err < 0) {
 		kfree(chip);
 		pci_disable_device(pci);
 		return err;
 	}
 
-	chip->addr = pci_resource_start(pci,0);
+	chip->addr = pci_resource_start(pci, 0);
 	chip->remap_addr = ioremap_nocache(chip->addr, pci_resource_len(pci,0));
 	if (chip->remap_addr == NULL) {
 		snd_printk(KERN_ERR SFX "ioremap error\n");
@@ -1519,7 +1523,7 @@ static int __devinit azx_create(struct s
 	}
 	chip->num_streams = chip->playback_streams + chip->capture_streams;
 	chip->azx_dev = kcalloc(chip->num_streams, sizeof(*chip->azx_dev), GFP_KERNEL);
-	if (! chip->azx_dev) {
+	if (!chip->azx_dev) {
 		snd_printk(KERN_ERR "cannot malloc azx_dev\n");
 		goto errout;
 	}
@@ -1550,7 +1554,7 @@ static int __devinit azx_create(struct s
 	chip->initialized = 1;
 
 	/* codec detection */
-	if (! chip->codec_mask) {
+	if (!chip->codec_mask) {
 		snd_printk(KERN_ERR SFX "no codecs found!\n");
 		err = -ENODEV;
 		goto errout;
@@ -1577,16 +1581,16 @@ static int __devinit azx_probe(struct pc
 {
 	struct snd_card *card;
 	struct azx *chip;
-	int err = 0;
+	int err;
 
 	card = snd_card_new(index, id, THIS_MODULE, 0);
-	if (NULL == card) {
+	if (!card) {
 		snd_printk(KERN_ERR SFX "Error creating card!\n");
 		return -ENOMEM;
 	}
 
-	if ((err = azx_create(card, pci, pci_id->driver_data,
-			      &chip)) < 0) {
+	err = azx_create(card, pci, pci_id->driver_data, &chip);
+	if (err < 0) {
 		snd_card_free(card);
 		return err;
 	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: sound/pci/hda/intel_hda: small cleanups

[Takashi Iwai]

At Thu, 31 Aug 2006 14:37:06 +0200,
Pavel Machek wrote:
> 
> @@ -271,8 +272,8 @@ struct azx_dev {
>  	/* for sanity check of position buffer */
>  	unsigned int period_intr;
>  
> -	unsigned int opened: 1;
> -	unsigned int running: 1;
> +	unsigned int opened :1;
> +	unsigned int running :1;
>  };
>  
>  /* CORB/RIRB */
> @@ -330,8 +331,8 @@ struct azx {
>  
>  	/* flags */
>  	int position_fix;
> -	unsigned int initialized: 1;
> -	unsigned int single_cmd: 1;
> +	unsigned int initialized :1;
> +	unsigned int single_cmd :1;
>  };

Any official standard reference for bit-field expressions?

>  
>  /* driver types */
> @@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
>  	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
>  
>  	count = 50;
> -	while (! azx_readb(chip, GCTL) && --count)
> +	while (!azx_readb(chip, GCTL) && --count)
>  		msleep(1);

Hm, it looks rather like a personal preference.
IMHO, it's harder to read without space...


> @@ -999,8 +1000,7 @@ static struct snd_pcm_hardware azx_pcm_h
>  	.info =			(SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED |
>  				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
>  				 SNDRV_PCM_INFO_MMAP_VALID |
> -				 SNDRV_PCM_INFO_PAUSE /*|*/
> -				 /*SNDRV_PCM_INFO_RESUME*/),
> +				 SNDRV_PCM_INFO_PAUSE),

The commented out item is there intentionally to indicate that the
full-resume isn't implemented.


I'll fix the volatile things separately.


Thanks,

Takashi

Re: sound/pci/hda/intel_hda: small cleanups

[Pavel Machek]

Hi!

> > @@ -271,8 +272,8 @@ struct azx_dev {
> >  	/* for sanity check of position buffer */
> >  	unsigned int period_intr;
> >  
> > -	unsigned int opened: 1;
> > -	unsigned int running: 1;
> > +	unsigned int opened :1;
> > +	unsigned int running :1;
> >  };
> >  
> >  /* CORB/RIRB */
> > @@ -330,8 +331,8 @@ struct azx {
> >  
> >  	/* flags */
> >  	int position_fix;
> > -	unsigned int initialized: 1;
> > -	unsigned int single_cmd: 1;
> > +	unsigned int initialized :1;
> > +	unsigned int single_cmd :1;
> >  };
> 
> Any official standard reference for bit-field expressions?

Well, logically : belongs to the 1, and include/linux understands it
like that...

> >  /* driver types */
> > @@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
> >  	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
> >  
> >  	count = 50;
> > -	while (! azx_readb(chip, GCTL) && --count)
> > +	while (!azx_readb(chip, GCTL) && --count)
> >  		msleep(1);
> 
> Hm, it looks rather like a personal preference.
> IMHO, it's harder to read without space...

Well, core parts (sched.c?) use it without the space, and I'd say (!
expression) is unusual in kernel, but no, could not find it codified.

> I'll fix the volatile things separately.

Thanks.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: sound/pci/hda/intel_hda: small cleanups

[Takashi Iwai]

At Thu, 31 Aug 2006 15:39:29 +0200,
Pavel Machek wrote:
> 
> Hi!
> 
> > > @@ -271,8 +272,8 @@ struct azx_dev {
> > >  	/* for sanity check of position buffer */
> > >  	unsigned int period_intr;
> > >  
> > > -	unsigned int opened: 1;
> > > -	unsigned int running: 1;
> > > +	unsigned int opened :1;
> > > +	unsigned int running :1;
> > >  };
> > >  
> > >  /* CORB/RIRB */
> > > @@ -330,8 +331,8 @@ struct azx {
> > >  
> > >  	/* flags */
> > >  	int position_fix;
> > > -	unsigned int initialized: 1;
> > > -	unsigned int single_cmd: 1;
> > > +	unsigned int initialized :1;
> > > +	unsigned int single_cmd :1;
> > >  };
> > 
> > Any official standard reference for bit-field expressions?
> 
> Well, logically : belongs to the 1, and include/linux understands it
> like that...

OK, "xxx :1;" looks major, too :)

> > >  /* driver types */
> > > @@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
> > >  	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
> > >  
> > >  	count = 50;
> > > -	while (! azx_readb(chip, GCTL) && --count)
> > > +	while (!azx_readb(chip, GCTL) && --count)
> > >  		msleep(1);
> > 
> > Hm, it looks rather like a personal preference.
> > IMHO, it's harder to read without space...
> 
> Well, core parts (sched.c?) use it without the space, and I'd say (!
> expression) is unusual in kernel, but no, could not find it codified.

I don't mind much to change it now, but hopefully people won't be too
strict about this rule...

> > I'll fix the volatile things separately.

The access is really over RAM, not MMIO.  So it should be OK to access
in that way.  But volatile seems superfluous.  I'll get rid of it.

Also, msleep() in the removal should be synchronize_irq().

I'll fix these changes and commit with your space fixes to ALSA tree.


Thanks,


Takashi

Re: sound/pci/hda/intel_hda: small cleanups

[Pavel Machek]

Hi!

> > > > @@ -271,8 +272,8 @@ struct azx_dev {
> > > >  	/* for sanity check of position buffer */
> > > >  	unsigned int period_intr;
> > > >  
> > > > -	unsigned int opened: 1;
> > > > -	unsigned int running: 1;
> > > > +	unsigned int opened :1;
> > > > +	unsigned int running :1;
> > > >  };
> > > >  
> > > >  /* CORB/RIRB */
> > > > @@ -330,8 +331,8 @@ struct azx {
> > > >  
> > > >  	/* flags */
> > > >  	int position_fix;
> > > > -	unsigned int initialized: 1;
> > > > -	unsigned int single_cmd: 1;
> > > > +	unsigned int initialized :1;
> > > > +	unsigned int single_cmd :1;
> > > >  };
> > > 
> > > Any official standard reference for bit-field expressions?
> > 
> > Well, logically : belongs to the 1, and include/linux understands it
> > like that...
> 
> OK, "xxx :1;" looks major, too :)
> 
> > > >  /* driver types */
> > > > @@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
> > > >  	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
> > > >  
> > > >  	count = 50;
> > > > -	while (! azx_readb(chip, GCTL) && --count)
> > > > +	while (!azx_readb(chip, GCTL) && --count)
> > > >  		msleep(1);
> > > 
> > > Hm, it looks rather like a personal preference.
> > > IMHO, it's harder to read without space...
> > 
> > Well, core parts (sched.c?) use it without the space, and I'd say (!
> > expression) is unusual in kernel, but no, could not find it codified.
> 
> I don't mind much to change it now, but hopefully people won't be too
> strict about this rule...

Thanks.

> > > I'll fix the volatile things separately.
> 
> The access is really over RAM, not MMIO.  So it should be OK to access
> in that way.  But volatile seems superfluous.  I'll get rid of it.

Sorry, I understood the code well enough to sense something is wrong,
but not enough to know how to fix it.

> Also, msleep() in the removal should be synchronize_irq().

Good.

> I'll fix these changes and commit with your space fixes to ALSA tree.

Thanks!
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Randy.Dunlap]

On Thu, 31 Aug 2006 15:01:45 +0200 Takashi Iwai wrote:

> At Thu, 31 Aug 2006 14:37:06 +0200,
> Pavel Machek wrote:
> > 
> > @@ -271,8 +272,8 @@ struct azx_dev {
> >  	/* for sanity check of position buffer */
> >  	unsigned int period_intr;
> >  
> > -	unsigned int opened: 1;
> > -	unsigned int running: 1;
> > +	unsigned int opened :1;
> > +	unsigned int running :1;
> >  };
> >  
> >  /* CORB/RIRB */
> > @@ -330,8 +331,8 @@ struct azx {
> >  
> >  	/* flags */
> >  	int position_fix;
> > -	unsigned int initialized: 1;
> > -	unsigned int single_cmd: 1;
> > +	unsigned int initialized :1;
> > +	unsigned int single_cmd :1;
> >  };
> 
> Any official standard reference for bit-field expressions?

Pavel knows how to submit patches to CodingStyle too.  :)

> >  /* driver types */
> > @@ -642,14 +643,14 @@ static int azx_reset(struct azx *chip)
> >  	azx_writeb(chip, GCTL, azx_readb(chip, GCTL) | ICH6_GCTL_RESET);
> >  
> >  	count = 50;
> > -	while (! azx_readb(chip, GCTL) && --count)
> > +	while (!azx_readb(chip, GCTL) && --count)
> >  		msleep(1);
> 
> Hm, it looks rather like a personal preference.
> IMHO, it's harder to read without space...

We have been tending toward not using space in cases like this
(in my unscientific memory-based survey).

So, just this morning I have seen questions and opinions about
the following that could (or could not) use more documentation
or codification and I'm sure that we could easily find more,
but do we want to codify Everything??


1.  Kconfig help text should be indented (it's not indented in the
	GFS2 patches)

2.  if (!condition1)	/* no space between ! and condition1 */

3.  don't use C99-style // comments

4.  unsigned int bitfield :<nr_bits>;


---
~Randy

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Pavel Machek]

Hi!

> > Hm, it looks rather like a personal preference.
> > IMHO, it's harder to read without space...
> 
> We have been tending toward not using space in cases like this
> (in my unscientific memory-based survey).
> 
> So, just this morning I have seen questions and opinions about
> the following that could (or could not) use more documentation
> or codification and I'm sure that we could easily find more,
> but do we want to codify Everything??
> 
> 
> 1.  Kconfig help text should be indented (it's not indented in the
> 	GFS2 patches)
> 
> 2.  if (!condition1)	/* no space between ! and condition1 */
> 
> 3.  don't use C99-style // comments
> 
> 4.  unsigned int bitfield :<nr_bits>;

Looks reasonable to me. Will you do the patch or should I ?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-- 
VGER BF report: U 0.489855

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Randy.Dunlap]

On Sun, 3 Sep 2006 01:15:10 +0200 Pavel Machek wrote:

> Hi!
> 
> > > Hm, it looks rather like a personal preference.
> > > IMHO, it's harder to read without space...
> > 
> > We have been tending toward not using space in cases like this
> > (in my unscientific memory-based survey).
> > 
> > So, just this morning I have seen questions and opinions about
> > the following that could (or could not) use more documentation
> > or codification and I'm sure that we could easily find more,
> > but do we want to codify Everything??
> > 
> > 
> > 1.  Kconfig help text should be indented (it's not indented in the
> > 	GFS2 patches)
> > 
> > 2.  if (!condition1)	/* no space between ! and condition1 */
> > 
> > 3.  don't use C99-style // comments
> > 
> > 4.  unsigned int bitfield :<nr_bits>;
> 
> Looks reasonable to me. Will you do the patch or should I ?

Please (you) go ahead with it.

Thanks,
---
~Randy

-- 
VGER BF report: U 0.5

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Pavel Machek]

Hi!

> > > So, just this morning I have seen questions and opinions about
> > > the following that could (or could not) use more documentation
> > > or codification and I'm sure that we could easily find more,
> > > but do we want to codify Everything??
> > > 
> > > 
> > > 1.  Kconfig help text should be indented (it's not indented in the
> > > 	GFS2 patches)

Chapter 10 speaks about that alread?

                Chapter 10: Configuration-files

For configuration options (arch/xxx/Kconfig, and all the Kconfig
files),
somewhat different indentation is used.

Help text is indented with 2 spaces.

if CONFIG_EXPERIMENTAL
        tristate CONFIG_BOOM
        default n
        help
          Apply nitroglycerine inside the keyboard (DANGEROUS)
        bool CONFIG_CHEER
        depends on CONFIG_BOOM
        default y
        help
          Output nice messages when you explode
endif

> > > 2.  if (!condition1)	/* no space between ! and condition1
*/
> > > 3.  don't use C99-style // comments
> > > 4.  unsigned int bitfield :<nr_bits>;

Ok.

> > Looks reasonable to me. Will you do the patch or should I ?
> 
> Please (you) go ahead with it.

Does it look okay?

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 6d2412e..d8e51a6 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -46,6 +46,16 @@ used for indentation, and the above exam
 
 Get a decent editor and don't leave whitespace at the end of lines.
 
+Bitfield variables should be indented like this:
+
+	unsigned int foo :1;
+
+Avoid extra spaces around ! operator, and do not place spaces around (s.
+
+	if (!buffer)
+
+is okay, if ( ! buffer ) is just ugly.	
+
 
 		Chapter 2: Breaking long lines and strings
 
@@ -280,7 +290,7 @@ int fun(int a)
 	int result = 0;
 	char *buffer = kmalloc(SIZE);
 
-	if (buffer == NULL)
+	if (!buffer)
 		return -ENOMEM;
 
 	if (condition1) {
@@ -316,6 +326,9 @@ When commenting the kernel API functions
 See the files Documentation/kernel-doc-nano-HOWTO.txt and scripts/kernel-doc
 for details.
 
+Please do not use C99-style comments, and do not use comments to
+comment out unused code.
+
 		Chapter 9: You've made a mess of it
 
 That's OK, we all do.  You've probably been told by your long-time Unix

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Takashi Iwai]

At Tue, 5 Sep 2006 10:08:14 +0200,
Pavel Machek wrote:
> 
> > > Looks reasonable to me. Will you do the patch or should I ?
> > 
> > Please (you) go ahead with it.
> 
> Does it look okay?
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 6d2412e..d8e51a6 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -46,6 +46,16 @@ used for indentation, and the above exam
>  
>  Get a decent editor and don't leave whitespace at the end of lines.
>  
> +Bitfield variables should be indented like this:
> +
> +	unsigned int foo :1;
> +
> +Avoid extra spaces around ! operator, and do not place spaces around (s.
> +
> +	if (!buffer)
> +
> +is okay, if ( ! buffer ) is just ugly.	
> +

I know I belong to an endagered species, but I still find
	if (! buffer)
		...
isn't so ugly.


Takashi

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Randy.Dunlap]

On Tue, 5 Sep 2006 10:08:14 +0200 Pavel Machek wrote:

> Hi!
> 
> > > > So, just this morning I have seen questions and opinions about
> > > > the following that could (or could not) use more documentation
> > > > or codification and I'm sure that we could easily find more,
> > > > but do we want to codify Everything??
> > > > 
> > > > 
> > > > 1.  Kconfig help text should be indented (it's not indented in the
> > > > 	GFS2 patches)
> 
> Chapter 10 speaks about that alread?

OK, good.
I had only looked in Documentation/kbuild/kconfig-language.txt.


> > > > 2.  if (!condition1)	/* no space between ! and condition1
> */
> > > > 3.  don't use C99-style // comments
> > > > 4.  unsigned int bitfield :<nr_bits>;
> 
> Ok.
> 
> > > Looks reasonable to me. Will you do the patch or should I ?
> > 
> > Please (you) go ahead with it.
> 
> Does it look okay?
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 6d2412e..d8e51a6 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -46,6 +46,16 @@ used for indentation, and the above exam
>  
>  Get a decent editor and don't leave whitespace at the end of lines.
>  
> +Bitfield variables should be indented like this:
> +
> +	unsigned int foo :1;
> +
> +Avoid extra spaces around ! operator, and do not place spaces around (s.

I would spell out "parentheses".

> +	if (!buffer)
> +
> +is okay, if ( ! buffer ) is just ugly.	
> +
>  
>  		Chapter 2: Breaking long lines and strings
>  
> @@ -280,7 +290,7 @@ int fun(int a)
>  	int result = 0;
>  	char *buffer = kmalloc(SIZE);
>  
> -	if (buffer == NULL)
> +	if (!buffer)
>  		return -ENOMEM;
>  
>  	if (condition1) {
> @@ -316,6 +326,9 @@ When commenting the kernel API functions
>  See the files Documentation/kernel-doc-nano-HOWTO.txt and scripts/kernel-doc
>  for details.
>  
> +Please do not use C99-style comments, and do not use comments to
                              ^
I would insert:               "// "

> +comment out unused code.
> +

Is there an acceptable way to leave source code in a file but
render it unused?  Like #if 0/#endif or #if BOGUS_SYMBOL/#endif ?


>  		Chapter 9: You've made a mess of it
>  
>  That's OK, we all do.  You've probably been told by your long-time Unix


---
~Randy

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Ingo Oeser]

Hi,

On Tuesday, 5. September 2006 17:43, Randy.Dunlap wrote:
> Is there an acceptable way to leave source code in a file but
> render it unused?  Like #if 0/#endif or #if BOGUS_SYMBOL/#endif ?

They are to reading like gaps in a road.

If you really need to keep unused code for reference
just do it in your LOCAL repository.

There are exceptions where you remove the last user of code 
in one patch and a different developer just needs this code
for his new stuff in another patch.

In this case an #if 0/#endif is quite common to reduce merging
conflicts.

Regards

Ingo Oeser

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Pavel Machek]

Hi!

> > +comment out unused code.
> > +
> 
> Is there an acceptable way to leave source code in a file but
> render it unused?  Like #if 0/#endif or #if BOGUS_SYMBOL/#endif ?

I'd say "no way is acceptable, but #if 0/#endif is least evil" :-).

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: CodingStyle (was: Re: sound/pci/hda/intel_hda: small cleanups)

[Chase Venters]

On Wed, 6 Sep 2006, Pavel Machek wrote:

> Hi!
>
>>> +comment out unused code.
>>> +
>>
>> Is there an acceptable way to leave source code in a file but
>> render it unused?  Like #if 0/#endif or #if BOGUS_SYMBOL/#endif ?
>
> I'd say "no way is acceptable, but #if 0/#endif is least evil" :-).

I'd say "no way is acceptable, but #if 0/#endif with proper comments is 
less evil."

Disabled code will never break if other parts of the code change 
without it; rather, it could just become plain wrong. People might either 
leave it alone (if they don't know what it is for) or try to change it (if 
they think they do).

If you must leave disabled code behind (which in my perfect world would be 
'never'), you should at least leave behind a comment explaining what the 
code is supposed to do and why it isn't enabled.

If it starts to drift from almost-functional to plain wrong, it becomes an 
even worse wart than it originally was.

Thanks,
Chase

Re: CodingStyle

[Stefan Richter]

Pavel Machek wrote:
> +Avoid extra spaces around ! operator, and do not place spaces around (s.

How about:

	Avoid extra spaces after the ! operator.
	Do not place spaces around parentheses.

Because "foo && !bar" is certainly OK.

Or more draconian for the former and less so for the latter rule:

	Do not put whitespace between any of the unary operators and
	their operand.

	It is usually unnecessary to have whitespace around parentheses
	as part of expressions, around brackets, or around the operators
	. and ->.

Rule 1 certainly applies likewise to ++, --, unary +, unary -, !, ~,
(typecast), unary *, unary &, sizeof.

Rule 2 applies to all of ( ), [ ], ., ->, except where line breaks and
indentation warrant whitespace, or where whitespace helps to read
expressions with more levels of braces. Although the latter should be
avoided anyway.
-- 
Stefan Richter
-=====-=-==- =--= --=-=
http://arcgraph.de/sr/

CodingStyle

[HarryWei]

> 2010/12/11 Robert P. J. Day <rpjday@crashcourse.ca>
> 
> > On Sat, 11 Dec 2010, HarryWei wrote:
> >
> > > Hi all,
> > >       I just read the CodingSyle at Documentation directory in
> > linux-2.6.23 kernel source code.
> > >       But a problem happened to me like following.
> > >       Functions in linux-2.6.23/fs/fs-writeback.c are "static int" and
> > function name are in different lines.(function name in following line)
> > >       I can't find the rule in CodingStyle. That We often program
> > functions are "static int" and function name are in the same line.
> > >       When do we do the first or second way? What is different between
> > them?
> > >
> > >   Any answer is okay.
> > >   Best regards.
> >
> >   the advantage to the form
> >
> > static int
> > function-name
> >
> > is that, if you're looking for the actual function *definition* in the
> > file, having the function name at the start of a new line means you
> > can search for it with the pattern "^function-name" so that you don't
> > have to wade through all of the invocations of that function.
> >
> >  i like that style; others claim that with proper cross-referencers
> > like cscope, doing that is redundant.  as far as i know, the kernel
> > coding style doesn't take a position on that, but i'm willing to be
> > corrected.
Thanks, i see :)
> >
> > rday
> >
> > --
> >
> > ========================================================================
> > Robert P. J. Day                               Waterloo, Ontario, CANADA
> >                        http://crashcourse.ca
> >
> > Twitter:                                       http://twitter.com/rpjday
> > LinkedIn:                               http://ca.linkedin.com/in/rpjday
> > ========================================================================
> >
> 
> 
> 
> -- 
> Best Regards.
> Harry Wei.
> Do what u like!

CodingStyle

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: 18 April 2013 05:14
> To: Opensource [Anthony Olech]
> Cc: Jean Delvare; Mark Brown; Randy Dunlap; lm-sensors@lm-sensors.org;
> LKML; David Dajun Chen
> Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
> 
[...]
> > +	if (cell == NULL) {
> > +		ret = -ENODEV;
> > +		goto exit;
> 
> Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
> 
[...]
> > +exit:
> > +	return ret;
> > +}
[...]

Hi Guenter,

I have read CodingStyle, chapter 7 and it seems to encourage a single exit point from functions.

During development there was some logging done at the (single) exit point but that has been stripped out for submission.
Whilst I can duplicate the logging and do an immediate 'return' at all those points, I am hesitant to do so when chapter 7 of the CodingStyle appears to say to use a single exit point. In addition I had thought that a single exit point from functions was a good thing.

Tony Olech

Re: CodingStyle

[Guenter Roeck]

On Thu, Apr 18, 2013 at 10:48:06AM +0000, Opensource [Anthony Olech] wrote:
> > -----Original Message-----
> > From: Guenter Roeck [mailto:linux@roeck-us.net]
> > Sent: 18 April 2013 05:14
> > To: Opensource [Anthony Olech]
> > Cc: Jean Delvare; Mark Brown; Randy Dunlap; lm-sensors@lm-sensors.org;
> > LKML; David Dajun Chen
> > Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
> > 
> [...]
> > > +	if (cell == NULL) {
> > > +		ret = -ENODEV;
> > > +		goto exit;
> > 
> > Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
> > 
> [...]
> > > +exit:
> > > +	return ret;
> > > +}
> [...]
> 
> Hi Guenter,
> 
> I have read CodingStyle, chapter 7 and it seems to encourage a single exit point from functions.
> 
Yes, unless you can return directly. There is even an example.
It actually says "... and some common work such as cleanup has to be done".

> During development there was some logging done at the (single) exit point but that has been stripped out for submission.
> Whilst I can duplicate the logging and do an immediate 'return' at all those points, I am hesitant to do so when chapter 7 of the CodingStyle appears to say to use a single exit point. In addition I had thought that a single exit point from functions was a good thing.
> 
Not if you add a goto to a return statement. That defeats the purpose
of easy readability.

Guenter

RE: CodingStyle

[Opensource [Anthony Olech]]

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: 18 April 2013 14:42
> To: Opensource [Anthony Olech]
> Cc: LKML
> Subject: Re: CodingStyle
> 
> On Thu, Apr 18, 2013 at 10:48:06AM +0000, Opensource [Anthony Olech]
> wrote:
> > > -----Original Message-----
> > > From: Guenter Roeck [mailto:linux@roeck-us.net]
> > > Sent: 18 April 2013 05:14
> > > To: Opensource [Anthony Olech]
> > > Cc: Jean Delvare; Mark Brown; Randy Dunlap;
> > > lm-sensors@lm-sensors.org; LKML; David Dajun Chen
> > > Subject: Re: [NEW DRIVER V5 6/7] drivers/hwmon: DA9058 HWMON driver
> > >
> > [...]
> > > > +	if (cell == NULL) {
> > > > +		ret = -ENODEV;
> > > > +		goto exit;
> > >
> > > Just return -ENODEV is good enough here. See CodingStyle, chapter 7.
> > >
> > [...]
> > > > +exit:
> > > > +	return ret;
> > > > +}
> > [...]
> >
> > Hi Guenter,
> >
> > I have read CodingStyle, chapter 7 and it seems to encourage a single exit
> point from functions.
> >
> Yes, unless you can return directly. There is even an example.
> It actually says "... and some common work such as cleanup has to be done".

OK, thank you for clarifying that. I will make the necessary changes.
 
> > During development there was some logging done at the (single) exit point
> but that has been stripped out for submission.
> > Whilst I can duplicate the logging and do an immediate 'return' at all those
> points, I am hesitant to do so when chapter 7 of the CodingStyle appears to say
> to use a single exit point. In addition I had thought that a single exit point from
> functions was a good thing.
> >
> Not if you add a goto to a return statement. That defeats the purpose of easy
> readability.
> Guenter