On Sun, Jul 28, 2019 at 05:12:09PM -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Sent: Sunday, July 21, 2019 9:26:59 AM
>
> > There are Exar quirks in 8250_port which belong to 8250_exar module.
> > Move out them to the correct module and do not contaminate generic code
> > with it.
>
> Andy,
>
> Your changes seem functionally equivalent based on my review and testing.
Thank you for testing, my answers below.
> However, based on this commit description, I was expecting more of the
> Exar-specific code to be moved from 8250_port.c to 8250_exar.c. I think that
> can reasonably be achieved without too much additional effort.
>
> There are two pieces of Exar-specific support in serial8250_do_startup():
Actually three.
> 1. The two reads to clear INT0, could be boiled down to a single read in
> exar_pci_probe(), immediately after registering the common IRQ handler.
> What do you think?
I'm not so familiar with the hardware, will it not give any side-effects?
> 2. The following block could be moved to a new exar_startup() function:
>
> if (port->type == PORT_XR17V35X) {
> /*
> * First enable access to IER [7:5], ISR [5:4], FCR [5:4],
> * MCR [7:5] and MSR [7:0]
> */
> serial_port_out(port, UART_XR_EFR, UART_EFR_ECB);
>
> /*
> * Make sure all interrups are masked until initialization is
> * complete and the FIFOs are cleared
> */
> serial_port_out(port, UART_IER, 0);
> }
>
> Do you agree?
I thought about this, but didn't come to a conclusion to move it right now.
It's not so straight forward. In the ->startup() we setup IO accessors in some
cases (perhaps doesn't apply to Exar case) and do some testing.
So, I prefer do this in a separate change, so we may see how it goes.
> The only thing that seems to need to stay put is UART_XR_EFR support in
> serial8250_do_set_termios().
Yes, though ideally it should be moved to 8250_exar as well.
> There are a couple additional notes below for you to find. One involves a
> compile warning.
> > #define UART_EXAR_INT0 0x80
>
> My note above about cleaning up serial8250_do_startup() would eliminate
> the need for UART_EXAR_INT0 to be defined here, too.
I see.
> > - goto out;
>
> Take the "out" label as well to avoid introducing a compile warning.
Thanks, I missed it somehow.
--
With Best Regards,
Andy Shevchenko
