Re: [PATCH v6 11/12] tty: serial: Add Nuvoton ma35d1 serial driver support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dear Jiri,


Thanks for your advice.

On 2023/3/28 下午 05:23, Jiri Slaby wrote:
On 28. 03. 23, 4:19, Jacky Huang wrote:
+static void transmit_chars(struct uart_ma35d1_port *up)
+{
+    struct circ_buf *xmit = &up->port.state->xmit;
+    int count;
+    u8 ch;
+
+    if (uart_tx_stopped(&up->port)) {
+        ma35d1serial_stop_tx(&up->port);
+        return;
+    }
+    if (uart_circ_empty(xmit)) {
+        __stop_tx(up);
+        return;
+    }

Why is this necessary?


I will remove this reduntant uart_circ_empty() check.

+    count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & FSR_TXPTR_MSK) >> 16);
+
+    uart_port_tx_limited(&up->port, ch, count,
+                 !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL),
+                 serial_out(up, UART_REG_THR, ch),
+                 ({}));
+
+    if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+        uart_write_wakeup(&up->port);
+
+    if (uart_circ_empty(xmit))
+        __stop_tx(up);

uart_port_tx_limited() should take care about the above and this too, right?

Yes, you are right. These two are also redundant. I will remove them.
Thank you.


+}
...
+static void receive_chars(struct uart_ma35d1_port *up)
+{
+    u8 flag;
+    u32 fsr;
+    unsigned int ch;

Shouldn't ch be u8 too?

OK, I will fix it as u8.


+    int max_count = 256;
+
+    fsr = serial_in(up, UART_REG_FSR);
+    do {
+        flag = TTY_NORMAL;
+        up->port.icount.rx++;
+
+        if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF))) {
+            if (fsr & FSR_BIF) {
+                up->port.icount.brk++;
+                if (uart_handle_break(&up->port))
+                    continue;
+            }
+            if (fsr & FSR_FEF)
+                up->port.icount.frame++;
+            if (fsr & FSR_PEF)
+                up->port.icount.parity++;
+            if (fsr & FSR_RX_OVER_IF)
+                up->port.icount.overrun++;
+
+            serial_out(up, UART_REG_FSR, fsr &
+                   (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF));
+
+            if (fsr & FSR_BIF)
+                flag = TTY_BREAK;
+            else if (fsr & FSR_PEF)
+                flag = TTY_PARITY;
+            else if (fsr & FSR_FEF)
+                flag = TTY_FRAME;
+        }
+
+        ch = serial_in(up, UART_REG_RBR);
+        if (uart_handle_sysrq_char(&up->port, ch))
+            continue;
+
+        spin_lock(&up->port.lock);
+        uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag);
+        spin_unlock(&up->port.lock);
+
+        fsr = serial_in(up, UART_REG_FSR);
+    } while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0));
+
+    spin_lock(&up->port.lock);
+    tty_flip_buffer_push(&up->port.state->port);
+    spin_unlock(&up->port.lock);
+}
...
+static int ma35d1serial_remove(struct platform_device *dev)
+{
+    struct uart_port *port = platform_get_drvdata(dev);
+
+    if (port) {

Can this ever be NULL?

OK, I will remove this NULL check.


+ uart_remove_one_port(&ma35d1serial_reg, port);
+        free_irq(port->irq, port);
+    }
+    return 0;
+}
+
+static int ma35d1serial_suspend(struct platform_device *dev, pm_message_t state)
+{
+    int i;
+    struct uart_ma35d1_port *up;
+
+    if (dev->dev.of_node)
+        i = of_alias_get_id(dev->dev.of_node, "serial");
+    if (i < 0) {
+        dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
+        return i;
+    }
+    up = &ma35d1serial_ports[i];

platform_get_drvdata(dev) ?

We will fix it.


+    if (i == 0) {
+        up->console_baud_rate = serial_in(up, UART_REG_BAUD);
+        up->console_line = serial_in(up, UART_REG_LCR);
+        up->console_int = serial_in(up, UART_REG_IER);
+    }
+    return 0;
+}
+
+static int ma35d1serial_resume(struct platform_device *dev)
+{
+    int i;
+    struct uart_ma35d1_port *up;
+
+    if (dev->dev.of_node)
+        i = of_alias_get_id(dev->dev.of_node, "serial");
+    if (i < 0) {
+        dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
+        return i;
+    }
+    up = &ma35d1serial_ports[i];
+    if (i == 0) {
+        serial_out(up, UART_REG_BAUD, up->console_baud_rate);
+        serial_out(up, UART_REG_LCR, up->console_line);
+        serial_out(up, UART_REG_IER, up->console_int);
+    }
+    return 0;
+}

No uart_suspend_port()/uart_resume_port()? You don't wait for transmitter to be empty in suspend. You don't stop tx etc.


We will fix it in the next version.

+static struct platform_driver ma35d1serial_driver = {
+    .probe      = ma35d1serial_probe,
+    .remove     = ma35d1serial_remove,
+    .suspend    = ma35d1serial_suspend,
+    .resume     = ma35d1serial_resume,
+    .driver     = {
+        .name   = "ma35d1-uart",
+        .owner  = THIS_MODULE,
+        .of_match_table = of_match_ptr(ma35d1_serial_of_match),
+    },
+};
+
+static int __init ma35d1serial_init(void)
+{
+    int ret;
+
+    ret = uart_register_driver(&ma35d1serial_reg);
+    if (ret)
+        return ret;
+    ret = platform_driver_register(&ma35d1serial_driver);
+    if (ret)
+        uart_unregister_driver(&ma35d1serial_reg);
+    return ret;
+}
+
+static void __exit ma35d1serial_exit(void)
+{
+    platform_driver_unregister(&ma35d1serial_driver);
+    uart_unregister_driver(&ma35d1serial_reg);
+}
+
+module_init(ma35d1serial_init);
+module_exit(ma35d1serial_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MA35D1 serial driver");


+MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);

Why is this needed? How are other platform drivers autoloaded?

thanks,

Yes, it's not needed. I will remove it.


Best regards,
Jacky Huang






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux