[PATCH 1/2] serial: stm32: Factor out GPIO RTS toggling into separate function

[PATCH 1/2] serial: stm32: Factor out GPIO RTS toggling into separate function

[Marek Vasut]

Pull out the GPIO RTS enable and disable handling into separate function.
Limit the scope of GPIO RTS toggling only to GPIO emulated RS485 too.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
Cc: Valentin Caron <valentin.caron@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/stm32-usart.c | 59 ++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index f75691e727295..224f359c6051e 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -442,6 +442,42 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
 		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_rs485_rts_enable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	struct serial_rs485 *rs485conf = &port->rs485;
+
+	if (stm32_port->hw_flow_control ||
+	    !(rs485conf->flags & SER_RS485_ENABLED))
+		return;
+
+	if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl | TIOCM_RTS);
+	} else {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl & ~TIOCM_RTS);
+	}
+}
+
+static void stm32_usart_rs485_rts_disable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	struct serial_rs485 *rs485conf = &port->rs485;
+
+	if (stm32_port->hw_flow_control ||
+	    !(rs485conf->flags & SER_RS485_ENABLED))
+		return;
+
+	if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl & ~TIOCM_RTS);
+	} else {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl | TIOCM_RTS);
+	}
+}
+
 static void stm32_usart_transmit_chars_pio(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
@@ -713,43 +749,24 @@ static void stm32_usart_disable_ms(struct uart_port *port)
 static void stm32_usart_stop_tx(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
-	struct serial_rs485 *rs485conf = &port->rs485;
 	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
 
 	stm32_usart_tx_interrupt_disable(port);
 	if (stm32_usart_tx_dma_started(stm32_port) && stm32_usart_tx_dma_enabled(stm32_port))
 		stm32_usart_clr_bits(port, ofs->cr3, USART_CR3_DMAT);
 
-	if (rs485conf->flags & SER_RS485_ENABLED) {
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl & ~TIOCM_RTS);
-		} else {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl | TIOCM_RTS);
-		}
-	}
+	stm32_usart_rs485_rts_disable(port);
 }
 
 /* There are probably characters waiting to be transmitted. */
 static void stm32_usart_start_tx(struct uart_port *port)
 {
-	struct stm32_port *stm32_port = to_stm32_port(port);
-	struct serial_rs485 *rs485conf = &port->rs485;
 	struct circ_buf *xmit = &port->state->xmit;
 
 	if (uart_circ_empty(xmit) && !port->x_char)
 		return;
 
-	if (rs485conf->flags & SER_RS485_ENABLED) {
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl | TIOCM_RTS);
-		} else {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl & ~TIOCM_RTS);
-		}
-	}
+	stm32_usart_rs485_rts_enable(port);
 
 	stm32_usart_transmit_chars(port);
 }
-- 
2.35.1

[PATCH 1/2] serial: stm32: Factor out GPIO RTS toggling into separate function

[Marek Vasut]

Pull out the GPIO RTS enable and disable handling into separate function.
Limit the scope of GPIO RTS toggling only to GPIO emulated RS485 too.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
Cc: Valentin Caron <valentin.caron@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/stm32-usart.c | 59 ++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index f75691e727295..224f359c6051e 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -442,6 +442,42 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
 		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_rs485_rts_enable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	struct serial_rs485 *rs485conf = &port->rs485;
+
+	if (stm32_port->hw_flow_control ||
+	    !(rs485conf->flags & SER_RS485_ENABLED))
+		return;
+
+	if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl | TIOCM_RTS);
+	} else {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl & ~TIOCM_RTS);
+	}
+}
+
+static void stm32_usart_rs485_rts_disable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	struct serial_rs485 *rs485conf = &port->rs485;
+
+	if (stm32_port->hw_flow_control ||
+	    !(rs485conf->flags & SER_RS485_ENABLED))
+		return;
+
+	if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl & ~TIOCM_RTS);
+	} else {
+		mctrl_gpio_set(stm32_port->gpios,
+			       stm32_port->port.mctrl | TIOCM_RTS);
+	}
+}
+
 static void stm32_usart_transmit_chars_pio(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
@@ -713,43 +749,24 @@ static void stm32_usart_disable_ms(struct uart_port *port)
 static void stm32_usart_stop_tx(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
-	struct serial_rs485 *rs485conf = &port->rs485;
 	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
 
 	stm32_usart_tx_interrupt_disable(port);
 	if (stm32_usart_tx_dma_started(stm32_port) && stm32_usart_tx_dma_enabled(stm32_port))
 		stm32_usart_clr_bits(port, ofs->cr3, USART_CR3_DMAT);
 
-	if (rs485conf->flags & SER_RS485_ENABLED) {
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl & ~TIOCM_RTS);
-		} else {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl | TIOCM_RTS);
-		}
-	}
+	stm32_usart_rs485_rts_disable(port);
 }
 
 /* There are probably characters waiting to be transmitted. */
 static void stm32_usart_start_tx(struct uart_port *port)
 {
-	struct stm32_port *stm32_port = to_stm32_port(port);
-	struct serial_rs485 *rs485conf = &port->rs485;
 	struct circ_buf *xmit = &port->state->xmit;
 
 	if (uart_circ_empty(xmit) && !port->x_char)
 		return;
 
-	if (rs485conf->flags & SER_RS485_ENABLED) {
-		if (rs485conf->flags & SER_RS485_RTS_ON_SEND) {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl | TIOCM_RTS);
-		} else {
-			mctrl_gpio_set(stm32_port->gpios,
-					stm32_port->port.mctrl & ~TIOCM_RTS);
-		}
-	}
+	stm32_usart_rs485_rts_enable(port);
 
 	stm32_usart_transmit_chars(port);
 }
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
transmission ended and the RS485 cannot work.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
Cc: Valentin Caron <valentin.caron@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/stm32-usart.c | 42 ++++++++++++++++++++++++++++++--
 drivers/tty/serial/stm32-usart.h |  1 +
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 224f359c6051e..764415b8e8f03 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -417,6 +417,14 @@ static void stm32_usart_tx_interrupt_enable(struct uart_port *port)
 		stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
+
+	stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
+}
+
 static void stm32_usart_rx_dma_complete(void *arg)
 {
 	struct uart_port *port = arg;
@@ -442,6 +450,14 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
 		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_tc_interrupt_disable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
+
+	stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TCIE);
+}
+
 static void stm32_usart_rs485_rts_enable(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
@@ -585,6 +601,13 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
 	u32 isr;
 	int ret;
 
+	if (!stm32_port->hw_flow_control &&
+	    port->rs485.flags & SER_RS485_ENABLED) {
+		stm32_port->txdone = false;
+		stm32_usart_tc_interrupt_disable(port);
+		stm32_usart_rs485_rts_enable(port);
+	}
+
 	if (port->x_char) {
 		if (stm32_usart_tx_dma_started(stm32_port) &&
 		    stm32_usart_tx_dma_enabled(stm32_port))
@@ -625,8 +648,14 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
-	if (uart_circ_empty(xmit))
+	if (uart_circ_empty(xmit)) {
 		stm32_usart_tx_interrupt_disable(port);
+		if (!stm32_port->hw_flow_control &&
+		    port->rs485.flags & SER_RS485_ENABLED) {
+			stm32_port->txdone = true;
+			stm32_usart_tc_interrupt_enable(port);
+		}
+	}
 }
 
 static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
@@ -640,6 +669,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 
 	sr = readl_relaxed(port->membase + ofs->isr);
 
+	if (!stm32_port->hw_flow_control &&
+	    port->rs485.flags & SER_RS485_ENABLED &&
+	    (sr & USART_SR_TC)) {
+		stm32_usart_tc_interrupt_disable(port);
+		stm32_usart_rs485_rts_disable(port);
+	}
+
 	if ((sr & USART_SR_RTOF) && ofs->icr != UNDEF_REG)
 		writel_relaxed(USART_ICR_RTOCF,
 			       port->membase + ofs->icr);
@@ -763,8 +799,10 @@ static void stm32_usart_start_tx(struct uart_port *port)
 {
 	struct circ_buf *xmit = &port->state->xmit;
 
-	if (uart_circ_empty(xmit) && !port->x_char)
+	if (uart_circ_empty(xmit) && !port->x_char) {
+		stm32_usart_rs485_rts_disable(port);
 		return;
+	}
 
 	stm32_usart_rs485_rts_enable(port);
 
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index d734c4a5fd24c..ee69c203b926d 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -271,6 +271,7 @@ struct stm32_port {
 	bool hw_flow_control;
 	bool swap;		 /* swap RX & TX pins */
 	bool fifoen;
+	bool txdone;
 	int rxftcfg;		/* RX FIFO threshold CFG      */
 	int txftcfg;		/* TX FIFO threshold CFG      */
 	bool wakeup_src;
-- 
2.35.1

[PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
transmission ended and the RS485 cannot work.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
Cc: Valentin Caron <valentin.caron@foss.st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/stm32-usart.c | 42 ++++++++++++++++++++++++++++++--
 drivers/tty/serial/stm32-usart.h |  1 +
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 224f359c6051e..764415b8e8f03 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -417,6 +417,14 @@ static void stm32_usart_tx_interrupt_enable(struct uart_port *port)
 		stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
+
+	stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
+}
+
 static void stm32_usart_rx_dma_complete(void *arg)
 {
 	struct uart_port *port = arg;
@@ -442,6 +450,14 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
 		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
 }
 
+static void stm32_usart_tc_interrupt_disable(struct uart_port *port)
+{
+	struct stm32_port *stm32_port = to_stm32_port(port);
+	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
+
+	stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TCIE);
+}
+
 static void stm32_usart_rs485_rts_enable(struct uart_port *port)
 {
 	struct stm32_port *stm32_port = to_stm32_port(port);
@@ -585,6 +601,13 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
 	u32 isr;
 	int ret;
 
+	if (!stm32_port->hw_flow_control &&
+	    port->rs485.flags & SER_RS485_ENABLED) {
+		stm32_port->txdone = false;
+		stm32_usart_tc_interrupt_disable(port);
+		stm32_usart_rs485_rts_enable(port);
+	}
+
 	if (port->x_char) {
 		if (stm32_usart_tx_dma_started(stm32_port) &&
 		    stm32_usart_tx_dma_enabled(stm32_port))
@@ -625,8 +648,14 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(port);
 
-	if (uart_circ_empty(xmit))
+	if (uart_circ_empty(xmit)) {
 		stm32_usart_tx_interrupt_disable(port);
+		if (!stm32_port->hw_flow_control &&
+		    port->rs485.flags & SER_RS485_ENABLED) {
+			stm32_port->txdone = true;
+			stm32_usart_tc_interrupt_enable(port);
+		}
+	}
 }
 
 static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
@@ -640,6 +669,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 
 	sr = readl_relaxed(port->membase + ofs->isr);
 
+	if (!stm32_port->hw_flow_control &&
+	    port->rs485.flags & SER_RS485_ENABLED &&
+	    (sr & USART_SR_TC)) {
+		stm32_usart_tc_interrupt_disable(port);
+		stm32_usart_rs485_rts_disable(port);
+	}
+
 	if ((sr & USART_SR_RTOF) && ofs->icr != UNDEF_REG)
 		writel_relaxed(USART_ICR_RTOCF,
 			       port->membase + ofs->icr);
@@ -763,8 +799,10 @@ static void stm32_usart_start_tx(struct uart_port *port)
 {
 	struct circ_buf *xmit = &port->state->xmit;
 
-	if (uart_circ_empty(xmit) && !port->x_char)
+	if (uart_circ_empty(xmit) && !port->x_char) {
+		stm32_usart_rs485_rts_disable(port);
 		return;
+	}
 
 	stm32_usart_rs485_rts_enable(port);
 
diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
index d734c4a5fd24c..ee69c203b926d 100644
--- a/drivers/tty/serial/stm32-usart.h
+++ b/drivers/tty/serial/stm32-usart.h
@@ -271,6 +271,7 @@ struct stm32_port {
 	bool hw_flow_control;
 	bool swap;		 /* swap RX & TX pins */
 	bool fifoen;
+	bool txdone;
 	int rxftcfg;		/* RX FIFO threshold CFG      */
 	int txftcfg;		/* TX FIFO threshold CFG      */
 	bool wakeup_src;
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Erwan LE RAY]

Hi Marek,

On 4/30/22 18:28, Marek Vasut wrote:
> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
> transmission ended and the RS485 cannot work.
> 
Could you please add a cover letter to explain the rational of the first 
patch ? I understood the goal of the first by reading the commit message 
of this second patch.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Erwan Le Ray <erwan.leray@foss.st.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
> Cc: Valentin Caron <valentin.caron@foss.st.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-serial@vger.kernel.org
> ---
>   drivers/tty/serial/stm32-usart.c | 42 ++++++++++++++++++++++++++++++--
>   drivers/tty/serial/stm32-usart.h |  1 +
>   2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 224f359c6051e..764415b8e8f03 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -417,6 +417,14 @@ static void stm32_usart_tx_interrupt_enable(struct uart_port *port)
>   		stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
I don't see the added value of this helper (only 1 instruction used 1 
time), and other Interrupt Enabled bits are already set/unset in others 
functions of this driver.
To keep an homogeneous code in the driver, could you please remove this 
helper and set TCIE directly when you need it ?

>   static void stm32_usart_rx_dma_complete(void *arg)
>   {
>   	struct uart_port *port = arg;
> @@ -442,6 +450,14 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
>   		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_disable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
Same comment here.

>   static void stm32_usart_rs485_rts_enable(struct uart_port *port)
>   {
>   	struct stm32_port *stm32_port = to_stm32_port(port);
> @@ -585,6 +601,13 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	u32 isr;
>   	int ret;
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED) {
> +		stm32_port->txdone = false;
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_enable(port);
> +	}
> +
>   	if (port->x_char) {
>   		if (stm32_usart_tx_dma_started(stm32_port) &&
>   		    stm32_usart_tx_dma_enabled(stm32_port))
> @@ -625,8 +648,14 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>   		uart_write_wakeup(port);
>   
> -	if (uart_circ_empty(xmit))
> +	if (uart_circ_empty(xmit)) {
>   		stm32_usart_tx_interrupt_disable(port);
> +		if (!stm32_port->hw_flow_control &&
> +		    port->rs485.flags & SER_RS485_ENABLED) {
> +			stm32_port->txdone = true;
> +			stm32_usart_tc_interrupt_enable(port);
> +		}
> +	}
>   }
>   
>   static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> @@ -640,6 +669,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   
>   	sr = readl_relaxed(port->membase + ofs->isr);
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED &&
> +	    (sr & USART_SR_TC)) {
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_disable(port);
> +	}
> +
>   	if ((sr & USART_SR_RTOF) && ofs->icr != UNDEF_REG)
>   		writel_relaxed(USART_ICR_RTOCF,
>   			       port->membase + ofs->icr);
> @@ -763,8 +799,10 @@ static void stm32_usart_start_tx(struct uart_port *port)
>   {
>   	struct circ_buf *xmit = &port->state->xmit;
>   
> -	if (uart_circ_empty(xmit) && !port->x_char)
> +	if (uart_circ_empty(xmit) && !port->x_char) {
> +		stm32_usart_rs485_rts_disable(port);
>   		return;
> +	}
>   
>   	stm32_usart_rs485_rts_enable(port);
>   
> diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
> index d734c4a5fd24c..ee69c203b926d 100644
> --- a/drivers/tty/serial/stm32-usart.h
> +++ b/drivers/tty/serial/stm32-usart.h
> @@ -271,6 +271,7 @@ struct stm32_port {
>   	bool hw_flow_control;
>   	bool swap;		 /* swap RX & TX pins */
>   	bool fifoen;
> +	bool txdone;
>   	int rxftcfg;		/* RX FIFO threshold CFG      */
>   	int txftcfg;		/* TX FIFO threshold CFG      */
>   	bool wakeup_src;

Regards, Erwan.

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Erwan LE RAY]

Hi Marek,

On 4/30/22 18:28, Marek Vasut wrote:
> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
> transmission ended and the RS485 cannot work.
> 
Could you please add a cover letter to explain the rational of the first 
patch ? I understood the goal of the first by reading the commit message 
of this second patch.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Erwan Le Ray <erwan.leray@foss.st.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
> Cc: Valentin Caron <valentin.caron@foss.st.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-serial@vger.kernel.org
> ---
>   drivers/tty/serial/stm32-usart.c | 42 ++++++++++++++++++++++++++++++--
>   drivers/tty/serial/stm32-usart.h |  1 +
>   2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 224f359c6051e..764415b8e8f03 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -417,6 +417,14 @@ static void stm32_usart_tx_interrupt_enable(struct uart_port *port)
>   		stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
I don't see the added value of this helper (only 1 instruction used 1 
time), and other Interrupt Enabled bits are already set/unset in others 
functions of this driver.
To keep an homogeneous code in the driver, could you please remove this 
helper and set TCIE directly when you need it ?

>   static void stm32_usart_rx_dma_complete(void *arg)
>   {
>   	struct uart_port *port = arg;
> @@ -442,6 +450,14 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
>   		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_disable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
Same comment here.

>   static void stm32_usart_rs485_rts_enable(struct uart_port *port)
>   {
>   	struct stm32_port *stm32_port = to_stm32_port(port);
> @@ -585,6 +601,13 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	u32 isr;
>   	int ret;
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED) {
> +		stm32_port->txdone = false;
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_enable(port);
> +	}
> +
>   	if (port->x_char) {
>   		if (stm32_usart_tx_dma_started(stm32_port) &&
>   		    stm32_usart_tx_dma_enabled(stm32_port))
> @@ -625,8 +648,14 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>   		uart_write_wakeup(port);
>   
> -	if (uart_circ_empty(xmit))
> +	if (uart_circ_empty(xmit)) {
>   		stm32_usart_tx_interrupt_disable(port);
> +		if (!stm32_port->hw_flow_control &&
> +		    port->rs485.flags & SER_RS485_ENABLED) {
> +			stm32_port->txdone = true;
> +			stm32_usart_tc_interrupt_enable(port);
> +		}
> +	}
>   }
>   
>   static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> @@ -640,6 +669,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   
>   	sr = readl_relaxed(port->membase + ofs->isr);
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED &&
> +	    (sr & USART_SR_TC)) {
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_disable(port);
> +	}
> +
>   	if ((sr & USART_SR_RTOF) && ofs->icr != UNDEF_REG)
>   		writel_relaxed(USART_ICR_RTOCF,
>   			       port->membase + ofs->icr);
> @@ -763,8 +799,10 @@ static void stm32_usart_start_tx(struct uart_port *port)
>   {
>   	struct circ_buf *xmit = &port->state->xmit;
>   
> -	if (uart_circ_empty(xmit) && !port->x_char)
> +	if (uart_circ_empty(xmit) && !port->x_char) {
> +		stm32_usart_rs485_rts_disable(port);
>   		return;
> +	}
>   
>   	stm32_usart_rs485_rts_enable(port);
>   
> diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
> index d734c4a5fd24c..ee69c203b926d 100644
> --- a/drivers/tty/serial/stm32-usart.h
> +++ b/drivers/tty/serial/stm32-usart.h
> @@ -271,6 +271,7 @@ struct stm32_port {
>   	bool hw_flow_control;
>   	bool swap;		 /* swap RX & TX pins */
>   	bool fifoen;
> +	bool txdone;
>   	int rxftcfg;		/* RX FIFO threshold CFG      */
>   	int txftcfg;		/* TX FIFO threshold CFG      */
>   	bool wakeup_src;

Regards, Erwan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

On 5/2/22 10:44, Erwan LE RAY wrote:
> Hi Marek,

Hi,

> On 4/30/22 18:28, Marek Vasut wrote:
>> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
>> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
>> transmission ended and the RS485 cannot work.
>>
> Could you please add a cover letter to explain the rational of the first 
> patch ? I understood the goal of the first by reading the commit message 
> of this second patch.

The rationale is trivial -- make sure we don't have five copies of the 
same block of code in the driver.

>> diff --git a/drivers/tty/serial/stm32-usart.c 
>> b/drivers/tty/serial/stm32-usart.c
>> index 224f359c6051e..764415b8e8f03 100644
>> --- a/drivers/tty/serial/stm32-usart.c
>> +++ b/drivers/tty/serial/stm32-usart.c
>> @@ -417,6 +417,14 @@ static void 
>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>   }
>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>> +{
>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>> +
>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>> +}
>> +
> I don't see the added value of this helper (only 1 instruction used 1 
> time), and other Interrupt Enabled bits are already set/unset in others 
> functions of this driver.
> To keep an homogeneous code in the driver, could you please remove this 
> helper and set TCIE directly when you need it ?

Should I also remove stm32_usart_tx_interrupt_enable() / 
stm32_usart_tx_interrupt_disable() , which does the same thing for other 
bits in the interrupt register ?

That sounds to me like making the code harder to read, not easier.

[...]

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

On 5/2/22 10:44, Erwan LE RAY wrote:
> Hi Marek,

Hi,

> On 4/30/22 18:28, Marek Vasut wrote:
>> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
>> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
>> transmission ended and the RS485 cannot work.
>>
> Could you please add a cover letter to explain the rational of the first 
> patch ? I understood the goal of the first by reading the commit message 
> of this second patch.

The rationale is trivial -- make sure we don't have five copies of the 
same block of code in the driver.

>> diff --git a/drivers/tty/serial/stm32-usart.c 
>> b/drivers/tty/serial/stm32-usart.c
>> index 224f359c6051e..764415b8e8f03 100644
>> --- a/drivers/tty/serial/stm32-usart.c
>> +++ b/drivers/tty/serial/stm32-usart.c
>> @@ -417,6 +417,14 @@ static void 
>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>   }
>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>> +{
>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>> +
>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>> +}
>> +
> I don't see the added value of this helper (only 1 instruction used 1 
> time), and other Interrupt Enabled bits are already set/unset in others 
> functions of this driver.
> To keep an homogeneous code in the driver, could you please remove this 
> helper and set TCIE directly when you need it ?

Should I also remove stm32_usart_tx_interrupt_enable() / 
stm32_usart_tx_interrupt_disable() , which does the same thing for other 
bits in the interrupt register ?

That sounds to me like making the code harder to read, not easier.

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Erwan LE RAY]

Hi Marek,

On 5/2/22 13:13, Marek Vasut wrote:
> On 5/2/22 10:44, Erwan LE RAY wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 4/30/22 18:28, Marek Vasut wrote:
>>> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
>>> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
>>> transmission ended and the RS485 cannot work.
>>>
>> Could you please add a cover letter to explain the rational of the 
>> first patch ? I understood the goal of the first by reading the commit 
>> message of this second patch.
> 
> The rationale is trivial -- make sure we don't have five copies of the 
> same block of code in the driver.
> Agree but factorization is needed only because of your second patch, 
reason why I suggested a cover letter for the series.

>>> diff --git a/drivers/tty/serial/stm32-usart.c 
>>> b/drivers/tty/serial/stm32-usart.c
>>> index 224f359c6051e..764415b8e8f03 100644
>>> --- a/drivers/tty/serial/stm32-usart.c
>>> +++ b/drivers/tty/serial/stm32-usart.c
>>> @@ -417,6 +417,14 @@ static void 
>>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>>   }
>>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>>> +{
>>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>>> +
>>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>>> +}
>>> +
>> I don't see the added value of this helper (only 1 instruction used 1 
>> time), and other Interrupt Enabled bits are already set/unset in 
>> others functions of this driver.
>> To keep an homogeneous code in the driver, could you please remove 
>> this helper and set TCIE directly when you need it ?
> 
> Should I also remove stm32_usart_tx_interrupt_enable() / 
> stm32_usart_tx_interrupt_disable() , which does the same thing for other 
> bits in the interrupt register ?
> 
In stm32_usart_tx_interrupt_enable() / 
stm32_usart_tx_interrupt_disable() case, 2 bits are configured 
differently under a condition, and stm32_usart_tx_interrupt_disable() is 
called 4 times in the driver. The factorization is triggered by the 
multiple calls to this code.
In your proposal, the helper is executing a single instruction, and is 
called only once, reason why I suggested to enable / disable the TCIE 
directly in your new functions stm32_usart_tc_interrupt_enable() / 
stm32_usart_tc_interrupt_disable().

> That sounds to me like making the code harder to read, not easier.
> 
> [...]

Regards, Erwan.

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Erwan LE RAY]

Hi Marek,

On 5/2/22 13:13, Marek Vasut wrote:
> On 5/2/22 10:44, Erwan LE RAY wrote:
>> Hi Marek,
> 
> Hi,
> 
>> On 4/30/22 18:28, Marek Vasut wrote:
>>> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
>>> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
>>> transmission ended and the RS485 cannot work.
>>>
>> Could you please add a cover letter to explain the rational of the 
>> first patch ? I understood the goal of the first by reading the commit 
>> message of this second patch.
> 
> The rationale is trivial -- make sure we don't have five copies of the 
> same block of code in the driver.
> Agree but factorization is needed only because of your second patch, 
reason why I suggested a cover letter for the series.

>>> diff --git a/drivers/tty/serial/stm32-usart.c 
>>> b/drivers/tty/serial/stm32-usart.c
>>> index 224f359c6051e..764415b8e8f03 100644
>>> --- a/drivers/tty/serial/stm32-usart.c
>>> +++ b/drivers/tty/serial/stm32-usart.c
>>> @@ -417,6 +417,14 @@ static void 
>>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>>   }
>>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>>> +{
>>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>>> +
>>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>>> +}
>>> +
>> I don't see the added value of this helper (only 1 instruction used 1 
>> time), and other Interrupt Enabled bits are already set/unset in 
>> others functions of this driver.
>> To keep an homogeneous code in the driver, could you please remove 
>> this helper and set TCIE directly when you need it ?
> 
> Should I also remove stm32_usart_tx_interrupt_enable() / 
> stm32_usart_tx_interrupt_disable() , which does the same thing for other 
> bits in the interrupt register ?
> 
In stm32_usart_tx_interrupt_enable() / 
stm32_usart_tx_interrupt_disable() case, 2 bits are configured 
differently under a condition, and stm32_usart_tx_interrupt_disable() is 
called 4 times in the driver. The factorization is triggered by the 
multiple calls to this code.
In your proposal, the helper is executing a single instruction, and is 
called only once, reason why I suggested to enable / disable the TCIE 
directly in your new functions stm32_usart_tc_interrupt_enable() / 
stm32_usart_tc_interrupt_disable().

> That sounds to me like making the code harder to read, not easier.
> 
> [...]

Regards, Erwan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

On 5/4/22 17:54, Erwan LE RAY wrote:

Hi,

[...]

>>>> diff --git a/drivers/tty/serial/stm32-usart.c 
>>>> b/drivers/tty/serial/stm32-usart.c
>>>> index 224f359c6051e..764415b8e8f03 100644
>>>> --- a/drivers/tty/serial/stm32-usart.c
>>>> +++ b/drivers/tty/serial/stm32-usart.c
>>>> @@ -417,6 +417,14 @@ static void 
>>>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>>>   }
>>>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>>>> +{
>>>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>>>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>>>> +
>>>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>>>> +}
>>>> +
>>> I don't see the added value of this helper (only 1 instruction used 1 
>>> time), and other Interrupt Enabled bits are already set/unset in 
>>> others functions of this driver.
>>> To keep an homogeneous code in the driver, could you please remove 
>>> this helper and set TCIE directly when you need it ?
>>
>> Should I also remove stm32_usart_tx_interrupt_enable() / 
>> stm32_usart_tx_interrupt_disable() , which does the same thing for 
>> other bits in the interrupt register ?
>>
> In stm32_usart_tx_interrupt_enable() / 
> stm32_usart_tx_interrupt_disable() case, 2 bits are configured 
> differently under a condition, and stm32_usart_tx_interrupt_disable() is 
> called 4 times in the driver. The factorization is triggered by the 
> multiple calls to this code.

stm32_usart_tc_interrupt_{en,dis}able() is called 3 times after 2/2, so 
having 3 copies of the same code sprinkled across the driver seems iffy 
at best. Also, stm32_usart_tc_interrupt_{en,dis}able() handles the 
register offset ($ofs variable), that would also have to be duplicated 
all over the driver. I don't like that, it would make the code harder to 
read.

> In your proposal, the helper is executing a single instruction

The helper first has to figure out the register offset from this $ofs 
table which is at least two instructions, and then does register RMW 
which are at least three instructions on arm32.

>, and is 
> called only once

Thrice in total.

> , reason why I suggested to enable / disable the TCIE 
> directly in your new functions stm32_usart_tc_interrupt_enable() / 
> stm32_usart_tc_interrupt_disable().
> 
>> That sounds to me like making the code harder to read, not easier.
Seems we have a coding style preference stall here.

Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Marek Vasut]

On 5/4/22 17:54, Erwan LE RAY wrote:

Hi,

[...]

>>>> diff --git a/drivers/tty/serial/stm32-usart.c 
>>>> b/drivers/tty/serial/stm32-usart.c
>>>> index 224f359c6051e..764415b8e8f03 100644
>>>> --- a/drivers/tty/serial/stm32-usart.c
>>>> +++ b/drivers/tty/serial/stm32-usart.c
>>>> @@ -417,6 +417,14 @@ static void 
>>>> stm32_usart_tx_interrupt_enable(struct uart_port *port)
>>>>           stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>>>>   }
>>>> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
>>>> +{
>>>> +    struct stm32_port *stm32_port = to_stm32_port(port);
>>>> +    const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
>>>> +
>>>> +    stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
>>>> +}
>>>> +
>>> I don't see the added value of this helper (only 1 instruction used 1 
>>> time), and other Interrupt Enabled bits are already set/unset in 
>>> others functions of this driver.
>>> To keep an homogeneous code in the driver, could you please remove 
>>> this helper and set TCIE directly when you need it ?
>>
>> Should I also remove stm32_usart_tx_interrupt_enable() / 
>> stm32_usart_tx_interrupt_disable() , which does the same thing for 
>> other bits in the interrupt register ?
>>
> In stm32_usart_tx_interrupt_enable() / 
> stm32_usart_tx_interrupt_disable() case, 2 bits are configured 
> differently under a condition, and stm32_usart_tx_interrupt_disable() is 
> called 4 times in the driver. The factorization is triggered by the 
> multiple calls to this code.

stm32_usart_tc_interrupt_{en,dis}able() is called 3 times after 2/2, so 
having 3 copies of the same code sprinkled across the driver seems iffy 
at best. Also, stm32_usart_tc_interrupt_{en,dis}able() handles the 
register offset ($ofs variable), that would also have to be duplicated 
all over the driver. I don't like that, it would make the code harder to 
read.

> In your proposal, the helper is executing a single instruction

The helper first has to figure out the register offset from this $ofs 
table which is at least two instructions, and then does register RMW 
which are at least three instructions on arm32.

>, and is 
> called only once

Thrice in total.

> , reason why I suggested to enable / disable the TCIE 
> directly in your new functions stm32_usart_tc_interrupt_enable() / 
> stm32_usart_tc_interrupt_disable().
> 
>> That sounds to me like making the code harder to read, not easier.
Seems we have a coding style preference stall here.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel