Re: [PATCH v1] serial: qcom-geni: Add support to increase UART ports efficiently

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, May 29, 2025 at 05:03:25PM +0800, zongjian wrote:
> Populate members of qcom_geni_uart_ports through a loop for
> better maintainability. 

What does this mean exactly?

> 
> Increase the UART ports to 5, as a few use cases require more than 3 UART ports.

You are doing two different things here in the same patch.  As you know,
this means this should be split up into multiple patches.

> Signed-off-by: zongjian <quic_zongjian@quicinc.com>

We need a full name here, not just an email alias.

> ---
>  drivers/tty/serial/qcom_geni_serial.c | 40 +++++++++------------------
>  1 file changed, 13 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 140c3ae5ead2..d969c96b9690 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -77,7 +77,7 @@
>  #define STALE_TIMEOUT			16
>  #define DEFAULT_BITS_PER_CHAR		10
>  #define GENI_UART_CONS_PORTS		1
> -#define GENI_UART_PORTS			3
> +#define GENI_UART_PORTS			5

You need a better justification for increasing the number of ports here
in the changelog other than what you wrote :)

>  #define DEF_FIFO_DEPTH_WORDS		16
>  #define DEF_TX_WM			2
>  #define DEF_FIFO_WIDTH_BITS		32
> @@ -153,6 +153,7 @@ static const struct uart_ops qcom_geni_console_pops;
>  static const struct uart_ops qcom_geni_uart_pops;
>  static struct uart_driver qcom_geni_console_driver;
>  static struct uart_driver qcom_geni_uart_driver;
> +static struct qcom_geni_serial_port qcom_geni_uart_ports[GENI_UART_PORTS];
>  
>  static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
>  static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
> @@ -163,32 +164,15 @@ static inline struct qcom_geni_serial_port *to_dev_port(struct uart_port *uport)
>  	return container_of(uport, struct qcom_geni_serial_port, uport);
>  }
>  
> -static struct qcom_geni_serial_port qcom_geni_uart_ports[GENI_UART_PORTS] = {
> -	[0] = {
> -		.uport = {
> -			.iotype = UPIO_MEM,
> -			.ops = &qcom_geni_uart_pops,
> -			.flags = UPF_BOOT_AUTOCONF,
> -			.line = 0,
> -		},
> -	},
> -	[1] = {
> -		.uport = {
> -			.iotype = UPIO_MEM,
> -			.ops = &qcom_geni_uart_pops,
> -			.flags = UPF_BOOT_AUTOCONF,
> -			.line = 1,
> -		},
> -	},
> -	[2] = {
> -		.uport = {
> -			.iotype = UPIO_MEM,
> -			.ops = &qcom_geni_uart_pops,
> -			.flags = UPF_BOOT_AUTOCONF,
> -			.line = 2,
> -		},
> -	},
> -};
> +static void qcom_geni_serial_port_init(void)
> +{
> +	for (int i = 0; i < GENI_UART_PORTS; i++) {
> +		qcom_geni_uart_ports[i].uport.iotype = UPIO_MEM;
> +		qcom_geni_uart_ports[i].uport.ops = &qcom_geni_uart_pops;
> +		qcom_geni_uart_ports[i].uport.flags = UPF_BOOT_AUTOCONF;
> +		qcom_geni_uart_ports[i].uport.line = i;
> +	}

If this is such a simple structure, that never changes, why is it needed
at all?  It can be easily determined from the "line" value, right?  Just
remove it entirely please, as it's much better to have a dynamic number
of ports (i.e. what is actually in the system), than a hard-coded one.

thanks,

greg k-h