Re: [PATCH 1/2] code style: Format ns16550 driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -14,7 +14,7 @@
>   * abstracted.
>   */
>  #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> -# define NS16550_PCI
> +#define NS16550_PCI
>  #endif

Hmm. Either form ought to be okay, so the line would want leaving untouched.

> @@ -43,12 +43,12 @@
>  
>  static struct ns16550 {
>      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> -    u64 io_base;   /* I/O port or memory-mapped I/O address. */
> +    u64 io_base; /* I/O port or memory-mapped I/O address. */
>      u64 io_size;
>      int reg_shift; /* Bits to shift register offset by */

Breaking alignment between comments (also in the immediately following hunk),
while at the same time ...

>      int reg_width; /* Size of access to use, the registers
>                      * themselves are still bytes */

... not taking care of the comment style violation here?

> @@ -63,8 +63,8 @@ static struct ns16550 {
>      bool dw_usr_bsy;
>  #ifdef NS16550_PCI
>      /* PCI card parameters. */
> -    bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind bridge */
> -    bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */
> +    bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> +    bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */

(Just leaving this here for context of the earlier comment.)

> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port 
> *port)
>      if ( ns16550_ioport_invalid(uart) )
>          return -EIO;
>  
> -    return ( (ns_read_reg(uart, UART_LSR) &
> -              uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
> +    return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
> +               ? uart->fifo_size
> +               : 0;

Indentation of the ? and : lines is clearly wrong here? What is the tool
doing?

> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>  #ifdef NS16550_PCI
>      if ( uart->bar && uart->io_base >= 0x10000 )
>      {
> -        pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> -                                  uart->ps_bdf[2]),
> -                         PCI_COMMAND, PCI_COMMAND_MEMORY);
> +        pci_conf_write16(
> +            PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
> +            PCI_COMMAND,
> +            PCI_COMMAND_MEMORY);
>          return;
>      }

Hmm, transforming a well-formed block into another well-formed one. No
gain? (Same again further down.)

> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>      else
>      {
>          /* Baud rate already set: read it out from the divisor latch. */
> -        divisor  = ns_read_reg(uart, UART_DLL);
> +        divisor = ns_read_reg(uart, UART_DLL);
>          divisor |= ns_read_reg(uart, UART_DLM) << 8;

An example where tabulation is being broken. There are quite a bit worse
ones further down.

> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>      ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>  
>      /* Enable and clear the FIFOs. Set a large trigger threshold. */
> -    ns_write_reg(uart, UART_FCR,
> -                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | 
> UART_FCR_TRG14);
> +    ns_write_reg(uart,
> +                 UART_FCR,
> +                 UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> +                     UART_FCR_TRG14);

What's the underlying indentation rule here? The way it's re-formatted
certainly looks odd to me. What we occasionally do in such cases is add
parentheses:

    ns_write_reg(uart,
                 UART_FCR,
                 (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
                  UART_FCR_TRG14));

Also - does the format they apply demand one argument per line? Else
why not

    ns_write_reg(uart, UART_FCR,
                 (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
                  UART_FCR_TRG14));

Plus what's their criteria to choose between this style of function calls
and the one in context higher up for calls to pci_conf_write16()?

> @@ -424,31 +430,37 @@ static void __init cf_check ns16550_init_postirq(struct 
> serial_port *port)
>  
>      /* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */
>      bits = uart->data_bits + uart->stop_bits + !!uart->parity;
> -    uart->timeout_ms = max_t(
> -        unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
> +    uart->timeout_ms =
> +        max_t(unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);

Again both forms look conforming to me. I think there's a general issue
with the tool making transformations when none are needed. I won't
further point out any such.

> @@ -1197,7 +1174,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, 
> unsigned int idx)
>  
>                  nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
>                                                 PCI_HEADER_TYPE) &
> -                               0x80)) ? f + 1 : 8;
> +                               0x80))
> +                            ? f + 1
> +                            : 8;

Again the odd indentation of ? and :.

> @@ -1422,19 +1409,19 @@ struct serial_param_var {
>   * com_console_options for serial port com1 and com2.
>   */
>  static const struct serial_param_var __initconst sp_vars[] = {
> -    {"baud", baud_rate},
> -    {"clock-hz", clock_hz},
> -    {"data-bits", data_bits},
> -    {"io-base", io_base},
> -    {"irq", irq},
> -    {"parity", parity},
> -    {"reg-shift", reg_shift},
> -    {"reg-width", reg_width},
> -    {"stop-bits", stop_bits},
> +    { "baud",      baud_rate  },
> +    { "clock-hz",  clock_hz   },
> +    { "data-bits", data_bits  },
> +    { "io-base",   io_base    },
> +    { "irq",       irq        },
> +    { "parity",    parity     },
> +    { "reg-shift", reg_shift  },
> +    { "reg-width", reg_width  },
> +    { "stop-bits", stop_bits  },
>  #ifdef CONFIG_HAS_PCI
> -    {"bridge", bridge_bdf},
> -    {"dev", device},
> -    {"port", port_bdf},
> +    { "bridge",    bridge_bdf },
> +    { "dev",       device     },
> +    { "port",      port_bdf   },
>  #endif
>  };

Interesting - here tabulation is being introduced.

> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>      if ( !parse_namevalue_pairs(str, uart) )
>          return;
>  
> - config_parsed:
> +config_parsed:

This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.

All of the remarks aside, there are also a lot of good changes that are being
made.

Jan