line6linux-devel

[Line6linux-devel] [PATCH 0/8] staging: line6: checkpatch.pl cleanups

[Stefan H. <ste...@gm...>]

This series addresses a number of checkpatch.pl warnings, mostly exceeding the
80 character line limit.

Some checkpatch.pl warnings remain in the line6 driver but they are either more
difficult to resolve or I intend to drop that code entirely.  So expect a
little more time before the driver passes checkpatch.pl completely.

Stefan Hajnoczi (8):
  staging: line6: wrap >80 char lines in capture.c
  staging: line6: fix quoted string across lines in midibuf.c
  staging: line6: shorten comment below 80 chars in pcm.c
  staging: line6: drop trailing whitespace in pcm.h
  staging: line6: wrap lines to 80 chars in playback.c
  staging: line6: replace deprecated strict_strtol() in toneport.c
  staging: line6: wrap lines to 80 chars in usbdefs.h
  staging: line6: wrap comment to 80 chars in variax.c

 drivers/staging/line6/capture.c  | 13 ++++++++-----
 drivers/staging/line6/midibuf.c  |  6 +++---
 drivers/staging/line6/pcm.c      |  4 ++--
 drivers/staging/line6/pcm.h      |  2 +-
 drivers/staging/line6/playback.c | 17 +++++++++++------
 drivers/staging/line6/toneport.c |  8 ++------
 drivers/staging/line6/usbdefs.h  | 10 +++++++---
 drivers/staging/line6/variax.c   |  4 +++-
 8 files changed, 37 insertions(+), 27 deletions(-)

-- 
1.7.12.1

[Line6linux-devel] [PATCH 3/8] staging: line6: shorten comment below 80 chars in pcm.c

[Stefan H. <ste...@gm...>]

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/pcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/line6/pcm.c b/drivers/staging/line6/pcm.c
index 7fe44a6..6c1e313 100644
--- a/drivers/staging/line6/pcm.c
+++ b/drivers/staging/line6/pcm.c
@@ -109,7 +109,7 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels)
 	line6pcm->prev_fbuf = NULL;
 
 	if (test_flags(flags_old, flags_new, LINE6_BITS_CAPTURE_BUFFER)) {
-		/* We may be invoked multiple times in a row so allocate once only */
+		/* Invoked multiple times in a row so allocate once only */
 		if (!line6pcm->buffer_in) {
 			line6pcm->buffer_in =
 				kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
@@ -148,7 +148,7 @@ int line6_pcm_acquire(struct snd_line6_pcm *line6pcm, int channels)
 	}
 
 	if (test_flags(flags_old, flags_new, LINE6_BITS_PLAYBACK_BUFFER)) {
-		/* We may be invoked multiple times in a row so allocate once only */
+		/* Invoked multiple times in a row so allocate once only */
 		if (!line6pcm->buffer_out) {
 			line6pcm->buffer_out =
 				kmalloc(LINE6_ISO_BUFFERS * LINE6_ISO_PACKETS *
-- 
1.7.12.1

[Line6linux-devel] [PATCH 4/8] staging: line6: drop trailing whitespace in pcm.h

[Stefan H. <ste...@gm...>]

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/pcm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/line6/pcm.h b/drivers/staging/line6/pcm.h
index 5210ec8..6aa0d46 100644
--- a/drivers/staging/line6/pcm.h
+++ b/drivers/staging/line6/pcm.h
@@ -167,7 +167,7 @@ enum {
 #endif
 	    LINE6_BIT_PCM_ALSA_CAPTURE_STREAM |
 	    LINE6_BIT_PCM_MONITOR_CAPTURE_STREAM,
-	
+
 	LINE6_BITS_STREAM =
 	    LINE6_BITS_PLAYBACK_STREAM |
 	    LINE6_BITS_CAPTURE_STREAM
-- 
1.7.12.1

[Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Stefan H. <ste...@gm...>]

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/capture.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/line6/capture.c b/drivers/staging/line6/capture.c
index c85c5b6..389c41f 100644
--- a/drivers/staging/line6/capture.c
+++ b/drivers/staging/line6/capture.c
@@ -256,8 +256,8 @@ static void audio_in_callback(struct urb *urb)
 #ifdef CONFIG_LINE6_USB_IMPULSE_RESPONSE
 		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
 #endif
-			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM, &line6pcm->flags)
-			    && (fsize > 0))
+			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
+				     &line6pcm->flags) && (fsize > 0))
 				line6_capture_copy(line6pcm, fbuf, fsize);
 	}
 
@@ -274,7 +274,8 @@ static void audio_in_callback(struct urb *urb)
 #ifdef CONFIG_LINE6_USB_IMPULSE_RESPONSE
 		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
 #endif
-			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM, &line6pcm->flags))
+			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
+				     &line6pcm->flags))
 				line6_capture_check_period(line6pcm, length);
 	}
 }
@@ -356,7 +357,8 @@ int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd)
 #ifdef CONFIG_PM
 	case SNDRV_PCM_TRIGGER_RESUME:
 #endif
-		err = line6_pcm_acquire(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
+		err = line6_pcm_acquire(line6pcm,
+					LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
 
 		if (err < 0)
 			return err;
@@ -367,7 +369,8 @@ int snd_line6_capture_trigger(struct snd_line6_pcm *line6pcm, int cmd)
 #ifdef CONFIG_PM
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 #endif
-		err = line6_pcm_release(line6pcm, LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
+		err = line6_pcm_release(line6pcm,
+					LINE6_BIT_PCM_ALSA_CAPTURE_STREAM);
 
 		if (err < 0)
 			return err;
-- 
1.7.12.1

[Line6linux-devel] [PATCH 5/8] staging: line6: wrap lines to 80 chars in playback.c

[Stefan H. <ste...@gm...>]

There are a few instances of 80+ character lines in playback.c.  Two
instances are just because of a useless comment "this is somewhat
paranoid", so drop the comment.  Other instances are straightforward
line wrapping.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/playback.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/line6/playback.c b/drivers/staging/line6/playback.c
index a0ab9d0..4cf23af 100644
--- a/drivers/staging/line6/playback.c
+++ b/drivers/staging/line6/playback.c
@@ -185,7 +185,7 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
 	if (urb_size == 0) {
 		/* can't determine URB size */
 		spin_unlock_irqrestore(&line6pcm->lock_audio_out, flags);
-		dev_err(line6pcm->line6->ifcdev, "driver bug: urb_size = 0\n");	/* this is somewhat paranoid */
+		dev_err(line6pcm->line6->ifcdev, "driver bug: urb_size = 0\n");
 		return -EINVAL;
 	}
 
@@ -218,7 +218,8 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
 				       len * bytes_per_frame, runtime->dma_area,
 				       (urb_frames - len) * bytes_per_frame);
 			} else
-				dev_err(line6pcm->line6->ifcdev, "driver bug: len = %d\n", len);	/* this is somewhat paranoid */
+				dev_err(line6pcm->line6->ifcdev, "driver bug: len = %d\n",
+					len);
 		} else {
 			memcpy(urb_out->transfer_buffer,
 			       runtime->dma_area +
@@ -319,7 +320,8 @@ void line6_unlink_audio_out_urbs(struct snd_line6_pcm *line6pcm)
 }
 
 /*
-	Wait until unlinking of all currently active playback URBs has been finished.
+	Wait until unlinking of all currently active playback URBs has been
+	finished.
 */
 void line6_wait_clear_audio_out_urbs(struct snd_line6_pcm *line6pcm)
 {
@@ -413,7 +415,8 @@ static void audio_out_callback(struct urb *urb)
 	if (!shutdown) {
 		submit_audio_out_urb(line6pcm);
 
-		if (test_bit(LINE6_INDEX_PCM_ALSA_PLAYBACK_STREAM, &line6pcm->flags)) {
+		if (test_bit(LINE6_INDEX_PCM_ALSA_PLAYBACK_STREAM,
+			     &line6pcm->flags)) {
 			line6pcm->bytes_out += length;
 			if (line6pcm->bytes_out >= line6pcm->period_out) {
 				line6pcm->bytes_out %= line6pcm->period_out;
@@ -499,7 +502,8 @@ int snd_line6_playback_trigger(struct snd_line6_pcm *line6pcm, int cmd)
 #ifdef CONFIG_PM
 	case SNDRV_PCM_TRIGGER_RESUME:
 #endif
-		err = line6_pcm_acquire(line6pcm, LINE6_BIT_PCM_ALSA_PLAYBACK_STREAM);
+		err = line6_pcm_acquire(line6pcm,
+					LINE6_BIT_PCM_ALSA_PLAYBACK_STREAM);
 
 		if (err < 0)
 			return err;
@@ -510,7 +514,8 @@ int snd_line6_playback_trigger(struct snd_line6_pcm *line6pcm, int cmd)
 #ifdef CONFIG_PM
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 #endif
-		err = line6_pcm_release(line6pcm, LINE6_BIT_PCM_ALSA_PLAYBACK_STREAM);
+		err = line6_pcm_release(line6pcm,
+					LINE6_BIT_PCM_ALSA_PLAYBACK_STREAM);
 
 		if (err < 0)
 			return err;
-- 
1.7.12.1

[Line6linux-devel] [PATCH 2/8] staging: line6: fix quoted string across lines in midibuf.c

[Stefan H. <ste...@gm...>]

Checkpatch warns when quoted strings are split across lines.  The
rationale is that quoted strings should be left on a single line so that
grep works.  (The 80 character line limit does not apply to quoted
strings.)

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/midibuf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/line6/midibuf.c b/drivers/staging/line6/midibuf.c
index 836e8c8..968e0de 100644
--- a/drivers/staging/line6/midibuf.c
+++ b/drivers/staging/line6/midibuf.c
@@ -64,9 +64,9 @@ int line6_midibuf_init(struct MidiBuffer *this, int size, int split)
 
 void line6_midibuf_status(struct MidiBuffer *this)
 {
-	pr_debug("midibuf size=%d split=%d pos_read=%d pos_write=%d "
-	       "full=%d command_prev=%02x\n", this->size, this->split,
-	       this->pos_read, this->pos_write, this->full, this->command_prev);
+	pr_debug("midibuf size=%d split=%d pos_read=%d pos_write=%d full=%d command_prev=%02x\n",
+		 this->size, this->split, this->pos_read, this->pos_write,
+		 this->full, this->command_prev);
 }
 
 int line6_midibuf_bytes_free(struct MidiBuffer *this)
-- 
1.7.12.1

[Line6linux-devel] [PATCH 6/8] staging: line6: replace deprecated strict_strtol() in toneport.c

[Stefan H. <ste...@gm...>]

The LED value is an int, so replace strict_strtol() with kstrtoint().
It's safe to pass in the actual variable instead of a local temporary
because strto*() doesn't write to the result unless the function returns
success.

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/toneport.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/line6/toneport.c b/drivers/staging/line6/toneport.c
index 31b624b..a529dd3 100644
--- a/drivers/staging/line6/toneport.c
+++ b/drivers/staging/line6/toneport.c
@@ -127,13 +127,11 @@ static ssize_t toneport_set_led_red(struct device *dev,
 				    const char *buf, size_t count)
 {
 	int retval;
-	long value;
 
-	retval = strict_strtol(buf, 10, &value);
+	retval = kstrtoint(buf, 10, &led_red);
 	if (retval)
 		return retval;
 
-	led_red = value;
 	toneport_update_led(dev);
 	return count;
 }
@@ -143,13 +141,11 @@ static ssize_t toneport_set_led_green(struct device *dev,
 				      const char *buf, size_t count)
 {
 	int retval;
-	long value;
 
-	retval = strict_strtol(buf, 10, &value);
+	retval = kstrtoint(buf, 10, &led_green);
 	if (retval)
 		return retval;
 
-	led_green = value;
 	toneport_update_led(dev);
 	return count;
 }
-- 
1.7.12.1

[Line6linux-devel] [PATCH 7/8] staging: line6: wrap lines to 80 chars in usbdefs.h

[Stefan H. <ste...@gm...>]

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/usbdefs.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/line6/usbdefs.h b/drivers/staging/line6/usbdefs.h
index 353d59d..43eb540 100644
--- a/drivers/staging/line6/usbdefs.h
+++ b/drivers/staging/line6/usbdefs.h
@@ -83,11 +83,15 @@ enum {
 	LINE6_BIT(VARIAX),
 
 	LINE6_BITS_PRO = LINE6_BIT_BASSPODXTPRO | LINE6_BIT_PODXTPRO,
-	LINE6_BITS_LIVE = LINE6_BIT_BASSPODXTLIVE | LINE6_BIT_PODXTLIVE | LINE6_BIT_PODX3LIVE,
-	LINE6_BITS_PODXTALL = LINE6_BIT_PODXT | LINE6_BIT_PODXTLIVE | LINE6_BIT_PODXTPRO,
+	LINE6_BITS_LIVE = LINE6_BIT_BASSPODXTLIVE | LINE6_BIT_PODXTLIVE |
+			  LINE6_BIT_PODX3LIVE,
+	LINE6_BITS_PODXTALL = LINE6_BIT_PODXT | LINE6_BIT_PODXTLIVE |
+			      LINE6_BIT_PODXTPRO,
 	LINE6_BITS_PODX3ALL = LINE6_BIT_PODX3 | LINE6_BIT_PODX3LIVE,
 	LINE6_BITS_PODHDALL = LINE6_BIT_PODHD300 | LINE6_BIT_PODHD500,
-	LINE6_BITS_BASSPODXTALL	= LINE6_BIT_BASSPODXT |	LINE6_BIT_BASSPODXTLIVE | LINE6_BIT_BASSPODXTPRO
+	LINE6_BITS_BASSPODXTALL	= LINE6_BIT_BASSPODXT |
+				  LINE6_BIT_BASSPODXTLIVE |
+				  LINE6_BIT_BASSPODXTPRO
 };
 
 /* device supports settings parameter via USB */
-- 
1.7.12.1

[Line6linux-devel] [PATCH 8/8] staging: line6: wrap comment to 80 chars in variax.c

[Stefan H. <ste...@gm...>]

Signed-off-by: Stefan Hajnoczi <ste...@gm...>
---
 drivers/staging/line6/variax.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/line6/variax.c b/drivers/staging/line6/variax.c
index f97416b..1b85ecc 100644
--- a/drivers/staging/line6/variax.c
+++ b/drivers/staging/line6/variax.c
@@ -160,7 +160,9 @@ static void variax_startup5(unsigned long data)
 	/* current model dump: */
 	line6_dump_request_async(&variax->dumpreq, &variax->line6, 0,
 				 VARIAX_DUMP_PASS1);
-	/* passes 2 and 3 are performed implicitly before entering variax_startup6 */
+	/* passes 2 and 3 are performed implicitly before entering
+	 * variax_startup6.
+	 */
 }
 
 static void variax_startup6(struct usb_line6_variax *variax)
-- 
1.7.12.1

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Dan C. <dan...@or...>]

On Sun, Nov 11, 2012 at 01:24:39PM +0100, Stefan Hajnoczi wrote:
> Signed-off-by: Stefan Hajnoczi <ste...@gm...>
> ---
>  drivers/staging/line6/capture.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/line6/capture.c b/drivers/staging/line6/capture.c
> index c85c5b6..389c41f 100644
> --- a/drivers/staging/line6/capture.c
> +++ b/drivers/staging/line6/capture.c
> @@ -256,8 +256,8 @@ static void audio_in_callback(struct urb *urb)
>  #ifdef CONFIG_LINE6_USB_IMPULSE_RESPONSE
>  		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
>  #endif
> -			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM, &line6pcm->flags)
> -			    && (fsize > 0))
> +			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,

The reason this is hitting the 80 character limit is because
"LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
isn't even clear from the name what it holds.  It's just a very crap
name.

regards,
dan carpenter

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Markus G. <gr...@ic...>]

Am Mittwoch, 14. November 2012, 17:33:05 schrieb Dan Carpenter:
> On Sun, Nov 11, 2012 at 01:24:39PM +0100, Stefan Hajnoczi wrote:
> > Signed-off-by: Stefan Hajnoczi <ste...@gm...>
> > ---
> > 
> >  drivers/staging/line6/capture.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/line6/capture.c
> > b/drivers/staging/line6/capture.c index c85c5b6..389c41f 100644
> > --- a/drivers/staging/line6/capture.c
> > +++ b/drivers/staging/line6/capture.c
> > @@ -256,8 +256,8 @@ static void audio_in_callback(struct urb *urb)
> > 
> >  #ifdef CONFIG_LINE6_USB_IMPULSE_RESPONSE
> >  
> >  		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
> >  
> >  #endif
> > 
> > -			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM, &line6pcm-
>flags)
> > -			    && (fsize > 0))
> > +			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
> 
> The reason this is hitting the 80 character limit is because
> "LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
> isn't even clear from the name what it holds.  It's just a very crap
> name.
Please refer to the file pcm.h for a detailed documentation of this and 
similar names (in fact, the documentation explains the LINE6_BIT_PCM_* names 
instead, but I bevlieve the correspondence is obvious). It's hard to define a 
shorter name which is at the same time descriptive, consistent, and not to be 
confused with related names.

Should such documentation be moved to a separate file (e.g., 
"Documentation/sound/alsa/line6usb.txt")?

	Kind regards,
		Markus

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Dan C. <dan...@or...>]

On Thu, Nov 15, 2012 at 10:03:27PM +0100, Markus Grabner wrote:
> Am Mittwoch, 14. November 2012, 17:33:05 schrieb Dan Carpenter:
> > On Sun, Nov 11, 2012 at 01:24:39PM +0100, Stefan Hajnoczi wrote:
> > > Signed-off-by: Stefan Hajnoczi <ste...@gm...>
> > > ---
> > > 
> > >  drivers/staging/line6/capture.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/line6/capture.c
> > > b/drivers/staging/line6/capture.c index c85c5b6..389c41f 100644
> > > --- a/drivers/staging/line6/capture.c
> > > +++ b/drivers/staging/line6/capture.c
> > > @@ -256,8 +256,8 @@ static void audio_in_callback(struct urb *urb)
> > > 
> > >  #ifdef CONFIG_LINE6_USB_IMPULSE_RESPONSE
> > >  
> > >  		if (!(line6pcm->flags & LINE6_BITS_PCM_IMPULSE))
> > >  
> > >  #endif
> > > 
> > > -			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM, &line6pcm-
> >flags)
> > > -			    && (fsize > 0))
> > > +			if (test_bit(LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM,
> > 
> > The reason this is hitting the 80 character limit is because
> > "LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
> > isn't even clear from the name what it holds.  It's just a very crap
> > name.
> Please refer to the file pcm.h for a detailed documentation of this and 
> similar names (in fact, the documentation explains the LINE6_BIT_PCM_* names 
> instead, but I bevlieve the correspondence is obvious). It's hard to define a 
> shorter name which is at the same time descriptive, consistent, and not to be 
> confused with related names.
> 
> Should such documentation be moved to a separate file (e.g., 
> "Documentation/sound/alsa/line6usb.txt")?

The documentation is pretty good actually and it belongs where it
is.  But 35 characters is just tooooooooooooooooooooooooooooooooooo
long.

To me the word "INDEX_" is confusing because what are we indexing?
In this context it means the the variable is a bit flag.  That was
obvious because we were calling test_bit().

I think we could drop the PCM_ as well, because why do we need that?

We could change the LINE6 to L6.

			if (test_bit(L6_ALSA_CAPTURE_STREAM, &line6pcm - flags)

It fits!

regards,
dan carpenter

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Joe P. <jo...@pe...>]

On Thu, 2012-11-15 at 22:03 +0100, Markus Grabner wrote:
> Am Mittwoch, 14. November 2012, 17:33:05 schrieb Dan Carpenter:
> > The reason this is hitting the 80 character limit is because
> > "LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
> > isn't even clear from the name what it holds.  It's just a very crap
> > name.
> Please refer to the file pcm.h for a detailed documentation of this and 
> similar names (in fact, the documentation explains the LINE6_BIT_PCM_* names 
> instead, but I bevlieve the correspondence is obvious). It's hard to define a 
> shorter name which is at the same time descriptive, consistent, and not to be 
> confused with related names.
> 
> Should such documentation be moved to a separate file (e.g., 
> "Documentation/sound/alsa/line6usb.txt")?

Documenting poor naming choices doesn't make it better.

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Markus G. <gr...@ic...>]

On Thursday 15 November 2012 14:12:31 Joe Perches wrote:
> On Thu, 2012-11-15 at 22:03 +0100, Markus Grabner wrote:
> > Am Mittwoch, 14. November 2012, 17:33:05 schrieb Dan Carpenter:
> > > The reason this is hitting the 80 character limit is because
> > > "LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
> > > isn't even clear from the name what it holds.  It's just a very crap
> > > name.
> > 
> > Please refer to the file pcm.h for a detailed documentation of this and
> > similar names (in fact, the documentation explains the LINE6_BIT_PCM_*
> > names instead, but I bevlieve the correspondence is obvious). It's hard
> > to define a shorter name which is at the same time descriptive,
> > consistent, and not to be confused with related names.
> > 
> > Should such documentation be moved to a separate file (e.g.,
> > "Documentation/sound/alsa/line6usb.txt")?
> 
> Documenting poor naming choices doesn't make it better.
Yes, but the documentation might help understanding why a particular naming 
was chosen and that it might not be as poor as it seemed at first sight. I 
assume that you are aware of the meaning of the LINE6_INDEX_PCM_* symbols (and 
of the issues that were fixed by introducing them), so which naming scheme 
would you propose instead?

	Kind regards,
		Markus

Re: [Line6linux-devel] [PATCH 1/8] staging: line6: wrap >80 char lines in capture.c

[Joe P. <jo...@pe...>]

On Fri, 2012-11-16 at 00:43 +0100, Markus Grabner wrote:
> On Thursday 15 November 2012 14:12:31 Joe Perches wrote:
> > On Thu, 2012-11-15 at 22:03 +0100, Markus Grabner wrote:
> > > Am Mittwoch, 14. November 2012, 17:33:05 schrieb Dan Carpenter:
> > > > The reason this is hitting the 80 character limit is because
> > > > "LINE6_INDEX_PCM_ALSA_CAPTURE_STREAM" is 35 characters long.  It
> > > > isn't even clear from the name what it holds.  It's just a very crap
> > > > name.
> > > 
> > > Please refer to the file pcm.h for a detailed documentation of this and
> > > similar names (in fact, the documentation explains the LINE6_BIT_PCM_*
> > > names instead, but I bevlieve the correspondence is obvious). It's hard
> > > to define a shorter name which is at the same time descriptive,
> > > consistent, and not to be confused with related names.
> > > 
> > > Should such documentation be moved to a separate file (e.g.,
> > > "Documentation/sound/alsa/line6usb.txt")?
> > 
> > Documenting poor naming choices doesn't make it better.
> Yes, but the documentation might help understanding why a particular naming 
> was chosen and that it might not be as poor as it seemed at first sight. I 
> assume that you are aware of the meaning of the LINE6_INDEX_PCM_* symbols (and 
> of the issues that were fixed by introducing them), so which naming scheme 

Hi Markus

Dunno why they were introduced, but I think
several things could be shorter as to me
none of these longish L6_BIT(foo) elements
means much and I'd need to read the code
to figure them out anyway.

the LINE6_ prefix is excessive,
L6_ would probably be fine.
(and that goes for all the functions too)

CAPTURE/RECORD could be RD/WR
MONITOR could be MON
IMPULSE could be IRM

BUFFER could be I
STREAM could be O
(or the other way 'round)

etc..., cheers, Joe