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.
