On Mon, 17 Dec 2007 23:49:32 +0000
Russell King - ARM Linux [off-list ref] wrote:

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

quoted

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() ?

Something like this perhaps?

#define at_readl(port, off)	__raw_readl((port)->membase + (off))
#define at_writel(v, port, off)	__raw_writel(v, (port)->membase + (off))

#define UART_PUT_CR(port, v)	at_writel(v, port, ATMEL_US_CR)
#define UART_PUT_MR(port, v)	at_writel(v, port, ATMEL_US_MR)
#define UART_PUT_IER(port, v)	at_writel(v, port, ATMEL_US_IER)
#define UART_PUT_IDR(port, v)	at_writel(v, port, ATMEL_US_IDR)
#define UART_PUT_CHAR(port, v)	at_writel(v, port, ATMEL_US_THR)
#define UART_PUT_BRGR(port, v)	at_writel(v, port, ATMEL_US_BRGR)
#define UART_PUT_RTOR(port, v)	at_writel(v, port, ATMEL_US_RTOR)

That said, I wonder if it may actually be nicer to just use
at_writel()/at_readl() throughout the driver...but that's for a later
cleanup.

quoted

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.

I can do it in a separate patch.

quoted

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.

Yes, the difference was wiped out after a few times back and forth. It
was visible (or...not actually visible, but you know how to detect
these things) in the first couple of e-mails.

Haavard