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.
