Messages in this thread | | | Date | Wed, 29 Mar 2023 16:06:31 +0800 | Subject | Re: [PATCH v6 11/12] tty: serial: Add Nuvoton ma35d1 serial driver support | From | Jacky Huang <> |
| |
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
| |