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

[Jiri Slaby <jirislaby@kernel.org>]

On 14. 10. 25, 21:15, 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 | 279 ++++++++++++++++++++++++++++
>   drivers/tty/serial/8250/Kconfig     |  13 ++
>   drivers/tty/serial/8250/Makefile    |   1 +
>   3 files changed, 293 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..e1dccc0e1026
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_keba.c
> @@ -0,0 +1,279 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 KEBA Industrial Automation GmbH
> + *
> + * Driver for KEBA UART FPGA IP core
> + */
> +
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/misc/keba.h>
> +
> +#include "8250.h"
> +
> +#define KUART "kuart"
> +
> +/* flags */
> +#define KUART_RS485		0x00000001
> +#define KUART_USE_CAPABILITY	0x00000002

use BIT()

> +/* registers */
> +#define KUART_VERSION		0x0000
> +#define KUART_REVISION		0x0001
> +#define KUART_CAPABILITY	0x0002
> +#define KUART_CONTROL		0x0004
> +#define KUART_BASE		0x000C
> +#define KUART_REGSHIFT		2
> +#define KUART_CLK		1843200
> +
> +/* mode flags */
> +#define KUART_MODE_NONE		0
> +#define KUART_MODE_RS485	1
> +#define KUART_MODE_RS422	2
> +#define KUART_MODE_RS232	3

Use enum.

> +/* capability flags */
> +#define KUART_CAPABILITY_NONE	(1<<KUART_MODE_NONE)
> +#define KUART_CAPABILITY_RS485	(1<<KUART_MODE_RS485)
> +#define KUART_CAPABILITY_RS422	(1<<KUART_MODE_RS422)
> +#define KUART_CAPABILITY_RS232	(1<<KUART_MODE_RS232)

use BIT()

> +#define KUART_CAPABILITY_MASK	0x0000000f

Use GENMASK().

> +/* Additional Control Register DTR line configuration */
> +#define UART_ACR_DTRLC_MASK		0x18
> +#define UART_ACR_DTRLC_COMPAT		0x00
> +#define UART_ACR_DTRLC_ENABLE_LOW	0x10
> +
> +struct kuart {
> +	struct keba_uart_auxdev *auxdev;
> +	void __iomem *base;
> +	int line;

unsigned

> +
> +	unsigned long flags;

I would say unsigned int would be enough.

> +	u8 capability;
> +	int mode;

Use the new enum as a type.

> +};
> +
> +static void kuart_set_phy_mode(struct kuart *kuart, int mode)

enum too.

> +{
> +	iowrite8(mode, kuart->base + KUART_CONTROL);
> +}
...
> +static int kuart_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			      struct serial_rs485 *rs485)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	struct kuart *kuart = port->private_data;
> +	int mode;

enum

...
> +	return 0;
> +}

Otherwise looks good.

thanks,
-- 
js
suse labs