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

[Gerhard Engleder <gerhard@engleder-embedded.com>]

On 26.11.25 16:46, Ilpo Järvinen wrote:
> On Thu, 23 Oct 2025, Gerhard Engleder wrote:
> 
>> From: Gerhard Engleder <eg@keba.com>
>>
>> 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.
>>
>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>> Tested-by: Daniel Gierlinger <gida@keba.com>
>> ---
>>   drivers/tty/serial/8250/8250_keba.c | 280 ++++++++++++++++++++++++++++
>>   drivers/tty/serial/8250/Kconfig     |  13 ++
>>   drivers/tty/serial/8250/Makefile    |   1 +
>>   3 files changed, 294 insertions(+)
>>   create mode 100644 drivers/tty/serial/8250/8250_keba.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_keba.c b/drivers/tty/serial/8250/8250_keba.c
>> new file mode 100644
>> index 000000000000..c05b89551b12
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_keba.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2025 KEBA Industrial Automation GmbH
>> + *
>> + * Driver for KEBA UART FPGA IP core
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/misc/keba.h>
>> +#include <linux/module.h>
> 
> + linux/serial_core.h

Is this really necessary even with the include of 8250.h below?

>> +
>> +#include "8250.h"
>> +
>> +#define KUART "kuart"

...

>> +static void kuart_enhanced_mode(struct uart_8250_port *up, bool enable)
>> +{
>> +	u8 lcr, efr;
>> +
>> +	/* backup LCR register */
> 
> Save + restore is quite obvious thing. IMO, no comment is needed about it.

Yes it could be ommited. The patch is already merged, so I would keep
it. Ok?

>> +	lcr = serial_in(up, UART_LCR);
>> +
>> +	/* enable 650 compatible register set (EFR, ...) */
>> +	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>> +
>> +	/* enable/disable enhanced mode with indexed control registers */
>> +	efr = serial_in(up, UART_EFR);
>> +	if (enable)
>> +		efr |= UART_EFR_ECB;
>> +	else
>> +		efr &= ~UART_EFR_ECB;
>> +	serial_out(up, UART_EFR, efr);
>> +
>> +	/* disable 650 compatible register set, restore LCR */
>> +	serial_out(up, UART_LCR, lcr);
>> +}
>> +
>> +static void kuart_dtr_line_config(struct uart_8250_port *up, u8 dtrlc)
>> +{
>> +	u8 acr;
>> +
>> +	/* set index register to 0 to access ACR register */
>> +	serial_out(up, UART_SCR, UART_ACR);
> 
> So the scratch register has some special use on this UART (register
> multiplexer?), it would probably better name it with define, if that's the
> case.

This UART supports an enhanced mode, which changes the behavior of some
registers. But the register still have their normal use with enhanced
mode disabled. So I would keep the register name.

>> +
>> +	/* set value register to 0x10 writing DTR mode (1,0) */
>> +	acr = serial_in(up, UART_LSR);
>> +	acr &= ~UART_ACR_DTRLC_MASK;
>> +	acr |= dtrlc;
>> +	serial_out(up, UART_LSR, acr);
>> +}

...

>> +	/*
>> +	 * 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;
>> +		}
>> +	}
> 
> Is it so that only one mode is supported or can that be changes using
> kuart_rs485_config() in which case you should have all flags listed (you
> seem to talk about priority so that sounds like all are supported)?

Both. As written in the commit message, there are 3 variants of the
device. 2 variants support only one mode and 1 variant supports up to
3 modes.

>> +
>> +	retval = serial8250_register_8250_port(&uart);
>> +	if (retval < 0) {
>> +		dev_err(&auxdev->dev, "UART registration failed!\n");
> 
> Missing header.

I will check for the header.

As this patch is already merged, I will do a follow up.

regards, gerhard