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

[Krzysztof Halasa <khc@pm.waw.pl>]

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).

> +		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;
+		}

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?

Just my 0.01$CURRENCY as usual :-)
-- 
Krzysztof Halasa