Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

Greg Kroah-Hartman
2014-09-08T23:17:35Z

On Thu, Aug 28, 2014 at 02:32:20PM +0200, Johannes Thumshirn wrote:
> 16z135 IP Core has changed so the driver needs to be updated to respect
> these changes. The following changes have been made:
>
> * Don't invert the 16z135 modem status register when reading.
> * Add module parameter to configure the (baud rate dependent) RX timeout.
> Character timeout in seconds = (timeout_reg * baud_reg * 4)/freq_reg.
> * Enable the handling of UART core's automatic flow control feature.
> When AFE is active disable generation of modem status IRQs.
> * Rework the handling of IRQs to be conform with newer FPGA versions and
> take precautions not to miss an interrupt because of the destructive read
> of the IIR register.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@men.de>
> ---
> drivers/tty/serial/men_z135_uart.c | 87 +++++++++++++++++++++++---------------
> 1 file changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
> index 30e9e60..d31b8a4 100644
> --- a/drivers/tty/serial/men_z135_uart.c
> +++ b/drivers/tty/serial/men_z135_uart.c
> @@ -34,12 +34,11 @@
> #define MEN_Z135_CONF_REG 0x808
> #define MEN_Z135_UART_FREQ 0x80c
> #define MEN_Z135_BAUD_REG 0x810
> -#define MENZ135_TIMEOUT 0x814
> +#define MEN_Z135_TIMEOUT 0x814
>
> #define MEN_Z135_MEM_SIZE 0x818
>
> -#define IS_IRQ(x) ((x) & 1)
> -#define IRQ_ID(x) (((x) >> 1) & 7)
> +#define IRQ_ID(x) ((x) & 0x1f)
>
> #define MEN_Z135_IER_RXCIEN BIT(0) /* RX Space IRQ */
> #define MEN_Z135_IER_TXCIEN BIT(1) /* TX Space IRQ */
> @@ -94,11 +93,11 @@
> #define MEN_Z135_LSR_TEXP BIT(6)
> #define MEN_Z135_LSR_RXFIFOERR BIT(7)
>
> -#define MEN_Z135_IRQ_ID_MST 0
> -#define MEN_Z135_IRQ_ID_TSA 1
> -#define MEN_Z135_IRQ_ID_RDA 2
> -#define MEN_Z135_IRQ_ID_RLS 3
> -#define MEN_Z135_IRQ_ID_CTI 6
> +#define MEN_Z135_IRQ_ID_RLS BIT(0)
> +#define MEN_Z135_IRQ_ID_RDA BIT(1)
> +#define MEN_Z135_IRQ_ID_CTI BIT(2)
> +#define MEN_Z135_IRQ_ID_TSA BIT(3)
> +#define MEN_Z135_IRQ_ID_MST BIT(4)
>
> #define LCR(x) (((x) >> MEN_Z135_LCR_SHIFT) & 0xff)
>
> @@ -118,6 +117,10 @@ static int align;
> module_param(align, int, S_IRUGO);
> MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
>
> +static int rx_timeout;
> +module_param(rx_timeout, uint, S_IRUGO);
> +MODULE_PARM_DESC(rx_timeout, "RX timeout");

Why add a module parameter? Who will know to set this? And to what
value?

And what about this driver working on old hardware, didn't you just
break this?

thanks,

greg k-h