Re: [PATCH 26/29] serial: 8250: use serial_in/out() helpers

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Feb 20, 2025 at 12:16:03PM +0100, Jiri Slaby (SUSE) wrote:
> There are serial_in/out() helpers to be used instead of direct
> p->serial_in/out(). Use them in various 8250 drivers.

Okay, I have checked all calls, and I think to avoid possible issues
we need to add a few comments here and there, see below.

But first of all, can we actually use serial_port_in()/serial_port_out()
in the cases where we have already a pointer to uart_port?

...

Here we should add a comment like

/*
 * This function is being called as part of the uart_port::serial_out()
 * routine. Hence it must not call serial_out() on itself against
 * the modified registers here, i.e. LCR.
 */

> static void dw8250_force_idle(struct uart_port *p)
> {
> 	struct uart_8250_port *up = up_to_u8250p(p);
> 	unsigned int lsr;

Here we should add a comment like

	/*
	 * The following call currently performs serial_out()
	 * against the FCR register. Because it differs to LCR
	 * there will be no dead loop, but if it ever gets
	 * modified, we might need a new custom version that
	 * avoids infinite recursion.
	 */

> 	serial8250_clear_and_reinit_fifos(up);

...

On top of this function we need to add the same comment as on top of
dw8250_force_idle() above.

>  static void dw8250_check_lcr(struct uart_port *p, int offset, int value)

...

The rest is legit as we shouldn't use those in the ->serial_in()/->serial_out().

-- 
With Best Regards,
Andy Shevchenko