diff mbox series

[v2,3/7] serial: imx: set_termios(): clarify RTS/CTS bits calculation

Message ID 1561558293-7683-4-git-send-email-sorganov@gmail.com (mailing list archive)
State New, archived
Headers show
Series serial: imx: fix RTS and RTS/CTS handling | expand

Commit Message

Sergey Organov June 26, 2019, 2:11 p.m. UTC 
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@pengutronix.de>
Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

Comments

Uwe Kleine-König June 27, 2019, 5:26 a.m. UTC | #1 
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@pengutronix.de>
> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  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.

> +
> +	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.

Best regards
Uwe
Sergey Organov June 27, 2019, 5:58 a.m. UTC | #2 
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 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@pengutronix.de>
>> Tested-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  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
diff mbox series

Patch

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);
+
+	if (termios->c_cflag & CRTSCTS)
+		ucr2 &= ~UCR2_IRTS;
 
 	if (termios->c_cflag & CSTOPB)
 		ucr2 |= UCR2_STPB;