Re: [PATCH v2 2/2] Staging: comedi: use inl() and outl() helper functions

[Dan Carpenter]

On Sun, Mar 02, 2014 at 08:52:33PM -0600, Chase Southwood wrote:
> Use the newly created helper functions to improve code readability and shorten
> several lines to under the character limit.
> 
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Signed-off-by: Chase Southwood <chase.southwood@xxxxxxxxx>
> ---
> 
> I've reviewed this as best as I can, but I know it's a bear to review.
> If there is some logical way that you'd like me to split this up into separate
> patches to make it easier to review, please let me know.
> 
> Also, Dan, as best as I can possibly tell, i_APCI1564_Reset() is absolutely
> buggy.  When making up this patch, I made the changes that seem correct (and
> somewhat obvious) from looking at the other addi-data drivers, the other
> functions in this file, and the hardware specs that I could find, but
> unfortunately I have no way to test the code.

You need to put this into the commit message...  You can't change how
the code works without at least mentioning it in the commit.

It would be easier to review these patches if you introduced the helpers
first and switched everything to use them.  Then in 2/2 you changed the
defines to the combined versions.

> @@ -333,81 +313,61 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
>  		devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
>  
>  		/* Disable the watchdog */
> -		outl(0x0,
> -			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
> -			APCI1564_TCW_PROG);
> +		outl_amcc(devpriv, 0x0,
> +				  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);


This isn't indented correctly.  It should be:

		outl_amcc(devpriv, 0x0,
			  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);

[tab][tab][tab][space][space]APCI1564_DIGITAL_OP_WATCHDOG...

The same thing in a couple other places as well.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/