On Wed, Mar 15, 2023 at 07:29:01AM +0000, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@xxxxxxxxxxx>
>
> 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?
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -279,4 +279,7 @@
> /* Sunplus UART */
> #define PORT_SUNPLUS 123
>
> +/* Nuvoton MA35D1 UART */
> +#define PORT_MA35D1 124
Again, why is this define needed?
thanks,
greg k-h
