On Mon, Dec 17, 2007 at 09:56:30PM +0100, Remy Bohmer wrote: > > > +#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() ? > > > /* > > > + * 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. > > > > > + 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. > > > > /* > > > - * 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. - To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
