Re: [PATCH] serial/arc-uart: Add new driver

[Alan Cox]

> +int __attribute__((weak)) running_on_iss;

Globals really ought to have sensible names, if they must exist at all.
Why is this needed ?

> +#define ARC_SERIAL_DEV_NAME	"ttyS"

ttyS is reserved by the 8250 driver. Use a different name

> /* ttySxx per Documentation/devices.txt */
> +#define ARC_SERIAL_MAJOR	4
> +#define ARC_SERIAL_MINOR	64

Use dynamic assignments

> +static void arc_serial_rx_chars(struct arc_uart_port *uart)
> +{
> +	struct tty_struct *tty = NULL;
> +	unsigned int status, ch, flg = 0;
> +
> +	tty = uart->port.state->port.tty;

Please get rid of the unneeded initialisers for things like tty - they
just hide bugs later.

For locking purposes you need to be doing

	tty = tty_port_tty_get(&uart->port.state->port);

	if (tty) {

		stuff

		tty_kref_put(tty);


so that the tty cannot vanish under you on a hangup. As you reference
it in several spots it might make sense to do that once if either
RXIENB/TXIENB is set and then pass it to the tx/rx handlers ?


> +static void arc_serial_break_ctl(struct uart_port *port, int
> break_state) +{
> +	/* ARC UART doesn't support sendind Break signal */

sending ..

> +static void arc_serial_set_ldisc(struct uart_port *port, int ld)
> +{
> +	/* this might need implementing for the touch driver */
> +}

Is this a left over comment or somehting else ?

> +static void
> +arc_serial_set_termios(struct uart_port *port, struct ktermios
> *termios,
> +		       struct ktermios *old)
> +{
> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> +	unsigned int baud, uartl, uarth, hw_val;
> +	unsigned long flags;
> +

Two things here. Firstly you want to write the actual baud back to the
termios (tty_termios_encode_baud_rate) unless B0 is set.

Secondly if you don't support hardware flow control, character size
setting etc then you need to set those bits back so the caller knows.
Two ways to do that. Either initialise the default termios for the tty
type to the correct values and use tty_termios_copy_hw() or set them in
the termios handler.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/