Re: [PATCH RFC 07/10] tty: serial: Add Cortina Systems CS75xx UART driver

[Fil Dunsky <filipp.dunsky@gmail.com>]

On Wed, Sep 30, 2026 at 09:14:34AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2026 at 10:00:06AM +0300, Fil Dunsky via B4 Relay wrote:
> > The ports are named ttyCS so that the driver can coexist with the 8250
> > driver in multiplatform kernels, and PORT_GENERIC is used rather than
> > allocating a new port type.
>
> But even if you build a multi-platform kernel, only one of the uarts is
> going to be on the system, so why have a new name for it?  Can't you
> just use the "default" name instead?  If you do that, what happens?

I tried it on the board: multi_v7_defconfig with the driver renamed to
ttyS. The 8250 driver registers its runtime ports (ttyS0..ttyS4) even
though there is no 8250 on this SoC, so the probe fails:

  sysfs: cannot create duplicate filename '/class/tty/ttyS0'
  ...
  cs75xx-uart f0070110.serial: Cannot register tty device on line 0
  Warning: unable to open an initial console.

That is why I went with a separate name, like ttyAMA or ttymxc. If
there is a better way to share ttyS with 8250 I am happy to change it.

> > +/*
> > + * UART driver for the Cortina Systems CS75xx (Goldengate G2) SoCs
> > + *
> > + * Based on the Cortina Systems vendor driver.
> > + */
>
> No copyright info?

The vendor file (serial_cortina.c) carries no copyright notice, only the
GPL-2.0 statement and MODULE_AUTHOR("Cortina-Systems"). I will add mine
and name the vendor file as the origin in v2.

> > +static void cs75xx_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > +{
> > +}
>
> Why an empty function?  Can't you just leave it out?

The block has no modem control lines. serial_core calls ->set_mctrl()
without a NULL check (uart_update_mctrl() and others), so it has to
exist. I will add a comment saying so.

> > +static void cs75xx_uart_break_ctl(struct uart_port *port, int break_state)
> > +{
> > +}
>
> Same here.

That one is optional, I will drop it.

> > +static void cs75xx_uart_release_port(struct uart_port *port)
> > +{
> > +}
>
> This feels wrong.
>
> > +static int cs75xx_uart_request_port(struct uart_port *port)
> > +{
> > +     return 0;
> > +}
>
> As does this.

Both are optional and the resources are managed in probe, so I will
drop them in v2.

Thanks for the review,
Fil