Re: [PATCH 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

[Greg KH]

On Mon, Nov 01, 2021 at 03:48:23PM +0800, hammer.hsieh wrote:
> Add Sunplus SoC UART Driver
> 
> Signed-off-by: hammer.hsieh <hammer.hsieh@xxxxxxxxxxx>

I doubt you sign your name with a "." in it, right?

Also this address does not match your From: address.  Please fix this up
when resending this.

> ---
>  MAINTAINERS                       |    2 +
>  drivers/tty/serial/Kconfig        |   23 +
>  drivers/tty/serial/Makefile       |    1 +
>  drivers/tty/serial/sunplus-uart.c | 1848 +++++++++++++++++++++++++++++++++++++
>  include/soc/sunplus/sp_uart.h     |  158 ++++
>  5 files changed, 2032 insertions(+)
>  create mode 100644 drivers/tty/serial/sunplus-uart.c
>  create mode 100644 include/soc/sunplus/sp_uart.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f863e97..0176026 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17949,6 +17949,8 @@ SUNPLUS UART DRIVER
>  M:	Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/serial/sunplus,uart.yaml
> +F:	drivers/tty/serial/sunplus-uart.c
> +F:	include/soc/sunplus/sp-uart.h
>  
>  SUPERH
>  M:	Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 131a6a5..319f5db 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1561,6 +1561,29 @@ config SERIAL_LITEUART_CONSOLE
>  	  and warnings and which allows logins in single user mode).
>  	  Otherwise, say 'N'.
>  
> +config SERIAL_SUNPLUS
> +	bool "Sunplus UART support"
> +	depends on OF
> +	select SERIAL_CORE
> +	help
> +	  Select this option if you would like to use Sunplus serial port on
> +	  Sunplus SoC SP7021.
> +	  If you enable this option, Sunplus serial ports in the system will
> +	  be registered as ttySx.
> +
> +config SERIAL_SUNPLUS_CONSOLE
> +	bool "Console on Sunplus UART"
> +	depends on SERIAL_SUNPLUS
> +	select SERIAL_CORE_CONSOLE
> +	select SERIAL_EARLYCON
> +	help
> +	  Select this option if you would like to use a Sunplus UART as the
> +	  system console.
> +	  Even if you say Y here, the currently visible virtual console
> +	  (/dev/tty0) will still be used as the system console by default, but
> +	  you can alter that using a kernel command line option such as
> +	  "console=ttySx".
> +
>  endmenu
>  
>  config SERIAL_MCTRL_GPIO
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 7da0856..61cc8de 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_SERIAL_RDA)	+= rda-uart.o
>  obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
>  obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
>  obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
> +obj-$(CONFIG_SERIAL_SUNPLUS)	+= sunplus-uart.o
>  
>  # GPIOLIB helpers for modem control lines
>  obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
> diff --git a/drivers/tty/serial/sunplus-uart.c b/drivers/tty/serial/sunplus-uart.c
> new file mode 100644
> index 0000000..d1524dc
> --- /dev/null
> +++ b/drivers/tty/serial/sunplus-uart.c
> @@ -0,0 +1,1848 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

I have to ask,. but are you sure about this license that it needs to be
"-or-later"?

> +/*
> + * Sunplus SoC UART driver
> + *
> + * Author: Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
> + * Tony Huang <tony.huang@xxxxxxxxxxx>
> + * Wells Lu <wells.lu@xxxxxxxxxxx>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/of_platform.h>
> +#include <asm/irq.h>
> +#if defined(CONFIG_MAGIC_SYSRQ)
> +#define SUPPORT_SYSRQ
> +#include <linux/sysrq.h>
> +#endif
> +#include <linux/serial_core.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/io.h>
> +#include <linux/dma-mapping.h>
> +#include <soc/sunplus/sp_uart.h>
> +
> +#ifdef CONFIG_PM_RUNTIME_UART
> +#include <linux/pm_runtime.h>
> +#endif

Why the #ifdef in the .c file?

> +
> +#include <linux/gpio.h>

GPIO in a serial driver?

> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <dt-bindings/clock/sp-sp7021.h>
> +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
> +#include <linux/delay.h>
> +#include <linux/hrtimer.h>

Please put the linux/ includes above the dt-bindings ones.

> +
> +#define DEVICE_NAME				"ttyS"
> +#define SP_UART_MAJOR			TTY_MAJOR

Why is this driver allowed to be named the same as the other serial
drivers?  What happens when you plug in a 8250 card to this device?

> +#define SP_UART_MINOR_START		64

Why here?

> +
> +#define NUM_UART		6	/* serial0,  ... */
> +#define NUM_UARTDMARX	2	/* serial10, ... */
> +#define NUM_UARTDMATX	2	/* serial20, ... */

Why are these hard codeded?

> +
> +#define ID_BASE_DMARX	10
> +#define ID_BASE_DMATX	20
> +
> +#define IS_UARTDMARX_ID(X) \
> +	(((X) >= (ID_BASE_DMARX)) && ((X) < (ID_BASE_DMARX + NUM_UARTDMARX)))
> +#define IS_UARTDMATX_ID(X) \
> +	(((X) >= (ID_BASE_DMATX)) && ((X) < (ID_BASE_DMATX + NUM_UARTDMATX)))
> +
> +//#define TTYS_KDBG_INFO
> +#define TTYS_KDBG_ERR
> +//#define TTYS_GPIO

Why is this commented out?

Why is this defined at all?

> +
> +#ifdef TTYS_KDBG_INFO
> +#define DBG_INFO(fmt, args ...)	pr_info("K_TTYS: " fmt, ## args)
> +#else
> +#define DBG_INFO(fmt, args ...)
> +#endif
> +#ifdef TTYS_KDBG_ERR
> +#define DBG_ERR(fmt, args ...)	pr_err("K_TTYS: " fmt, ## args)
> +#else
> +#define DBG_ERR(fmt, args ...)
> +#endif

No, please use dev_info(), dev_err() and dev_dbg() only.  Do not create
your own logging macros for a single driver, use the ones the kernel
gives you.

> +
> +#define SP_UART_CREAD_DISABLED		(1 << 16)
> +
> +#define UARXDMA_BUF_SZ			PAGE_SIZE
> +#define MAX_SZ_RXDMA_ISR		(1 << 9)
> +#define UATXDMA_BUF_SZ			PAGE_SIZE
> +
> +#define CLK_HIGH_UART			202500000
> +#define UART_RATIO				232
> +
> +#if defined(CONFIG_SP_MON)

What sets this?

I've stopped reviewing here.

thanks,

greg k-h