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

[Marek Vasut <marex@denx.de>]

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.