[PATCH v5 0/8] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[PATCH v5 0/8] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[Tapio Reijonen]

The MAX310X hardware can express at most 15 bit-times of RS485 RTS
setup/hold delay, while struct serial_rs485 expresses the delays in
milliseconds. The driver rejected anything above 0x0f with -ERANGE,
upon which uart_rs485_config() wipes port->rs485 and silently disables
RS485 - a device tree asking for a 20 ms setup delay boots with RS485
off and an unusable bus. The values that were accepted got written
into HDPIXDELAY unconverted, milliseconds as bit-times.

Patches 1-4 fix pre-existing bugs found on the way: a termios write
clobbering an active break; breaks never reaching the wire on RS485
ports because auto-RTS only drives the transceiver for FIFO data; the
milliseconds-as-bit-times unit bug; and close() truncating the final
character because tx_empty() does not cover the transmit shift
register. Patch 5 adds active-low RTS on the hardware path via
IRDA.RTSINVERT. Patch 6 is preparation, and patch 7 adds the
software-timed RTS path that takes over whenever the hardware cannot
represent the requested timing, clamping the delays to the UART core's
maximum instead of rejecting them. Patch 8 fixes a reconfigure-versus-
write race the asynchronous rs485 config application has had since
2016, which the software path would have made worse.

v4 was all of this in a single patch; Greg asked for it to be broken
up into one change at a time [1]. Splitting it meant re-verifying each
patch in isolation on hardware, and that re-verification found two
bugs v4 contained: a set_termios() or TIOCSRS485 during an active
break released the transceiver mid-break while the break bookkeeping
still looked correct (prevented by the tx_break ownership guard in
patches 2 and 3), and the patch-8 race, where a TIOCSRS485 followed
immediately by a write could put an entire transfer on the wire with
the transceiver released.

Tested on a MAX14830 (SPI, i.MX6SX) driving RS485 transceivers: for
each patch the bug it fixes was first reproduced on the wire with a
logic analyzer against the kernel one patch earlier, then shown fixed.
The complete series additionally passed an automated 25-scenario
regression matrix covering both RTS paths, both polarities,
RS485/RS232 mode round-trips, close-during-transmission, and termios/
TIOCSRS485 disturbances landing in every envelope phase (setup, data,
hold, break), each scenario checked both on the wire and against the
driver's reported state.

Changes in v5, beyond the split:
- teardown interlock (tx_teardown): shutdown() and the rs485-disable
  path set it under port->lock, and start_tx() checks it on entry and
  again after retaking the dropped lock, so a racing write can no
  longer re-arm the delay timer or queue RTS work against a port being
  torn down (addresses the remaining review-bot findings on v4)
- shutdown() also cancels tx_work, previously only cancelled in
  remove()
- the per-character duration is stored as unsigned int microseconds
  instead of ktime_t: single-copy atomic on 32-bit, so a torn read of
  the 64-bit value is gone by construction
- the TXEMPTY handling documents that the interrupt latches on the
  FIFO becoming empty, so a stale interrupt cannot pump data during an
  RTS setup delay
- new in v5: the tx_break ownership guard (patches 2/3) and the
  reconfigure-pending gate (patch 8), both found during the per-patch
  hardware re-testing described above
- also new in v5, from a review pass over the split series: startup()
  clears a latched break (nothing clears TXBREAK when a port is closed
  with a break still asserted - 8250 does the same); a reconfigure
  arriving during a break is now deferred and applied at break-end
  instead of partially dropped; the rs485-config worker runs under
  port->mutex so its break-guarded register writes cannot straddle a
  break edge; the termios-path idle settle re-checks tx_state after
  writing and requeues rts_work if an envelope started meanwhile; and
  the hardware-delay ceiling is computed in u64

[1] https://lore.kernel.org/all/2026092326-truth-unweave-c773@gregkh/

---
Tapio Reijonen (8):
      serial: max310x: don't clobber the TX break bit in set_termios
      serial: max310x: assert the transceiver during a break
      serial: max310x: convert RS485 delays from milliseconds to bit-times
      serial: max310x: wait for TX to drain before powering down in shutdown
      serial: max310x: support active-low RTS on the hardware path
      serial: max310x: schedule tx_work directly from the IRQ handler
      serial: max310x: drive RTS in software when hardware delays are too short
      serial: max310x: don't transmit while an RS485 reconfigure is pending

 drivers/tty/serial/max310x.c | 511 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 477 insertions(+), 34 deletions(-)
---
base-commit: 9505146e885b1a842118aa6410f737290c4a5a32
change-id: 20260513-max310x-rs485-sw-delay-a306d783d529

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

[PATCH v5 1/8] serial: max310x: don't clobber the TX break bit in set_termios

[Tapio Reijonen]

max310x_set_termios() writes the LCR register absolutely, but LCR also
carries the TX break bit that max310x_break_ctl() drives. A break is a
state, not an event: TIOCSBRK sets the bit and it must stay set until
TIOCCBRK. Any termios change in between - no concurrency required -
rewrites LCR from the termios bits alone and silently ends the break
early.

Update only the LCR bits that are derived from termios and leave the
TX break and RTS pin control bits untouched. Since nothing clears a
break when a port is closed with the break still asserted - the tty
core sends no break-off on release, and the absolute write here was
the accidental recovery - clear TXBREAK in startup(), the same way
8250 does.

Fixes: f65444187a66 ("serial: New serial driver MAX310X")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 022502986c5fcf1ff4de9328746ddc71677be730..4c1e10e0765f45a51e0c74ca965588f39872efd5 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -158,6 +158,8 @@
 #define MAX310X_LCR_FORCEPARITY_BIT	(1 << 5) /* 9-bit multidrop parity */
 #define MAX310X_LCR_TXBREAK_BIT		(1 << 6) /* TX break enable */
 #define MAX310X_LCR_RTS_BIT		(1 << 7) /* RTS pin control */
+/* LCR bits owned by termios; TX break and RTS are driven elsewhere */
+#define MAX310X_LCR_TERMIOS_MASK	GENMASK(5, 0)
 
 /* IRDA register bits */
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
@@ -969,8 +971,12 @@ static void max310x_set_termios(struct uart_port *port,
 	if (termios->c_cflag & CSTOPB)
 		lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
 
-	/* Update LCR register */
-	max310x_port_write(port, MAX310X_LCR_REG, lcr);
+	/*
+	 * Update LCR register. Leave the TX break bit alone: it is driven by
+	 * break_ctl(), and an absolute write here would end a break in
+	 * progress.
+	 */
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
 
 	/* Set read status mask */
 	port->read_status_mask = MAX310X_LSR_RXOVR_BIT;
@@ -1088,6 +1094,13 @@ static int max310x_startup(struct uart_port *port)
 
 	max310x_power(port, 1);
 
+	/*
+	 * Clear a latched break: nothing clears TXBREAK when a port is
+	 * closed with a break still asserted, and set_termios() no longer
+	 * rewrites it.
+	 */
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
+
 	/* Configure MODE1 register */
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);

-- 
2.47.3

Re: [PATCH v5 1/8] serial: max310x: don't clobber the TX break bit in set_termios

[Hugo Villeneuve]

Hi Tapio,

On Tue, 29 Sep 2026 09:37:55 +0000
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> max310x_set_termios() writes the LCR register absolutely, but LCR also

What do you mean by "absolutely"? I think you should rephrase that...


> carries the TX break bit that max310x_break_ctl() drives. A break is a
> state, not an event: TIOCSBRK sets the bit and it must stay set until
> TIOCCBRK. Any termios change in between - no concurrency required -
> rewrites LCR from the termios bits alone and silently ends the break
> early.
> 
> Update only the LCR bits that are derived from termios and leave the
> TX break and RTS pin control bits untouched. Since nothing clears a
> break when a port is closed with the break still asserted - the tty
> core sends no break-off on release, and the absolute write here was
> the accidental recovery - clear TXBREAK in startup(), the same way
> 8250 does.
> 
> Fixes: f65444187a66 ("serial: New serial driver MAX310X")
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
>  drivers/tty/serial/max310x.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 022502986c5fcf1ff4de9328746ddc71677be730..4c1e10e0765f45a51e0c74ca965588f39872efd5 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -158,6 +158,8 @@
>  #define MAX310X_LCR_FORCEPARITY_BIT	(1 << 5) /* 9-bit multidrop parity */
>  #define MAX310X_LCR_TXBREAK_BIT		(1 << 6) /* TX break enable */
>  #define MAX310X_LCR_RTS_BIT		(1 << 7) /* RTS pin control */
> +/* LCR bits owned by termios; TX break and RTS are driven elsewhere */
> +#define MAX310X_LCR_TERMIOS_MASK	GENMASK(5, 0)
>  
>  /* IRDA register bits */
>  #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
> @@ -969,8 +971,12 @@ static void max310x_set_termios(struct uart_port *port,
>  	if (termios->c_cflag & CSTOPB)
>  		lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
>  
> -	/* Update LCR register */
> -	max310x_port_write(port, MAX310X_LCR_REG, lcr);
> +	/*
> +	 * Update LCR register. Leave the TX break bit alone: it is driven by
> +	 * break_ctl(), and an absolute write here would end a break in

Same here


> +	 * progress.
> +	 */
> +	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
>  
>  	/* Set read status mask */
>  	port->read_status_mask = MAX310X_LSR_RXOVR_BIT;
> @@ -1088,6 +1094,13 @@ static int max310x_startup(struct uart_port *port)
>  
>  	max310x_power(port, 1);
>  
> +	/*
> +	 * Clear a latched break: nothing clears TXBREAK when a port is
> +	 * closed with a break still asserted, and set_termios() no longer
> +	 * rewrites it.
> +	 */
> +	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
> +
>  	/* Configure MODE1 register */
>  	max310x_port_update(port, MAX310X_MODE1_REG,
>  			    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
> 
> -- 
> 2.47.3
> 
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v5 1/8] serial: max310x: don't clobber the TX break bit in set_termios

[Tapio Reijonen]

Hi Hugo,

On Tue, 29 Sep 2026 09:40:19 -0400, Hugo Villeneuve wrote:
> > max310x_set_termios() writes the LCR register absolutely, but LCR
> > also
>
> What do you mean by "absolutely"? I think you should rephrase that...

Agreed, that reads wrong. v6 says "unconditionally"/"unconditional"
instead, in the commit message and in both code comments (the short
one added here and the expanded one in the software-RTS patch).

Tapio

Re: [PATCH v5 1/8] serial: max310x: don't clobber the TX break bit in set_termios

[Hugo Villeneuve]

On Fri, 2 Oct 2026 10:25:13 +0300
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> Hi Hugo,
> 
> On Tue, 29 Sep 2026 09:40:19 -0400, Hugo Villeneuve wrote:
> > > max310x_set_termios() writes the LCR register absolutely, but LCR
> > > also
> >
> > What do you mean by "absolutely"? I think you should rephrase that...
> 
> Agreed, that reads wrong. v6 says "unconditionally"/"unconditional"
> instead, in the commit message and in both code comments (the short
> one added here and the expanded one in the software-RTS patch).

Ok, so if I understand correctly you mean that max310x_set_termios()
overwrites the whole LCR register?



-- 
Hugo Villeneuve

[PATCH v5 2/8] serial: max310x: assert the transceiver during a break

[Tapio Reijonen]

The chip's auto-RTS (MODE1.TRNSCVCTRL) asserts the RS485 transceiver
only while FIFO data is shifting out, and a break is not FIFO data: on
an RS485 port a requested break sets the TX break bit but the
transceiver is never enabled, so the break never reaches the wire.
Break-led protocols cannot work at all.

Disable auto-RTS for the break duration and drive RTS manually via the
LCR RTS bit, then restore auto-RTS when the break ends. Track the break
in tx_break and leave MODE1 alone in the rs485-config worker while it
is set - a TIOCSRS485 arriving mid-break would otherwise re-enable
auto-RTS on top of the manual RTS and release the transceiver before
the break ends; break_ctl() restores auto-RTS from the then-current
configuration when the break completes. The worker runs under
port->mutex - break_ctl() and set_termios() already do - so the
tx_break test and the MODE1 write cannot straddle a break starting or
ending, and startup() clears tx_break alongside the latched TXBREAK
bit, since a port can be closed with a break still asserted.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 4c1e10e0765f45a51e0c74ca965588f39872efd5..693decd04de104051b07357973364bd587ab3d91 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -298,6 +298,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	bool			tx_break;	/* break_ctl() owns the transceiver */
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
@@ -682,6 +683,12 @@ static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int l
 	regmap_noinc_read(one->regmap, MAX310X_RHR_REG, rxbuf, len);
 }
 
+static void max310x_rts_ctl(struct uart_port *port, bool rts_state)
+{
+	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_RTS_BIT,
+			    rts_state ? MAX310X_LCR_RTS_BIT : 0);
+}
+
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -929,9 +936,27 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 static void max310x_break_ctl(struct uart_port *port, int break_state)
 {
+	struct max310x_one *one = to_max310x_port(port);
+
+	one->tx_break = break_state;
+
 	max310x_port_update(port, MAX310X_LCR_REG,
 			    MAX310X_LCR_TXBREAK_BIT,
 			    break_state ? MAX310X_LCR_TXBREAK_BIT : 0);
+
+	if (!(port->rs485.flags & SER_RS485_ENABLED))
+		return;
+
+	/*
+	 * The chip's auto-RTS asserts the transceiver only while FIFO data is
+	 * shifting out, and a break is not FIFO data. Disable auto-RTS for the
+	 * break duration and drive RTS manually so the break reaches the wire;
+	 * restore auto-RTS when the break ends.
+	 */
+	max310x_port_update(port, MAX310X_MODE1_REG,
+			    MAX310X_MODE1_TRNSCVCTRL_BIT,
+			    break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
+	max310x_rts_ctl(port, break_state);
 }
 
 static void max310x_set_termios(struct uart_port *port,
@@ -1055,6 +1080,13 @@ static void max310x_rs_proc(struct work_struct *ws)
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
 	unsigned int delay, mode1 = 0, mode2 = 0;
 
+	/*
+	 * Serialize against break_ctl() and set_termios(), which run under
+	 * port->mutex: the tx_break test below and the MODE1 write must not
+	 * straddle a break starting or ending.
+	 */
+	guard(mutex)(&one->port.state->port.mutex);
+
 	delay = (one->port.rs485.delay_rts_before_send << 4) |
 		one->port.rs485.delay_rts_after_send;
 	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
@@ -1066,8 +1098,14 @@ static void max310x_rs_proc(struct work_struct *ws)
 			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
 	}
 
-	max310x_port_update(&one->port, MAX310X_MODE1_REG,
-			MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+	/*
+	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
+	 * drives RTS manually, and restores it when the break ends. Leave
+	 * MODE1 alone meanwhile or the break goes undriven mid-way.
+	 */
+	if (!one->tx_break)
+		max310x_port_update(&one->port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
 			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
@@ -1090,6 +1128,7 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
 
 static int max310x_startup(struct uart_port *port)
 {
+	struct max310x_one *one = to_max310x_port(port);
 	unsigned int val;
 
 	max310x_power(port, 1);
@@ -1099,6 +1138,7 @@ static int max310x_startup(struct uart_port *port)
 	 * closed with a break still asserted, and set_termios() no longer
 	 * rewrites it.
 	 */
+	one->tx_break = false;
 	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);
 
 	/* Configure MODE1 register */

-- 
2.47.3

[PATCH v5 3/8] serial: max310x: convert RS485 delays from milliseconds to bit-times

[Tapio Reijonen]

The HDPIXDELAY register counts the RTS setup and hold delays in
bit-times, four bits per direction, but the driver has been writing the
struct serial_rs485 delay_rts_before_send/delay_rts_after_send values
into it unconverted - and the uapi expresses those in milliseconds. A
requested 9 ms setup delay is programmed as 9 bit-times, which at 9600
baud is 0.94 ms, roughly a tenth of what userspace asked for; the error
grows with the baud rate.

Cache the baud rate in set_termios() and convert the delays to
bit-times at the current rate, rounding up so the delay on the wire is
never shorter than requested, and capping at the 15 bit-times the
4-bit field can hold. Centralize the HDPIXDELAY and MODE1.TRNSCVCTRL
programming in max310x_set_rts_ctl_params(), called from set_termios()
(the conversion depends on the baud rate), the rs485-config worker and
startup(), which each had their own copy.

The rs485-config worker's break guard moves into the helper with the
MODE1 write it protects, and break-off now reapplies the current
configuration through the helper instead of hand-restoring MODE1, so a
reconfigure that arrived during the break takes effect when the break
ends instead of being dropped.

The delays a 4-bit bit-time field can represent still fall well short
of the milliseconds the uapi can express; requests beyond 15 bit-times
are capped, and the -ERANGE rejection of values above 15 ms remains in
place for now.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 120 +++++++++++++++++++++++++++++--------------
 1 file changed, 82 insertions(+), 38 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 693decd04de104051b07357973364bd587ab3d91..f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -165,6 +165,10 @@
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
 #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
 
+/* HDPIXDELAY accessor macros */
+#define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
+#define MAX310X_HDPIXDELAY_HOLD(val)	((val) & 0x0f)
+
 /* Flow control trigger level register masks */
 #define MAX310X_FLOWLVL_HALT_MASK	GENMASK(3, 0) /* Flow control halt level */
 #define MAX310X_FLOWLVL_RES_MASK	GENMASK(7, 4) /* Flow control resume level */
@@ -298,6 +302,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	unsigned int		baud;
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
@@ -934,6 +939,48 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	schedule_work(&one->md_work);
 }
 
+/*
+ * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
+ * count bit-times, four bits per direction, while the uapi expresses the
+ * delays in milliseconds: convert at the current baud rate, rounding up, and
+ * cap at the field maximum.
+ */
+static void max310x_set_rts_ctl_params(struct max310x_one *one)
+{
+	const unsigned int max_bit_dly = 15;
+	struct uart_port *port = &one->port;
+	unsigned int setup = 0, hold = 0;
+	u8 mode1 = 0;
+
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		/* Convert milliseconds to bit-times, rounding up. */
+		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
+				     MSEC_PER_SEC);
+		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
+				     MSEC_PER_SEC);
+		setup = min(setup, max_bit_dly);
+		hold  = min(hold,  max_bit_dly);
+
+		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+	}
+
+	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
+			   MAX310X_HDPIXDELAY_SETUP(setup) |
+			   MAX310X_HDPIXDELAY_HOLD(hold));
+
+	/*
+	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
+	 * drives RTS manually, and restores it from the current
+	 * configuration when the break ends. Touching MODE1 here would
+	 * release the transceiver mid-break.
+	 */
+	if (one->tx_break)
+		return;
+
+	max310x_port_update(port, MAX310X_MODE1_REG,
+			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+}
+
 static void max310x_break_ctl(struct uart_port *port, int break_state)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -953,10 +1000,20 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 	 * break duration and drive RTS manually so the break reaches the wire;
 	 * restore auto-RTS when the break ends.
 	 */
-	max310x_port_update(port, MAX310X_MODE1_REG,
-			    MAX310X_MODE1_TRNSCVCTRL_BIT,
-			    break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
-	max310x_rts_ctl(port, break_state);
+	if (break_state) {
+		max310x_port_update(port, MAX310X_MODE1_REG,
+				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+		max310x_rts_ctl(port, 1);
+	} else {
+		/*
+		 * Reapply the current configuration: a reconfigure that
+		 * arrived during the break was deferred by the tx_break
+		 * guard. Then release the manual RTS - auto-RTS owns the
+		 * pin again.
+		 */
+		max310x_set_rts_ctl_params(one);
+		max310x_rts_ctl(port, 0);
+	}
 }
 
 static void max310x_set_termios(struct uart_port *port,
@@ -1073,41 +1130,35 @@ static void max310x_set_termios(struct uart_port *port,
 
 	/* Update timeout according to new baud rate */
 	uart_update_timeout(port, termios->c_cflag, baud);
+
+	/*
+	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
+	 * millisecond-to-bit-time conversion depends on it.
+	 */
+	to_max310x_port(port)->baud = baud;
+	max310x_set_rts_ctl_params(to_max310x_port(port));
 }
 
 static void max310x_rs_proc(struct work_struct *ws)
 {
 	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
-	unsigned int delay, mode1 = 0, mode2 = 0;
+	unsigned int mode2 = 0;
 
 	/*
 	 * Serialize against break_ctl() and set_termios(), which run under
-	 * port->mutex: the tx_break test below and the MODE1 write must not
+	 * port->mutex: the tx_break-guarded register writes must not
 	 * straddle a break starting or ending.
 	 */
 	guard(mutex)(&one->port.state->port.mutex);
 
-	delay = (one->port.rs485.delay_rts_before_send << 4) |
-		one->port.rs485.delay_rts_after_send;
-	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
+	max310x_set_rts_ctl_params(one);
 
-	if (one->port.rs485.flags & SER_RS485_ENABLED) {
-		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+	if (one->port.rs485.flags & SER_RS485_ENABLED &&
+	    !(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
+		mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
 
-		if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
-			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
-	}
-
-	/*
-	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
-	 * drives RTS manually, and restores it when the break ends. Leave
-	 * MODE1 alone meanwhile or the break goes undriven mid-way.
-	 */
-	if (!one->tx_break)
-		max310x_port_update(&one->port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
-			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
+			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
 
 static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
@@ -1151,21 +1202,14 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
-	/* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
-	val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
-		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
-	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
+	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
+	max310x_set_rts_ctl_params(one);
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		max310x_port_update(port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT);
-
-		if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
-			max310x_port_update(port, MAX310X_MODE2_REG,
-					    MAX310X_MODE2_ECHOSUPR_BIT,
-					    MAX310X_MODE2_ECHOSUPR_BIT);
-	}
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+		max310x_port_update(port, MAX310X_MODE2_REG,
+				    MAX310X_MODE2_ECHOSUPR_BIT,
+				    MAX310X_MODE2_ECHOSUPR_BIT);
 
 	/*
 	 * Configure flow control levels:

-- 
2.47.3

Re: [PATCH v5 3/8] serial: max310x: convert RS485 delays from milliseconds to bit-times

[Hugo Villeneuve]

Hi Tapio,

On Tue, 29 Sep 2026 09:37:57 +0000
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> The HDPIXDELAY register counts the RTS setup and hold delays in
> bit-times, four bits per direction, but the driver has been writing the
> struct serial_rs485 delay_rts_before_send/delay_rts_after_send values
> into it unconverted - and the uapi expresses those in milliseconds. A
> requested 9 ms setup delay is programmed as 9 bit-times, which at 9600
> baud is 0.94 ms, roughly a tenth of what userspace asked for; the error
> grows with the baud rate.
> 
> Cache the baud rate in set_termios() and convert the delays to
> bit-times at the current rate, rounding up so the delay on the wire is
> never shorter than requested, and capping at the 15 bit-times the
> 4-bit field can hold. Centralize the HDPIXDELAY and MODE1.TRNSCVCTRL
> programming in max310x_set_rts_ctl_params(), called from set_termios()
> (the conversion depends on the baud rate), the rs485-config worker and
> startup(), which each had their own copy.
> 
> The rs485-config worker's break guard moves into the helper with the
> MODE1 write it protects, and break-off now reapplies the current
> configuration through the helper instead of hand-restoring MODE1, so a
> reconfigure that arrived during the break takes effect when the break
> ends instead of being dropped.
> 
> The delays a 4-bit bit-time field can represent still fall well short
> of the milliseconds the uapi can express; requests beyond 15 bit-times
> are capped, and the -ERANGE rejection of values above 15 ms remains in
> place for now.
> 
> Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
>  drivers/tty/serial/max310x.c | 120 +++++++++++++++++++++++++++++--------------
>  1 file changed, 82 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 693decd04de104051b07357973364bd587ab3d91..f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -165,6 +165,10 @@
>  #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
>  #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
>  
> +/* HDPIXDELAY accessor macros */
> +#define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
> +#define MAX310X_HDPIXDELAY_HOLD(val)	((val) & 0x0f)
> +
>  /* Flow control trigger level register masks */
>  #define MAX310X_FLOWLVL_HALT_MASK	GENMASK(3, 0) /* Flow control halt level */
>  #define MAX310X_FLOWLVL_RES_MASK	GENMASK(7, 4) /* Flow control resume level */
> @@ -298,6 +302,7 @@ struct max310x_one {
>  	struct work_struct	md_work;
>  	struct work_struct	rs_work;
>  	struct regmap		*regmap;
> +	unsigned int		baud;
>  	bool			tx_break;	/* break_ctl() owns the transceiver */
>  
>  	u8 rx_buf[MAX310X_FIFO_SIZE];
> @@ -934,6 +939,48 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	schedule_work(&one->md_work);
>  }
>  
> +/*
> + * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
> + * count bit-times, four bits per direction, while the uapi expresses the
> + * delays in milliseconds: convert at the current baud rate, rounding up, and
> + * cap at the field maximum.
> + */
> +static void max310x_set_rts_ctl_params(struct max310x_one *one)
> +{
> +	const unsigned int max_bit_dly = 15;
> +	struct uart_port *port = &one->port;
> +	unsigned int setup = 0, hold = 0;
> +	u8 mode1 = 0;
> +
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		/* Convert milliseconds to bit-times, rounding up. */
> +		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
> +				     MSEC_PER_SEC);
> +		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
> +				     MSEC_PER_SEC);
> +		setup = min(setup, max_bit_dly);
> +		hold  = min(hold,  max_bit_dly);
> +
> +		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
> +	}
> +
> +	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
> +			   MAX310X_HDPIXDELAY_SETUP(setup) |
> +			   MAX310X_HDPIXDELAY_HOLD(hold));
> +
> +	/*
> +	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
> +	 * drives RTS manually, and restores it from the current
> +	 * configuration when the break ends. Touching MODE1 here would
> +	 * release the transceiver mid-break.
> +	 */
> +	if (one->tx_break)
> +		return;
> +
> +	max310x_port_update(port, MAX310X_MODE1_REG,
> +			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
> +}
> +
>  static void max310x_break_ctl(struct uart_port *port, int break_state)
>  {
>  	struct max310x_one *one = to_max310x_port(port);
> @@ -953,10 +1000,20 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
>  	 * break duration and drive RTS manually so the break reaches the wire;
>  	 * restore auto-RTS when the break ends.
>  	 */
> -	max310x_port_update(port, MAX310X_MODE1_REG,
> -			    MAX310X_MODE1_TRNSCVCTRL_BIT,
> -			    break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
> -	max310x_rts_ctl(port, break_state);
> +	if (break_state) {
> +		max310x_port_update(port, MAX310X_MODE1_REG,
> +				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
> +		max310x_rts_ctl(port, 1);
> +	} else {
> +		/*
> +		 * Reapply the current configuration: a reconfigure that
> +		 * arrived during the break was deferred by the tx_break
> +		 * guard. Then release the manual RTS - auto-RTS owns the
> +		 * pin again.
> +		 */
> +		max310x_set_rts_ctl_params(one);
> +		max310x_rts_ctl(port, 0);
> +	}

Maybe leave original code here to save a few lines:

   max310x_rts_ctl(port, break_state);


>  }
>  
>  static void max310x_set_termios(struct uart_port *port,
> @@ -1073,41 +1130,35 @@ static void max310x_set_termios(struct uart_port *port,
>  
>  	/* Update timeout according to new baud rate */
>  	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +	/*
> +	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
> +	 * millisecond-to-bit-time conversion depends on it.
> +	 */
> +	to_max310x_port(port)->baud = baud;
> +	max310x_set_rts_ctl_params(to_max310x_port(port));
>  }
>  
>  static void max310x_rs_proc(struct work_struct *ws)
>  {
>  	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
> -	unsigned int delay, mode1 = 0, mode2 = 0;
> +	unsigned int mode2 = 0;
>  
>  	/*
>  	 * Serialize against break_ctl() and set_termios(), which run under
> -	 * port->mutex: the tx_break test below and the MODE1 write must not
> +	 * port->mutex: the tx_break-guarded register writes must not
>  	 * straddle a break starting or ending.
>  	 */
>  	guard(mutex)(&one->port.state->port.mutex);
>  
> -	delay = (one->port.rs485.delay_rts_before_send << 4) |
> -		one->port.rs485.delay_rts_after_send;
> -	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
> +	max310x_set_rts_ctl_params(one);
>  
> -	if (one->port.rs485.flags & SER_RS485_ENABLED) {
> -		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
> +	if (one->port.rs485.flags & SER_RS485_ENABLED &&
> +	    !(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +		mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
>  
> -		if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
> -	}
> -
> -	/*
> -	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
> -	 * drives RTS manually, and restores it when the break ends. Leave
> -	 * MODE1 alone meanwhile or the break goes undriven mid-way.
> -	 */
> -	if (!one->tx_break)
> -		max310x_port_update(&one->port, MAX310X_MODE1_REG,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
>  	max310x_port_update(&one->port, MAX310X_MODE2_REG,
> -			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
> +			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
>  }
>  
>  static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
> @@ -1151,21 +1202,14 @@ static int max310x_startup(struct uart_port *port)
>  	max310x_port_update(port, MAX310X_MODE2_REG,
>  			    MAX310X_MODE2_FIFORST_BIT, 0);
>  
> -	/* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
> -	val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
> -		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
> -	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
> +	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
> +	max310x_set_rts_ctl_params(one);
>  
> -	if (port->rs485.flags & SER_RS485_ENABLED) {
> -		max310x_port_update(port, MAX310X_MODE1_REG,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT);
> -
> -		if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> -			max310x_port_update(port, MAX310X_MODE2_REG,
> -					    MAX310X_MODE2_ECHOSUPR_BIT,
> -					    MAX310X_MODE2_ECHOSUPR_BIT);
> -	}
> +	if (port->rs485.flags & SER_RS485_ENABLED &&
> +	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +		max310x_port_update(port, MAX310X_MODE2_REG,
> +				    MAX310X_MODE2_ECHOSUPR_BIT,
> +				    MAX310X_MODE2_ECHOSUPR_BIT);
>  
>  	/*
>  	 * Configure flow control levels:
> 
> -- 
> 2.47.3
> 
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v5 3/8] serial: max310x: convert RS485 delays from milliseconds to bit-times

[Hugo Villeneuve]

Hi Tapio,

On Tue, 29 Sep 2026 09:37:57 +0000
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> The HDPIXDELAY register counts the RTS setup and hold delays in
> bit-times, four bits per direction, but the driver has been writing the
> struct serial_rs485 delay_rts_before_send/delay_rts_after_send values
> into it unconverted - and the uapi expresses those in milliseconds. A
> requested 9 ms setup delay is programmed as 9 bit-times, which at 9600
> baud is 0.94 ms, roughly a tenth of what userspace asked for; the error
> grows with the baud rate.
> 
> Cache the baud rate in set_termios() and convert the delays to
> bit-times at the current rate, rounding up so the delay on the wire is
> never shorter than requested, and capping at the 15 bit-times the
> 4-bit field can hold. Centralize the HDPIXDELAY and MODE1.TRNSCVCTRL
> programming in max310x_set_rts_ctl_params(), called from set_termios()

Could this centralization be done in a separate patch, without
any functional changes intented, in order to facilitate the
review of this patch?


> (the conversion depends on the baud rate), the rs485-config worker and
> startup(), which each had their own copy.
> 
> The rs485-config worker's break guard moves into the helper with the
> MODE1 write it protects, and break-off now reapplies the current
> configuration through the helper instead of hand-restoring MODE1, so a
> reconfigure that arrived during the break takes effect when the break
> ends instead of being dropped.
> 
> The delays a 4-bit bit-time field can represent still fall well short
> of the milliseconds the uapi can express; requests beyond 15 bit-times
> are capped, and the -ERANGE rejection of values above 15 ms remains in
> place for now.
> 
> Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
>  drivers/tty/serial/max310x.c | 120 +++++++++++++++++++++++++++++--------------
>  1 file changed, 82 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 693decd04de104051b07357973364bd587ab3d91..f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -165,6 +165,10 @@
>  #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
>  #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
>  
> +/* HDPIXDELAY accessor macros */
> +#define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
> +#define MAX310X_HDPIXDELAY_HOLD(val)	((val) & 0x0f)
> +
>  /* Flow control trigger level register masks */
>  #define MAX310X_FLOWLVL_HALT_MASK	GENMASK(3, 0) /* Flow control halt level */
>  #define MAX310X_FLOWLVL_RES_MASK	GENMASK(7, 4) /* Flow control resume level */
> @@ -298,6 +302,7 @@ struct max310x_one {
>  	struct work_struct	md_work;
>  	struct work_struct	rs_work;
>  	struct regmap		*regmap;
> +	unsigned int		baud;
>  	bool			tx_break;	/* break_ctl() owns the transceiver */
>  
>  	u8 rx_buf[MAX310X_FIFO_SIZE];
> @@ -934,6 +939,48 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	schedule_work(&one->md_work);
>  }
>  
> +/*
> + * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
> + * count bit-times, four bits per direction, while the uapi expresses the
> + * delays in milliseconds: convert at the current baud rate, rounding up, and
> + * cap at the field maximum.
> + */
> +static void max310x_set_rts_ctl_params(struct max310x_one *one)
> +{
> +	const unsigned int max_bit_dly = 15;
> +	struct uart_port *port = &one->port;
> +	unsigned int setup = 0, hold = 0;
> +	u8 mode1 = 0;
> +
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		/* Convert milliseconds to bit-times, rounding up. */
> +		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
> +				     MSEC_PER_SEC);
> +		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
> +				     MSEC_PER_SEC);
> +		setup = min(setup, max_bit_dly);
> +		hold  = min(hold,  max_bit_dly);
> +
> +		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
> +	}
> +
> +	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
> +			   MAX310X_HDPIXDELAY_SETUP(setup) |
> +			   MAX310X_HDPIXDELAY_HOLD(hold));
> +
> +	/*
> +	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
> +	 * drives RTS manually, and restores it from the current
> +	 * configuration when the break ends. Touching MODE1 here would
> +	 * release the transceiver mid-break.
> +	 */
> +	if (one->tx_break)
> +		return;
> +
> +	max310x_port_update(port, MAX310X_MODE1_REG,
> +			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
> +}
> +
>  static void max310x_break_ctl(struct uart_port *port, int break_state)
>  {
>  	struct max310x_one *one = to_max310x_port(port);
> @@ -953,10 +1000,20 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
>  	 * break duration and drive RTS manually so the break reaches the wire;
>  	 * restore auto-RTS when the break ends.
>  	 */
> -	max310x_port_update(port, MAX310X_MODE1_REG,
> -			    MAX310X_MODE1_TRNSCVCTRL_BIT,
> -			    break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
> -	max310x_rts_ctl(port, break_state);
> +	if (break_state) {
> +		max310x_port_update(port, MAX310X_MODE1_REG,
> +				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
> +		max310x_rts_ctl(port, 1);
> +	} else {
> +		/*
> +		 * Reapply the current configuration: a reconfigure that
> +		 * arrived during the break was deferred by the tx_break
> +		 * guard. Then release the manual RTS - auto-RTS owns the
> +		 * pin again.
> +		 */
> +		max310x_set_rts_ctl_params(one);
> +		max310x_rts_ctl(port, 0);
> +	}
>  }
>  
>  static void max310x_set_termios(struct uart_port *port,
> @@ -1073,41 +1130,35 @@ static void max310x_set_termios(struct uart_port *port,
>  
>  	/* Update timeout according to new baud rate */
>  	uart_update_timeout(port, termios->c_cflag, baud);
> +
> +	/*
> +	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
> +	 * millisecond-to-bit-time conversion depends on it.
> +	 */
> +	to_max310x_port(port)->baud = baud;
> +	max310x_set_rts_ctl_params(to_max310x_port(port));
>  }
>  
>  static void max310x_rs_proc(struct work_struct *ws)
>  {
>  	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
> -	unsigned int delay, mode1 = 0, mode2 = 0;
> +	unsigned int mode2 = 0;
>  
>  	/*
>  	 * Serialize against break_ctl() and set_termios(), which run under
> -	 * port->mutex: the tx_break test below and the MODE1 write must not
> +	 * port->mutex: the tx_break-guarded register writes must not
>  	 * straddle a break starting or ending.
>  	 */
>  	guard(mutex)(&one->port.state->port.mutex);
>  
> -	delay = (one->port.rs485.delay_rts_before_send << 4) |
> -		one->port.rs485.delay_rts_after_send;
> -	max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
> +	max310x_set_rts_ctl_params(one);
>  
> -	if (one->port.rs485.flags & SER_RS485_ENABLED) {
> -		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
> +	if (one->port.rs485.flags & SER_RS485_ENABLED &&
> +	    !(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +		mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
>  
> -		if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -			mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
> -	}
> -
> -	/*
> -	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
> -	 * drives RTS manually, and restores it when the break ends. Leave
> -	 * MODE1 alone meanwhile or the break goes undriven mid-way.
> -	 */
> -	if (!one->tx_break)
> -		max310x_port_update(&one->port, MAX310X_MODE1_REG,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
>  	max310x_port_update(&one->port, MAX310X_MODE2_REG,
> -			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
> +			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
>  }
>  
>  static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
> @@ -1151,21 +1202,14 @@ static int max310x_startup(struct uart_port *port)
>  	max310x_port_update(port, MAX310X_MODE2_REG,
>  			    MAX310X_MODE2_FIFORST_BIT, 0);
>  
> -	/* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
> -	val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
> -		clamp(port->rs485.delay_rts_after_send, 0U, 15U);
> -	max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
> +	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
> +	max310x_set_rts_ctl_params(one);
>  
> -	if (port->rs485.flags & SER_RS485_ENABLED) {
> -		max310x_port_update(port, MAX310X_MODE1_REG,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT,
> -				    MAX310X_MODE1_TRNSCVCTRL_BIT);
> -
> -		if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> -			max310x_port_update(port, MAX310X_MODE2_REG,
> -					    MAX310X_MODE2_ECHOSUPR_BIT,
> -					    MAX310X_MODE2_ECHOSUPR_BIT);
> -	}
> +	if (port->rs485.flags & SER_RS485_ENABLED &&
> +	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +		max310x_port_update(port, MAX310X_MODE2_REG,
> +				    MAX310X_MODE2_ECHOSUPR_BIT,
> +				    MAX310X_MODE2_ECHOSUPR_BIT);
>  
>  	/*
>  	 * Configure flow control levels:
> 
> -- 
> 2.47.3
> 
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v5 3/8] serial: max310x: convert RS485 delays from milliseconds to bit-times

[Tapio Reijonen]

Hi Hugo,

On Thu, 1 Oct 2026 15:11:25 -0400, Hugo Villeneuve wrote:
> > never shorter than requested, and capping at the 15 bit-times the
> > 4-bit field can hold. Centralize the HDPIXDELAY and MODE1.TRNSCVCTRL
> > programming in max310x_set_rts_ctl_params(), called from
> > set_termios() (the conversion depends on the baud rate), the
> > rs485-config worker and startup(), which each had their own copy.
>
> Could this centralization be done in a separate patch, without
> any functional changes intented, in order to facilitate the
> review of this patch?

Yes, good idea. In v6 this becomes two patches: one that only
introduces max310x_set_rts_ctl_params() and folds the three existing
copies into it, still writing the raw millisecond values (no
functional change), and this one, which then only adds the baud
caching and the millisecond-to-bit-time conversion inside the helper.

Your earlier comment on this patch is also folded in: break_ctl()
keeps the single max310x_rts_ctl(port, break_state) call at the end
instead of duplicating it in both branches.

Tapio

[PATCH v5 4/8] serial: max310x: wait for TX to drain before powering down in shutdown

[Tapio Reijonen]

max310x_tx_empty() reports the chip TX FIFO level and nothing else, so
both tcdrain() and the tty layer's wait-until-sent on close() return
while the final character is still clocking out of the transmit shift
register. max310x_shutdown() then powers the port down mid-character
and the last byte is truncated on the wire. At 9600 baud the ~1 ms
window is easy to miss; at 1200 baud a write()-then-close() reliably
corrupts the final byte (observed on the wire: 0x24 transmitted as a
0x04 frame with a framing error).

Wait in shutdown() for the FIFO to drain, bounded by one character
duration per FIFO word, plus one more character for the byte in the
shift register, before powering the port down. The per-character
duration is computed in set_termios() from the frame size and baud
rate.

Fixes: f65444187a66 ("serial: New serial driver MAX310X")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0..cd3b1913aaadba94805f4728b5eba18e970e34c8 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -302,6 +302,7 @@ struct max310x_one {
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
 	struct regmap		*regmap;
+	unsigned int		one_char_duration_us;
 	unsigned int		baud;
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 
@@ -1020,6 +1021,7 @@ static void max310x_set_termios(struct uart_port *port,
 				struct ktermios *termios,
 				const struct ktermios *old)
 {
+	unsigned int frame_bits = tty_get_frame_size(termios->c_cflag);
 	unsigned int lcr = 0, flow = 0;
 	int baud;
 
@@ -1132,10 +1134,13 @@ static void max310x_set_termios(struct uart_port *port,
 	uart_update_timeout(port, termios->c_cflag, baud);
 
 	/*
-	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
-	 * millisecond-to-bit-time conversion depends on it.
+	 * Cache the new baud rate and the time it takes to clock out one
+	 * character, then reprogram the RS485 RTS delays, whose
+	 * millisecond-to-bit-time conversion depends on the baud rate.
 	 */
 	to_max310x_port(port)->baud = baud;
+	to_max310x_port(port)->one_char_duration_us =
+		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);
 	max310x_set_rts_ctl_params(to_max310x_port(port));
 }
 
@@ -1231,6 +1236,20 @@ static int max310x_startup(struct uart_port *port)
 
 static void max310x_shutdown(struct uart_port *port)
 {
+	struct max310x_one *one = to_max310x_port(port);
+	unsigned int loops = port->fifosize + 1;
+
+	/*
+	 * The tty layer waits for tx_empty() before close(), but tx_empty()
+	 * only reflects the chip TX FIFO - the last character may still be in
+	 * the transmit shift register. Let the FIFO drain and the final
+	 * character clock out before the port is powered down, otherwise
+	 * close() truncates the last byte on the wire.
+	 */
+	while (!max310x_tx_empty(port) && loops-- > 0)
+		fsleep(one->one_char_duration_us);
+	fsleep(one->one_char_duration_us);
+
 	/* Disable all interrupts */
 	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
 

-- 
2.47.3

Re: [PATCH v5 4/8] serial: max310x: wait for TX to drain before powering down in shutdown

[Hugo Villeneuve]

Hi Tapio,

On Tue, 29 Sep 2026 09:37:58 +0000
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> max310x_tx_empty() reports the chip TX FIFO level and nothing else, so
> both tcdrain() and the tty layer's wait-until-sent on close() return
> while the final character is still clocking out of the transmit shift
> register. max310x_shutdown() then powers the port down mid-character
> and the last byte is truncated on the wire. At 9600 baud the ~1 ms
> window is easy to miss; at 1200 baud a write()-then-close() reliably
> corrupts the final byte (observed on the wire: 0x24 transmitted as a
> 0x04 frame with a framing error).
> 
> Wait in shutdown() for the FIFO to drain, bounded by one character
> duration per FIFO word, plus one more character for the byte in the
> shift register, before powering the port down. The per-character
> duration is computed in set_termios() from the frame size and baud
> rate.
> 
> Fixes: f65444187a66 ("serial: New serial driver MAX310X")
> Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
> ---
>  drivers/tty/serial/max310x.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0..cd3b1913aaadba94805f4728b5eba18e970e34c8 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -302,6 +302,7 @@ struct max310x_one {
>  	struct work_struct	md_work;
>  	struct work_struct	rs_work;
>  	struct regmap		*regmap;
> +	unsigned int		one_char_duration_us;

char_time_us?


>  	unsigned int		baud;
>  	bool			tx_break;	/* break_ctl() owns the transceiver */
>  
> @@ -1020,6 +1021,7 @@ static void max310x_set_termios(struct uart_port *port,
>  				struct ktermios *termios,
>  				const struct ktermios *old)
>  {
> +	unsigned int frame_bits = tty_get_frame_size(termios->c_cflag);
>  	unsigned int lcr = 0, flow = 0;
>  	int baud;
>  
> @@ -1132,10 +1134,13 @@ static void max310x_set_termios(struct uart_port *port,
>  	uart_update_timeout(port, termios->c_cflag, baud);
>  
>  	/*
> -	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
> -	 * millisecond-to-bit-time conversion depends on it.
> +	 * Cache the new baud rate and the time it takes to clock out one
> +	 * character, then reprogram the RS485 RTS delays, whose
> +	 * millisecond-to-bit-time conversion depends on the baud rate.
>  	 */
>  	to_max310x_port(port)->baud = baud;
> +	to_max310x_port(port)->one_char_duration_us =
> +		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);

Would it be a good idea to if you moved these two lines after
max310x_set_rts_ctl_params(), then you could probably leave the
original comments and simply add a new comment to indicate "Compute
time it takes to clock out one character", simplifying the diff
(review) and readability?

>  	max310x_set_rts_ctl_params(to_max310x_port(port));
>  }
>  
> @@ -1231,6 +1236,20 @@ static int max310x_startup(struct uart_port *port)
>  
>  static void max310x_shutdown(struct uart_port *port)
>  {
> +	struct max310x_one *one = to_max310x_port(port);
> +	unsigned int loops = port->fifosize + 1;

tries?

> +
> +	/*
> +	 * The tty layer waits for tx_empty() before close(), but tx_empty()
> +	 * only reflects the chip TX FIFO - the last character may still be in
> +	 * the transmit shift register. Let the FIFO drain and the final
> +	 * character clock out before the port is powered down, otherwise
> +	 * close() truncates the last byte on the wire.
> +	 */

Based on these comments, does it mean that the FIFO has already been
validated empty at this point by the tty layer, so you don't need the
loop at all, just the unconditional last fsleep()?

> +	while (!max310x_tx_empty(port) && loops-- > 0)
> +		fsleep(one->one_char_duration_us);

For certain combinations of large fifo_sizes and high-baud rates, that
could mean a lot of I2C/SPI transactions?


> +	fsleep(one->one_char_duration_us);
> +
>  	/* Disable all interrupts */
>  	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
>  
> 
> -- 
> 2.47.3
> 
> 
> 


-- 
Hugo Villeneuve

Re: [PATCH v5 4/8] serial: max310x: wait for TX to drain before powering down in shutdown

[Tapio Reijonen]

Hi Hugo,

On Thu, 1 Oct 2026 16:00:20 -0400, Hugo Villeneuve wrote:
> > +	unsigned int		one_char_duration_us;
>
> char_time_us?

Renamed in v6.

> > +	to_max310x_port(port)->baud = baud;
> > +	to_max310x_port(port)->one_char_duration_us =
> > +		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);
>
> Would it be a good idea to if you moved these two lines after
> max310x_set_rts_ctl_params(), then you could probably leave the
> original comments and simply add a new comment to indicate "Compute
> time it takes to clock out one character", simplifying the diff
> (review) and readability?

I would prefer not to move them: the helper consumes both values.
The baud is what the millisecond-to-bit-time conversion divides by,
so it must be cached before the call. And as of v6 the helper can
also arm the after-send hold directly - v6 adds a fix for the case
where a reconfigure moves the port off the hardware RTS path while a
transmission is still in flight, and the takeover computes the hold
from char_time_us - so the character time has to be current at that
point as well.

> > +	unsigned int loops = port->fifosize + 1;
>
> tries?

Renamed in v6.

> Based on these comments, does it mean that the FIFO has already been
> validated empty at this point by the tty layer, so you don't need the
> loop at all, just the unconditional last fsleep()?

No - that wait is not guaranteed. uart_wait_until_sent() runs only on
the close path and is bounded by closing_wait, which can be configured
to none, and hangup reaches shutdown() with no wait at all. In
testing, a vhangup issued mid-transfer entered shutdown() with the
chip FIFO still holding over a hundred characters; this loop is what
drained them before power-down.

> For certain combinations of large fifo_sizes and high-baud rates,
> that could mean a lot of I2C/SPI transactions?

It is bounded at one FIFO-level read per character time, at most
fifosize + 1 of them, only on the close/hangup path, and it stops as
soon as the FIFO reads empty - in total no longer than the remaining
transmit time of the data itself. At high baud rates the character
time shrinks, so the polls get more frequent but the window they can
occupy shrinks with it.

Tapio

Re: [PATCH v5 4/8] serial: max310x: wait for TX to drain before powering down in shutdown

[Hugo Villeneuve]

Hi Tapio,
On Fri, 2 Oct 2026 10:28:59 +0300
Tapio Reijonen <tapio.reijonen@vaisala.com> wrote:

> Hi Hugo,
> 
> On Thu, 1 Oct 2026 16:00:20 -0400, Hugo Villeneuve wrote:
> > > +	unsigned int		one_char_duration_us;
> >
> > char_time_us?
> 
> Renamed in v6.
> 
> > > +	to_max310x_port(port)->baud = baud;
> > > +	to_max310x_port(port)->one_char_duration_us =
> > > +		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);
> >
> > Would it be a good idea to if you moved these two lines after
> > max310x_set_rts_ctl_params(), then you could probably leave the
> > original comments and simply add a new comment to indicate "Compute
> > time it takes to clock out one character", simplifying the diff
> > (review) and readability?
> 
> I would prefer not to move them: the helper consumes both values.
> The baud is what the millisecond-to-bit-time conversion divides by,
> so it must be cached before the call. And as of v6 the helper can
> also arm the after-send hold directly - v6 adds a fix for the case
> where a reconfigure moves the port off the hardware RTS path while a
> transmission is still in flight, and the takeover computes the hold
> from char_time_us - so the character time has to be current at that
> point as well.

I meant only these two lines:
    to_max310x_port(port)->one_char_duration_us =
	DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);

> 
> > > +	unsigned int loops = port->fifosize + 1;
> >
> > tries?
> 
> Renamed in v6.
> 
> > Based on these comments, does it mean that the FIFO has already been
> > validated empty at this point by the tty layer, so you don't need the
> > loop at all, just the unconditional last fsleep()?
> 
> No - that wait is not guaranteed. uart_wait_until_sent() runs only on
> the close path and is bounded by closing_wait, which can be configured
> to none, and hangup reaches shutdown() with no wait at all. In
> testing, a vhangup issued mid-transfer entered shutdown() with the
> chip FIFO still holding over a hundred characters; this loop is what
> drained them before power-down.

Its unfortunate that you trimmed some parts of the original email so we
no longer see the relevant code...

But maybe you should reword/improve your comments then?

> 
> > For certain combinations of large fifo_sizes and high-baud rates,
> > that could mean a lot of I2C/SPI transactions?
> 
> It is bounded at one FIFO-level read per character time, at most
> fifosize + 1 of them, only on the close/hangup path, and it stops as
> soon as the FIFO reads empty - in total no longer than the remaining
> transmit time of the data itself. At high baud rates the character
> time shrinks, so the polls get more frequent but the window they can
> occupy shrinks with it.

So at 115200, this could mean 128 reads each ~90-100 us?

Could using the interrupt to detect tx empty could improve
efficiency and reduce load on I2C/SPI bus?

I am not saying your approach is wrong, but it just made me think of
potential issues and things to consider.

-- 
Hugo Villeneuve

[PATCH v5 5/8] serial: max310x: support active-low RTS on the hardware path

[Tapio Reijonen]

The chip's auto-RTS engine asserts the RTS_ pin high while data is
shifting out, so a transceiver with an active-low driver-enable could
not use the hardware RS485 path at all: SER_RS485_RTS_AFTER_SEND is
not in the supported flags and the core normalizes it away with
"invalid RTS setting, using RTS_ON_SEND instead".

The output stage is invertible: program IRDA.RTSINVERT when the
requested polarity is active-low and advertise SER_RS485_RTS_AFTER_SEND
in rs485_supported. A break already drives break_state onto the RTS
bit unadjusted, which remains correct because RTSINVERT inverts the
output stage itself, not the register value.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index cd3b1913aaadba94805f4728b5eba18e970e34c8..e07fb87f21f102cfa3fdae5275cb7810be1c3dbd 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -164,6 +164,7 @@
 /* IRDA register bits */
 #define MAX310X_IRDA_IRDAEN_BIT		(1 << 0) /* IRDA mode enable */
 #define MAX310X_IRDA_SIR_BIT		(1 << 1) /* SIR mode enable */
+#define MAX310X_IRDA_RTSINVERT_BIT	(1 << 2) /* Invert RTS output */
 
 /* HDPIXDELAY accessor macros */
 #define MAX310X_HDPIXDELAY_SETUP(val)	(((val) & 0x0f) << 4)
@@ -951,7 +952,7 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 	const unsigned int max_bit_dly = 15;
 	struct uart_port *port = &one->port;
 	unsigned int setup = 0, hold = 0;
-	u8 mode1 = 0;
+	u8 mode1 = 0, irda = 0;
 
 	if (port->rs485.flags & SER_RS485_ENABLED) {
 		/* Convert milliseconds to bit-times, rounding up. */
@@ -963,6 +964,12 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 		hold  = min(hold,  max_bit_dly);
 
 		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+		/*
+		 * The auto-RTS engine asserts RTS high on send; for an
+		 * active-low RTS let IRDA.RTSINVERT invert the output stage.
+		 */
+		if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
+			irda = MAX310X_IRDA_RTSINVERT_BIT;
 	}
 
 	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
@@ -980,6 +987,8 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+	max310x_port_update(port, MAX310X_IRDA_REG,
+			    MAX310X_IRDA_RTSINVERT_BIT, irda);
 }
 
 static void max310x_break_ctl(struct uart_port *port, int break_state)
@@ -999,7 +1008,9 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 	 * The chip's auto-RTS asserts the transceiver only while FIFO data is
 	 * shifting out, and a break is not FIFO data. Disable auto-RTS for the
 	 * break duration and drive RTS manually so the break reaches the wire;
-	 * restore auto-RTS when the break ends.
+	 * restore auto-RTS when the break ends. For an active-low RTS,
+	 * IRDA.RTSINVERT already inverts the RTS_ output stage, so break_state
+	 * is driven as it is.
 	 */
 	if (break_state) {
 		max310x_port_update(port, MAX310X_MODE1_REG,
@@ -1407,7 +1418,8 @@ static int max310x_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 #endif
 
 static const struct serial_rs485 max310x_rs485_supported = {
-	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX,
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+		 SER_RS485_RTS_AFTER_SEND | SER_RS485_RX_DURING_TX,
 	.delay_rts_before_send = 1,
 	.delay_rts_after_send = 1,
 };

-- 
2.47.3

[PATCH v5 6/8] serial: max310x: schedule tx_work directly from the IRQ handler

[Tapio Reijonen]

The TXEMPTY interrupt asks for a FIFO refill, and start_tx() does
nothing beyond scheduling tx_work, so going through it makes no
functional difference. It does conflate two distinct events, though:
start_tx() is the serial core starting a new transmission, while
TXEMPTY can only fire for a transmission that is already running -
the bit latches on the FIFO's non-empty to empty transition, and the
IRQ handler's read of IRQSTS consumes the latch, so a stale TXEMPTY
cannot exist on an idle port (if the bit were level-triggered, the
handler's read-until-clear loop would never terminate).

Schedule tx_work directly, keeping the interrupt path a pure FIFO
refill. This is preparation for a following patch that adds
software-timed RS485 RTS control, where start_tx() also starts the
RTS envelope and a refill must not restart it.

No functional change.

Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index e07fb87f21f102cfa3fdae5275cb7810be1c3dbd..8f8817b127c891f18af52bd3d24efb269db78211 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -858,8 +858,12 @@ static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
 		}
 		if (rxlen)
 			max310x_handle_rx(port, rxlen);
+		/*
+		 * TXEMPTY latches on the FIFO becoming empty, so a stale
+		 * interrupt cannot pump data during an RTS setup delay.
+		 */
 		if (ists & MAX310X_IRQ_TXEMPTY_BIT)
-			max310x_start_tx(port);
+			schedule_work(&s->p[portno].tx_work);
 	} while (1);
 
 	return res;

-- 
2.47.3

[PATCH v5 7/8] serial: max310x: drive RTS in software when hardware delays are too short

[Tapio Reijonen]

The chip's auto-RTS path can express at most 15 bit-times of RTS setup
and hold delay - a fraction of a millisecond at typical baud rates -
while the uapi expresses the delays in milliseconds up to the UART
core's RS485_MAX_RTS_DELAY. Requests beyond the field were rejected
with -ERANGE, which makes the core silently wipe port->rs485 and
disable RS485; a device tree asking for a 20 ms setup delay ends up
with no RS485 at all and an unusable bus.

Add a software-driven RTS path that takes over whenever the hardware
cannot represent the requested timing, and clamp the delays to
RS485_MAX_RTS_DELAY instead of rejecting them.

max310x_set_rts_ctl_params() picks the path: software if either delay
exceeds what 15 bit-times encode at the current baud rate, or if the
requested RTS polarity combination cannot be produced by the auto-RTS
engine; hardware otherwise, as before.

On the software path the RTS envelope is driven by a single hrtimer,
re-used for the before- and after-send phases (tracked in tx_state),
plus a single rts_work that toggles RTS. start_tx() begins the
envelope; rts_work asserts RTS and only then arms the before-send
timer, so data is never shifted before RTS is on the wire. The timer
expiry kicks tx_work; once the chip FIFO drains, the same timer is
re-armed for one character (the byte still in the shift register)
plus the after-send delay, after which rts_work releases RTS. One
timer and one work keep the phases mutually exclusive and the RTS
toggles ordered.

Teardown is interlocked: shutdown() and an RS485-disabling
TIOCSRS485 set tx_teardown under port->lock before cancelling the
timer and works, and start_tx() checks it on entry and again after
the hrtimer_try_to_cancel(-1) path retakes the dropped lock -
otherwise a write racing the teardown could re-arm the timer or
queue rts_work against a port being shut down, leaving the
transceiver driving the bus after close. The rs485-disable path
additionally kicks tx_work afterwards, since a racing write may have
queued data with no envelope left to pump it, and shutdown() now also
cancels tx_work, which was previously cancelled only in remove().

set_rts_ctl_params() publishes sw_rts_during_tx with a single store
and settles the RTS idle level only while tx_state is off, because
serial_core calls set_termios() without port->lock and rs485_config()
schedules a reconfigure on every TIOCSRS485 - either could otherwise
release the transceiver mid-envelope. The settle also re-checks
tx_state after its write and requeues rts_work if an envelope started
meanwhile: the state read and the register write are not atomic, and
rts_work re-derives the level from tx_state, so this converges without
locking. shutdown() waits out an
in-flight envelope (bounded) before powering down, and cancels the
timer and works unconditionally: a TIOCSRS485 can clear
sw_rts_during_tx while an envelope is still in flight, and neither
may outlive the port. break_ctl() on the software path applies the
configured RTS polarity itself.

Fixes: 55367c620aed ("serial: max310x: Add support for RS-485 mode")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 386 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 341 insertions(+), 45 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 8f8817b127c891f18af52bd3d24efb269db78211..6ad27eb8a32586ec5719773e6eadf1249155bf19 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio/driver.h>
+#include <linux/hrtimer.h>
 #include <linux/i2c.h>
 #include <linux/kconfig.h>
 #include <linux/module.h>
@@ -297,15 +298,29 @@ struct max310x_devtype {
 	u8	power_bit; /* Bit for sleep or power-off mode (active high). */
 };
 
+/* Software-timed RS485 RTS envelope phase */
+enum max310x_tx_state {
+	MAX310X_TX_OFF,			/* idle, RTS released */
+	MAX310X_TX_WAIT_BEFORE_SEND,	/* RTS asserted, before-send delay */
+	MAX310X_TX_SEND,		/* data in flight, awaiting TX-empty */
+	MAX310X_TX_WAIT_AFTER_SEND,	/* data drained, after-send hold */
+};
+
 struct max310x_one {
 	struct uart_port	port;
 	struct work_struct	tx_work;
 	struct work_struct	md_work;
 	struct work_struct	rs_work;
+	struct work_struct	rts_work;
+	struct hrtimer		tx_delay_tmr;
 	struct regmap		*regmap;
 	unsigned int		one_char_duration_us;
 	unsigned int		baud;
+	bool			sw_rts_during_tx;
+	bool			cancel_tx_delay_tmr;
+	bool			tx_teardown;	/* envelope being torn down */
 	bool			tx_break;	/* break_ctl() owns the transceiver */
+	enum max310x_tx_state	tx_state;
 
 	u8 rx_buf[MAX310X_FIFO_SIZE];
 };
@@ -696,6 +711,36 @@ static void max310x_rts_ctl(struct uart_port *port, bool rts_state)
 			    rts_state ? MAX310X_LCR_RTS_BIT : 0);
 }
 
+/* RTS level for the transmitting or the idle phase of an RS485 envelope */
+static bool max310x_rts_level(struct uart_port *port, bool active)
+{
+	return active ? (port->rs485.flags & SER_RS485_RTS_ON_SEND) :
+			(port->rs485.flags & SER_RS485_RTS_AFTER_SEND);
+}
+
+/*
+ * Drive the RS485 RTS line to match the current tx_state. This is the only
+ * place that touches RTS, and it reads tx_state rather than a fixed
+ * assert/deassert intent, so a newer assert is never clobbered by a stale
+ * release. It also arms the before-send timer once the RTS edge is on the wire,
+ * so data is never shifted before RTS is asserted.
+ */
+static void max310x_rts_work_proc(struct work_struct *ws)
+{
+	struct max310x_one *one = container_of(ws, struct max310x_one, rts_work);
+	struct uart_port *port = &one->port;
+	bool rts_on = READ_ONCE(one->tx_state) != MAX310X_TX_OFF;
+
+	max310x_rts_ctl(port, max310x_rts_level(port, rts_on));
+
+	guard(spinlock_irqsave)(&port->lock);
+	if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_BEFORE_SEND &&
+	    !one->cancel_tx_delay_tmr && !hrtimer_active(&one->tx_delay_tmr))
+		hrtimer_start(&one->tx_delay_tmr,
+			      ms_to_ktime(port->rs485.delay_rts_before_send),
+			      HRTIMER_MODE_REL);
+}
+
 static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 {
 	struct max310x_one *one = to_max310x_port(port);
@@ -792,6 +837,68 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
 	tty_flip_buffer_push(&port->state->port);
 }
 
+static enum hrtimer_restart max310x_tmr_tx(struct hrtimer *timer)
+{
+	struct max310x_one *one = container_of(timer, struct max310x_one,
+					       tx_delay_tmr);
+
+	guard(spinlock_irqsave)(&one->port.lock);
+	if (!one->cancel_tx_delay_tmr) {
+		if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND) {
+			/* After-send hold elapsed: drop RTS via the rts worker. */
+			WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+			schedule_work(&one->rts_work);
+		} else {
+			WRITE_ONCE(one->tx_state, MAX310X_TX_SEND);
+			schedule_work(&one->tx_work);
+		}
+	}
+
+	return HRTIMER_NORESTART;
+}
+
+static void max310x_delayed_stop_tx(struct uart_port *port)
+{
+	struct max310x_one *one = to_max310x_port(port);
+	unsigned int txlvl;
+
+	if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF)
+		return;
+
+	/*
+	 * The kfifo can be empty while the chip TX FIFO is still draining, so arm
+	 * the after-send hold only once the chip FIFO is empty too - the TX-empty
+	 * interrupt re-invokes us then. Otherwise the hold starts early and RTS
+	 * drops mid-character, clipping the last byte(s).
+	 */
+	txlvl = max310x_port_read(port, MAX310X_TXFIFOLVL_REG);
+	if (txlvl)
+		return;
+
+	/*
+	 * Runs from tx_work without port->lock, so re-check the state under it:
+	 * shutdown() may have cancelled the envelope meanwhile. Only
+	 * MAX310X_TX_SEND may arm the hold.
+	 */
+	guard(spinlock_irqsave)(&one->port.lock);
+	if (one->cancel_tx_delay_tmr ||
+	    READ_ONCE(one->tx_state) != MAX310X_TX_SEND)
+		return;
+
+	if (!hrtimer_active(&one->tx_delay_tmr)) {
+		/*
+		 * Add one character for the byte still in the shift register -
+		 * TX-empty fires as it enters, not as it leaves.
+		 */
+		ktime_t delay = us_to_ktime(one->one_char_duration_us +
+					    port->rs485.delay_rts_after_send *
+					    USEC_PER_MSEC);
+
+		WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_AFTER_SEND);
+		hrtimer_start(&one->tx_delay_tmr, delay, HRTIMER_MODE_REL);
+	}
+}
+
 static void max310x_handle_tx(struct uart_port *port)
 {
 	struct tty_port *tport = &port->state->port;
@@ -803,8 +910,13 @@ static void max310x_handle_tx(struct uart_port *port)
 		return;
 	}
 
-	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port))
+	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
+		struct max310x_one *one = to_max310x_port(port);
+
+		if (READ_ONCE(one->sw_rts_during_tx))
+			max310x_delayed_stop_tx(port);
 		return;
+	}
 
 	/*
 	 * It's a circ buffer -- wrap around.
@@ -829,11 +941,58 @@ static void max310x_handle_tx(struct uart_port *port)
 		uart_write_wakeup(port);
 }
 
+/*
+ * Begin a software-timed RTS envelope: set the before-send phase and queue the
+ * rts worker to assert RTS. tx_state is set synchronously here (start_tx() holds
+ * port.lock) so close()/shutdown can see an envelope is in flight; rts_work then
+ * asserts RTS and arms the before-send timer (see there).
+ */
+static void max310x_delayed_start_tx(struct uart_port *port)
+{
+	struct max310x_one *one = to_max310x_port(port);
+
+	WRITE_ONCE(one->tx_state, MAX310X_TX_WAIT_BEFORE_SEND);
+	one->cancel_tx_delay_tmr = false;
+	schedule_work(&one->rts_work);
+}
+
+/* called with port.lock taken and irqs off */
 static void max310x_start_tx(struct uart_port *port)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
-	schedule_work(&one->tx_work);
+	/* A teardown is in progress; nothing may start an envelope or TX. */
+	if (one->tx_teardown)
+		return;
+
+	if (READ_ONCE(one->sw_rts_during_tx)) {
+		/*
+		 * The before- and after-send phases share one delay timer. If an
+		 * after-send release is pending, cancel it before starting a new
+		 * TX so the just-asserted RTS is not yanked; re-arming the timer
+		 * for the before-send phase then supersedes the release.
+		 */
+		int res = 0;
+
+		if (READ_ONCE(one->tx_state) == MAX310X_TX_WAIT_AFTER_SEND)
+			res = hrtimer_try_to_cancel(&one->tx_delay_tmr);
+		if (unlikely(res == -1)) {
+			one->cancel_tx_delay_tmr = true;
+			uart_port_unlock(port);
+			hrtimer_cancel(&one->tx_delay_tmr);
+			uart_port_lock(port);
+			/*
+			 * The lock was dropped: a teardown may have run to
+			 * completion meanwhile. Re-check before starting.
+			 */
+			if (one->tx_teardown)
+				return;
+		}
+
+		max310x_delayed_start_tx(port);
+	} else {
+		schedule_work(&one->tx_work);
+	}
 }
 
 static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
@@ -946,36 +1105,50 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
 }
 
 /*
- * Program the chip's RS485 RTS timing. The HDPIXDELAY setup and hold fields
- * count bit-times, four bits per direction, while the uapi expresses the
- * delays in milliseconds: convert at the current baud rate, rounding up, and
- * cap at the field maximum.
+ * Pick hardware or software RTS timing for the current port. The chip can
+ * deliver up to 15 bit-times of setup/hold delay via HDPIXDELAY; anything
+ * longer (or any RTS polarity the chip cannot produce automatically) must
+ * be driven by software via tx_delay_tmr and rts_work.
  */
 static void max310x_set_rts_ctl_params(struct max310x_one *one)
 {
 	const unsigned int max_bit_dly = 15;
 	struct uart_port *port = &one->port;
+	u64 max_hw_delay_ns = 0;
 	unsigned int setup = 0, hold = 0;
 	u8 mode1 = 0, irda = 0;
+	bool sw_rts = false;
 
-	if (port->rs485.flags & SER_RS485_ENABLED) {
-		/* Convert milliseconds to bit-times, rounding up. */
-		setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
-				     MSEC_PER_SEC);
-		hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
-				     MSEC_PER_SEC);
-		setup = min(setup, max_bit_dly);
-		hold  = min(hold,  max_bit_dly);
-
-		mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
-		/*
-		 * The auto-RTS engine asserts RTS high on send; for an
-		 * active-low RTS let IRDA.RTSINVERT invert the output stage.
-		 */
-		if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
-			irda = MAX310X_IRDA_RTSINVERT_BIT;
+	if (!(port->rs485.flags & SER_RS485_ENABLED))
+		goto out;
+
+	if (one->baud)
+		max_hw_delay_ns = NSEC_PER_SEC / one->baud * max_bit_dly;
+
+	if ((u64)port->rs485.delay_rts_before_send * NSEC_PER_MSEC > max_hw_delay_ns ||
+	    (u64)port->rs485.delay_rts_after_send  * NSEC_PER_MSEC > max_hw_delay_ns ||
+	    !!(port->rs485.flags & SER_RS485_RTS_ON_SEND) ==
+	    !!(port->rs485.flags & SER_RS485_RTS_AFTER_SEND))
+		sw_rts = true;
+
+	if (sw_rts) {
+		setup = 0;
+		hold  = 0;
+		goto out;
 	}
 
+	/* Convert milliseconds to bit-times, rounding up. */
+	setup = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_before_send,
+			     MSEC_PER_SEC);
+	hold  = DIV_ROUND_UP(one->baud * port->rs485.delay_rts_after_send,
+			     MSEC_PER_SEC);
+	setup = min(setup, max_bit_dly);
+	hold  = min(hold,  max_bit_dly);
+
+out:
+	/* Assign once; a transient false would be seen by other readers. */
+	WRITE_ONCE(one->sw_rts_during_tx, sw_rts);
+
 	max310x_port_write(port, MAX310X_HDPIXDELAY_REG,
 			   MAX310X_HDPIXDELAY_SETUP(setup) |
 			   MAX310X_HDPIXDELAY_HOLD(hold));
@@ -983,12 +1156,48 @@ static void max310x_set_rts_ctl_params(struct max310x_one *one)
 	/*
 	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
 	 * drives RTS manually, and restores it from the current
-	 * configuration when the break ends. Touching MODE1 here would
+	 * configuration when the break ends. Touching them here would
 	 * release the transceiver mid-break.
 	 */
 	if (one->tx_break)
 		return;
 
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (sw_rts) {
+			/*
+			 * Only settle RTS at idle when no transmission owns it.
+			 * A reconfigure while one is in flight - rs_work runs
+			 * on every TIOCSRS485 - would otherwise release the
+			 * transceiver mid-character.
+			 */
+			if (READ_ONCE(one->tx_state) == MAX310X_TX_OFF) {
+				max310x_rts_ctl(port,
+						max310x_rts_level(port, false));
+				/*
+				 * serial_core calls set_termios() without
+				 * port->lock, so an envelope may have started
+				 * while the idle level was written and the
+				 * settle can land after its RTS assert.
+				 * rts_work re-derives the level from
+				 * tx_state; requeue it to converge.
+				 */
+				if (READ_ONCE(one->tx_state) != MAX310X_TX_OFF)
+					schedule_work(&one->rts_work);
+			}
+		} else {
+			mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+			/*
+			 * The auto-RTS engine asserts RTS high on send; for an
+			 * active-low RTS let IRDA.RTSINVERT invert the output
+			 * stage.
+			 */
+			if (!(port->rs485.flags & SER_RS485_RTS_ON_SEND))
+				irda = MAX310X_IRDA_RTSINVERT_BIT;
+		}
+	} else {
+		max310x_rts_ctl(port, 0);
+	}
+
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
 	max310x_port_update(port, MAX310X_IRDA_REG,
@@ -1017,18 +1226,24 @@ static void max310x_break_ctl(struct uart_port *port, int break_state)
 	 * is driven as it is.
 	 */
 	if (break_state) {
-		max310x_port_update(port, MAX310X_MODE1_REG,
-				    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
-		max310x_rts_ctl(port, 1);
+		if (READ_ONCE(one->sw_rts_during_tx)) {
+			max310x_rts_ctl(port, max310x_rts_level(port, 1));
+		} else {
+			max310x_port_update(port, MAX310X_MODE1_REG,
+					    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
+			max310x_rts_ctl(port, 1);
+		}
 	} else {
 		/*
 		 * Reapply the current configuration: a reconfigure that
 		 * arrived during the break was deferred by the tx_break
-		 * guard. Then release the manual RTS - auto-RTS owns the
-		 * pin again.
+		 * guard. On the software path the helper also settles the
+		 * idle level; on the hardware path release the manual RTS -
+		 * auto-RTS owns the pin again.
 		 */
 		max310x_set_rts_ctl_params(one);
-		max310x_rts_ctl(port, 0);
+		if (!READ_ONCE(one->sw_rts_during_tx))
+			max310x_rts_ctl(port, 0);
 	}
 }
 
@@ -1071,9 +1286,10 @@ static void max310x_set_termios(struct uart_port *port,
 		lcr |= MAX310X_LCR_STOPLEN_BIT; /* 2 stops */
 
 	/*
-	 * Update LCR register. Leave the TX break bit alone: it is driven by
-	 * break_ctl(), and an absolute write here would end a break in
-	 * progress.
+	 * Update LCR register. Leave the TX break and RTS bits alone: they are
+	 * driven by break_ctl() and by the software-timed RS485 RTS, and an
+	 * absolute write here would end a break in progress or release the
+	 * transceiver mid-character.
 	 */
 	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TERMIOS_MASK, lcr);
 
@@ -1181,14 +1397,37 @@ static void max310x_rs_proc(struct work_struct *ws)
 			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
 }
 
+/* called with port.lock taken and irqs off */
 static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios,
 				struct serial_rs485 *rs485)
 {
 	struct max310x_one *one = to_max310x_port(port);
 
-	if ((rs485->delay_rts_before_send > 0x0f) ||
-	    (rs485->delay_rts_after_send > 0x0f))
-		return -ERANGE;
+	rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+	rs485->delay_rts_after_send  = min(rs485->delay_rts_after_send,  100U);
+
+	/*
+	 * Make sure no SW-timed RTS toggle survives an RS485 disable, even
+	 * if the delay timer happens to be running right now.
+	 */
+	if (!(rs485->flags & SER_RS485_ENABLED)) {
+		one->tx_teardown = true;
+		one->cancel_tx_delay_tmr = true;
+		if (hrtimer_try_to_cancel(&one->tx_delay_tmr) == -1) {
+			uart_port_unlock(port);
+			hrtimer_cancel(&one->tx_delay_tmr);
+			uart_port_lock(port);
+		}
+		WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
+		one->tx_teardown = false;
+		/*
+		 * The port stays alive, and a write that raced the teardown
+		 * may have left data queued with no envelope left to pump it.
+		 * Kick tx_work; RS485 is disabled, so the plain path is right.
+		 */
+		if (!kfifo_is_empty(&port->state->port.xmit_fifo))
+			schedule_work(&one->tx_work);
+	}
 
 	port->rs485 = *rs485;
 
@@ -1222,7 +1461,15 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
-	/* Configure the RS485 RTS timing and the RS485/RS232 mode bits. */
+	one->tx_teardown = false;
+
+	/*
+	 * Configure the RS485 RTS timing (HW auto-RTS vs software-driven) and
+	 * the RS485/RS232 mode bits. Don't hardcode HW auto-RTS here - let
+	 * max310x_set_rts_ctl_params() pick HW or SW per the configured
+	 * delays, otherwise the chip's auto-RTS would override the software
+	 * RTS hold and the after-send delay is lost.
+	 */
 	max310x_set_rts_ctl_params(one);
 
 	if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -1252,22 +1499,64 @@ static int max310x_startup(struct uart_port *port)
 static void max310x_shutdown(struct uart_port *port)
 {
 	struct max310x_one *one = to_max310x_port(port);
-	unsigned int loops = port->fifosize + 1;
+
+	if (READ_ONCE(one->sw_rts_during_tx)) {
+		/*
+		 * Drain any in-flight software-timed RTS envelope before the
+		 * port is powered down, so the last character and its
+		 * after-send hold complete - close() can reach shutdown with
+		 * data still queued and a before-send delay pending. The loop
+		 * ends when the envelope does (tx_state == OFF); the bound is
+		 * just a worst-case safety cap.
+		 */
+		unsigned int loops = port->rs485.delay_rts_before_send +
+			port->rs485.delay_rts_after_send +
+			DIV_ROUND_UP_ULL((u64)(kfifo_len(&port->state->port.xmit_fifo) +
+					       port->fifosize) *
+					 one->one_char_duration_us,
+					 USEC_PER_MSEC);
+
+		while (READ_ONCE(one->tx_state) != MAX310X_TX_OFF && loops-- > 0)
+			fsleep(USEC_PER_MSEC);
+	} else {
+		/*
+		 * The tty layer waits for tx_empty() before close(), but
+		 * tx_empty() only reflects the chip TX FIFO - the last
+		 * character may still be in the transmit shift register. Let
+		 * the FIFO drain and the final character clock out before the
+		 * port is powered down, otherwise close() truncates the last
+		 * byte on the wire.
+		 */
+		unsigned int loops = port->fifosize + 1;
+
+		while (!max310x_tx_empty(port) && loops-- > 0)
+			fsleep(one->one_char_duration_us);
+		fsleep(one->one_char_duration_us);
+	}
 
 	/*
-	 * The tty layer waits for tx_empty() before close(), but tx_empty()
-	 * only reflects the chip TX FIFO - the last character may still be in
-	 * the transmit shift register. Let the FIFO drain and the final
-	 * character clock out before the port is powered down, otherwise
-	 * close() truncates the last byte on the wire.
+	 * Set the teardown interlock under port->lock, so a concurrent
+	 * start_tx() either sees it or happens-before the cancellations
+	 * below. Then cancel unconditionally: the SW/HW decision is
+	 * recomputed on every reconfigure, so a TIOCSRS485 can clear
+	 * sw_rts_during_tx while an envelope is still in flight, and
+	 * neither the timer nor the works may outlive the port.
 	 */
-	while (!max310x_tx_empty(port) && loops-- > 0)
-		fsleep(one->one_char_duration_us);
-	fsleep(one->one_char_duration_us);
+	scoped_guard(spinlock_irqsave, &port->lock) {
+		one->tx_teardown = true;
+		one->cancel_tx_delay_tmr = true;
+	}
+	cancel_work_sync(&one->tx_work);
+	hrtimer_cancel(&one->tx_delay_tmr);
+	cancel_work_sync(&one->rts_work);
+	WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
 
 	/* Disable all interrupts */
 	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
 
+	if (READ_ONCE(one->sw_rts_during_tx))
+		max310x_rts_ctl(port, max310x_rts_level(port, false));
+
 	max310x_power(port, 0);
 }
 
@@ -1557,6 +1846,11 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
 		INIT_WORK(&s->p[i].md_work, max310x_md_proc);
 		/* Initialize queue for changing RS485 mode */
 		INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
+		/* Initialize queue for software-driven RTS toggling */
+		INIT_WORK(&s->p[i].rts_work, max310x_rts_work_proc);
+		hrtimer_setup(&s->p[i].tx_delay_tmr, max310x_tmr_tx,
+			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		s->p[i].tx_state = MAX310X_TX_OFF;
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -1667,6 +1961,8 @@ static void max310x_remove(struct device *dev)
 	int i;
 
 	for (i = 0; i < s->devtype->nr; i++) {
+		hrtimer_cancel(&s->p[i].tx_delay_tmr);
+		cancel_work_sync(&s->p[i].rts_work);
 		cancel_work_sync(&s->p[i].tx_work);
 		cancel_work_sync(&s->p[i].md_work);
 		cancel_work_sync(&s->p[i].rs_work);

-- 
2.47.3

[PATCH v5 8/8] serial: max310x: don't transmit while an RS485 reconfigure is pending

[Tapio Reijonen]

TIOCSRS485 applies its register changes asynchronously: rs485_config()
stores the new configuration and schedules rs_work, which programs
HDPIXDELAY, MODE1.TRNSCVCTRL and the RTS path. A write() issued right
after the ioctl therefore transmits against the old, half-switched
state. On a single core the ordering is even deterministic: start_tx()
picks the stale path first, then rs_work reprograms the chip, then
tx_work pumps the data - with the transceiver already released. A
TIOCSRS485 switching from the hardware to the software RTS path
followed immediately by a write puts the whole transfer on the wire
with the transceiver disabled: nothing reaches the bus and no error is
reported anywhere. The inverse direction is as old as the asynchronous
reconfigure itself: enabling RS485 and writing immediately shifts the
first bytes out before rs_work has enabled the chip's auto-RTS.

Defer instead: rs485_config() marks the reconfigure pending under
port->lock, start_tx() leaves the data in the kfifo while the mark is
set, and rs_work restarts the transmission itself once the new
configuration is fully applied. The rs485-disable path's direct
tx_work kick is replaced by the same mechanism, which also orders that
flush after the reconfigure instead of before it.

Fixes: 5bdb48b501e8 ("serial: max310x: Fix RS485 handling")
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
 drivers/tty/serial/max310x.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 6ad27eb8a32586ec5719773e6eadf1249155bf19..7036710942a42ef096340a38a9cee87cb0c4efbf 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -319,6 +319,7 @@ struct max310x_one {
 	bool			sw_rts_during_tx;
 	bool			cancel_tx_delay_tmr;
 	bool			tx_teardown;	/* envelope being torn down */
+	bool			rs485_pending;	/* rs_work not yet applied */
 	bool			tx_break;	/* break_ctl() owns the transceiver */
 	enum max310x_tx_state	tx_state;
 
@@ -965,6 +966,16 @@ static void max310x_start_tx(struct uart_port *port)
 	if (one->tx_teardown)
 		return;
 
+	/*
+	 * An RS485 reconfigure is scheduled but not applied yet: transmitting
+	 * now would use the old path against half-programmed registers - a
+	 * TIOCSRS485 switching paths followed immediately by a write puts the
+	 * data on the wire with the transceiver released. Leave the data in
+	 * the kfifo; rs_work restarts TX once the configuration is applied.
+	 */
+	if (one->rs485_pending)
+		return;
+
 	if (READ_ONCE(one->sw_rts_during_tx)) {
 		/*
 		 * The before- and after-send phases share one delay timer. If an
@@ -1395,6 +1406,16 @@ static void max310x_rs_proc(struct work_struct *ws)
 
 	max310x_port_update(&one->port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_ECHOSUPR_BIT, mode2);
+
+	/*
+	 * The configuration is applied: release any TX that start_tx()
+	 * deferred while the reconfigure was pending, now on the right path.
+	 */
+	scoped_guard(spinlock_irqsave, &one->port.lock) {
+		one->rs485_pending = false;
+		if (!kfifo_is_empty(&one->port.state->port.xmit_fifo))
+			max310x_start_tx(&one->port);
+	}
 }
 
 /* called with port.lock taken and irqs off */
@@ -1420,17 +1441,11 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
 		}
 		WRITE_ONCE(one->tx_state, MAX310X_TX_OFF);
 		one->tx_teardown = false;
-		/*
-		 * The port stays alive, and a write that raced the teardown
-		 * may have left data queued with no envelope left to pump it.
-		 * Kick tx_work; RS485 is disabled, so the plain path is right.
-		 */
-		if (!kfifo_is_empty(&port->state->port.xmit_fifo))
-			schedule_work(&one->tx_work);
 	}
 
 	port->rs485 = *rs485;
 
+	one->rs485_pending = true;
 	schedule_work(&one->rs_work);
 
 	return 0;

-- 
2.47.3

Re: [PATCH v5 0/8] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[Greg Kroah-Hartman]

On Tue, Sep 29, 2026 at 09:37:54AM +0000, Tapio Reijonen wrote:
> The MAX310X hardware can express at most 15 bit-times of RS485 RTS
> setup/hold delay, while struct serial_rs485 expresses the delays in
> milliseconds. The driver rejected anything above 0x0f with -ERANGE,
> upon which uart_rs485_config() wipes port->rs485 and silently disables
> RS485 - a device tree asking for a 20 ms setup delay boots with RS485
> off and an unusable bus. The values that were accepted got written
> into HDPIXDELAY unconverted, milliseconds as bit-times.
> 
> Patches 1-4 fix pre-existing bugs found on the way: a termios write
> clobbering an active break; breaks never reaching the wire on RS485
> ports because auto-RTS only drives the transceiver for FIFO data; the
> milliseconds-as-bit-times unit bug; and close() truncating the final
> character because tx_empty() does not cover the transmit shift
> register. Patch 5 adds active-low RTS on the hardware path via
> IRDA.RTSINVERT. Patch 6 is preparation, and patch 7 adds the
> software-timed RTS path that takes over whenever the hardware cannot
> represent the requested timing, clamping the delays to the UART core's
> maximum instead of rejecting them. Patch 8 fixes a reconfigure-versus-
> write race the asynchronous rs485 config application has had since
> 2016, which the software path would have made worse.
> 
> v4 was all of this in a single patch; Greg asked for it to be broken
> up into one change at a time [1]. Splitting it meant re-verifying each
> patch in isolation on hardware, and that re-verification found two
> bugs v4 contained: a set_termios() or TIOCSRS485 during an active
> break released the transceiver mid-break while the break bookkeeping
> still looked correct (prevented by the tx_break ownership guard in
> patches 2 and 3), and the patch-8 race, where a TIOCSRS485 followed
> immediately by a write could put an entire transfer on the wire with
> the transceiver released.
> 
> Tested on a MAX14830 (SPI, i.MX6SX) driving RS485 transceivers: for
> each patch the bug it fixes was first reproduced on the wire with a
> logic analyzer against the kernel one patch earlier, then shown fixed.
> The complete series additionally passed an automated 25-scenario
> regression matrix covering both RTS paths, both polarities,
> RS485/RS232 mode round-trips, close-during-transmission, and termios/
> TIOCSRS485 disturbances landing in every envelope phase (setup, data,
> hold, break), each scenario checked both on the wire and against the
> driver's reported state.
> 
> Changes in v5, beyond the split:
> - teardown interlock (tx_teardown): shutdown() and the rs485-disable
>   path set it under port->lock, and start_tx() checks it on entry and
>   again after retaking the dropped lock, so a racing write can no
>   longer re-arm the delay timer or queue RTS work against a port being
>   torn down (addresses the remaining review-bot findings on v4)
> - shutdown() also cancels tx_work, previously only cancelled in
>   remove()
> - the per-character duration is stored as unsigned int microseconds
>   instead of ktime_t: single-copy atomic on 32-bit, so a torn read of
>   the 64-bit value is gone by construction
> - the TXEMPTY handling documents that the interrupt latches on the
>   FIFO becoming empty, so a stale interrupt cannot pump data during an
>   RTS setup delay
> - new in v5: the tx_break ownership guard (patches 2/3) and the
>   reconfigure-pending gate (patch 8), both found during the per-patch
>   hardware re-testing described above
> - also new in v5, from a review pass over the split series: startup()
>   clears a latched break (nothing clears TXBREAK when a port is closed
>   with a break still asserted - 8250 does the same); a reconfigure
>   arriving during a break is now deferred and applied at break-end
>   instead of partially dropped; the rs485-config worker runs under
>   port->mutex so its break-guarded register writes cannot straddle a
>   break edge; the termios-path idle settle re-checks tx_state after
>   writing and requeues rts_work if an envelope started meanwhile; and
>   the hardware-delay ceiling is computed in u64
> 
> [1] https://lore.kernel.org/all/2026092326-truth-unweave-c773@gregkh/
> 
> ---
> Tapio Reijonen (8):
>       serial: max310x: don't clobber the TX break bit in set_termios
>       serial: max310x: assert the transceiver during a break
>       serial: max310x: convert RS485 delays from milliseconds to bit-times
>       serial: max310x: wait for TX to drain before powering down in shutdown
>       serial: max310x: support active-low RTS on the hardware path
>       serial: max310x: schedule tx_work directly from the IRQ handler
>       serial: max310x: drive RTS in software when hardware delays are too short
>       serial: max310x: don't transmit while an RS485 reconfigure is pending
> 
>  drivers/tty/serial/max310x.c | 511 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 477 insertions(+), 34 deletions(-)
> ---
> base-commit: 9505146e885b1a842118aa6410f737290c4a5a32
> change-id: 20260513-max310x-rs485-sw-delay-a306d783d529
> 
> Best regards,
> -- 
> Tapio Reijonen <tapio.reijonen@vaisala.com>
> 

Did you forget the Assisted-by: tag for this series?

thanks,

greg k-h

Re: [PATCH v5 0/8] serial: max310x: RS485 delay and RTS fixes, software-timed delays

[Tapio Reijonen]

On 10/1/26 11:35, Greg Kroah-Hartman wrote:
> Did you forget the Assisted-by: tag for this series?

Indeed I did, sorry about that. The series was developed with AI
assistance (Anthropic's Claude), with the review and the per-patch
hardware verification described in the cover letter done on my side.

I'll add the tag to every patch in v6:

  Assisted-by: Claude:claude-fable-5

v6 will also address Hugo's review comments; I was planning to wait
for his review of the remaining patches before posting it.

Thanks,
Tapio