[PATCH 1/5] staging: bcm2835-audio: bcm2835.h: fix block comment warning

[PATCH 1/5] staging: bcm2835-audio: bcm2835.h: fix block comment warning

[Nathan Howard]

Fix checkpatch.pl warning: "Block comments use * on subsequent lines."

Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
---
 drivers/staging/bcm2835-audio/bcm2835.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
index 36e3ef8..66af445 100644
--- a/drivers/staging/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/bcm2835-audio/bcm2835.h
@@ -27,8 +27,8 @@
 #include <linux/workqueue.h>
 
 /*
-#define AUDIO_DEBUG_ENABLE
-#define AUDIO_VERBOSE_DEBUG_ENABLE
+ * #define AUDIO_DEBUG_ENABLE
+ * #define AUDIO_VERBOSE_DEBUG_ENABLE
  */
 
 /* Debug macros */
-- 
2.7.4

[PATCH 2/5] staging: bcm2835-audio: bcm2835.h: fix printk coding style issue

[Nathan Howard]

Fix checkpatch.pl warning of the form "WARNING: Prefer
subsystem eg: netdev]_info([subsystem]dev, ...
then dev_info(dev, ... then pr_info(...  to printk(KERN_INFO ..."

Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
---
 drivers/staging/bcm2835-audio/bcm2835.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
index 66af445..81fdb10 100644
--- a/drivers/staging/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/bcm2835-audio/bcm2835.h
@@ -37,10 +37,10 @@
 #ifdef AUDIO_VERBOSE_DEBUG_ENABLE
 
 #define audio_debug(fmt, arg...) \
-	printk(KERN_INFO"%s:%d " fmt, __func__, __LINE__, ##arg)
+	pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
 
 #define audio_info(fmt, arg...) \
-	printk(KERN_INFO"%s:%d " fmt, __func__, __LINE__, ##arg)
+	pr_info("%s:%d " fmt, __func__, __LINE__, ##arg)
 
 #else
 
@@ -59,13 +59,13 @@
 #endif /* AUDIO_DEBUG_ENABLE */
 
 #define audio_error(fmt, arg...) \
-	printk(KERN_ERR"%s:%d " fmt, __func__, __LINE__, ##arg)
+	pr_err("%s:%d " fmt, __func__, __LINE__, ##arg)
 
 #define audio_warning(fmt, arg...) \
-	printk(KERN_WARNING"%s:%d " fmt, __func__, __LINE__, ##arg)
+	pr_warn("%s:%d " fmt, __func__, __LINE__, ##arg)
 
 #define audio_alert(fmt, arg...) \
-	printk(KERN_ALERT"%s:%d " fmt, __func__, __LINE__, ##arg)
+	pr_alert("%s:%d " fmt, __func__, __LINE__, ##arg)
 
 #define MAX_SUBSTREAMS   (8)
 #define AVAIL_SUBSTREAMS_MASK  (0xff)
-- 
2.7.4

[PATCH 3/5] staging: bcm2835-audio: bcm2835.h: fix macro coding style issue

[Nathan Howard]

Fix checkpatch.pl warning of the form "CHECK: Macro argument 'vol' may be
better as '(vol)' to avoid precedence issues."

Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
---
 drivers/staging/bcm2835-audio/bcm2835.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
index 81fdb10..2f9d1c9 100644
--- a/drivers/staging/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/bcm2835-audio/bcm2835.h
@@ -77,8 +77,11 @@ enum {
 
 /* macros for alsa2chip and chip2alsa, instead of functions */
 
-#define alsa2chip(vol) (uint)(-((vol << 8) / 100)) /* convert alsa to chip volume (defined as macro rather than function call) */
-#define chip2alsa(vol) -((vol * 100) >> 8)   /* convert chip to alsa volume */
+// convert alsa to chip volume (defined as macro rather than function call)
+#define alsa2chip(vol) (uint)(-(((vol) << 8) / 100))
+
+// convert chip to alsa volume
+#define chip2alsa(vol) -(((vol) * 100) >> 8)
 
 /* Some constants for values .. */
 enum snd_bcm2835_route {
-- 
2.7.4

[PATCH 4/5] staging: bcm2835-audio: bcm2835.h: fix volatile coding style issue

[Nathan Howard]

Fix checkpatch.pl warning of the form "WARNING: Use of volatile is
usually wrong: see Documentation/process/volatile-considered-harmful.rst."

Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
---
 drivers/staging/bcm2835-audio/bcm2835.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
index 2f9d1c9..08f7ad6 100644
--- a/drivers/staging/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/bcm2835-audio/bcm2835.h
@@ -125,8 +125,8 @@ struct bcm2835_alsa_stream {
 	struct semaphore buffers_update_sem;
 	struct semaphore control_sem;
 	spinlock_t lock;
-	volatile unsigned int control;
-	volatile unsigned int status;
+	unsigned int control;
+	unsigned int status;
 
 	int open;
 	int running;
-- 
2.7.4

[PATCH 4/5] staging: bcm2835-audio: bcm2835.h: fix volatile coding style issue

[Joe Perches]

On Fri, 2017-02-17 at 15:16 -0500, Nathan Howard wrote:
> Fix checkpatch.pl warning of the form "WARNING: Use of volatile is
> usually wrong: see Documentation/process/volatile-considered-harmful.rst."

Why are you sure the volatile use is not necessary?

> Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
> ---
>  drivers/staging/bcm2835-audio/bcm2835.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
> index 2f9d1c9..08f7ad6 100644
> --- a/drivers/staging/bcm2835-audio/bcm2835.h
> +++ b/drivers/staging/bcm2835-audio/bcm2835.h
> @@ -125,8 +125,8 @@ struct bcm2835_alsa_stream {
>  	struct semaphore buffers_update_sem;
>  	struct semaphore control_sem;
>  	spinlock_t lock;
> -	volatile unsigned int control;
> -	volatile unsigned int status;
> +	unsigned int control;
> +	unsigned int status;
>  
>  	int open;
>  	int running;

[PATCH 4/5] staging: bcm2835-audio: bcm2835.h: fix volatile coding style issue

[Adan Hawthorn]

Thank you, Joe.

Please remove this patch at this time; it was sent in error.

On Fri, Feb 17, 2017 at 6:04 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-02-17 at 15:16 -0500, Nathan Howard wrote:
>> Fix checkpatch.pl warning of the form "WARNING: Use of volatile is
>> usually wrong: see Documentation/process/volatile-considered-harmful.rst."
>
> Why are you sure the volatile use is not necessary?
>
>> Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
>> ---
>>  drivers/staging/bcm2835-audio/bcm2835.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
>> index 2f9d1c9..08f7ad6 100644
>> --- a/drivers/staging/bcm2835-audio/bcm2835.h
>> +++ b/drivers/staging/bcm2835-audio/bcm2835.h
>> @@ -125,8 +125,8 @@ struct bcm2835_alsa_stream {
>>       struct semaphore buffers_update_sem;
>>       struct semaphore control_sem;
>>       spinlock_t lock;
>> -     volatile unsigned int control;
>> -     volatile unsigned int status;
>> +     unsigned int control;
>> +     unsigned int status;
>>
>>       int open;
>>       int running;

[PATCH 5/5] staging: bcm2835-audio: bcm2835.h: fix line length coding style issue

[Nathan Howard]

Fix checkpatch.pl warning of the form "WARNING: line over 80 characters."

Signed-off-by: Nathan Howard <adanhawthorn@gmail.com>
---
 drivers/staging/bcm2835-audio/bcm2835.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
index 08f7ad6..16d7006 100644
--- a/drivers/staging/bcm2835-audio/bcm2835.h
+++ b/drivers/staging/bcm2835-audio/bcm2835.h
@@ -163,8 +163,10 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count,
 			void *src);
 void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream);
-unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream);
+unsigned int bcm2835_audio_retrieve_buffers(
+		struct bcm2835_alsa_stream *alsa_stream);
 void bcm2835_audio_flush_buffers(struct bcm2835_alsa_stream *alsa_stream);
-void bcm2835_audio_flush_playback_buffers(struct bcm2835_alsa_stream *alsa_stream);
+void bcm2835_audio_flush_playback_buffers(
+		struct bcm2835_alsa_stream *alsa_stream);
 
 #endif /* __SOUND_ARM_BCM2835_H */
-- 
2.7.4

[PATCH 5/5] staging: bcm2835-audio: bcm2835.h: fix line length coding style issue

[Joe Perches]

On Fri, 2017-02-17 at 15:16 -0500, Nathan Howard wrote:
> Fix checkpatch.pl warning of the form "WARNING: line over 80 characters."
[]
> diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
[]
> @@ -163,8 +163,10 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
>  			unsigned int count,
>  			void *src);
>  void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream);
> -unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream);
> +unsigned int bcm2835_audio_retrieve_buffers(
> +		struct bcm2835_alsa_stream *alsa_stream);

This is not a good change.

This line exceeds 80 columns only because
it uses very long identifiers (30+ chars).

Anything that uses these very long names
is going to be silly looking when forced
to use 80 column line length maximums.

Basically, it's OK as it is and if you
really want to change it for any reason
the other style to use is to have the
return value on a separate line like:

unsigned int
bcm2836_audio_retrieve_buffers(struct bcm2835_also_stream *alsa_stream);

Even so, that's not a good change either.

[PATCH 5/5] staging: bcm2835-audio: bcm2835.h: fix line length coding style issue

[Adan Hawthorn]

Thanks, Joe.

Is this to say that scripts/checkpatch.pl should be updated to some
higher column limit?  I have made these cleanup changes before in a
like manner.

On Fri, Feb 17, 2017 at 8:17 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-02-17 at 15:16 -0500, Nathan Howard wrote:
>> Fix checkpatch.pl warning of the form "WARNING: line over 80 characters."
> []
>> diff --git a/drivers/staging/bcm2835-audio/bcm2835.h b/drivers/staging/bcm2835-audio/bcm2835.h
> []
>> @@ -163,8 +163,10 @@ int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
>>                       unsigned int count,
>>                       void *src);
>>  void bcm2835_playback_fifo(struct bcm2835_alsa_stream *alsa_stream);
>> -unsigned int bcm2835_audio_retrieve_buffers(struct bcm2835_alsa_stream *alsa_stream);
>> +unsigned int bcm2835_audio_retrieve_buffers(
>> +             struct bcm2835_alsa_stream *alsa_stream);
>
> This is not a good change.
>
> This line exceeds 80 columns only because
> it uses very long identifiers (30+ chars).
>
> Anything that uses these very long names
> is going to be silly looking when forced
> to use 80 column line length maximums.
>
> Basically, it's OK as it is and if you
> really want to change it for any reason
> the other style to use is to have the
> return value on a separate line like:
>
> unsigned int
> bcm2836_audio_retrieve_buffers(struct bcm2835_also_stream *alsa_stream);
>
> Even so, that's not a good change either.

[PATCH 5/5] staging: bcm2835-audio: bcm2835.h: fix line length coding style issue

[Joe Perches]

On Fri, 2017-02-17 at 20:32 -0500, Adan Hawthorn wrote:
> Thanks, Joe.
> 
> Is this to say that scripts/checkpatch.pl should be updated to some
> higher column limit?  I have made these cleanup changes before in a
> like manner.

Hard to say.

There could be some sensitivity to long identifier
name lengths added to checkpatch, but < 80 column
line length is still currently "strongly preferred".

I don't care much one way or another if it's 80
or 100 or something else as long as it's context
appropriate.

Awhile ago, Linus Torvalds wrote:

On Thu, 2016-12-15@18:10 -0800, Linus Torvalds wrote:
> On Thu, Dec 15, 2016 at 5:57 PM, Joe Perches <joe@perches.com> wrote:
> > > 
> > > In fact, I thought we already upped the check-patch limit to 100?
> > 
> > Nope, CodingStyle neither.
> > 
> > Last time I tried was awhile ago.
> 
> Ok, it must have been just talked about, and with the exceptions for
> strings etc I may not have seen as many of the really annoying line
> breaks lately.
> 
> I don't mind a 80-column "soft limit" per se: if some code
> consistently goes over 80 columns, there really is something seriously
> wrong there. So 80 columns may well be the right limit for that kind
> of check (or even less).
> 
> But if we have just a couple of lines that are longer (in a file that
> is 3k+ lines), I'd rather not break those.
> 
> I tend use "git grep" a lot, and it's much easier to see function
> argument use if it's all on one line.
> 
> Of course, some function calls really are *so* long that they have to
> be broken up, but that's where the "if it's a couple of lines that go
> a bit over the 80 column limit..." exception basically comes in.
> 
> Put another way: long lines definitely aren't good. But breaking long
> lines has some downsides too, so there should be a balance between the
> two, rather than some black-and-white limit.
> 
> In fact, we've seldom had cases where black-and-white limits work well.
> 

[PATCH 5/5] staging: bcm2835-audio: bcm2835.h: fix line length coding style issue

[Adan Hawthorn]

Ok, I personally like that better anyhow.  Also, there was mention of
the line limit even exceeding 100 characters (130ish?) and that was
being tossed around at one point (if I remember correctly).

On Fri, Feb 17, 2017 at 8:38 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-02-17 at 20:32 -0500, Adan Hawthorn wrote:
>> Thanks, Joe.
>>
>> Is this to say that scripts/checkpatch.pl should be updated to some
>> higher column limit?  I have made these cleanup changes before in a
>> like manner.
>
> Hard to say.
>
> There could be some sensitivity to long identifier
> name lengths added to checkpatch, but < 80 column
> line length is still currently "strongly preferred".
>
> I don't care much one way or another if it's 80
> or 100 or something else as long as it's context
> appropriate.
>
> Awhile ago, Linus Torvalds wrote:
>
> On Thu, 2016-12-15 at 18:10 -0800, Linus Torvalds wrote:
>> On Thu, Dec 15, 2016 at 5:57 PM, Joe Perches <joe@perches.com> wrote:
>> > >
>> > > In fact, I thought we already upped the check-patch limit to 100?
>> >
>> > Nope, CodingStyle neither.
>> >
>> > Last time I tried was awhile ago.
>>
>> Ok, it must have been just talked about, and with the exceptions for
>> strings etc I may not have seen as many of the really annoying line
>> breaks lately.
>>
>> I don't mind a 80-column "soft limit" per se: if some code
>> consistently goes over 80 columns, there really is something seriously
>> wrong there. So 80 columns may well be the right limit for that kind
>> of check (or even less).
>>
>> But if we have just a couple of lines that are longer (in a file that
>> is 3k+ lines), I'd rather not break those.
>>
>> I tend use "git grep" a lot, and it's much easier to see function
>> argument use if it's all on one line.
>>
>> Of course, some function calls really are *so* long that they have to
>> be broken up, but that's where the "if it's a couple of lines that go
>> a bit over the 80 column limit..." exception basically comes in.
>>
>> Put another way: long lines definitely aren't good. But breaking long
>> lines has some downsides too, so there should be a balance between the
>> two, rather than some black-and-white limit.
>>
>> In fact, we've seldom had cases where black-and-white limits work well.
>>