Re: [PATCH v2 18/25] tty: serial: samsung_tty: add s3c24xx_port_type

Krzysztof Kozlowski
2021-02-15T18:28:07Z

On Mon, Feb 15, 2021 at 09:17:06PM +0900, Hector Martin wrote:
> This decouples the TTY layer PORT_ types, which are exposed to
> userspace, from the driver-internal flag of what kind of port this is.
>
> This removes s3c24xx_serial_has_interrupt_mask, which was just checking
> for a specific type anyway, and adds the ucon_mask port info member to
> avoid having S3C2440 as a distinct type.

Please split setting the ucon_mask to separate patch. It's a nice
code simplification on its own.

>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
> drivers/tty/serial/samsung_tty.c | 131 ++++++++++++++++++-------------
> 1 file changed, 77 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 6b661f3ec1ae..21955be680a4 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -56,9 +56,15 @@
> /* flag to ignore all characters coming in */
> #define RXSTAT_DUMMY_READ (0x10000000)
>
> +enum s3c24xx_port_type {
> + TYPE_S3C24XX,
> + TYPE_S3C6400,
> +};
> +
> struct s3c24xx_uart_info {
> char *name;
> - unsigned int type;
> + enum s3c24xx_port_type type;
> + unsigned int port_type;
> unsigned int fifosize;
> unsigned long rx_fifomask;
> unsigned long rx_fifoshift;
> @@ -70,6 +76,7 @@ struct s3c24xx_uart_info {
> unsigned long num_clks;
> unsigned long clksel_mask;
> unsigned long clksel_shift;
> + unsigned long ucon_mask;
>
> /* uart port features */
>
> @@ -228,16 +235,6 @@ static int s3c24xx_serial_txempty_nofifo(struct uart_port *port)
> return rd_regl(port, S3C2410_UTRSTAT) & S3C2410_UTRSTAT_TXE;
> }
>
> -/*
> - * s3c64xx and later SoC's include the interrupt mask and status registers in
> - * the controller itself, unlike the s3c24xx SoC's which have these registers
> - * in the interrupt controller. Check if the port type is s3c64xx or higher.
> - */
> -static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)
> -{
> - return to_ourport(port)->info->type == PORT_S3C6400;
> -}
> -
> static void s3c24xx_serial_rx_enable(struct uart_port *port)
> {
> struct s3c24xx_uart_port *ourport = to_ourport(port);
> @@ -289,10 +286,14 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
> if (!ourport->tx_enabled)
> return;
>
> - if (s3c24xx_serial_has_interrupt_mask(port))
> + switch (ourport->info->type) {
> + case TYPE_S3C6400:
> s3c24xx_set_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);
> - else
> + break;
> + default:
> disable_irq_nosync(ourport->tx_irq);
> + break;
> + }
>
> if (dma && dma->tx_chan && ourport->tx_in_progress == S3C24XX_TX_DMA) {
> dmaengine_pause(dma->tx_chan);
> @@ -353,10 +354,14 @@ static void enable_tx_dma(struct s3c24xx_uart_port *ourport)
> u32 ucon;
>
> /* Mask Tx interrupt */
> - if (s3c24xx_serial_has_interrupt_mask(port))
> + switch (ourport->info->type) {
> + case TYPE_S3C6400:
> s3c24xx_set_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);
> - else
> + break;
> + default:
> disable_irq_nosync(ourport->tx_irq);
> + break;
> + }
>
> /* Enable tx dma mode */
> ucon = rd_regl(port, S3C2410_UCON);
> @@ -386,11 +391,14 @@ static void enable_tx_pio(struct s3c24xx_uart_port *ourport)
> wr_regl(port, S3C2410_UCON, ucon);
>
> /* Unmask Tx interrupt */
> - if (s3c24xx_serial_has_interrupt_mask(port))
> - s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD,
> - S3C64XX_UINTM);
> - else
> + switch (ourport->info->type) {
> + case TYPE_S3C6400:
> + s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);

Please do not re-wrap. It makes reviewing more difficult. You can
perform proper re-wrapping as a separate cleanup patch.

> + break;
> + default:
> enable_irq(ourport->tx_irq);
> + break;
> + }
>
> ourport->tx_mode = S3C24XX_TX_PIO;
> }
> @@ -513,11 +521,14 @@ static void s3c24xx_serial_stop_rx(struct uart_port *port)
>
> if (ourport->rx_enabled) {
> dev_dbg(port->dev, "stopping rx\n");
> - if (s3c24xx_serial_has_interrupt_mask(port))
> - s3c24xx_set_bit(port, S3C64XX_UINTM_RXD,
> - S3C64XX_UINTM);

The same.

Best regards,
Krzysztof