Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <89268a72-468f-415d-0f52-e1e2b8f1ad1e@gmail.com>
Date:   Wed, 29 Mar 2023 16:06:31 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v6 11/12] tty: serial: Add Nuvoton ma35d1 serial driver
 support
Content-Language: en-US
To:     Jiri Slaby <jirislaby@kernel.org>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, lee@kernel.org,
        mturquette@baylibre.com, sboyd@kernel.org, p.zabel@pengutronix.de,
        gregkh@linuxfoundation.org
Cc:     devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        arnd@arndb.de, schung@nuvoton.com, mjchen@nuvoton.com,
        Jacky Huang <ychuang3@nuvoton.com>
References: <20230328021912.177301-1-ychuang570808@gmail.com>
 <20230328021912.177301-12-ychuang570808@gmail.com>
 <f44fffcf-c592-0c73-6e6c-f5914de82503@kernel.org>
From:   Jacky Huang <ychuang570808@gmail.com>
In-Reply-To: <f44fffcf-c592-0c73-6e6c-f5914de82503@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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