Re: [PATCH V3] serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers

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

> @@ -2325,10 +2323,19 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>  		if ((baud < 2400 && !up->dma) || fifo_bug) {
>  			fcr &= ~UART_FCR_TRIGGER_MASK;
>  			fcr |= UART_FCR_TRIGGER_1;
> +			/* Don't use user setting RX trigger */
> +			up->fcr = 0;

This breaks

	set fcr via sysfs
	set baud rate below 2400
	set baud rate higher

If baud < 2400 and the user has set a value then probably we should honour
it. If fifo_bug is set then we should never honour it (and should perhaps
eventually error it in the sysfs set).


> +static unsigned char convert_fcr2val(struct uart_8250_port *up,
> +				     unsigned char fcr)
> +{
> +	unsigned char val = 0, trig_raw = fcr & UART_FCR_TRIGGER_MASK;
> +
> +	switch (up->port.type) {
> +	case PORT_16550A:
> +		if (trig_raw == UART_FCR_R_TRIG_00)
> +			val = 1;
> +		else if (trig_raw == UART_FCR_R_TRIG_01)
> +			val = 4;
> +		else if (trig_raw == UART_FCR_R_TRIG_10)
> +			val = 8;
> +		else if (trig_raw == UART_FCR_R_TRIG_11)
> +			val = 14;
> +		break;

Surely the default case should be returning 1 not 0 ?


> +static int convert_val2rxtrig(struct uart_8250_port *up, unsigned char val)
> +{
> +	switch (up->port.type) {
> +	case PORT_16550A:
> +		if (val == 1)
> +			return UART_FCR_R_TRIG_00;
> +		else if (val == 4)
> +			return UART_FCR_R_TRIG_01;
> +		else if (val == 8)
> +			return UART_FCR_R_TRIG_10;
> +		else if (val == 14)
> +			return UART_FCR_R_TRIG_11;

What happens if you specify a meaningless value. Doing exact matching
means that you have to know the hardware exactly. How about

	if (val < 4)
		return UART_FCR_R_TRIG_00;
	else if (val < 8)
		return UART_FCR_R_TRIG_01;
	else if (val < 14)
		return UART_FCR_R_TRIG_10;
	else
		return UART_FCR_R_TRIG_11;

so you get the nearest lower value that the hardware can provide ?

> +		break;
> +	default:
> +		pr_info("Not support RX-trigger setting for this serial %u\n",
> +			up->port.type);

That lets users spew into the logs. I think actually you just want

	default:
		return -EOPNOTSUPP;

or similar



Otherwise this looks good to me. The interface is sensible, it can be
expanded easily to other devices and device types, and the logic looks
right.

Alan