Re: [PATCH v4 2/2] serial: 8250: add driver for KEBA UART

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Oct 23, 2025 at 05:12:29PM +0200, Gerhard Engleder wrote:
> 
> The KEBA UART is found in the system FPGA of KEBA PLC devices. It is
> mostly 8250 compatible with extension for some UART modes.
> 
> 3 different variants exist. The simpliest variant supports only RS-232
> and is used for debug interfaces. The next variant supports only RS-485
> and is used mostly for communication with KEBA panel devices. The third
> variant is able to support RS-232, RS-485 and RS-422. For this variant
> not only the mode of the UART is configured, also the physics and
> transceivers are switched according to the mode.

...

> +#include <linux/auxiliary_bus.h>

+ bits.h
+ container_of.h

> +#include <linux/device.h>

I don't see how it's being used.
What I see are

+ dev_printk.h
+ device/devres.h

+ err.h

> +#include <linux/io.h>
> +#include <linux/misc/keba.h>

+ mod_devicetable.h

> +#include <linux/module.h>

+ spinlock.h
+ types.h

...

> +static int kuart_probe(struct auxiliary_device *auxdev,
> +		       const struct auxiliary_device_id *id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct uart_8250_port uart = {};
> +	struct resource res;
> +	struct kuart *kuart;
> +	int retval;
> +
> +	kuart = devm_kzalloc(dev, sizeof(*kuart), GFP_KERNEL);
> +	if (!kuart)
> +		return -ENOMEM;
> +	kuart->auxdev = container_of(auxdev, struct keba_uart_auxdev, auxdev);
> +	kuart->flags = id->driver_data;
> +	auxiliary_set_drvdata(auxdev, kuart);
> +
> +	/*
> +	 * map only memory in front of UART registers, UART registers will be
> +	 * mapped by serial port
> +	 */
> +	res = kuart->auxdev->io;
> +	res.end = res.start + KUART_BASE - 1;
> +	kuart->base = devm_ioremap_resource(dev, &res);
> +	if (IS_ERR(kuart->base))
> +		return PTR_ERR(kuart->base);
> +
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/*
> +		 * supported modes are read from capability register, at least
> +		 * one mode other than none must be supported
> +		 */
> +		kuart->capability = ioread8(kuart->base + KUART_CAPABILITY) &
> +				    KUART_CAPABILITY_MASK;
> +		if ((kuart->capability & ~KUART_CAPABILITY_NONE) == 0)
> +			return -EIO;
> +	}
> +
> +	spin_lock_init(&uart.port.lock);
> +	uart.port.dev = dev;
> +	uart.port.mapbase = kuart->auxdev->io.start + KUART_BASE;
> +	uart.port.irq = kuart->auxdev->irq;
> +	uart.port.uartclk = KUART_CLK;
> +	uart.port.private_data = kuart;
> +
> +	/* 8 bit registers are 32 bit aligned => shift register offset */
> +	uart.port.iotype = UPIO_MEM32;
> +	uart.port.regshift = KUART_REGSHIFT;

Can't you call uart_read_port_properties()?

If ever you gain some properties either via FW description or via software
nodes, they will be automatically used without need to update the driver!

> +	/*
> +	 * UART mixes 16550, 16750 and 16C950 (for RS485) standard => auto
> +	 * configuration works best
> +	 */
> +	uart.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_IOREMAP;
> +
> +	/*
> +	 * UART supports RS485, RS422 and RS232 with switching of physical
> +	 * interface
> +	 */
> +	uart.port.rs485_config = kuart_rs485_config;
> +	if (kuart->flags & KUART_RS485) {
> +		uart.port.rs485_supported.flags = SER_RS485_ENABLED |
> +						  SER_RS485_RTS_ON_SEND;
> +		uart.port.rs485.flags = SER_RS485_ENABLED |
> +					SER_RS485_RTS_ON_SEND;
> +	}
> +	if (kuart->flags & KUART_USE_CAPABILITY) {
> +		/* default mode priority is RS485 > RS422 > RS232 */
> +		if (kuart->capability & KUART_CAPABILITY_RS422) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND |
> +							   SER_RS485_MODE_RS422;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND |
> +						SER_RS485_MODE_RS422;
> +		}
> +		if (kuart->capability & KUART_CAPABILITY_RS485) {
> +			uart.port.rs485_supported.flags |= SER_RS485_ENABLED |
> +							   SER_RS485_RTS_ON_SEND;
> +			uart.port.rs485.flags = SER_RS485_ENABLED |
> +						SER_RS485_RTS_ON_SEND;
> +		}
> +	}
> +
> +	retval = serial8250_register_8250_port(&uart);
> +	if (retval < 0) {

> +		dev_err(&auxdev->dev, "UART registration failed!\n");
> +		return retval;

		return dev_err_probe(...);

> +	}
> +	kuart->line = retval;
> +
> +	return 0;
> +}

...

Since driver is about to be applied to serial-next, I suggest to send a
followup(s) to address my comments.

-- 
With Best Regards,
Andy Shevchenko