Re: [PATCH/RFC] UART driver for PNX8550/8950 revised

["Vitaly Wool" <vitalywool@gmail.com>]

Hello Russell,

> serial_pnx8xxx.h just contains structure and register definitions for
> this driver - wouldn't it make more sense for it to be in drivers/serial
> along side this driver?

Well it's used in arch/mips/philips/... so I doubt it's a right thing
to move it to drivers/serial.
I'd rather put it into include/asm-mips/mach-pnx8550 but it's gonna be
shared between this and other similar SoCs (when/if they are submitted
into mainline) so I'm not sure about that either.

> > +
> > +#include <asm/io.h>
> > +#include <asm/irq.h>
> > +
> > +#include <uart.h>
>
> What is uart.h?  It isn't in this patch, neither is it part of mainline.
I guess it's include/asm-mips/arch-pnx8550/uart.h, but it is not
needed and will be removed in the next version :)


> Still not using read_status_mask here, as detailed in my previous review.
>
>                         status &= sport->port.read_status_mask;
>
> is what's missing.

Oh, got it now, thanks.

> > +     /*
> > +      * Disable all interrupts, port and break condition.
> > +      */
> > +     serial_out(sport, PNX8XXX_IEN, 0);
>
> This comment's not correct - where is the break condition disabled?
> I thought it might be in the next serial_out() but it seems to be
> missing from there as well?

I don't think you're right here - break condition is also disabled
unsetting the corresponding bit in  IEN register for this particular
UART.

> > +     if (termios->c_iflag & IGNBRK) {
> > +             sport->port.ignore_status_mask |=
> > +                     ISTAT_TO_SM(PNX8XXX_UART_INT_BREAK);
> > +             /*
> > +              * If we're ignoring parity and break indicators,
> > +              * ignore overruns too (for real raw support).
> > +              */
> > +             if (termios->c_iflag & IGNPAR)
> > +                     sport->port.ignore_status_mask |=
> > +                             ISTAT_TO_SM(PNX8XXX_UART_INT_RXOVRN);
> > +     }
>
> How about CREAD support?

I'll add it to the new version.

Vitaly