On Mon, Dec 17, 2007 at 09:56:30PM +0100, Remy Bohmer wrote:

quoted

quoted

+#define lread(port)          __raw_readl(port)
+#define lwrite(v, port)              __raw_writel(v, port)

Why is this necessary, and what does 'l' stand for?

There is a huge list of macros below these definitions. By doing it
this way, the macros still fit on 80 characters wide, while without
them, I had split up the macros over several lines, which does not
make it more readable. That's all.
'l' refers at the last letter of __raw_readl, and writel. Nothing special.

So why not keep to the Linux convention?  How about at_readl() and
at_writel() ?

quoted

quoted

 /*
+ * receive interrupt handler.
+ */
+static inline void
+atmel_handle_receive(struct uart_port *port, unsigned int pending)

Please drop "inline" here. The compiler will do it automatically if it
has only one caller, and if it at some point gets several callers, we
might not want to inline it after all.

Funny, This was the first thing that Andrew started complaining about.
He suggested to put an inline there which I had not. I already
mentioned that this was against the CodingStyle, but I also mentioned
that I did not wanted to start a fight about this :-)
So, to prevent a discussion, I added the inline...

There's two schools of thought - those who want to add 'inline' keywords
all over the place and those who don't.  It's quite correct that if a
static function will be inlined by the compiler as it sees fit.  It
_can_ be that the compiler will chose not to inline it and that may
result in better register allocation in the caller, resulting in overall
faster code.

quoted

quoted

+     while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
+             barrier();

Should probably use cpu_relax(), but it's probably out of scope for a
codingstyle cleanup patch (and I don't think it matters on AVR32 or
ARM.)

Agree.

Even though it doesn't matter at the moment, I rather like to think a
bit about the future.  If the kernel has a simple and cheap mechanism
there's no reason to avoid using it.

quoted

quoted

      /*
-      *      First, save IMR and then disable interrupts
+      *      First, save IMR and then disable interrupts
       */
      imr = UART_GET_IMR(port);       /* get interrupt mask */
      UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);

@@ -790,30 +828,32 @@ static void atmel_console_write(struct c
      uart_console_write(port, s, count, atmel_console_putchar);

      /*
-      *      Finally, wait for transmitter to become empty
-      *      and restore IMR
+      *      Finally, wait for transmitter to become empty
+      *      and restore IMR
       */

Looks like you're replacing TABs with spaces. Why?

????

I think someone's mailer might be messing with the patches.  The above
removed and added lines appear to be identical.