Re: [PATCH v10 10/10] tty: serial: Add Nuvoton ma35d1 serial driver support

[Arnd Bergmann]

On Tue, May 9, 2023, at 12:17, Ilpo Järvinen wrote:
> On Mon, 8 May 2023, Jacky Huang wrote:
>> +
>> +#define UART_NR			17
>> +
>> +#define UART_REG_RBR		0x00
>> +#define UART_REG_THR		0x00
>> +#define UART_REG_IER		0x04
>> +#define UART_REG_FCR		0x08
>> +#define UART_REG_LCR		0x0C
>> +#define UART_REG_MCR		0x10
>
> These duplicate include/uapi/linux/serial_reg.h ones, use the std ones 
> directly.
>
> Setup regshift too and use it in serial_in.

I think this came up in previous reviews, but it turned out that
only the first six registers are compatible, while the later
ones are all different, and it's not 8250 compatible.

It might be helpful to rename the registers to something
with a prefix other than UART_REG_*, to avoid the confusion
and possible namespace clash.

>
>> +/* UART_REG_IER - Interrupt Enable Register */
>> +#define IER_RDA_IEN		BIT(0)  /* RBR Available Interrupt Enable */
>> +#define IER_THRE_IEN		BIT(1)  /* THR Empty Interrupt Enable */
>> +#define IER_RLS_IEN		BIT(2)  /* RX Line Status Interrupt Enable */
>
> These look same as UART_IER bits, use the std ones.
...
> Are these same as UART_FCR_CLEAR_* functionality wise? If they're use std 
> ones.

Again, I'd think we're better off having a distinct naming for
them than trying to share the definitions with 8250.

>> +static struct uart_driver ma35d1serial_reg = {
>> +	.owner        = THIS_MODULE,
>> +	.driver_name  = "serial",
>> +	.dev_name     = "ttyS",
>> +	.major        = TTY_MAJOR,
>> +	.minor        = 64,
>> +	.cons         = MA35D1SERIAL_CONSOLE,
>> +	.nr           = UART_NR,
>> +};
>
> This doesn't seem necessary, 8250 core will have the uart_driver for you
> and most of the console stuff too. You just need to setup a few things 
> correctly (see the setup functions in 8250_early for ideas/examples).
>...
>> +
>> +	ret = uart_add_one_port(&ma35d1serial_reg, &up->port);
>
> For 8250, you should be using serial8250_register_8250_port(). See the 
> other drivers how to setup the console functions.

Consequently, this should also be kept separate from the serial8250
driver, I don't see a way to fit the nuvoton code into the existing
driver without making the resulting driver worse for everyone.

There is one thing that absolutely needs to be changed though:
the driver_name/dev_name/major/minor fields all clash with the
8250 driver, so you cannot have a kernel that has both drivers
built-in. All of these should change to get out of the way of the
existing drivers.

        Arnd