[PATCH 0/5] serial: Remove custom frame size calculation

[PATCH 0/5] serial: Remove custom frame size calculation

[Ilpo Järvinen]

A few drivers still calculate number of frame bits on their own. As core
can do that for them these days, take advantage of core's capabilities.

Ilpo Järvinen (5):
  serial: ucc_uart: Remove custom frame size calculation
  serial: cpm_uart: Remove custom frame size calculation
  serial: fsl_lpuart: Remove custom frame size calculation
  serial: sunsab: Remove frame size calculation dead-code
  serial: tegra: Remove custom frame size calculation

 drivers/tty/serial/cpm_uart/cpm_uart_core.c | 28 ++++++---------------
 drivers/tty/serial/fsl_lpuart.c             | 10 +++-----
 drivers/tty/serial/serial-tegra.c           | 12 +++------
 drivers/tty/serial/sunsab.c                 | 20 ++++++---------
 drivers/tty/serial/ucc_uart.c               | 15 +----------
 5 files changed, 22 insertions(+), 63 deletions(-)

-- 
2.30.2

[PATCH 1/5] serial: ucc_uart: Remove custom frame size calculation

[Ilpo Järvinen]

The number of bits can be calculated using tty_get_frame_size(), no
need for the driver to do it on its own.

Also remove a comment on number of bits that doesn't match the code nor
the comment on ucc_uart_pram's rx_length ("minus 1" part differs). That
comment seems a verbatim copy of that in cpm_uart/cpm_uart_core.c
anyway so perhaps it was just copied over w/o much thinking.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/ucc_uart.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c
index 3cc9ef08455c..7331964163c5 100644
--- a/drivers/tty/serial/ucc_uart.c
+++ b/drivers/tty/serial/ucc_uart.c
@@ -853,13 +853,6 @@ static void qe_uart_set_termios(struct uart_port *port,
 	u16 upsmr = ioread16be(&uccp->upsmr);
 	struct ucc_uart_pram __iomem *uccup = qe_port->uccup;
 	u16 supsmr = ioread16be(&uccup->supsmr);
-	u8 char_length = 2; /* 1 + CL + PEN + 1 + SL */
-
-	/* Character length programmed into the mode register is the
-	 * sum of: 1 start bit, number of data bits, 0 or 1 parity bit,
-	 * 1 or 2 stop bits, minus 1.
-	 * The value 'bits' counts this for us.
-	 */
 
 	/* byte size */
 	upsmr &= UCC_UART_UPSMR_CL_MASK;
@@ -869,22 +862,18 @@ static void qe_uart_set_termios(struct uart_port *port,
 	case CS5:
 		upsmr |= UCC_UART_UPSMR_CL_5;
 		supsmr |= UCC_UART_SUPSMR_CL_5;
-		char_length += 5;
 		break;
 	case CS6:
 		upsmr |= UCC_UART_UPSMR_CL_6;
 		supsmr |= UCC_UART_SUPSMR_CL_6;
-		char_length += 6;
 		break;
 	case CS7:
 		upsmr |= UCC_UART_UPSMR_CL_7;
 		supsmr |= UCC_UART_SUPSMR_CL_7;
-		char_length += 7;
 		break;
 	default:	/* case CS8 */
 		upsmr |= UCC_UART_UPSMR_CL_8;
 		supsmr |= UCC_UART_SUPSMR_CL_8;
-		char_length += 8;
 		break;
 	}
 
@@ -892,13 +881,11 @@ static void qe_uart_set_termios(struct uart_port *port,
 	if (termios->c_cflag & CSTOPB) {
 		upsmr |= UCC_UART_UPSMR_SL;
 		supsmr |= UCC_UART_SUPSMR_SL;
-		char_length++;  /* + SL */
 	}
 
 	if (termios->c_cflag & PARENB) {
 		upsmr |= UCC_UART_UPSMR_PEN;
 		supsmr |= UCC_UART_SUPSMR_PEN;
-		char_length++;  /* + PEN */
 
 		if (!(termios->c_cflag & PARODD)) {
 			upsmr &= ~(UCC_UART_UPSMR_RPM_MASK |
@@ -953,7 +940,7 @@ static void qe_uart_set_termios(struct uart_port *port,
 	iowrite16be(upsmr, &uccp->upsmr);
 	if (soft_uart) {
 		iowrite16be(supsmr, &uccup->supsmr);
-		iowrite8(char_length, &uccup->rx_length);
+		iowrite8(tty_get_frame_size(termios->c_cflag), &uccup->rx_length);
 
 		/* Soft-UART requires a 1X multiplier for TX */
 		qe_setbrg(qe_port->us_info.rx_clock, baud, 16);
-- 
2.30.2

[PATCH 2/5] serial: cpm_uart: Remove custom frame size calculation

[Ilpo Järvinen]

The number of bits can be calculated using helpers in core, no need for
the driver to do it on its own.

The mode register is programmed with frame bits minus 1, rearrange the
comments related to that "feature" closer to the actual write.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/cpm_uart/cpm_uart_core.c | 28 ++++++---------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
index db07d6a5d764..96c6ee379c03 100644
--- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
@@ -489,7 +489,6 @@ static void cpm_uart_set_termios(struct uart_port *port,
 	int baud;
 	unsigned long flags;
 	u16 cval, scval, prev_mode;
-	int bits, sbits;
 	struct uart_cpm_port *pinfo =
 		container_of(port, struct uart_cpm_port, port);
 	smc_t __iomem *smcp = pinfo->smcp;
@@ -515,28 +514,17 @@ static void cpm_uart_set_termios(struct uart_port *port,
 	if (maxidl > 0x10)
 		maxidl = 0x10;
 
-	/* Character length programmed into the mode register is the
-	 * sum of: 1 start bit, number of data bits, 0 or 1 parity bit,
-	 * 1 or 2 stop bits, minus 1.
-	 * The value 'bits' counts this for us.
-	 */
 	cval = 0;
 	scval = 0;
 
-	/* byte size */
-	bits = tty_get_char_size(termios->c_cflag);
-	sbits = bits - 5;
-
 	if (termios->c_cflag & CSTOPB) {
 		cval |= SMCMR_SL;	/* Two stops */
 		scval |= SCU_PSMR_SL;
-		bits++;
 	}
 
 	if (termios->c_cflag & PARENB) {
 		cval |= SMCMR_PEN;
 		scval |= SCU_PSMR_PEN;
-		bits++;
 		if (!(termios->c_cflag & PARODD)) {
 			cval |= SMCMR_PM_EVEN;
 			scval |= (SCU_PSMR_REVP | SCU_PSMR_TEVP);
@@ -580,11 +568,6 @@ static void cpm_uart_set_termios(struct uart_port *port,
 
 	spin_lock_irqsave(&port->lock, flags);
 
-	/* Start bit has not been added (so don't, because we would just
-	 * subtract it later), and we need to add one for the number of
-	 * stops bits (there is always at least one).
-	 */
-	bits++;
 	if (IS_SMC(pinfo)) {
 		/*
 		 * MRBLR can be changed while an SMC/SCC is operating only
@@ -604,13 +587,16 @@ static void cpm_uart_set_termios(struct uart_port *port,
 		 */
 		prev_mode = in_be16(&smcp->smc_smcmr) & (SMCMR_REN | SMCMR_TEN);
 		/* Output in *one* operation, so we don't interrupt RX/TX if they
-		 * were already enabled. */
-		out_be16(&smcp->smc_smcmr, smcr_mk_clen(bits) | cval |
-		    SMCMR_SM_UART | prev_mode);
+		 * were already enabled.
+		 * Character length programmed into the register is frame bits minus 1.
+		 */
+		out_be16(&smcp->smc_smcmr, smcr_mk_clen(tty_get_frame_size(termios->c_cflag) - 1) |
+					   cval | SMCMR_SM_UART | prev_mode);
 	} else {
 		out_be16(&pinfo->sccup->scc_genscc.scc_mrblr, pinfo->rx_fifosize);
 		out_be16(&pinfo->sccup->scc_maxidl, maxidl);
-		out_be16(&sccp->scc_psmr, (sbits << 12) | scval);
+		out_be16(&sccp->scc_psmr, (UART_LCR_WLEN(tty_get_char_size(termios->c_cflag))
+					   << 12) | scval);
 	}
 
 	if (pinfo->clk)
-- 
2.30.2

Re: [PATCH 2/5] serial: cpm_uart: Remove custom frame size calculation

[Andy Shevchenko]

On Thu, Aug 25, 2022 at 11:58 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> The number of bits can be calculated using helpers in core, no need for
> the driver to do it on its own.
>
> The mode register is programmed with frame bits minus 1, rearrange the
> comments related to that "feature" closer to the actual write.

...

> +               out_be16(&smcp->smc_smcmr, smcr_mk_clen(tty_get_frame_size(termios->c_cflag) - 1) |
> +                                          cval | SMCMR_SM_UART | prev_mode);
>         } else {
>                 out_be16(&pinfo->sccup->scc_genscc.scc_mrblr, pinfo->rx_fifosize);
>                 out_be16(&pinfo->sccup->scc_maxidl, maxidl);
> -               out_be16(&sccp->scc_psmr, (sbits << 12) | scval);
> +               out_be16(&sccp->scc_psmr, (UART_LCR_WLEN(tty_get_char_size(termios->c_cflag))
> +                                          << 12) | scval);

Seems it's better to calc it beforehand in the temporary variable for
both branches of the conditional and fix a bit strange indentation
here.

>         }


-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/5] serial: fsl_lpuart: Remove custom frame size calculation

[Ilpo Järvinen]

The number of bits can be calculated using tty_get_frame_size(), no
need for the driver to do it on its own.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/fsl_lpuart.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index f6c33cd228c8..926460f1dc08 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1284,23 +1284,19 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	struct dma_slave_config dma_rx_sconfig = {};
 	struct circ_buf *ring = &sport->rx_ring;
 	int ret, nent;
-	int bits, baud;
+	int baud;
 	struct tty_port *port = &sport->port.state->port;
 	struct tty_struct *tty = port->tty;
 	struct ktermios *termios = &tty->termios;
 	struct dma_chan *chan = sport->dma_rx_chan;
 
 	baud = tty_get_baud_rate(tty);
-
-	bits = (termios->c_cflag & CSIZE) == CS7 ? 9 : 10;
-	if (termios->c_cflag & PARENB)
-		bits++;
-
 	/*
 	 * Calculate length of one DMA buffer size to keep latency below
 	 * 10ms at any baud rate.
 	 */
-	sport->rx_dma_rng_buf_len = (DMA_RX_TIMEOUT * baud /  bits / 1000) * 2;
+	sport->rx_dma_rng_buf_len = (DMA_RX_TIMEOUT * baud / tty_get_frame_size(termios->c_cflag) /
+				     1000) * 2;
 	sport->rx_dma_rng_buf_len = (1 << (fls(sport->rx_dma_rng_buf_len) - 1));
 	if (sport->rx_dma_rng_buf_len < 16)
 		sport->rx_dma_rng_buf_len = 16;
-- 
2.30.2

Re: [PATCH 3/5] serial: fsl_lpuart: Remove custom frame size calculation

[Andy Shevchenko]

On Thu, Aug 25, 2022 at 11:59 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> The number of bits can be calculated using tty_get_frame_size(), no
> need for the driver to do it on its own.

...

>         struct dma_slave_config dma_rx_sconfig = {};
>         struct circ_buf *ring = &sport->rx_ring;
>         int ret, nent;
> -       int bits, baud;
> +       int baud;
>         struct tty_port *port = &sport->port.state->port;
>         struct tty_struct *tty = port->tty;
>         struct ktermios *termios = &tty->termios;
>         struct dma_chan *chan = sport->dma_rx_chan;
>
>         baud = tty_get_baud_rate(tty);

You can move baud and join with the assignment, right?
At the same time provide a temporary variable for bits here with the assignment.

> -       bits = (termios->c_cflag & CSIZE) == CS7 ? 9 : 10;
> -       if (termios->c_cflag & PARENB)
> -               bits++;
> -
>         /*
>          * Calculate length of one DMA buffer size to keep latency below
>          * 10ms at any baud rate.
>          */
> -       sport->rx_dma_rng_buf_len = (DMA_RX_TIMEOUT * baud /  bits / 1000) * 2;

This will be left untouched.

> +       sport->rx_dma_rng_buf_len = (DMA_RX_TIMEOUT * baud / tty_get_frame_size(termios->c_cflag) /
> +                                    1000) * 2;

And no strange indentation.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/5] serial: sunsab: Remove frame size calculation dead-code

[Ilpo Järvinen]

The driver features a custom frame length calculation but the result is
never used. Remove it.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/sunsab.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/sunsab.c b/drivers/tty/serial/sunsab.c
index 6ea52293d9f3..f7968f73753d 100644
--- a/drivers/tty/serial/sunsab.c
+++ b/drivers/tty/serial/sunsab.c
@@ -681,27 +681,23 @@ static void sunsab_convert_to_sab(struct uart_sunsab_port *up, unsigned int cfla
 				  unsigned int quot)
 {
 	unsigned char dafo;
-	int bits, n, m;
+	int n, m;
 
 	/* Byte size and parity */
 	switch (cflag & CSIZE) {
-	      case CS5: dafo = SAB82532_DAFO_CHL5; bits = 7; break;
-	      case CS6: dafo = SAB82532_DAFO_CHL6; bits = 8; break;
-	      case CS7: dafo = SAB82532_DAFO_CHL7; bits = 9; break;
-	      case CS8: dafo = SAB82532_DAFO_CHL8; bits = 10; break;
+	      case CS5: dafo = SAB82532_DAFO_CHL5; break;
+	      case CS6: dafo = SAB82532_DAFO_CHL6; break;
+	      case CS7: dafo = SAB82532_DAFO_CHL7; break;
+	      case CS8: dafo = SAB82532_DAFO_CHL8; break;
 	      /* Never happens, but GCC is too dumb to figure it out */
-	      default:  dafo = SAB82532_DAFO_CHL5; bits = 7; break;
+	      default:  dafo = SAB82532_DAFO_CHL5; break;
 	}
 
-	if (cflag & CSTOPB) {
+	if (cflag & CSTOPB)
 		dafo |= SAB82532_DAFO_STOP;
-		bits++;
-	}
 
-	if (cflag & PARENB) {
+	if (cflag & PARENB)
 		dafo |= SAB82532_DAFO_PARE;
-		bits++;
-	}
 
 	if (cflag & PARODD) {
 		dafo |= SAB82532_DAFO_PAR_ODD;
-- 
2.30.2

[PATCH 5/5] serial: tegra: Remove custom frame size calculation

[Ilpo Järvinen]

The number of bits can be calculated using tty_get_frame_size(), no
need for the driver to do it on its own.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/serial-tegra.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index ad4f3567ff90..5a56bd20abcd 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1278,7 +1278,6 @@ static void tegra_uart_set_termios(struct uart_port *u,
 	unsigned long flags;
 	unsigned int lcr;
 	unsigned char char_bits;
-	int symb_bit = 1;
 	struct clk *parent_clk = clk_get_parent(tup->uart_clk);
 	unsigned long parent_clk_rate = clk_get_rate(parent_clk);
 	int max_divider = (tup->cdata->support_clk_src_div) ? 0x7FFF : 0xFFFF;
@@ -1305,7 +1304,6 @@ static void tegra_uart_set_termios(struct uart_port *u,
 	termios->c_cflag &= ~CMSPAR;
 
 	if ((termios->c_cflag & PARENB) == PARENB) {
-		symb_bit++;
 		if (termios->c_cflag & PARODD) {
 			lcr |= UART_LCR_PARITY;
 			lcr &= ~UART_LCR_EPAR;
@@ -1318,22 +1316,18 @@ static void tegra_uart_set_termios(struct uart_port *u,
 	}
 
 	char_bits = tty_get_char_size(termios->c_cflag);
-	symb_bit += char_bits;
 	lcr &= ~UART_LCR_WLEN8;
 	lcr |= UART_LCR_WLEN(char_bits);
 
 	/* Stop bits */
-	if (termios->c_cflag & CSTOPB) {
+	if (termios->c_cflag & CSTOPB)
 		lcr |= UART_LCR_STOP;
-		symb_bit += 2;
-	} else {
+	else
 		lcr &= ~UART_LCR_STOP;
-		symb_bit++;
-	}
 
 	tegra_uart_write(tup, lcr, UART_LCR);
 	tup->lcr_shadow = lcr;
-	tup->symb_bit = symb_bit;
+	tup->symb_bit = tty_get_frame_size(termios->c_cflag);
 
 	/* Baud rate. */
 	baud = uart_get_baud_rate(u, termios, oldtermios,
-- 
2.30.2

Re: [PATCH 0/5] serial: Remove custom frame size calculation

[Andy Shevchenko]

On Thu, Aug 25, 2022 at 11:58 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> A few drivers still calculate number of frame bits on their own. As core
> can do that for them these days, take advantage of core's capabilities.

For non-commented patches:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

With small style things addressed as suggested, you may spread the tag
to them as well.

> Ilpo Järvinen (5):
>   serial: ucc_uart: Remove custom frame size calculation
>   serial: cpm_uart: Remove custom frame size calculation
>   serial: fsl_lpuart: Remove custom frame size calculation
>   serial: sunsab: Remove frame size calculation dead-code
>   serial: tegra: Remove custom frame size calculation
>
>  drivers/tty/serial/cpm_uart/cpm_uart_core.c | 28 ++++++---------------
>  drivers/tty/serial/fsl_lpuart.c             | 10 +++-----
>  drivers/tty/serial/serial-tegra.c           | 12 +++------
>  drivers/tty/serial/sunsab.c                 | 20 ++++++---------
>  drivers/tty/serial/ucc_uart.c               | 15 +----------
>  5 files changed, 22 insertions(+), 63 deletions(-)
>
> --
> 2.30.2
>


-- 
With Best Regards,
Andy Shevchenko