Re: [PATCH] RFC: WIP: sc16is7xx [v0.4]

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

> +#define DRV_NAME	"sc16is7xx"
> +#define DRV_VERSION	"0.4"
> +#define SC16IS7XX_MAJOR	204
> +#define SC16IS7XX_MINOR	8

Use dynamic minors for any new device

> +static inline u8 sc16is7xx_read(struct uart_port *port, u8 reg)

I wouldn't inline this - the compiler will figure it out for you and
you've got a fair bit code there.


> +static void sc16is7xx_handle_rx(struct uart_port *port)

This gets called from handle_events which gets called from the ist
handler with IRQs disabled. I'm surprised it works at all but you
definitely don't want to be doing all this work with interrupts blocked.
That may be part of your problem.

> +static void sc16is7xx_handle_tx(struct uart_port *port)

Ditto


> +static void sc16is7xx_set_termios(struct uart_port *port,
> +				  struct ktermios *termios, struct ktermios *old)
> +{
> +	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
> +	unsigned long flags;
> +	u8 cval;
> +	u8 fcr;
> +	int baud;
> +
> +	spin_lock_irqsave(&s->lock, flags);
> +
> +	/* Mask termios capabilities we don't support */
> +	termios->c_cflag &= ~CMSPAR;
> +
> +	/* Disable RX & TX, reset break condition, status and FIFOs */
> +	fcr = sc16is7xx_read(port, UART_FCR);
> +	fcr |= UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> +	fcr &= ~UART_FCR_ENABLE_FIFO;
> +	sc16is7xx_write(port, UART_FCR, fcr);
> +
> +	/* Word size */
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS5:
> +		cval = UART_LCR_WLEN5;
> +		break;
> +	case CS6:
> +		cval = UART_LCR_WLEN6;
> +		break;
> +	case CS7:
> +		cval = UART_LCR_WLEN7;
> +		break;
> +	case CS8:
> +	default:
> +		cval = UART_LCR_WLEN8;

(for the unknown case you also need to do
		termios->c_cflag &= ~CSIZE;
		termios->c_cflag |= CS8;

so the caller gets informed they didn't get their requested feature

Likewise btw for unsupported features. In this case CMSPAR appears not to
be supported ?)

Reading drivers/tty/serial/max310x.c may be helpful with regards to the
locking. It is SPI based so also has to defer some of the processing from
the IRQ handler. The threaded IRQ support we now have may also help you.

Alan