Re: [PATCH] Staging: rar: fixed up rar_driver.{h,c}

[Greg KH <gregkh@suse.de>]

On Fri, Feb 05, 2010 at 01:32:52AM +0100, Krzysztof Halasa wrote:
> Josh Holland <jrh@joshh.co.uk> writes:
> 
> > This is a patch to the rar_driver.c and rar_driver.h files to remove
> > style issues found by the checkpatch.pl script.
> >
> > +++ b/drivers/staging/rar/rar_driver.c
> > @@ -68,7 +68,8 @@ static void __exit rar_exit_handler(void);
> >  /*
> >    function that is activated on the successfull probe of the RAR device
> >  */
> > -static int __devinit rar_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> > +static int __devinit rar_probe(struct pci_dev *pdev,
> > +	const struct pci_device_id *ent);
> 
> It's agreed such changes make it worse. The 80-column "ERROR" should be
> ignored, and it will be removed from checkpatch.
> 
> > -	printk(KERN_WARNING "rar- result from send ctl register is %x\n"
> > -	  ,result);
> > +	printk(KERN_WARNING "rar- result from send ctl register is %x\n",
> > +			result);
> 
> Also, here (and then) - I'd just make it a single line if you're changing
> it. I'd be far from "unwrapping" all code across the kernel, though
> (without otherwise changing the lines in question).

No, this is fine, no problem with this change.

> > +		if (memrar_get_rar_addr(pdev, (*i).low, &(rar_addr[n].low))
> > +			|| memrar_get_rar_addr(pdev, (*i).high,
> > +			   &(rar_addr[n].high))) {
> > +			result = -1;
> > +			break;
> > +		}
> 
> Isn't the following a bit more readable?
> 
> +		if (memrar_get_rar_addr(pdev, i->low, &rar_addr[n].low) ||
> +		    memrar_get_rar_addr(pdev, i->high, &rar_addr[n].high)) {
> +			result = -1;
> +			break;
> +		}

The latter is nicer, but it doesn't really matter :)

> It doesn't make sense to split the printk, at least every single output
> line printed shouldn't be broken into pieces (but perhaps one single
> line for the whole printk() is best).
> Also I like the post-increments (z++) more, but maybe it's just me.
> 
> > +		size_t z;
> > +		for (z = 0; z != MRST_NUM_RAR; ++z) {
> > +			printk(KERN_WARNING "rar - "
> > +			"BRAR[%Zd] physical address low\n"
> > +			"\tlow:  0x%08x\n"
> > +			"\thigh: 0x%08x\n",
> > +			z,
> > +			rar_addr[z].low,
> > +			rar_addr[z].high);
> 
> 
> > -#define DEBUG_PRINT_0(DEBUG_LEVEL , info) \
> > -do \
> > -{ \
> > -  if(DEBUG_LEVEL) \
> > -  { \
> > -    printk(KERN_WARNING info); \
> > -  } \
> > -}while(0)
> > +#define DEBUG_PRINT_0(DEBUG_LEVEL, info) \
> > +do { \
> > +	if (DEBUG_LEVEL) \
> > +		printk(KERN_WARNING info); \
> > +} while (0)
> 
> Also I think moving these backslashes to the right of the macro code is
> preferred, isn't it?


It doesn't matter all that much.

Overall this looks fine, I'll queue it up.

thanks,

greg k-h