On 12/12/15 23:39, Andy Shevchenko wrote:

On Wed, Dec 2, 2015 at 11:33 AM, Vladimir Murzin
[off-list ref] wrote:

quoted

This driver adds support to the UART controller found on ARM MPS2
platform.

Just few comments (have neither time not big desire to do full review).

Still better than nothing ;) I'm mostly agree on points you had, so I've
just left some I'm doubt about...

quoted

+
+static void mps2_uart_enable_ms(struct uart_port *port)
+{
+}
+
+static void mps2_uart_break_ctl(struct uart_port *port, int ctl)
+{
+}

Are those required to be present? If not, remove them until you have
alive code there.

A quick grep shows that core calls mps2_uart_break_ctl()
unconditionally, but, yes, it checks for presence of
mps2_uart_enable_ms() before jumping there, so it is safe to remove latter.

quoted

+static irqreturn_t mps2_uart_oerrirq(int irq, void *data)
+{
+       irqreturn_t handled = IRQ_NONE;
+       struct uart_port *port = data;
+       u8 irqflag = mps2_uart_read8(port, UARTn_INT);
+
+       spin_lock(&port->lock);
+
+       if (irqflag & UARTn_INT_RX_OVERRUN) {
+               struct tty_port *tport = &port->state->port;
+
+               mps2_uart_write8(port, UARTn_INT_RX_OVERRUN, UARTn_INT);
+               tty_insert_flip_char(tport, 0, TTY_OVERRUN);
+               port->icount.overrun++;
+               handled = IRQ_HANDLED;
+       }
+
+       /* XXX: this shouldn't happen? */

If shouldn't why it's there? Otherwise better to explain which
conditions may lead to this.

In practice I've never seen that happened and I think it never *should*
happen since we check if there is room in TX buffer. However, I could be
wrong here, so it is why that statement has question mark.

quoted

+       if (irqflag & UARTn_INT_TX_OVERRUN) {
+               mps2_uart_write8(port, UARTn_INT_TX_OVERRUN, UARTn_INT);
+               handled = IRQ_HANDLED;
+       }
+
+       spin_unlock(&port->lock);
+
+       return handled;
+}
+

...

quoted

+static void mps2_uart_release_port(struct uart_port *port)
+{
+}
+
+static int mps2_uart_request_port(struct uart_port *port)
+{
+       return 0;
+}
+

Same question about empty stubs.

Looks like they called unconditionally by the core.

quoted

+static int __init mps2_uart_init(void)
+{
+       int ret;
+
+       ret = uart_register_driver(&mps2_uart_driver);
+       if (ret)
+               return ret;
+
+       ret = platform_driver_register(&mps2_serial_driver);
+       if (ret)
+               uart_unregister_driver(&mps2_uart_driver);
+
+       pr_info("MPS2 UART driver initialized\n");
+
+       return ret;
+}
+module_init(mps2_uart_init);
+
+static void __exit mps2_uart_exit(void)
+{
+       platform_driver_unregister(&mps2_serial_driver);
+       uart_unregister_driver(&mps2_uart_driver);
+}
+module_exit(mps2_uart_exit);

module_platform_driver();
And move uart_*register calls to probe/remove.

With this move we'll get uart_*register for every device probed, no?

Thanks
Vladimir