[PATCH] staging:comedi: Fixed coding convention issues.

[PATCH] staging:comedi: Fixed coding convention issues.

[Henri Häkkinen]

Cleaned up and fixed coding convention issues as reporteed by
checkpatch.pl tool on the file `drivers.c'.

Signed-off-by: Henri Häkkinen <henuxd@gmail.com>
---
 drivers/staging/comedi/drivers.c |   85 +++++++++++++++++++------------------
 1 files changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 4a29ed7..55521d5 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -117,8 +117,8 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	for (driv = comedi_drivers; driv; driv = driv->next) {
 		if (!try_module_get(driv->module)) {
-			printk
-			    (KERN_INFO "comedi: failed to increment module count, skipping\n");
+			printk(KERN_INFO "comedi: failed to increment module "
+			       "count, skipping\n");
 			continue;
 		}
 		if (driv->num_names) {
@@ -205,9 +205,8 @@ int comedi_driver_unregister(struct comedi_driver *driver)
 		mutex_lock(&dev->mutex);
 		if (dev->attached && dev->driver == driver) {
 			if (dev->use_count)
-				printk
-				    (KERN_WARNING "BUG! detaching device with use_count=%d\n",
-				     dev->use_count);
+				printk(KERN_WARNING "BUG! detaching device "
+				       "with use_count=%d\n", dev->use_count);
 			comedi_device_detach(dev);
 		}
 		mutex_unlock(&dev->mutex);
@@ -441,21 +440,19 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 	if (async->buf_page_list) {
 		unsigned i;
 		for (i = 0; i < async->n_buf_pages; ++i) {
-			if (async->buf_page_list[i].virt_addr) {
-				clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+			void *virt_addr = async->buf_page_list[i].virt_addr;
+			dma_addr_t dma_addr = async->buf_page_list[i].dma_addr;
+
+			if (virt_addr) {
+				struct page *page = virt_to_page(virt_addr);
+				clear_bit(PG_reserved, &page->flags);
 				if (s->async_dma_dir != DMA_NONE) {
 					dma_free_coherent(dev->hw_dev,
 							  PAGE_SIZE,
-							  async->
-							  buf_page_list
-							  [i].virt_addr,
-							  async->
-							  buf_page_list
-							  [i].dma_addr);
+							  virt_addr,
+							  dma_addr);
 				} else {
-					free_page((unsigned long)
-						  async->buf_page_list[i].
-						  virt_addr);
+					free_page((unsigned long) virt_addr);
 				}
 			}
 		}
@@ -478,26 +475,29 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 		}
 		if (pages) {
 			for (i = 0; i < n_pages; i++) {
+				struct comedi_buf_page *buf_page =
+					&async->buf_page_list[i];
+				dma_addr_t dma_addr = buf_page->dma_addr;
+				struct page *page;
+
 				if (s->async_dma_dir != DMA_NONE) {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    dma_alloc_coherent(dev->hw_dev,
 							       PAGE_SIZE,
-							       &async->
-							       buf_page_list
-							       [i].dma_addr,
+							       &dma_addr,
 							       GFP_KERNEL |
 							       __GFP_COMP);
 				} else {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    (void *)
 					    get_zeroed_page(GFP_KERNEL);
 				}
-				if (async->buf_page_list[i].virt_addr == NULL)
+				if (buf_page->virt_addr == NULL)
 					break;
 
-				set_bit(PG_reserved,
-					&(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
-				pages[i] = virt_to_page(async->buf_page_list[i].virt_addr);
+				page = virt_to_page(buf_page->virt_addr);
+				set_bit(PG_reserved, &page->flags);
+				pages[i] = page;
 			}
 		}
 		if (i == n_pages) {
@@ -510,24 +510,27 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 			/* Some allocation failed above. */
 			if (async->buf_page_list) {
 				for (i = 0; i < n_pages; i++) {
-					if (async->buf_page_list[i].virt_addr ==
-					    NULL) {
+					struct comedi_buf_page *buf_page =
+						&async->buf_page_list[i];
+					void *virt_addr =
+						buf_page->virt_addr;
+					dma_addr_t dma_addr =
+						buf_page->dma_addr;
+					struct page *page =
+						virt_to_page(virt_addr);
+
+					if (virt_addr == NULL)
 						break;
-					}
-					clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+
+					clear_bit(PG_reserved, &page->flags);
 					if (s->async_dma_dir != DMA_NONE) {
 						dma_free_coherent(dev->hw_dev,
 								  PAGE_SIZE,
-								  async->
-								  buf_page_list
-								  [i].virt_addr,
-								  async->
-								  buf_page_list
-								  [i].dma_addr);
+								  virt_addr,
+								  dma_addr);
 					} else {
 						free_page((unsigned long)
-							  async->buf_page_list
-							  [i].virt_addr);
+							  virt_addr);
 					}
 				}
 				vfree(async->buf_page_list);
@@ -646,8 +649,8 @@ unsigned comedi_buf_write_free(struct comedi_async *async, unsigned int nbytes)
 {
 	if ((int)(async->buf_write_count + nbytes -
 		  async->buf_write_alloc_count) > 0) {
-		printk
-		    (KERN_INFO "comedi: attempted to write-free more bytes than have been write-allocated.\n");
+		printk(KERN_INFO "comedi: attempted to write-free more bytes "
+		       "than have been write-allocated.\n");
 		nbytes = async->buf_write_alloc_count - async->buf_write_count;
 	}
 	async->buf_write_count += nbytes;
@@ -683,8 +686,8 @@ unsigned comedi_buf_read_free(struct comedi_async *async, unsigned int nbytes)
 	smp_mb();
 	if ((int)(async->buf_read_count + nbytes -
 		  async->buf_read_alloc_count) > 0) {
-		printk(KERN_INFO
-		       "comedi: attempted to read-free more bytes than have been read-allocated.\n");
+		printk(KERN_INFO "comedi: attempted to read-free more bytes "
+		       "than have been read-allocated.\n");
 		nbytes = async->buf_read_alloc_count - async->buf_read_count;
 	}
 	async->buf_read_count += nbytes;
-- 
1.7.1

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Mark Rankilor]

2010/6/12 Henri Häkkinen <henuxd@gmail.com>:
> -                       printk
> -                           (KERN_INFO "comedi: failed to increment module count, skipping\n");
> +                       printk(KERN_INFO "comedi: failed to increment module "
> +                              "count, skipping\n");

Hi Henri,

Regarding your breaking up of printk statements, although some of
those lines do go over 80 characters, it is preferable to keep the
strings together since then those are searchable within the code.

I figure it is quite acceptable to break the string after "comedi: ",
so maybe that will fix the line length issue, otherwise it is
preferable to keep the checkpatch warning in this case.

Mark

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Joe Perches]

On Sun, 2010-06-13 at 10:14 +0800, Mark Rankilor wrote:
> 2010/6/12 Henri Häkkinen <henuxd@gmail.com>:
> > -                       printk
> > -                           (KERN_INFO "comedi: failed to increment module count, skipping\n");
> > +                       printk(KERN_INFO "comedi: failed to increment module "
> > +                              "count, skipping\n");
> 
> Regarding your breaking up of printk statements, although some of
> those lines do go over 80 characters, it is preferable to keep the
> strings together since then those are searchable within the code.
> 
> I figure it is quite acceptable to break the string after "comedi: ",
> so maybe that will fix the line length issue, otherwise it is
> preferable to keep the checkpatch warning in this case.

A couple of options for comedi:

1: Use #define pr_fmt(fmt) "comedi: " fmt
   pr_<level>(format, ...)

2: Create some comedi logging functions or macros like:
   	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
   where "comedi:" is always prefixed and an
   optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
   could be used.

That'd shorten line lengths quite a bit and add
some better standardization to comedi.

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Joe Perches]

On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
> 2: Create some comedi logging functions or macros like:
>    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
>    where "comedi:" is always prefixed and an
>    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>    could be used.

Maybe this is a start:

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/staging/comedi/comedidev.h |   54 ++++++++++++++++++++++++++++++++++--
 1 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 4eb2b77..6c2bdde 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -43,11 +43,59 @@
 
 #include "comedi.h"
 
-#define DPRINTK(format, args...)	do {		\
-	if (comedi_debug)				\
-		printk(KERN_DEBUG "comedi: " format , ## args);	\
+#define comedi_printk(level, fmt, args...)			\
+	printk(level "comedi: " pr_fmt(fmt), ##args)
+
+#define DPRINTK(format, args...)				\
+do {								\
+	if (comedi_debug)					\
+		comedi_printk(KERN_DEBUG, fmt, ##args);		\
 } while (0)
 
+#define comedi_emerg(fmt, ...)				\
+	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)
+#define comedi_alert(fmt, ...)				\
+	comedi_printk(KERN_ALERT, fmt, ##__VA_ARGS__)
+#define comedi_crit(fmt, ...)				\
+	comedi_printk(KERN_CRIT, fmt, ##__VA_ARGS__)
+#define comedi_err(fmt, ...)				\
+	comedi_printk(KERN_ERR, fmt, ##__VA_ARGS__)
+#define comedi_warn(fmt, ...)				\
+	comedi_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
+#define comedi_notice(fmt, ...)				\
+	comedi_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
+#define comedi_info(fmt, ...)				\
+	comedi_printk(KERN_INFO, fmt, ##__VA_ARGS__)
+
+/* comedi_devel() should produce zero code unless DEBUG is defined */
+#ifdef DEBUG
+#define comedi_devel(fmt, ...)				\
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#else
+#define comedi_devel(fmt, ...)					\
+({								\
+	if (0)							\
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__);	\
+	0;							\
+})
+#endif
+
+#if defined(DEBUG)
+#define comedi_debug(fmt, ...)				\
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#elif defined(CONFIG_DYNAMIC_DEBUG)
+/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
+#define comedi_debug(fmt, ...)				\
+	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#else
+#define comedi_debug(fmt, ...)					\
+({								\
+	if (0)							\
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__);	\
+	0;							\
+})
+#endif
+
 #define COMEDI_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
 #define COMEDI_VERSION_CODE COMEDI_VERSION(COMEDI_MAJORVERSION, \
 	COMEDI_MINORVERSION, COMEDI_MICROVERSION)

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Henri Häkkinen]

Hello

There are several printk statements without the "comedi:" prefix. Such as:

printk(KERN_WARNING "BUG: dev->driver=NULL in comedi_device_detach()\n");

Do you think it is better to leave these as they are, or should they be changed to use comedi_xxx macros (which will print the "comedi:" prefix)?

Also even with logging macros, there will be few lines which go beyond the 80 character boundary.


On 13.6.2010, at 8.30, Joe Perches wrote:

> On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
>> 2: Create some comedi logging functions or macros like:
>>   	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
>>   where "comedi:" is always prefixed and an
>>   optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>   could be used.
> 
> Maybe this is a start:
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/staging/comedi/comedidev.h |   54 ++++++++++++++++++++++++++++++++++--
> 1 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
> index 4eb2b77..6c2bdde 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -43,11 +43,59 @@
> 
> #include "comedi.h"
> 
> -#define DPRINTK(format, args...)	do {		\
> -	if (comedi_debug)				\
> -		printk(KERN_DEBUG "comedi: " format , ## args);	\
> +#define comedi_printk(level, fmt, args...)			\
> +	printk(level "comedi: " pr_fmt(fmt), ##args)
> +
> +#define DPRINTK(format, args...)				\
> +do {								\
> +	if (comedi_debug)					\
> +		comedi_printk(KERN_DEBUG, fmt, ##args);		\
> } while (0)
> 
> +#define comedi_emerg(fmt, ...)				\
> +	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)
> +#define comedi_alert(fmt, ...)				\
> +	comedi_printk(KERN_ALERT, fmt, ##__VA_ARGS__)
> +#define comedi_crit(fmt, ...)				\
> +	comedi_printk(KERN_CRIT, fmt, ##__VA_ARGS__)
> +#define comedi_err(fmt, ...)				\
> +	comedi_printk(KERN_ERR, fmt, ##__VA_ARGS__)
> +#define comedi_warn(fmt, ...)				\
> +	comedi_printk(KERN_WARNING, fmt, ##__VA_ARGS__)
> +#define comedi_notice(fmt, ...)				\
> +	comedi_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
> +#define comedi_info(fmt, ...)				\
> +	comedi_printk(KERN_INFO, fmt, ##__VA_ARGS__)
> +
> +/* comedi_devel() should produce zero code unless DEBUG is defined */
> +#ifdef DEBUG
> +#define comedi_devel(fmt, ...)				\
> +	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
> +#else
> +#define comedi_devel(fmt, ...)					\
> +({								\
> +	if (0)							\
> +		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__);	\
> +	0;							\
> +})
> +#endif
> +
> +#if defined(DEBUG)
> +#define comedi_debug(fmt, ...)				\
> +	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
> +#elif defined(CONFIG_DYNAMIC_DEBUG)
> +/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> +#define comedi_debug(fmt, ...)				\
> +	dynamic_pr_debug(fmt, ##__VA_ARGS__)
> +#else
> +#define comedi_debug(fmt, ...)					\
> +({								\
> +	if (0)							\
> +		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__);	\
> +	0;							\
> +})
> +#endif
> +
> #define COMEDI_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
> #define COMEDI_VERSION_CODE COMEDI_VERSION(COMEDI_MAJORVERSION, \
> 	COMEDI_MINORVERSION, COMEDI_MICROVERSION)
> 
> 

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Joe Perches]

On Sun, 2010-06-13 at 14:27 +0300, Henri Häkkinen wrote:
> There are several printk statements without the "comedi:" prefix.
> Do you think it is better to leave these as they are, or should they
> be changed to use comedi_xxx macros (which will print the "comedi:"
> prefix)?
> 
> printk(KERN_WARNING "BUG: dev->driver=NULL in comedi_device_detach()\n");

I think it's better to convert them.

Anything with "BUG" in the format
maybe should be converted to BUG_ON.

> Also even with logging macros, there will be few lines which go beyond
> the 80 character boundary.

I'd ignore printk related long line warnings.

I suggest coalescing the format string to a single line
where reasonable.  If a single printk has non trailing
'\n's in a format, it may be better to split them up.

comedi_info("some incredibly long output line with error: %d\n"
	    "Another line with some other information: %d\n",
	    err, info);

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Greg KH]

On Sat, Jun 12, 2010 at 10:30:50PM -0700, Joe Perches wrote:
> On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
> > 2: Create some comedi logging functions or macros like:
> >    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
> >    where "comedi:" is always prefixed and an
> >    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >    could be used.
> 
> Maybe this is a start:
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/staging/comedi/comedidev.h |   54 ++++++++++++++++++++++++++++++++++--
>  1 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
> index 4eb2b77..6c2bdde 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -43,11 +43,59 @@
>  
>  #include "comedi.h"
>  
> -#define DPRINTK(format, args...)	do {		\
> -	if (comedi_debug)				\
> -		printk(KERN_DEBUG "comedi: " format , ## args);	\
> +#define comedi_printk(level, fmt, args...)			\
> +	printk(level "comedi: " pr_fmt(fmt), ##args)
> +
> +#define DPRINTK(format, args...)				\
> +do {								\
> +	if (comedi_debug)					\
> +		comedi_printk(KERN_DEBUG, fmt, ##args);		\
>  } while (0)
>  
> +#define comedi_emerg(fmt, ...)				\
> +	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)

I would prefer the conversion of everything over to the dev_printk()
versions instead of creating a new macro for every individual subsystem.
That way you get the advantage of logging everything in the common
format and the dynamic debug functionality as well.

thanks,

greg k-h

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Joe Perches]

On Thu, 2010-06-17 at 15:51 -0700, Greg KH wrote:
> On Sat, Jun 12, 2010 at 10:30:50PM -0700, Joe Perches wrote:
> > On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
> > > 2: Create some comedi logging functions or macros like:
> > >    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
> > >    where "comedi:" is always prefixed and an
> > >    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > >    could be used.

> I would prefer the conversion of everything over to the dev_printk()
> versions instead of creating a new macro for every individual subsystem.
> That way you get the advantage of logging everything in the common
> format and the dynamic debug functionality as well.

What I posted has dynamic_debug.

+#elif defined(CONFIG_DYNAMIC_DEBUG)
+/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
+#define comedi_debug(fmt, ...)                         \
+       dynamic_pr_debug(fmt, ##__VA_ARGS__)

As far as I know, comedi doesn't always take a struct device *.
I believe it's only used when there's a DMA.

In struct comedi_device, there are two struct device *'s.

	struct device *class_dev;
...
	struct device *hw_dev;

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Greg KH]

On Thu, Jun 17, 2010 at 04:15:58PM -0700, Joe Perches wrote:
> On Thu, 2010-06-17 at 15:51 -0700, Greg KH wrote:
> > On Sat, Jun 12, 2010 at 10:30:50PM -0700, Joe Perches wrote:
> > > On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
> > > > 2: Create some comedi logging functions or macros like:
> > > >    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
> > > >    where "comedi:" is always prefixed and an
> > > >    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > >    could be used.
> 
> > I would prefer the conversion of everything over to the dev_printk()
> > versions instead of creating a new macro for every individual subsystem.
> > That way you get the advantage of logging everything in the common
> > format and the dynamic debug functionality as well.
> 
> What I posted has dynamic_debug.
> 
> +#elif defined(CONFIG_DYNAMIC_DEBUG)
> +/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> +#define comedi_debug(fmt, ...)                         \
> +       dynamic_pr_debug(fmt, ##__VA_ARGS__)
> 
> As far as I know, comedi doesn't always take a struct device *.
> I believe it's only used when there's a DMA.

No, there's a struct device down in the device almost always.

> In struct comedi_device, there are two struct device *'s.
> 
> 	struct device *class_dev;
> ...
> 	struct device *hw_dev;

hw_dev is what we want to use.

thanks,

greg k-h

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Joe Perches]

On Thu, 2010-06-17 at 16:28 -0700, Greg KH wrote:
> On Thu, Jun 17, 2010 at 04:15:58PM -0700, Joe Perches wrote:
> > On Thu, 2010-06-17 at 15:51 -0700, Greg KH wrote:
> > > On Sat, Jun 12, 2010 at 10:30:50PM -0700, Joe Perches wrote:
> > > > On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
> > > > > 2: Create some comedi logging functions or macros like:
> > > > >    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
> > > > >    where "comedi:" is always prefixed and an
> > > > >    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > > >    could be used.
> > 
> > > I would prefer the conversion of everything over to the dev_printk()
> > > versions instead of creating a new macro for every individual subsystem.
> > > That way you get the advantage of logging everything in the common
> > > format and the dynamic debug functionality as well.
> > 
> > What I posted has dynamic_debug.
> > 
> > +#elif defined(CONFIG_DYNAMIC_DEBUG)
> > +/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
> > +#define comedi_debug(fmt, ...)                         \
> > +       dynamic_pr_debug(fmt, ##__VA_ARGS__)
> > 
> > As far as I know, comedi doesn't always take a struct device *.
> > I believe it's only used when there's a DMA.
> 
> No, there's a struct device down in the device almost always.
> 
> > In struct comedi_device, there are two struct device *'s.
> > 
> > 	struct device *class_dev;
> > ...
> > 	struct device *hw_dev;
> 
> hw_dev is what we want to use.

Perhaps Ian or Frank might clarify if
that's reasonable.  It doesn't look like
it to me.

Look at comedi_set_hw_dev.
See how often it's used?

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Ian Abbott]

On 18/06/10 00:47, Joe Perches wrote:
> On Thu, 2010-06-17 at 16:28 -0700, Greg KH wrote:
>> On Thu, Jun 17, 2010 at 04:15:58PM -0700, Joe Perches wrote:
>>> On Thu, 2010-06-17 at 15:51 -0700, Greg KH wrote:
>>>> On Sat, Jun 12, 2010 at 10:30:50PM -0700, Joe Perches wrote:
>>>>> On Sat, 2010-06-12 at 22:07 -0700, Joe Perches wrote:
>>>>>> 2: Create some comedi logging functions or macros like:
>>>>>>    	comedi_<level>(fmt, arg...) (ie: comedi_info, comedi_err, etc)
>>>>>>    where "comedi:" is always prefixed and an
>>>>>>    optional #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>>>>    could be used.
>>>
>>>> I would prefer the conversion of everything over to the dev_printk()
>>>> versions instead of creating a new macro for every individual subsystem.
>>>> That way you get the advantage of logging everything in the common
>>>> format and the dynamic debug functionality as well.
>>>
>>> What I posted has dynamic_debug.
>>>
>>> +#elif defined(CONFIG_DYNAMIC_DEBUG)
>>> +/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
>>> +#define comedi_debug(fmt, ...)                         \
>>> +       dynamic_pr_debug(fmt, ##__VA_ARGS__)
>>>
>>> As far as I know, comedi doesn't always take a struct device *.
>>> I believe it's only used when there's a DMA.
>>
>> No, there's a struct device down in the device almost always.
>>
>>> In struct comedi_device, there are two struct device *'s.
>>>
>>> 	struct device *class_dev;
>>> ...
>>> 	struct device *hw_dev;
>>
>> hw_dev is what we want to use.
> 
> Perhaps Ian or Frank might clarify if
> that's reasonable.  It doesn't look like
> it to me.
> 
> Look at comedi_set_hw_dev.
> See how often it's used?

It's only set by a few drivers currently.  Perhaps it should be set by
comedi_alloc_board_minor() using the 'hardware_device' parameter.
However, in the case of "legacy" comedi devices (the ones not configured
automatically via comedi_auto_config()), that parameter will be NULL.

On a side note, if comedi_alloc_board_minor() were changed to call
comedi_set_hw_dev(), then the driver "_attach" routines could be changed
to check that they really are attaching to the correct hardware device
rather than relying on the struct comedi_devconfig options array values
(which aren't set anyway for auto-configured USB devices).
comedi_set_hw_dev() also ought to return early if the new hw_dev value
is the same as the previous value.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging:comedi: Fixed coding convention issues.

[Greg KH]

On Mon, Jun 14, 2010 at 09:34:15AM +0300, Henri Häkkinen wrote:
> Cleaned up and fixed coding convention issues as reporteed by
> checkpatch.pl tool on the file `drivers.c'. Added logging macros
> to `comedidev.h'.  Replaced "BUG:" printk functions calls with
> BUG_ON macro.
> 
> Signed-off-by: Henri Häkkinen <henuxd@gmail.com>
> ---
>  drivers/staging/comedi/comedidev.h |   54 +++++++++++++++-
>  drivers/staging/comedi/drivers.c   |  118 +++++++++++++++--------------------
>  2 files changed, 102 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
> index 4eb2b77..5c78564 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -43,11 +43,59 @@
>  
>  #include "comedi.h"
>  
> -#define DPRINTK(format, args...)	do {		\
> -	if (comedi_debug)				\
> -		printk(KERN_DEBUG "comedi: " format , ## args);	\
> +#define comedi_printk(level, fmt, args...) \
> +	printk(level "comedi: " pr_fmt(fmt), ##args)
> +
> +#define DPRINTK(format, args...) \
> +do { \
> +	if (comedi_debug) \
> +		comedi_printk(KERN_DEBUG, fmt, ##args); \
>  } while (0)
>  
> +#define comedi_emerg(fmt, ...) \
> +	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)

I'd much rather you use the real dev_printk() versions of this instead
(dev_warn, dev_err, etc.) instead of creating a new macro.

thanks,

greg k-h

[PATCH] staging:comedi: Fixed coding convention issues.

[Henri Häkkinen]

Cleaned up and fixed coding convention issues as reporteed by
checkpatch.pl tool on the file `drivers.c'. Added logging macros
to `comedidev.h'.  Replaced "BUG:" printk functions calls with
BUG_ON macro.

Signed-off-by: Henri Häkkinen <henuxd@gmail.com>
---
 drivers/staging/comedi/comedidev.h |   54 +++++++++++++++-
 drivers/staging/comedi/drivers.c   |  118 +++++++++++++++--------------------
 2 files changed, 102 insertions(+), 70 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 4eb2b77..5c78564 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -43,11 +43,59 @@
 
 #include "comedi.h"
 
-#define DPRINTK(format, args...)	do {		\
-	if (comedi_debug)				\
-		printk(KERN_DEBUG "comedi: " format , ## args);	\
+#define comedi_printk(level, fmt, args...) \
+	printk(level "comedi: " pr_fmt(fmt), ##args)
+
+#define DPRINTK(format, args...) \
+do { \
+	if (comedi_debug) \
+		comedi_printk(KERN_DEBUG, fmt, ##args); \
 } while (0)
 
+#define comedi_emerg(fmt, ...) \
+	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)
+#define comedi_alert(fmt, ...) \
+	comedi_printk(KERN_ALERT, fmt, ##__VA_ARGS__)
+#define comedi_crit(fmt, ...) \
+	comedi_printk(KERN_CRIT, fmt, ##__VA_ARGS__)
+#define comedi_err(fmt, ...) \
+	comedi_printk(KERN_ERR, fmt, ##__VA_ARGS__)
+#define comedi_warn(fmt, ...) \
+	comedi_printk(KERN_WARN, fmt, ##__VA_ARGS__)
+#define comedi_notice(fmt, ...) \
+	comedi_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
+#define comedi_info(fmt, ...) \
+	comedi_printk(KERN_INFO, fmt, ##__VA_ARGS__)
+
+/* comedi_devel() should produce zero code unless DEBUG is defined */
+#ifdef DEBUG
+#define comedi_devel(fmt, ...) \
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#else
+#define comedi_devel(fmt, ...) \
+({ \
+	if (0) \
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS_); \
+	0; \
+})
+#endif
+
+#if defined(DEBUG)
+#define comedi_debug(fmt, ..) \
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#elif defined(CONFIG_DYNAMIC_DEBUG)
+/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
+#define comedi_debug(fmt, ...) \
+	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#else
+#define comedi_debug(fmt, ...) \
+({ \
+	if (0) \
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__); \
+	0; \
+})
+#endif
+
 #define COMEDI_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
 #define COMEDI_VERSION_CODE COMEDI_VERSION(COMEDI_MAJORVERSION, \
 	COMEDI_MINORVERSION, COMEDI_MICROVERSION)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 4a29ed7..410e83e 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -92,11 +92,8 @@ static void cleanup_device(struct comedi_device *dev)
 static void __comedi_device_detach(struct comedi_device *dev)
 {
 	dev->attached = 0;
-	if (dev->driver)
-		dev->driver->detach(dev);
-	else
-		printk(KERN_WARNING
-		       "BUG: dev->driver=NULL in comedi_device_detach()\n");
+	BUG_ON(!dev->driver);
+	dev->driver->detach(dev);
 	cleanup_device(dev);
 }
 
@@ -117,8 +114,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	for (driv = comedi_drivers; driv; driv = driv->next) {
 		if (!try_module_get(driv->module)) {
-			printk
-			    (KERN_INFO "comedi: failed to increment module count, skipping\n");
+			comedi_info("failed to increment module count, skipping\n");
 			continue;
 		}
 		if (driv->num_names) {
@@ -149,8 +145,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	/*  report valid board names before returning error */
 	for (driv = comedi_drivers; driv; driv = driv->next) {
 		if (!try_module_get(driv->module)) {
-			printk(KERN_INFO
-			       "comedi: failed to increment module count\n");
+			comedi_info("failed to increment module count\n");
 			continue;
 		}
 		comedi_report_boards(driv);
@@ -167,11 +162,7 @@ attached:
 		return ret;
 	}
 
-	if (!dev->board_name) {
-		printk(KERN_WARNING "BUG: dev->board_name=<%p>\n",
-		       dev->board_name);
-		dev->board_name = "BUG";
-	}
+	BUG_ON(!dev->board_name);
 	smp_wmb();
 	dev->attached = 1;
 
@@ -204,10 +195,7 @@ int comedi_driver_unregister(struct comedi_driver *driver)
 
 		mutex_lock(&dev->mutex);
 		if (dev->attached && dev->driver == driver) {
-			if (dev->use_count)
-				printk
-				    (KERN_WARNING "BUG! detaching device with use_count=%d\n",
-				     dev->use_count);
+			BUG_ON(dev->use_count);
 			comedi_device_detach(dev);
 		}
 		mutex_unlock(&dev->mutex);
@@ -252,8 +240,7 @@ static int postconfig(struct comedi_device *dev)
 			async =
 			    kzalloc(sizeof(struct comedi_async), GFP_KERNEL);
 			if (async == NULL) {
-				printk(KERN_INFO
-				       "failed to allocate async struct\n");
+				comedi_info("failed to allocate async struct\n");
 				return -ENOMEM;
 			}
 			init_waitqueue_head(&async->wait_head);
@@ -268,7 +255,7 @@ static int postconfig(struct comedi_device *dev)
 			async->prealloc_buf = NULL;
 			async->prealloc_bufsz = 0;
 			if (comedi_buf_alloc(dev, s, DEFAULT_BUF_SIZE) < 0) {
-				printk(KERN_INFO "Buffer allocation failed\n");
+				comedi_info("Buffer allocation failed\n");
 				return -ENOMEM;
 			}
 			if (s->buf_change) {
@@ -325,17 +312,17 @@ static void comedi_report_boards(struct comedi_driver *driv)
 	unsigned int i;
 	const char *const *name_ptr;
 
-	printk(KERN_INFO "comedi: valid board names for %s driver are:\n",
-	       driv->driver_name);
+	comedi_info("valid board names for %s driver are:\n",
+		    driv->driver_name);
 
 	name_ptr = driv->board_name;
 	for (i = 0; i < driv->num_names; i++) {
-		printk(KERN_INFO " %s\n", *name_ptr);
+		comedi_info("%s\n", *name_ptr);
 		name_ptr = (const char **)((char *)name_ptr + driv->offset);
 	}
 
 	if (driv->num_names == 0)
-		printk(KERN_INFO " %s\n", driv->driver_name);
+		comedi_info(" %s\n", driv->driver_name);
 }
 
 static int poll_invalid(struct comedi_device *dev, struct comedi_subdevice *s)
@@ -441,21 +428,19 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 	if (async->buf_page_list) {
 		unsigned i;
 		for (i = 0; i < async->n_buf_pages; ++i) {
-			if (async->buf_page_list[i].virt_addr) {
-				clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+			void *virt_addr = async->buf_page_list[i].virt_addr;
+			dma_addr_t dma_addr = async->buf_page_list[i].dma_addr;
+
+			if (virt_addr) {
+				struct page *page = virt_to_page(virt_addr);
+				clear_bit(PG_reserved, &page->flags);
 				if (s->async_dma_dir != DMA_NONE) {
 					dma_free_coherent(dev->hw_dev,
 							  PAGE_SIZE,
-							  async->
-							  buf_page_list
-							  [i].virt_addr,
-							  async->
-							  buf_page_list
-							  [i].dma_addr);
+							  virt_addr,
+							  dma_addr);
 				} else {
-					free_page((unsigned long)
-						  async->buf_page_list[i].
-						  virt_addr);
+					free_page((unsigned long) virt_addr);
 				}
 			}
 		}
@@ -478,26 +463,29 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 		}
 		if (pages) {
 			for (i = 0; i < n_pages; i++) {
+				struct comedi_buf_page *buf_page =
+					&async->buf_page_list[i];
+				dma_addr_t dma_addr = buf_page->dma_addr;
+				struct page *page;
+
 				if (s->async_dma_dir != DMA_NONE) {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    dma_alloc_coherent(dev->hw_dev,
 							       PAGE_SIZE,
-							       &async->
-							       buf_page_list
-							       [i].dma_addr,
+							       &dma_addr,
 							       GFP_KERNEL |
 							       __GFP_COMP);
 				} else {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    (void *)
 					    get_zeroed_page(GFP_KERNEL);
 				}
-				if (async->buf_page_list[i].virt_addr == NULL)
+				if (buf_page->virt_addr == NULL)
 					break;
 
-				set_bit(PG_reserved,
-					&(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
-				pages[i] = virt_to_page(async->buf_page_list[i].virt_addr);
+				page = virt_to_page(buf_page->virt_addr);
+				set_bit(PG_reserved, &page->flags);
+				pages[i] = page;
 			}
 		}
 		if (i == n_pages) {
@@ -510,24 +498,27 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 			/* Some allocation failed above. */
 			if (async->buf_page_list) {
 				for (i = 0; i < n_pages; i++) {
-					if (async->buf_page_list[i].virt_addr ==
-					    NULL) {
+					struct comedi_buf_page *buf_page =
+						&async->buf_page_list[i];
+					void *virt_addr =
+						buf_page->virt_addr;
+					dma_addr_t dma_addr =
+						buf_page->dma_addr;
+					struct page *page =
+						virt_to_page(virt_addr);
+
+					if (virt_addr == NULL)
 						break;
-					}
-					clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+
+					clear_bit(PG_reserved, &page->flags);
 					if (s->async_dma_dir != DMA_NONE) {
 						dma_free_coherent(dev->hw_dev,
 								  PAGE_SIZE,
-								  async->
-								  buf_page_list
-								  [i].virt_addr,
-								  async->
-								  buf_page_list
-								  [i].dma_addr);
+								  virt_addr,
+								  dma_addr);
 					} else {
 						free_page((unsigned long)
-							  async->buf_page_list
-							  [i].virt_addr);
+							  virt_addr);
 					}
 				}
 				vfree(async->buf_page_list);
@@ -562,12 +553,7 @@ static unsigned int comedi_buf_munge(struct comedi_async *async,
 		int block_size;
 
 		block_size = num_bytes - count;
-		if (block_size < 0) {
-			printk(KERN_WARNING
-			       "%s: %s: bug! block_size is negative\n",
-			       __FILE__, __func__);
-			break;
-		}
+		BUG_ON(block_size < 0);
 		if ((int)(async->munge_ptr + block_size -
 			  async->prealloc_bufsz) > 0)
 			block_size = async->prealloc_bufsz - async->munge_ptr;
@@ -646,8 +632,7 @@ unsigned comedi_buf_write_free(struct comedi_async *async, unsigned int nbytes)
 {
 	if ((int)(async->buf_write_count + nbytes -
 		  async->buf_write_alloc_count) > 0) {
-		printk
-		    (KERN_INFO "comedi: attempted to write-free more bytes than have been write-allocated.\n");
+		comedi_info("attempted to write-free more bytes than have been write-allocated.\n");
 		nbytes = async->buf_write_alloc_count - async->buf_write_count;
 	}
 	async->buf_write_count += nbytes;
@@ -683,8 +668,7 @@ unsigned comedi_buf_read_free(struct comedi_async *async, unsigned int nbytes)
 	smp_mb();
 	if ((int)(async->buf_read_count + nbytes -
 		  async->buf_read_alloc_count) > 0) {
-		printk(KERN_INFO
-		       "comedi: attempted to read-free more bytes than have been read-allocated.\n");
+		comedi_info("attempted to read-free more bytes than have been read-allocated.\n");
 		nbytes = async->buf_read_alloc_count - async->buf_read_count;
 	}
 	async->buf_read_count += nbytes;
-- 
1.7.1

[PATCH] staging:comedi: Fixed coding convention issues.

[Henri Häkkinen]

Cleaned up and fixed coding convention issues as reporteed by
checkpatch.pl tool on the file `drivers.c'. Added logging macros
to `comedidev.h'.

Signed-off-by: Henri Häkkinen <henuxd@gmail.com>
---
 drivers/staging/comedi/comedidev.h |   54 ++++++++++++++++++++-
 drivers/staging/comedi/drivers.c   |   89 ++++++++++++++++++------------------
 2 files changed, 95 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
index 4eb2b77..5c78564 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -43,11 +43,59 @@
 
 #include "comedi.h"
 
-#define DPRINTK(format, args...)	do {		\
-	if (comedi_debug)				\
-		printk(KERN_DEBUG "comedi: " format , ## args);	\
+#define comedi_printk(level, fmt, args...) \
+	printk(level "comedi: " pr_fmt(fmt), ##args)
+
+#define DPRINTK(format, args...) \
+do { \
+	if (comedi_debug) \
+		comedi_printk(KERN_DEBUG, fmt, ##args); \
 } while (0)
 
+#define comedi_emerg(fmt, ...) \
+	comedi_printk(KERN_EMERG, fmt, ##__VA_ARGS__)
+#define comedi_alert(fmt, ...) \
+	comedi_printk(KERN_ALERT, fmt, ##__VA_ARGS__)
+#define comedi_crit(fmt, ...) \
+	comedi_printk(KERN_CRIT, fmt, ##__VA_ARGS__)
+#define comedi_err(fmt, ...) \
+	comedi_printk(KERN_ERR, fmt, ##__VA_ARGS__)
+#define comedi_warn(fmt, ...) \
+	comedi_printk(KERN_WARN, fmt, ##__VA_ARGS__)
+#define comedi_notice(fmt, ...) \
+	comedi_printk(KERN_NOTICE, fmt, ##__VA_ARGS__)
+#define comedi_info(fmt, ...) \
+	comedi_printk(KERN_INFO, fmt, ##__VA_ARGS__)
+
+/* comedi_devel() should produce zero code unless DEBUG is defined */
+#ifdef DEBUG
+#define comedi_devel(fmt, ...) \
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#else
+#define comedi_devel(fmt, ...) \
+({ \
+	if (0) \
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS_); \
+	0; \
+})
+#endif
+
+#if defined(DEBUG)
+#define comedi_debug(fmt, ..) \
+	comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__)
+#elif defined(CONFIG_DYNAMIC_DEBUG)
+/* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
+#define comedi_debug(fmt, ...) \
+	dynamic_pr_debug(fmt, ##__VA_ARGS__)
+#else
+#define comedi_debug(fmt, ...) \
+({ \
+	if (0) \
+		comedi_printk(KERN_DEBUG, fmt, ##__VA_ARGS__); \
+	0; \
+})
+#endif
+
 #define COMEDI_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))
 #define COMEDI_VERSION_CODE COMEDI_VERSION(COMEDI_MAJORVERSION, \
 	COMEDI_MINORVERSION, COMEDI_MICROVERSION)
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 4a29ed7..a649e7f 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -117,8 +117,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	for (driv = comedi_drivers; driv; driv = driv->next) {
 		if (!try_module_get(driv->module)) {
-			printk
-			    (KERN_INFO "comedi: failed to increment module count, skipping\n");
+			comedi_info("failed to increment module count, skipping\n");
 			continue;
 		}
 		if (driv->num_names) {
@@ -149,8 +148,7 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	/*  report valid board names before returning error */
 	for (driv = comedi_drivers; driv; driv = driv->next) {
 		if (!try_module_get(driv->module)) {
-			printk(KERN_INFO
-			       "comedi: failed to increment module count\n");
+			comedi_info("failed to increment module count\n");
 			continue;
 		}
 		comedi_report_boards(driv);
@@ -205,9 +203,8 @@ int comedi_driver_unregister(struct comedi_driver *driver)
 		mutex_lock(&dev->mutex);
 		if (dev->attached && dev->driver == driver) {
 			if (dev->use_count)
-				printk
-				    (KERN_WARNING "BUG! detaching device with use_count=%d\n",
-				     dev->use_count);
+				printk(KERN_WARNING
+				       "BUG! detaching device with use_count=%d\n", dev->use_count);
 			comedi_device_detach(dev);
 		}
 		mutex_unlock(&dev->mutex);
@@ -325,8 +322,8 @@ static void comedi_report_boards(struct comedi_driver *driv)
 	unsigned int i;
 	const char *const *name_ptr;
 
-	printk(KERN_INFO "comedi: valid board names for %s driver are:\n",
-	       driv->driver_name);
+	comedi_info("valid board names for %s driver are:\n",
+		    driv->driver_name);
 
 	name_ptr = driv->board_name;
 	for (i = 0; i < driv->num_names; i++) {
@@ -441,21 +438,19 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 	if (async->buf_page_list) {
 		unsigned i;
 		for (i = 0; i < async->n_buf_pages; ++i) {
-			if (async->buf_page_list[i].virt_addr) {
-				clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+			void *virt_addr = async->buf_page_list[i].virt_addr;
+			dma_addr_t dma_addr = async->buf_page_list[i].dma_addr;
+
+			if (virt_addr) {
+				struct page *page = virt_to_page(virt_addr);
+				clear_bit(PG_reserved, &page->flags);
 				if (s->async_dma_dir != DMA_NONE) {
 					dma_free_coherent(dev->hw_dev,
 							  PAGE_SIZE,
-							  async->
-							  buf_page_list
-							  [i].virt_addr,
-							  async->
-							  buf_page_list
-							  [i].dma_addr);
+							  virt_addr,
+							  dma_addr);
 				} else {
-					free_page((unsigned long)
-						  async->buf_page_list[i].
-						  virt_addr);
+					free_page((unsigned long) virt_addr);
 				}
 			}
 		}
@@ -478,26 +473,29 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 		}
 		if (pages) {
 			for (i = 0; i < n_pages; i++) {
+				struct comedi_buf_page *buf_page =
+					&async->buf_page_list[i];
+				dma_addr_t dma_addr = buf_page->dma_addr;
+				struct page *page;
+
 				if (s->async_dma_dir != DMA_NONE) {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    dma_alloc_coherent(dev->hw_dev,
 							       PAGE_SIZE,
-							       &async->
-							       buf_page_list
-							       [i].dma_addr,
+							       &dma_addr,
 							       GFP_KERNEL |
 							       __GFP_COMP);
 				} else {
-					async->buf_page_list[i].virt_addr =
+					buf_page->virt_addr =
 					    (void *)
 					    get_zeroed_page(GFP_KERNEL);
 				}
-				if (async->buf_page_list[i].virt_addr == NULL)
+				if (buf_page->virt_addr == NULL)
 					break;
 
-				set_bit(PG_reserved,
-					&(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
-				pages[i] = virt_to_page(async->buf_page_list[i].virt_addr);
+				page = virt_to_page(buf_page->virt_addr);
+				set_bit(PG_reserved, &page->flags);
+				pages[i] = page;
 			}
 		}
 		if (i == n_pages) {
@@ -510,24 +508,27 @@ int comedi_buf_alloc(struct comedi_device *dev, struct comedi_subdevice *s,
 			/* Some allocation failed above. */
 			if (async->buf_page_list) {
 				for (i = 0; i < n_pages; i++) {
-					if (async->buf_page_list[i].virt_addr ==
-					    NULL) {
+					struct comedi_buf_page *buf_page =
+						&async->buf_page_list[i];
+					void *virt_addr =
+						buf_page->virt_addr;
+					dma_addr_t dma_addr =
+						buf_page->dma_addr;
+					struct page *page =
+						virt_to_page(virt_addr);
+
+					if (virt_addr == NULL)
 						break;
-					}
-					clear_bit(PG_reserved, &(virt_to_page(async->buf_page_list[i].virt_addr)->flags));
+
+					clear_bit(PG_reserved, &page->flags);
 					if (s->async_dma_dir != DMA_NONE) {
 						dma_free_coherent(dev->hw_dev,
 								  PAGE_SIZE,
-								  async->
-								  buf_page_list
-								  [i].virt_addr,
-								  async->
-								  buf_page_list
-								  [i].dma_addr);
+								  virt_addr,
+								  dma_addr);
 					} else {
 						free_page((unsigned long)
-							  async->buf_page_list
-							  [i].virt_addr);
+							  virt_addr);
 					}
 				}
 				vfree(async->buf_page_list);
@@ -646,8 +647,7 @@ unsigned comedi_buf_write_free(struct comedi_async *async, unsigned int nbytes)
 {
 	if ((int)(async->buf_write_count + nbytes -
 		  async->buf_write_alloc_count) > 0) {
-		printk
-		    (KERN_INFO "comedi: attempted to write-free more bytes than have been write-allocated.\n");
+		comedi_info("attempted to write-free more bytes than have been write-allocated.\n");
 		nbytes = async->buf_write_alloc_count - async->buf_write_count;
 	}
 	async->buf_write_count += nbytes;
@@ -683,8 +683,7 @@ unsigned comedi_buf_read_free(struct comedi_async *async, unsigned int nbytes)
 	smp_mb();
 	if ((int)(async->buf_read_count + nbytes -
 		  async->buf_read_alloc_count) > 0) {
-		printk(KERN_INFO
-		       "comedi: attempted to read-free more bytes than have been read-allocated.\n");
+		comedi_info("attempted to read-free more bytes than have been read-allocated.\n");
 		nbytes = async->buf_read_alloc_count - async->buf_read_count;
 	}
 	async->buf_read_count += nbytes;
-- 
1.7.1