[PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Harald Seiler]

Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
are 0, the hrtimer is triggered (with timeout 0) which can introduce a
few 100us of additional overhead on slower i.MX platforms.

Implement a fast path when the delays are 0, where the RTS signal is
toggled immediately instead of going through an hrtimer.  This fast path
behaves identical to the code before delay support was implemented.

Signed-off-by: Harald Seiler <hws@denx.de>
---
 drivers/tty/serial/imx.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index df8a0c8b8b29..67bbbb69229d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
 	if (port->rs485.flags & SER_RS485_ENABLED) {
 		if (sport->tx_state == SEND) {
 			sport->tx_state = WAIT_AFTER_SEND;
-			start_hrtimer_ms(&sport->trigger_stop_tx,
+
+			if (port->rs485.delay_rts_after_send > 0) {
+				start_hrtimer_ms(&sport->trigger_stop_tx,
 					 port->rs485.delay_rts_after_send);
-			return;
+				return;
+			}
+
+			/* continue without any delay */
 		}
 
 		if (sport->tx_state == WAIT_AFTER_RTS ||
@@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port)
 				imx_uart_stop_rx(port);
 
 			sport->tx_state = WAIT_AFTER_RTS;
-			start_hrtimer_ms(&sport->trigger_start_tx,
+
+			if (port->rs485.delay_rts_before_send > 0) {
+				start_hrtimer_ms(&sport->trigger_start_tx,
 					 port->rs485.delay_rts_before_send);
-			return;
+				return;
+			}
+
+			/* continue without any delay */
 		}
 
 		if (sport->tx_state == WAIT_AFTER_SEND
-- 
2.34.1

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> few 100us of additional overhead on slower i.MX platforms.
> 
> Implement a fast path when the delays are 0, where the RTS signal is
> toggled immediately instead of going through an hrtimer.  This fast path
> behaves identical to the code before delay support was implemented.
> 
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>  drivers/tty/serial/imx.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index df8a0c8b8b29..67bbbb69229d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
>  	if (port->rs485.flags & SER_RS485_ENABLED) {
>  		if (sport->tx_state == SEND) {
>  			sport->tx_state = WAIT_AFTER_SEND;
> -			start_hrtimer_ms(&sport->trigger_stop_tx,
> +
> +			if (port->rs485.delay_rts_after_send > 0) {
> +				start_hrtimer_ms(&sport->trigger_stop_tx,
>  					 port->rs485.delay_rts_after_send);
> -			return;
> +				return;
> +			}
> +
> +			/* continue without any delay */

Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?

>  		}
>  
>  		if (sport->tx_state == WAIT_AFTER_RTS ||
> @@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port)
>  				imx_uart_stop_rx(port);
>  
>  			sport->tx_state = WAIT_AFTER_RTS;
> -			start_hrtimer_ms(&sport->trigger_start_tx,
> +
> +			if (port->rs485.delay_rts_before_send > 0) {
> +				start_hrtimer_ms(&sport->trigger_start_tx,
>  					 port->rs485.delay_rts_before_send);
> -			return;
> +				return;
> +			}
> +
> +			/* continue without any delay */

Here similar question here about sport->tx_state = WAIT_AFTER_RTS;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Harald Seiler]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

Hi,

On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote:
> On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> > Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> > are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> > few 100us of additional overhead on slower i.MX platforms.
> > 
> > Implement a fast path when the delays are 0, where the RTS signal is
> > toggled immediately instead of going through an hrtimer.  This fast path
> > behaves identical to the code before delay support was implemented.
> > 
> > Signed-off-by: Harald Seiler <hws@denx.de>
> > ---
> >  drivers/tty/serial/imx.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index df8a0c8b8b29..67bbbb69229d 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
> >  	if (port->rs485.flags & SER_RS485_ENABLED) {
> >  		if (sport->tx_state == SEND) {
> >  			sport->tx_state = WAIT_AFTER_SEND;
> > -			start_hrtimer_ms(&sport->trigger_stop_tx,
> > +
> > +			if (port->rs485.delay_rts_after_send > 0) {
> > +				start_hrtimer_ms(&sport->trigger_stop_tx,
> >  					 port->rs485.delay_rts_after_send);
> > -			return;
> > +				return;
> > +			}
> > +
> > +			/* continue without any delay */
> 
> Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?

I am keeping the assignment intentionally, to fall into the
if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF).
I originally had the code structured like this:

	if (port->rs485.delay_rts_after_send > 0) {
		sport->tx_state = WAIT_AFTER_SEND;
		start_hrtimer_ms(&sport->trigger_stop_tx,
			 port->rs485.delay_rts_after_send);
		return;
	} else {
		/* continue without any delay */
		sport->tx_state = WAIT_AFTER_SEND;
	}

This is functionally identical, but maybe a bit more explicit.

Not sure what is more clear to read?

> >  		}
> >  
> >  		if (sport->tx_state == WAIT_AFTER_RTS ||
> > @@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port)
> >  				imx_uart_stop_rx(port);
> >  
> >  			sport->tx_state = WAIT_AFTER_RTS;
> > -			start_hrtimer_ms(&sport->trigger_start_tx,
> > +
> > +			if (port->rs485.delay_rts_before_send > 0) {
> > +				start_hrtimer_ms(&sport->trigger_start_tx,
> >  					 port->rs485.delay_rts_before_send);
> > -			return;
> > +				return;
> > +			}
> > +
> > +			/* continue without any delay */
> 
> Here similar question here about sport->tx_state = WAIT_AFTER_RTS;

Same as above, but with WAIT_AFTER_RTS of course...

-- 
Harald

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 850 bytes --]

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote:
> Hi,
> 
> On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> > > few 100us of additional overhead on slower i.MX platforms.
> > > 
> > > Implement a fast path when the delays are 0, where the RTS signal is
> > > toggled immediately instead of going through an hrtimer.  This fast path
> > > behaves identical to the code before delay support was implemented.
> > > 
> > > Signed-off-by: Harald Seiler <hws@denx.de>
> > > ---
> > >  drivers/tty/serial/imx.c | 18 ++++++++++++++----
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index df8a0c8b8b29..67bbbb69229d 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
> > >  	if (port->rs485.flags & SER_RS485_ENABLED) {
> > >  		if (sport->tx_state == SEND) {
> > >  			sport->tx_state = WAIT_AFTER_SEND;
> > > -			start_hrtimer_ms(&sport->trigger_stop_tx,
> > > +
> > > +			if (port->rs485.delay_rts_after_send > 0) {
> > > +				start_hrtimer_ms(&sport->trigger_stop_tx,
> > >  					 port->rs485.delay_rts_after_send);
> > > -			return;
> > > +				return;
> > > +			}
> > > +
> > > +			/* continue without any delay */
> > 
> > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?
> 
> I am keeping the assignment intentionally, to fall into the
> if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF).
> I originally had the code structured like this:
> 
> 	if (port->rs485.delay_rts_after_send > 0) {
> 		sport->tx_state = WAIT_AFTER_SEND;
> 		start_hrtimer_ms(&sport->trigger_stop_tx,
> 			 port->rs485.delay_rts_after_send);
> 		return;
> 	} else {
> 		/* continue without any delay */
> 		sport->tx_state = WAIT_AFTER_SEND;
> 	}
> 
> This is functionally identical, but maybe a bit more explicit.
> 
> Not sure what is more clear to read?

I didn't oppose to the readability thing. With your patch you skip
starting the stop_tx timer and that would usually care for calling
imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen
with your patch any more.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Harald Seiler]

[-- Attachment #1: Type: text/plain, Size: 3660 bytes --]

Hi,

On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote:
> On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote:
> > Hi,
> > 
> > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote:
> > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> > > > few 100us of additional overhead on slower i.MX platforms.
> > > > 
> > > > Implement a fast path when the delays are 0, where the RTS signal is
> > > > toggled immediately instead of going through an hrtimer.  This fast path
> > > > behaves identical to the code before delay support was implemented.
> > > > 
> > > > Signed-off-by: Harald Seiler <hws@denx.de>
> > > > ---
> > > >  drivers/tty/serial/imx.c | 18 ++++++++++++++----
> > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index df8a0c8b8b29..67bbbb69229d 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
> > > >  	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > >  		if (sport->tx_state == SEND) {
> > > >  			sport->tx_state = WAIT_AFTER_SEND;
> > > > -			start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > +
> > > > +			if (port->rs485.delay_rts_after_send > 0) {
> > > > +				start_hrtimer_ms(&sport->trigger_stop_tx,
> > > >  					 port->rs485.delay_rts_after_send);
> > > > -			return;
> > > > +				return;
> > > > +			}
> > > > +
> > > > +			/* continue without any delay */
> > > 
> > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?
> > 
> > I am keeping the assignment intentionally, to fall into the
> > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF).
> > I originally had the code structured like this:
> > 
> > 	if (port->rs485.delay_rts_after_send > 0) {
> > 		sport->tx_state = WAIT_AFTER_SEND;
> > 		start_hrtimer_ms(&sport->trigger_stop_tx,
> > 			 port->rs485.delay_rts_after_send);
> > 		return;
> > 	} else {
> > 		/* continue without any delay */
> > 		sport->tx_state = WAIT_AFTER_SEND;
> > 	}
> > 
> > This is functionally identical, but maybe a bit more explicit.
> > 
> > Not sure what is more clear to read?
> 
> I didn't oppose to the readability thing. With your patch you skip
> starting the stop_tx timer and that would usually care for calling
> imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen
> with your patch any more.

Not starting the timer is the entire point of the patch - instead, the
code which would run inside the timer callback now runs immediately. To
do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early
return which leads into the if(tx_state == WAIT_AFTER_SEND) below.  This
is the code-path which normally runs later in the hrtimer callback.

I suppose it would have been good to provide more context lines in the
patch... Here is the relevant bit (in the changed version now):

	if (sport->tx_state == SEND) {
		sport->tx_state = WAIT_AFTER_SEND;

		if (port->rs485.delay_rts_after_send > 0) {
			start_hrtimer_ms(&sport->trigger_stop_tx,
				 port->rs485.delay_rts_after_send);
			return;
		}

		/* continue without any delay */
	}

	if (sport->tx_state == WAIT_AFTER_RTS ||
	    sport->tx_state == WAIT_AFTER_SEND) {
		/* ... actual rts toggling ... */

		sport->tx_state = OFF;
	}

Regards,
-- 
Harald

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 850 bytes --]

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Greg Kroah-Hartman]

On Wed, Jan 19, 2022 at 05:59:46PM +0100, Harald Seiler wrote:
> Hi,
> 
> On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote:
> > > Hi,
> > > 
> > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote:
> > > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> > > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> > > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> > > > > few 100us of additional overhead on slower i.MX platforms.
> > > > > 
> > > > > Implement a fast path when the delays are 0, where the RTS signal is
> > > > > toggled immediately instead of going through an hrtimer.  This fast path
> > > > > behaves identical to the code before delay support was implemented.
> > > > > 
> > > > > Signed-off-by: Harald Seiler <hws@denx.de>
> > > > > ---
> > > > >  drivers/tty/serial/imx.c | 18 ++++++++++++++----
> > > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > index df8a0c8b8b29..67bbbb69229d 100644
> > > > > --- a/drivers/tty/serial/imx.c
> > > > > +++ b/drivers/tty/serial/imx.c
> > > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
> > > > >  	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > > >  		if (sport->tx_state == SEND) {
> > > > >  			sport->tx_state = WAIT_AFTER_SEND;
> > > > > -			start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > > +
> > > > > +			if (port->rs485.delay_rts_after_send > 0) {
> > > > > +				start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > >  					 port->rs485.delay_rts_after_send);
> > > > > -			return;
> > > > > +				return;
> > > > > +			}
> > > > > +
> > > > > +			/* continue without any delay */
> > > > 
> > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?
> > > 
> > > I am keeping the assignment intentionally, to fall into the
> > > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF).
> > > I originally had the code structured like this:
> > > 
> > > 	if (port->rs485.delay_rts_after_send > 0) {
> > > 		sport->tx_state = WAIT_AFTER_SEND;
> > > 		start_hrtimer_ms(&sport->trigger_stop_tx,
> > > 			 port->rs485.delay_rts_after_send);
> > > 		return;
> > > 	} else {
> > > 		/* continue without any delay */
> > > 		sport->tx_state = WAIT_AFTER_SEND;
> > > 	}
> > > 
> > > This is functionally identical, but maybe a bit more explicit.
> > > 
> > > Not sure what is more clear to read?
> > 
> > I didn't oppose to the readability thing. With your patch you skip
> > starting the stop_tx timer and that would usually care for calling
> > imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen
> > with your patch any more.
> 
> Not starting the timer is the entire point of the patch - instead, the
> code which would run inside the timer callback now runs immediately. To
> do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early
> return which leads into the if(tx_state == WAIT_AFTER_SEND) below.  This
> is the code-path which normally runs later in the hrtimer callback.
> 
> I suppose it would have been good to provide more context lines in the
> patch... Here is the relevant bit (in the changed version now):
> 
> 	if (sport->tx_state == SEND) {
> 		sport->tx_state = WAIT_AFTER_SEND;
> 
> 		if (port->rs485.delay_rts_after_send > 0) {
> 			start_hrtimer_ms(&sport->trigger_stop_tx,
> 				 port->rs485.delay_rts_after_send);
> 			return;
> 		}
> 
> 		/* continue without any delay */
> 	}
> 
> 	if (sport->tx_state == WAIT_AFTER_RTS ||
> 	    sport->tx_state == WAIT_AFTER_SEND) {
> 		/* ... actual rts toggling ... */
> 
> 		sport->tx_state = OFF;
> 	}
> 

Uwe, any thoughts about if this patch should be taken or not?

thanks,

greg k-h

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4458 bytes --]

Hello Greg,

On Tue, Feb 08, 2022 at 11:03:56AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 19, 2022 at 05:59:46PM +0100, Harald Seiler wrote:
> > On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote:
> > > On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote:
> > > > Hi,
> > > > 
> > > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote:
> > > > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote:
> > > > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send`
> > > > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a
> > > > > > few 100us of additional overhead on slower i.MX platforms.
> > > > > > 
> > > > > > Implement a fast path when the delays are 0, where the RTS signal is
> > > > > > toggled immediately instead of going through an hrtimer.  This fast path
> > > > > > behaves identical to the code before delay support was implemented.
> > > > > > 
> > > > > > Signed-off-by: Harald Seiler <hws@denx.de>
> > > > > > ---
> > > > > >  drivers/tty/serial/imx.c | 18 ++++++++++++++----
> > > > > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > > > index df8a0c8b8b29..67bbbb69229d 100644
> > > > > > --- a/drivers/tty/serial/imx.c
> > > > > > +++ b/drivers/tty/serial/imx.c
> > > > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port)
> > > > > >  	if (port->rs485.flags & SER_RS485_ENABLED) {
> > > > > >  		if (sport->tx_state == SEND) {
> > > > > >  			sport->tx_state = WAIT_AFTER_SEND;
> > > > > > -			start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > > > +
> > > > > > +			if (port->rs485.delay_rts_after_send > 0) {
> > > > > > +				start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > > >  					 port->rs485.delay_rts_after_send);
> > > > > > -			return;
> > > > > > +				return;
> > > > > > +			}
> > > > > > +
> > > > > > +			/* continue without any delay */
> > > > > 
> > > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ?
> > > > 
> > > > I am keeping the assignment intentionally, to fall into the
> > > > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF).
> > > > I originally had the code structured like this:
> > > > 
> > > > 	if (port->rs485.delay_rts_after_send > 0) {
> > > > 		sport->tx_state = WAIT_AFTER_SEND;
> > > > 		start_hrtimer_ms(&sport->trigger_stop_tx,
> > > > 			 port->rs485.delay_rts_after_send);
> > > > 		return;
> > > > 	} else {
> > > > 		/* continue without any delay */
> > > > 		sport->tx_state = WAIT_AFTER_SEND;
> > > > 	}
> > > > 
> > > > This is functionally identical, but maybe a bit more explicit.
> > > > 
> > > > Not sure what is more clear to read?
> > > 
> > > I didn't oppose to the readability thing. With your patch you skip
> > > starting the stop_tx timer and that would usually care for calling
> > > imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen
> > > with your patch any more.
> > 
> > Not starting the timer is the entire point of the patch - instead, the
> > code which would run inside the timer callback now runs immediately. To
> > do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early
> > return which leads into the if(tx_state == WAIT_AFTER_SEND) below.  This
> > is the code-path which normally runs later in the hrtimer callback.
> > 
> > I suppose it would have been good to provide more context lines in the
> > patch... Here is the relevant bit (in the changed version now):
> > 
> > 	if (sport->tx_state == SEND) {
> > 		sport->tx_state = WAIT_AFTER_SEND;
> > 
> > 		if (port->rs485.delay_rts_after_send > 0) {
> > 			start_hrtimer_ms(&sport->trigger_stop_tx,
> > 				 port->rs485.delay_rts_after_send);
> > 			return;
> > 		}
> > 
> > 		/* continue without any delay */
> > 	}
> > 
> > 	if (sport->tx_state == WAIT_AFTER_RTS ||
> > 	    sport->tx_state == WAIT_AFTER_SEND) {
> > 		/* ... actual rts toggling ... */
> > 
> > 		sport->tx_state = OFF;
> > 	}
> > 
> 
> Uwe, any thoughts about if this patch should be taken or not?

I will take a deeper look later today and tell you my thoughts.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

Hello Greg,

On Tue, Feb 08, 2022 at 12:12:03PM +0100, Uwe Kleine-König wrote:
> On Tue, Feb 08, 2022 at 11:03:56AM +0100, Greg Kroah-Hartman wrote:
> > Uwe, any thoughts about if this patch should be taken or not?
> 
> I will take a deeper look later today and tell you my thoughts.

It took a bit longer, but after looking again with the driver open in my
editor I agree that patch is fine.

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]