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

[Erwan LE RAY <erwan.leray@foss.st.com>]

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.