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
