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