Message-ID: <20fc81c9-5517-ce1e-639a-3b425cf27759@gmail.com>
Date:   Wed, 15 Mar 2023 17:40:43 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 14/15] tty: serial: Add Nuvoton ma35d1 serial driver
 support
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        lee@kernel.org, mturquette@baylibre.com, sboyd@kernel.org,
        p.zabel@pengutronix.de, jirislaby@kernel.org,
        devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        schung@nuvoton.com, Jacky Huang <ychuang3@nuvoton.com>
References: <20230315072902.9298-1-ychuang570808@gmail.com>
 <20230315072902.9298-15-ychuang570808@gmail.com> <ZBF1z5Bx9jnrpxox@kroah.com>
From:   Jacky Huang <ychuang570808@gmail.com>
In-Reply-To: <ZBF1z5Bx9jnrpxox@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Dear Greg,

Thank you for your review.


On 2023/3/15 下午 03:37, Greg KH wrote:
> 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:

This UART controller was designed for over 15 years ago and was used on 
many Nuvoton chips.
The register interface is not compatible with 8250, so the 8250 driver 
cannot be applied, but
the functions are compatible.

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

OK, we will put the .h into .c in the next version.

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

Actually, we do not use it from userspace. I will remove it in the next 
version.


>> +static void
>> +receive_chars(struct uart_ma35d1_port *up)
> Please just put all one one line.
>

OK, I will fix it.

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

Thanks for pointing this out. I will add a timeout check to this 
infinite loop.

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

Yes, we do not handle ioctls.
I will remove both ma35d1serial_ioctl() and "ioctl  = 
ma35d1serial_ioctl," in the next version.


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

OK, we will fix it in the next version.

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

The ma35d1serial_suspend_port() and ma35d1serial_resume_port() were used in
previous ARM9 projects for userspace proprietary suspend/resume control.

As it's obsoleted in ma35s1, I will remove these two functions in the 
next version.

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

As replied above, we will remove the serial_core.h modification from 
this patch.


> thanks,
>
> greg k-h

Best Regards,

Jacky Huang