Re: [PATCH 15/33] serial: 8250: extract serial8250_THRE_test()

Jiri Slaby
2025-06-12T08:54:10Z

On 11. 06. 25, 14:03, Ilpo Järvinen wrote:
> On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:
...
>> + /*
>> + * Test for UARTs that do not reassert THRE when the transmitter is idle and the interrupt
>> + * has already been cleared. Real 16550s should always reassert this interrupt whenever the
>> + * transmitter is idle and the interrupt is enabled. Delays are necessary to allow register
>> + * changes to become visible.
>
> Very long comment lines are hard to read. (This is mostly not related to
> line length limits, but with eye movement required.)
>
> It may make sense to place some of the descriptive comment text into a
> function comment instead of placing them mid-function.
>
>> + *
>> + * Synchronize UART_IER access against the console.
>> + */
>> + uart_port_lock_irqsave(port, &flags);
>> +
>> + wait_for_xmitr(up, UART_LSR_THRE);
>> + serial_port_out_sync(port, UART_IER, UART_IER_THRI);
>> + udelay(1); /* allow THRE to set */
>
> These comments mix visually into the code making this look a big wall of
> text overall. Maybe consider adding empty lines to the logic as well as
> there are what looks clear steps in this logic.

What about this:
> /*
> * Test for UARTs that do not reassert THRE when the transmitter is idle and the
> * interrupt has already been cleared. Real 16550s should always reassert this
> * interrupt whenever the transmitter is idle and the interrupt is enabled.
> * Delays are necessary to allow register changes to become visible.
> */
> static void serial8250_THRE_test(struct uart_port *port)
> {
> struct uart_8250_port *up = up_to_u8250p(port);
> unsigned long flags;
> bool iir_noint1, iir_noint2;
>
> if (!port->irq)
> return;
>
> if (up->port.flags & UPF_NO_THRE_TEST)
> return;
>
> if (port->irqflags & IRQF_SHARED)
> disable_irq_nosync(port->irq);
>
> /* Synchronize UART_IER access against the console. */
> uart_port_lock_irqsave(port, &flags);
>
> wait_for_xmitr(up, UART_LSR_THRE);
> serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> /* allow THRE to set */
> udelay(1);
>
> iir_noint1 = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
> serial_port_out(port, UART_IER, 0);
> serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> /* allow a working UART time to re-assert THRE */
> udelay(1);
>
> iir_noint2 = serial_port_in(port, UART_IIR) & UART_IIR_NO_INT;
> serial_port_out(port, UART_IER, 0);
>
> uart_port_unlock_irqrestore(port, flags);
>
> if (port->irqflags & IRQF_SHARED)
> enable_irq(port->irq);
>
> /*
> * If the interrupt is not reasserted, or we otherwise don't trust the
> * iir, setup a timer to kick the UART on a regular basis.
> */
> if ((!iir_noint1 && iir_noint2) || up->port.flags & UPF_BUG_THRE)
> up->bugs |= UART_BUG_THRE;
> }
>

?

thanks,
--
js
suse labs