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

[Greg KH]

On Wed, May 13, 2026 at 04:46:57PM +0800, liu.qingtao2 at zte.com.cn wrote:
> >From 9eba3be2e9b4d5c77956258e3c5db95049c3a895 Mon Sep 17 00:00:00 2001
> From: Wenhong Liu <liu.wenhong35 at zte.com.cn>
> 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 at intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140029.NXkDToZ7-lkp@intel.com/
> | Reported-by: kernel test robot <lkp at intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202602140108.kLMOYbwS-lkp@intel.com/

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'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'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's most likely the console
> + * So let's leave module_exit but move module_init to an earlier place

Again, your tooling is broken :(

thanks,

greg k-h