lkml.org 
[lkml]   [2023]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver support
    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?

    > --- 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
    \
     
     \ /
      Last update: 2023-03-27 01:00    [W:7.655 / U:0.080 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site