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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <acd2a5930608190234y4b4bee8dqfc17d109f86d4318@mail.gmail.com>
Date:	Sat, 19 Aug 2006 13:34:26 +0400
From:	"Vitaly Wool" <vitalywool@...il.com>
To:	"Vitaly Wool" <vitalywool@...il.com>, jean-paul.saman@...lips.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC] UART driver for PNX8550/8950 revised

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/