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]

 



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?

+	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?

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

Shouldn't ch be u8 too?

+	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?

+		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) ?

+	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.

+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,
--
js
suse labs




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux