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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2921 bytes --]

On Tue, 9 May 2023, Arnd Bergmann wrote:

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

So use the normal name for compatible ones and HW specific names for the 
others?

It might not be compatible in everything but surely 8250 influence is 
visible here and there.

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

That is what I also suggested for the rest of the registers.

-- 
 i.

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