Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> writes:
> On Wed, Jun 26, 2019 at 05:11:29PM +0300, Sergey Organov wrote:
>> Avoid repeating the same code for rs485 twice.
>>
>> Make it obvious we clear CRTSCTS bit in termios->c_cflag whenever
>> sport->have_rtscts is false.
>>
>> Make it obvious we clear UCR2_IRTS whenever CRTSCTS is set.
>>
>> Reviewed-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Tested-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>> Signed-off-by: Sergey Organov <sorganov@xxxxxxxxx>
>> ---
>> drivers/tty/serial/imx.c | 36 +++++++++++++-----------------------
>> 1 file changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 87802fd..17e2322 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -1567,35 +1567,25 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
>> if ((termios->c_cflag & CSIZE) == CS8)
>> ucr2 |= UCR2_WS;
>>
>> - if (termios->c_cflag & CRTSCTS) {
>> - if (sport->have_rtscts) {
>> - ucr2 &= ~UCR2_IRTS;
>> + if (!sport->have_rtscts)
>> + termios->c_cflag &= ~CRTSCTS;
>>
>> - if (port->rs485.flags & SER_RS485_ENABLED) {
>> - /*
>> - * RTS is mandatory for rs485 operation, so keep
>> - * it under manual control and keep transmitter
>> - * disabled.
>> - */
>> - if (port->rs485.flags &
>> - SER_RS485_RTS_AFTER_SEND)
>> - imx_uart_rts_active(sport, &ucr2);
>> - else
>> - imx_uart_rts_inactive(sport, &ucr2);
>> - } else {
>> - imx_uart_rts_auto(sport, &ucr2);
>> - }
>> - } else {
>> - termios->c_cflag &= ~CRTSCTS;
>> - }
>> - } else if (port->rs485.flags & SER_RS485_ENABLED) {
>> - /* disable transmitter */
>> + if (port->rs485.flags & SER_RS485_ENABLED) {
>> + /*
>> + * RTS is mandatory for rs485 operation, so keep
>> + * it under manual control and keep transmitter
>> + * disabled.
>> + */
>> if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> imx_uart_rts_active(sport, &ucr2);
>> else
>> imx_uart_rts_inactive(sport, &ucr2);
>> - }
>>
>> + } else if (termios->c_cflag & CRTSCTS)
>> + imx_uart_rts_auto(sport, &ucr2);
>
> Here a set of braces is needed even if the body has only a single
> line.
Really? scripts/checkpatch.pl didn't catch this.
If needed, is it essential enough to fix here, as final result has this
chunk different anyway (and with braces)?
>
>> +
>> + if (termios->c_cflag & CRTSCTS)
>> + ucr2 &= ~UCR2_IRTS;
>>
>> if (termios->c_cflag & CSTOPB)
>> ucr2 |= UCR2_STPB;
>
> Is this patch intended to not change semantic? I wonder if it hides a
> fix because if imx_uart_set_termios() was called with termios->c_cflag
> & CRTSCTS and !sport->have_rtscts the rs485 block was not reached. Now
> it is.
As comment says "RTS is mandatory for rs485 operation", I assumed
SER_RS485_ENABLED and !sport->have_rtscts are incompatible, so
there should be no actual semantic change here. I mean:
if (port->rs485.flags & SER_RS485_ENABLED) {
assert(sport->have_rtscts);
should never fire.
Do you think I rather need to put additional check for
sport->have_rtscts inside the SER_RS485_ENABLED case?
Thanks!
-- Sergey
