On Wed, Mar 15, 2023 at 07:29:01AM +0000, Jacky Huang wrote:

From: Jacky Huang <ychuang3@nuvoton.com>

This adds UART and console driver for Nuvoton ma35d1 Soc.

MA35D1 SoC provides up to 17 UART controllers, each with one uart port.
The ma35d1 uart controller is not compatible with 8250.

A new UART being designed that is not an 8250 compatible?  Why????

Anyway, some minor comments:

 drivers/tty/serial/ma35d1_serial.c | 842 +++++++++++++++++++++++++++++
 drivers/tty/serial/ma35d1_serial.h |  93 ++++

Why do you have a .h file for only a single .c file?  Please just put
them both together into one .c file.

 include/uapi/linux/serial_core.h   |   3 +

Why do you need this #define?  Are you using it in userspace now?  If
not, why have it?

+static void
+receive_chars(struct uart_ma35d1_port *up)

Please just put all one one line.

+{
+	u8 ch;
+	u32 fsr;
+	u32 isr;
+	u32 dcnt;
+	char flag;
+
+	isr = serial_in(up, UART_REG_ISR);
+	fsr = serial_in(up, UART_REG_FSR);
+
+	while (!(fsr & RX_EMPTY)) {

You have no way out if the hardware is stuck?  That feels wrong.

+static int ma35d1serial_ioctl(struct uart_port *port, u32 cmd, unsigned long arg)
+{
+	switch (cmd) {
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}

You do not need to handle any ioctls?

+static void ma35d1serial_console_putchar(struct uart_port *port,
+					 unsigned char ch)
+{
+	struct uart_ma35d1_port *up = (struct uart_ma35d1_port *)port;
+
+	do {
+	} while ((serial_in(up, UART_REG_FSR) & TX_FULL));

Again, no way out if this gets stuck in a loop?

+/**
+ *  Suspend one serial port.
+ */
+void ma35d1serial_suspend_port(int line)
+{
+	uart_suspend_port(&ma35d1serial_reg, &ma35d1serial_ports[line].port);
+}
+EXPORT_SYMBOL(ma35d1serial_suspend_port);

Why is this exported?  Who uses it?

And why not EXPORT_SYMBOL_GPL()?

+
+/**
+ *  Resume one serial port.
+ */
+void ma35d1serial_resume_port(int line)
+{
+	struct uart_ma35d1_port *up = &ma35d1serial_ports[line];
+
+	uart_resume_port(&ma35d1serial_reg, &up->port);
+}
+EXPORT_SYMBOL(ma35d1serial_resume_port);

Same here, who calls this and why?

Again, why is this define needed?

thanks,

greg k-h