lkml.org 
[lkml]   [2007]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH]: Atmel Serial Console interrupt handler splitup
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.


\
 
 \ /
  Last update: 2007-12-18 00:55    [W:0.082 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site