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

[Marek Vasut <marex@denx.de>]

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.

[...]