Re: [PATCH v2 2/2] tty: serial: Add LRX UART driver

From: Greg KH

Date: Wed May 13 2026 - 06:36:04 EST

On Wed, May 13, 2026 at 04:46:57PM +0800, liu.qingtao2@xxxxxxxxxx wrote:
> >From 9eba3be2e9b4d5c77956258e3c5db95049c3a895 Mon Sep 17 00:00:00 2001
> From: Wenhong Liu <liu.wenhong35@xxxxxxxxxx>
> Date: Tue, 28 Apr 2026 22:30:40 +0800
> Subject: [PATCH v2 2/2] tty: serial: Add LRX UART driver
>
> Add support for the ZTE LRX UART controller with the following features:
> - Support for FIFO mode (16-byte depth)
> - Baud rate configuration
> - Standard asynchronous communication formats:
> * Data bits: 5, 6, 7, 8, 9 bits
> * Parity: odd, even, fixed, none
> * Stop bits: 1 or 2 bits
> - Hardware flow control (RTS/CTS)
> - Multiple interrupt reporting mechanisms
> - DMA support for improved performance
>
> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140029.NXkDToZ7-lkp@xxxxxxxxx/
> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140108.kLMOYbwS-lkp@xxxxxxxxx/

Why are these 4 lines here? The kernel test robot found bugs in your
previous versions, so fix that, but no need to list that here, right?

> --- /dev/null
> +++ b/drivers/tty/serial/lrx_uart.c
> @@ -0,0 +1,2822 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial Port driver for ZTE LRX
> + *
> + * Copyright (c) 2025, ZTE Corporation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysrq.h>
> +#include <linux/device.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial_core.h>
> +#include <linux/serial.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/scatterlist.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/sizes.h>
> +#include <linux/io.h>
> +#include <linux/acpi.h>
> +
> +#define UART_NR 14

Why 14? Why limit yourself at all?

> +
> +#define ISR_PASS_LIMIT 256
> +
> +#define LRX_UART_NAME "lrx-uart"

KBUILD_MODNAME?

> +#define LRX_UART_TTY_PREFIX "ttyLRX"

Why are you using a new name and not the existing ones? Why is this not
just a variant of the existing 8250 serial driver? Surely this is not a
totally new UART that looks nothing like that one, right?

> +/* There is by now at least one vendor with differing details, so handle it */

So a new UART already has conflicting implementations? How can that
happen?

> +/* Deals with DMA transactions */
> +
> +struct lrx_uart_dmabuf {
> + dma_addr_t dma;
> + size_t len;
> + char *buf;

u8?

> +/*
> + * We wrap our port structure around the generic uart_port.
> + */
> +struct lrx_uart_port {
> + struct uart_port port;
> + const u16 *reg_offset;
> + struct clk *clk;
> + const struct vendor_data *vendor;
> + unsigned int im; /* interrupt mask */
> + unsigned int old_status;
> + unsigned int fifosize; /* vendor-specific */
> + unsigned int fixed_baud; /* vendor-set fixed baud rate */
> + char type[16];
> + bool rs485_tx_started;
> + unsigned int rs485_tx_drain_interval; /* usecs */
> +#ifdef CONFIG_DMA_ENGINE

Why would this UART not use the DMA engine? When would that ever be
disabled?

> +/*
> + * Reads up to 256 characters from the FIFO or until it&apos;s empty and

This implies that some tool wrote this code. Please always properly
document that and where it copied the information from in order to write
this code.

> +/*
> + * All the DMA operation mode stuff goes inside this ifdef.
> + * This assumes that you have a generic DMA device interface,
> + * no custom DMA interfaces are supported.
> + */
> +#ifdef CONFIG_DMA_ENGINE
> +
> +#define LRX_UART_DMA_BUFFER_SIZE PAGE_SIZE

Why not just use PAGE_SIZE? And how do you know the dma buffer is the
same size always? When using 16k pages, the hardware knows this?

> +/*
> + * Try to refill the TX DMA buffer.
> + * Locking: called with port lock held and IRQs disabled.
> + * Returns:
> + * 1 if we queued up a TX DMA buffer.
> + * 0 if we didn&apos;t want to handle this by DMA

Again, fix your tooling :(

> +/*
> + * Flush the transmit buffer.
> + * Locking: called with port lock held and IRQs disabled.
> + */
> +static void lrx_uart_dma_flush_buffer(struct uart_port *port)
> +__releases(&sup->port.lock)
> +__acquires(&sup->port.lock)

Ok, but:

> +{
> + struct lrx_uart_port *sup =
> + container_of(port, struct lrx_uart_port, port);
> +
> + if (!sup->using_tx_dma)
> + return;
> +
> + dmaengine_terminate_async(sup->dmatx.chan);
> +
> + if (sup->dmatx.queued) {
> + dma_unmap_single(sup->dmatx.chan->device->dev, sup->dmatx.dma,
> + sup->dmatx.len, DMA_TO_DEVICE);
> + sup->dmatx.queued = false;
> + sup->dmacr &= ~UARTFCCR_TXDMAE;
> + lrx_uart_write(sup->dmacr, sup, REG_FCCR);
> + }
> +}

I don't see those locks being touched, where did that happen?

> +static irqreturn_t lrx_uart_int(int irq, void *dev_id)
> +{
> + struct lrx_uart_port *sup = dev_id;
> + unsigned int status, pass_counter = ISR_PASS_LIMIT;
> + int handled = 0;

bool?

> +static int lrx_uart_hwinit(struct uart_port *port)
> +{
> + struct lrx_uart_port *sup =
> + container_of(port, struct lrx_uart_port, port);

You do this "container_of()" everywhere, why not write a simple macro
for it to make it more obvious like to_lxr_uart_port()?

And why not use container_of_const()?

> +static int lrx_uart_allocate_irq(struct lrx_uart_port *sup)
> +{
> + lrx_uart_write(sup->im, sup, REG_IMSC);
> +
> + return request_irq(sup->port.irq, lrx_uart_int, IRQF_SHARED, "lrx-uart", sup);

You had a driver name way above, why not use that instead of this string
here?

> + /* Set baud rate */
> + lrx_uart_write(quot & 0x3f, sup, REG_FD);
> + lrx_uart_write(quot >> 6, sup, REG_IND);
> +
> + /*
> + * ----------v----------v----------v----------v-----
> + * NOTE: REG_FRCR MUST BE WRITTEN AFTER REG_FD & REG_IND.
> + * ----------^----------^----------^----------^-----
> + */

Why the odd comment style?

> + lrx_uart_write(frcr, sup, REG_FRCR);
> +
> + lrx_uart_write(fccr, sup, REG_FCCR);

Why the blank line between these?

> +static struct lrx_uart_port *lrx_uart_console_ports[UART_NR];

Why isn't this a dynamic list?

> +/*
> + * While this can be a module, if builtin it&apos;s most likely the console
> + * So let&apos;s leave module_exit but move module_init to an earlier place

Again, your tooling is broken :(

thanks,

greg k-h