All of lore.kernel.org
 help / color / mirror / Atom feed
From: joe@perches.com (Joe Perches)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] staging: bcm2835-audio: bcm2835.h: fix various coding style issues
Date: Fri, 17 Feb 2017 11:52:09 -0800	[thread overview]
Message-ID: <1487361129.2198.4.camel@perches.com> (raw)
In-Reply-To: <1487356776-11449-1-git-send-email-adanhawthorn@gmail.com>

On Fri, 2017-02-17 at 13:39 -0500, Nathan Howard wrote:
> The following coding style issues (as per checkpatch.pl) were resolved.

What Greg said is true, and the volatile conversion
especially needs to be verified.

> diff --git 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
>   */

Using #define DEBUG would be more common.
 
>  /* Debug macros */
> @@ -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)

These might as well be removed and converted to
the direct pr_<level> equivalents and have

#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__

added before any include, but honestly the
__func__ and __LINE__ aren't particularly useful.

> @@ -122,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;

Unless you can absolutely verify that that
doesn't change hardware access, you should
leave this alone.
'

WARNING: multiple messages have this Message-ID (diff)
From: Joe Perches <joe@perches.com>
To: Nathan Howard <adanhawthorn@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	bcm-kernel-feedback-list@broadcom.com,
	Stephen Warren <swarren@wwwdotorg.org>,
	Lee Jones <lee@kernel.org>, Eric Anholt <eric@anholt.net>,
	Michael Zoran <mzoran@crowfest.net>,
	devel@driverdev.osuosl.org, linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: bcm2835-audio: bcm2835.h: fix various coding style issues
Date: Fri, 17 Feb 2017 11:52:09 -0800	[thread overview]
Message-ID: <1487361129.2198.4.camel@perches.com> (raw)
In-Reply-To: <1487356776-11449-1-git-send-email-adanhawthorn@gmail.com>

On Fri, 2017-02-17 at 13:39 -0500, Nathan Howard wrote:
> The following coding style issues (as per checkpatch.pl) were resolved.

What Greg said is true, and the volatile conversion
especially needs to be verified.

> diff --git 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
>   */

Using #define DEBUG would be more common.
 
>  /* Debug macros */
> @@ -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)

These might as well be removed and converted to
the direct pr_<level> equivalents and have

#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__

added before any include, but honestly the
__func__ and __LINE__ aren't particularly useful.

> @@ -122,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;

Unless you can absolutely verify that that
doesn't change hardware access, you should
leave this alone.
'

  parent reply	other threads:[~2017-02-17 19:52 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-17 18:39 [PATCH] staging: bcm2835-audio: bcm2835.h: fix various coding style issues Nathan Howard
2017-02-17 18:39 ` Nathan Howard
2017-02-17 18:43 ` Greg Kroah-Hartman
2017-02-17 18:43   ` Greg Kroah-Hartman
2017-02-17 19:52 ` Joe Perches [this message]
2017-02-17 19:52   ` Joe Perches
  -- strict thread matches above, loose matches on Subject: below --
2017-02-16 23:12 Nathan Howard
2017-02-16 23:12 ` Nathan Howard
2017-02-17  0:57 ` Greg Kroah-Hartman
2017-02-17  0:57   ` Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1487361129.2198.4.camel@perches.com \
    --to=joe@perches.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.