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

[Hugo Villeneuve <hugo@hugovil.com>]

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