| Message ID | 20250216102108.2665222-2-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] code style: Format ns16550 driver | expand |
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
Hello, Jan! On 17.02.25 12:20, Jan Beulich wrote: > 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. This is controlled by PPIndentWidth option [1]: we have it set to "-1" which means "When set to -1 (default) *IndentWidth* is used also for preprocessor statements." Unfortunately, I was not able to see a change with PPIndentWidth + IndentWidth PPIndentWidth: -1 IndentWidth: 4 --- test_define.c 2025-02-19 11:20:30.708096428 +0200 +++ test_define_minus1.c 2025-02-19 11:21:42.313013373 +0200 @@ -1,5 +1,5 @@ #ifdef __linux__ -# define FOO +#define FOO #else -# define BAR +#define BAR #endif PPIndentWidth: 1 IndentWidth: 4 --- test_define.c 2025-02-19 11:20:30.708096428 +0200 +++ test_define_1.c 2025-02-19 11:23:46.986618706 +0200 @@ -1,5 +1,5 @@ #ifdef __linux__ -# define FOO +#define FOO #else -# define BAR +#define BAR #endif I thought it is a bug in the latest clang-format (21), but I was not able to get the expected result with clang-format 18 as well. I am not sure if I'm doing anything wrong with .clang-format, but this works one way for me as ow now. Takeaway: I was not able to control this, but the result seems to be acceptable It should be noted in the coding style though >> @@ -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 ... This one... >> int reg_width; /* Size of access to use, the registers >> * themselves are still bytes */ > ... not taking care of the comment style violation here? This is controlled by ReflowComments option [3]: From the tool point of view the comment is formatted correctly I didn't find a way to convert such comments into /* * Size of access to use, the registers * themselves are still bytes */ if this is what you mean. >> @@ -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.) ... and this one. It seems that the tool tries to always have a single space before the comment. There is an option SpacesBeforeTrailingComments [2] which unfortunately only controls C++ style comments and "...does not affect trailing block comments (|/*| - comments)..." So, it seems that this is the rule to consider >> @@ -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? There are number of options that have influence on this formatting: AllowShortBlocksOnASingleLine [4] BreakBeforeTernaryOperators [5] AlignOperands [6] I was not able to tweak these options to have the previous form. >> @@ -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.) No, gain from human point of view But there is a gain that it is now formatted automatically. >> @@ -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. This one will be impossible to implement with clang-format IMO. Because there are cases where you want this (like above) and you know why: the assignments look prettier here that way. But this doesn't mean that in some other places other assignments will look got for you if formatted the same way. The question here what is the metric (human perception?) in this case and how this perception can be be programmed into clang-format configuration? >> @@ -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()? This is controlled with AlignAfterOpenBracket [7] So this could be: *AlignAfterOpenBracket: Align* - 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); *AlignAfterOpenBracket: |DontAlign* - 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); |*AlignAfterOpenBracket: |AlwaysBreak* - 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); |*AlignAfterOpenBracket: |BlockIndent* - 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 + ); || | >> @@ -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. Again, this is something which a human can decide what is better for their taste. clang-format won't be able to treat such cases differently >> @@ -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. This is controlled with AlignArrayOfStructures [8] and we can have it as is if we set it to None The same was discussed before, so one can refresh that at [9] >> @@ -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. Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10] and is a binary option, which leaves no means to disable it as both true and false will re-format the code true:false: intf(){vs.intf(){ if(foo()){if(foo()){ label1:label1: bar();bar(); }} label2:label2: return1;return1; }} > All of the remarks aside, there are also a lot of good changes that are being > made. Agree and at least applying some of those changes may improve the code to be more consistent. > Jan Thank you, Oleksandr [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#ppindentwidth [2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#spacesbeforetrailingcomments [3] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments [4] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline [5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakbeforeternaryoperators [6] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands [7] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignafteropenbracket [8] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignarrayofstructures [9] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg02172.html [10] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
On 19.02.2025 13:39, Oleksandr Andrushchenko wrote: > On 17.02.25 12:20, Jan Beulich wrote: >> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>> @@ -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 ... > This one... >>> int reg_width; /* Size of access to use, the registers >>> * themselves are still bytes */ >> ... not taking care of the comment style violation here? > This is controlled by ReflowComments option [3]: > > From the tool point of view the comment is formatted correctly > I didn't find a way to convert such comments into > /* > * Size of access to use, the registers * themselves are still bytes */ if this is what you mean. Above you see what I received. I can't really deduce from this what formatting you suggested. In the case at hand, though, I think it's not the best idea anyway to put a multi-line comment past a declaration (or statement). /* * Size of access to use, the registers * themselves are still bytes */ int reg_width; is what I think would be better in such a case (artificially keeping this to be multi-line, even if it looks as if it might fit on just one line then). >>> @@ -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? > There are number of options that have influence on this formatting: > AllowShortBlocksOnASingleLine [4] > BreakBeforeTernaryOperators [5] > AlignOperands [6] > > I was not able to tweak these options to have the previous form. Right, sticking to the original form (with just the stray blanks zapped) would of course be best. Yet again - the tool is doing more transformations despite there not being any need. If, however, it does so, then one of my expectations would be that the ? and : are properly indented: return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; or return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; or return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask ? uart->fifo_size : 0); (not going to list more variants which are all okay). In any event, a fundamental requirement of mine is that such a tool would only apply adjustments when and where style is actively violated. I.e. in the case here: return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; That's not overly neat wrapping, but in line with our style. If the other form was demanded going forward, I'd be curious how you'd verbally describe the requirement in ./CODING_STYLE. >>> @@ -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.) > No, gain from human point of view > But there is a gain that it is now formatted automatically. See above: I'd first like to see a written, textual description for all these requirements. After all it needs to be possible for a human to write code that the tool then wouldn't try to re-arrange. Which in turn requires that the restrictions / constraints on the layout are spelled out. I'm not looking forward to pass all my patches through such a tool. I can write style- conforming code pretty well, with - of course - occasional oversights, right now. And that in multiple projects all with different styles. I expect to be in the position to do so also going forward. This, imo, requires that there be left some room for variations. Which in turn requires that the tool would leave alone anything that is not in conflict with the written down or defacto style. >>> @@ -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. > This one will be impossible to implement with clang-format IMO. > Because there are cases where you want this (like above) and you know why: > the assignments look prettier here that way. But this doesn't mean > that in some other places other assignments will look got for you if > formatted the same way. > The question here what is the metric (human perception?) in this case > and how this perception can be be programmed into clang-format > configuration? Which imo is a pretty strong argument against using any auto-formatting. At least up to the point where AI would end up being smart enough to mimic this human perception. >>> @@ -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()? > This is controlled with AlignAfterOpenBracket [7] > So this could be: > *AlignAfterOpenBracket: Align* > > - 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); As before indentation is bogus here, ... > *AlignAfterOpenBracket: |DontAlign* > > - 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); ... and even more so here, ... > |*AlignAfterOpenBracket: |AlwaysBreak* > > - 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); ... while this at least matches one of the forms we might presently use. Yet again - there was nothing wrong with the original layout. > |*AlignAfterOpenBracket: |BlockIndent* > > - 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 > + ); This clearly moves too far away from our present style. >>> @@ -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. > Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10] > and is a binary option, which leaves no means to disable it as both true and > false will re-format the code > > true:false: > intf(){vs.intf(){ > if(foo()){if(foo()){ > label1:label1: > bar();bar(); > }} > label2:label2: > return1;return1; > }} If there was some indentation meant to be in that blob, it was all lost, I'm afraid. Still: The name of the control being IndentGotoLabels and it being of boolean nature as you say, why would it do anything with an already-indented label? Jan
Hello, Jan! On 19.02.25 15:18, Jan Beulich wrote: > On 19.02.2025 13:39, Oleksandr Andrushchenko wrote: >> On 17.02.25 12:20, Jan Beulich wrote: >>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>> @@ -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 ... >> This one... >>>> int reg_width; /* Size of access to use, the registers >>>> * themselves are still bytes */ >>> ... not taking care of the comment style violation here? >> This is controlled by ReflowComments option [3]: >> >> From the tool point of view the comment is formatted correctly >> I didn't find a way to convert such comments into >> /* >> * Size of access to use, the registers * themselves are still bytes */ if this is what you mean. > Above you see what I received. I can't really deduce from this what > formatting you suggested. In the case at hand, though, I think it's not > the best idea anyway to put a multi-line comment past a declaration (or > statement). Sorry, for the formatting > /* > * Size of access to use, the registers > * themselves are still bytes > */ This is what I was trying to send > int reg_width; > > is what I think would be better in such a case (artificially keeping > this to be multi-line, even if it looks as if it might fit on just one > line then). > >>>> @@ -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? >> There are number of options that have influence on this formatting: >> AllowShortBlocksOnASingleLine [4] >> BreakBeforeTernaryOperators [5] >> AlignOperands [6] >> >> I was not able to tweak these options to have the previous form. > Right, sticking to the original form (with just the stray blanks zapped) > would of course be best. Yet again - the tool is doing more transformations > despite there not being any need. If, however, it does so, then one of my > expectations would be that the ? and : are properly indented: > > return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) > ? uart->fifo_size > : 0; This only differs from what the tool is doing by the fact it applies the following rule: *IndentWidth: 4*, e.g. it has indented your construct by 4 spaces, see [1]. Which, IMO, is acceptable change. > or > > return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) > ? uart->fifo_size : 0; > > or > > return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask > ? uart->fifo_size > : 0); > > (not going to list more variants which are all okay). > > In any event, a fundamental requirement of mine is that such a tool would > only apply adjustments when and where style is actively violated. I.e. in > the case here: > > return ((ns_read_reg(uart, UART_LSR) & > uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; > > That's not overly neat wrapping, but in line with our style. If the other > form was demanded going forward, I'd be curious how you'd verbally > describe the requirement in ./CODING_STYLE. I believe this can be stated around the fact that we need to indent, e.g. apply the same rule as for other constructs already in use > >>>> @@ -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.) >> No, gain from human point of view >> But there is a gain that it is now formatted automatically. > See above: I'd first like to see a written, textual description for all these > requirements. After all it needs to be possible for a human to write code > that the tool then wouldn't try to re-arrange. Which in turn requires that > the restrictions / constraints on the layout are spelled out. Agree, the existing coding style document will require some extension: at least clarifications and addition of the rules not described yet. > I'm not looking > forward to pass all my patches through such a tool. I can write style- > conforming code pretty well, with - of course - occasional oversights, Which the tool will allow not to have for less accurate developers > right > now. And that in multiple projects all with different styles. I expect to be > in the position to do so also going forward. This, imo, requires that there > be left some room for variations. Which in turn requires that the tool would > leave alone anything that is not in conflict with the written down or defacto > style. Not sure it is possible with any tool at all: it just makes the changes without distinguishing what can be skipped or not even if it does not violate the rules. It will always seek to improve or "improve" the code > >>>> @@ -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. >> This one will be impossible to implement with clang-format IMO. >> Because there are cases where you want this (like above) and you know why: >> the assignments look prettier here that way. But this doesn't mean >> that in some other places other assignments will look got for you if >> formatted the same way. >> The question here what is the metric (human perception?) in this case >> and how this perception can be be programmed into clang-format >> configuration? > Which imo is a pretty strong argument against using any auto-formatting. At > least up to the point where AI would end up being smart enough to mimic this > human perception. Well, you already have the answer: only human or AI? Obviously, this cannot be done with any of the tools, IMO > >>>> @@ -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()? >> This is controlled with AlignAfterOpenBracket [7] >> So this could be: >> *AlignAfterOpenBracket: Align* >> >> - 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); > As before indentation is bogus here, ... > >> *AlignAfterOpenBracket: |DontAlign* >> >> - 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); > ... and even more so here, ... > >> |*AlignAfterOpenBracket: |AlwaysBreak* >> >> - 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); > ... while this at least matches one of the forms we might presently use. > Yet again - there was nothing wrong with the original layout. So, we can stick with this option setting: AlignAfterOpenBracket: AlwaysBreak > >> |*AlignAfterOpenBracket: |BlockIndent* >> >> - 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 >> + ); > This clearly moves too far away from our present style. > >>>> @@ -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. >> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10] >> and is a binary option, which leaves no means to disable it as both true and >> false will re-format the code >> >> true:false: >> intf(){vs.intf(){ >> if(foo()){if(foo()){ >> label1:label1: >> bar();bar(); >> }} >> label2:label2: >> return1;return1; >> }} > If there was some indentation meant to be in that blob, it was all lost, > I'm afraid. Yes, sorry about that. The sample I was trying to put can be found at [2] > Still: The name of the control being IndentGotoLabels and it > being of boolean nature as you say, why would it do anything with an > already-indented label? Not really: unfortunately it is not an enum which would have "Leave" option both true and false values *will* apply formatting, but different > > Jan Thank you, Oleksandr [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentwidth [2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
On 19.02.2025 14:52, Oleksandr Andrushchenko wrote: > On 19.02.25 15:18, Jan Beulich wrote: >> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote: >>> On 17.02.25 12:20, Jan Beulich wrote: >>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>>> @@ -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? >>> There are number of options that have influence on this formatting: >>> AllowShortBlocksOnASingleLine [4] >>> BreakBeforeTernaryOperators [5] >>> AlignOperands [6] >>> >>> I was not able to tweak these options to have the previous form. >> Right, sticking to the original form (with just the stray blanks zapped) >> would of course be best. Yet again - the tool is doing more transformations >> despite there not being any need. If, however, it does so, then one of my >> expectations would be that the ? and : are properly indented: >> >> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) >> ? uart->fifo_size >> : 0; > This only differs from what the tool is doing by the fact it applies > the following rule: *IndentWidth: 4*, e.g. it has indented your construct > by 4 spaces, see [1]. Which, IMO, is acceptable change. I don't view this as acceptable. It falls in the same class then as ns_write_reg(uart, UART_FCR, UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14); that I also commented on in my initial reply. >> or >> >> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) >> ? uart->fifo_size : 0; >> >> or >> >> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask >> ? uart->fifo_size >> : 0); >> >> (not going to list more variants which are all okay). >> >> In any event, a fundamental requirement of mine is that such a tool would >> only apply adjustments when and where style is actively violated. I.e. in >> the case here: >> >> return ((ns_read_reg(uart, UART_LSR) & >> uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; >> >> That's not overly neat wrapping, but in line with our style. If the other >> form was demanded going forward, I'd be curious how you'd verbally >> describe the requirement in ./CODING_STYLE. > I believe this can be stated around the fact that we need to indent, > e.g. apply the same rule as for other constructs already in use Except here the tool didn't merely adjust indentation, but moved tokens between lines. >>>>> @@ -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.) >>> No, gain from human point of view >>> But there is a gain that it is now formatted automatically. >> See above: I'd first like to see a written, textual description for all these >> requirements. After all it needs to be possible for a human to write code >> that the tool then wouldn't try to re-arrange. Which in turn requires that >> the restrictions / constraints on the layout are spelled out. > Agree, the existing coding style document will require some extension: > at least clarifications and addition of the rules not described yet. >> I'm not looking >> forward to pass all my patches through such a tool. I can write style- >> conforming code pretty well, with - of course - occasional oversights, > Which the tool will allow not to have for less accurate developers I fear I don't understand this reply of yours. >> right >> now. And that in multiple projects all with different styles. I expect to be >> in the position to do so also going forward. This, imo, requires that there >> be left some room for variations. Which in turn requires that the tool would >> leave alone anything that is not in conflict with the written down or defacto >> style. > Not sure it is possible with any tool at all: it just makes the changes > without distinguishing what can be skipped or not even if it does not > violate the rules. It will always seek to improve or "improve" the > code Which by now I view as the core problem. >>>>> @@ -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. >>> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10] >>> and is a binary option, which leaves no means to disable it as both true and >>> false will re-format the code >>> >>> true:false: >>> intf(){vs.intf(){ >>> if(foo()){if(foo()){ >>> label1:label1: >>> bar();bar(); >>> }} >>> label2:label2: >>> return1;return1; >>> }} >> If there was some indentation meant to be in that blob, it was all lost, >> I'm afraid. > Yes, sorry about that. The sample I was trying to put can be found at [2] Funny, even with the setting "true" label2 there is unindented. We demand that all labels be indented, even when - contextually - at function scope. (Nor do we demand that labels be indented according to their - contextual - scope.) Jan
Hello, Jan! On 19.02.25 14:39, Oleksandr Andrushchenko wrote: > Hello, Jan! > > On 17.02.25 12:20, Jan Beulich wrote: >> 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. It seems this can actually have 3 forms under IndentPPDirectives control Please see [1]. I would go with BeforeHash personally [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives
On 19.02.2025 15:23, Oleksandr Andrushchenko wrote: > On 19.02.25 14:39, Oleksandr Andrushchenko wrote: >> On 17.02.25 12:20, Jan Beulich wrote: >>> 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. > It seems this can actually have 3 forms under IndentPPDirectives control > Please see [1]. I saw that. None of the three variants matches the original above, i.e. here we're again observing a change just for the sake of making a change. The style used is conforming, after all. Jan > I would go with BeforeHash personally > > [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives > >
On 19.02.25 16:05, Jan Beulich wrote: > On 19.02.2025 14:52, Oleksandr Andrushchenko wrote: >> On 19.02.25 15:18, Jan Beulich wrote: >>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote: >>>> On 17.02.25 12:20, Jan Beulich wrote: >>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>>>> @@ -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? >>>> There are number of options that have influence on this formatting: >>>> AllowShortBlocksOnASingleLine [4] >>>> BreakBeforeTernaryOperators [5] >>>> AlignOperands [6] >>>> >>>> I was not able to tweak these options to have the previous form. >>> Right, sticking to the original form (with just the stray blanks zapped) >>> would of course be best. Yet again - the tool is doing more transformations >>> despite there not being any need. If, however, it does so, then one of my >>> expectations would be that the ? and : are properly indented: >>> >>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) >>> ? uart->fifo_size >>> : 0; >> This only differs from what the tool is doing by the fact it applies >> the following rule: *IndentWidth: 4*, e.g. it has indented your construct >> by 4 spaces, see [1]. Which, IMO, is acceptable change. > I don't view this as acceptable. It falls in the same class then as > > ns_write_reg(uart, > UART_FCR, > UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | > UART_FCR_TRG14); > > that I also commented on in my initial reply. Ok, then how would you have it defined in the coding style as a rule? Such a diversity in defining indentation? So, will you have a dedicated rule for the ternary? > >>> or >>> >>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) >>> ? uart->fifo_size : 0; >>> >>> or >>> >>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask >>> ? uart->fifo_size >>> : 0); >>> >>> (not going to list more variants which are all okay). >>> >>> In any event, a fundamental requirement of mine is that such a tool would >>> only apply adjustments when and where style is actively violated. I.e. in >>> the case here: >>> >>> return ((ns_read_reg(uart, UART_LSR) & >>> uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0; >>> >>> That's not overly neat wrapping, but in line with our style. If the other >>> form was demanded going forward, I'd be curious how you'd verbally >>> describe the requirement in ./CODING_STYLE. >> I believe this can be stated around the fact that we need to indent, >> e.g. apply the same rule as for other constructs already in use > Except here the tool didn't merely adjust indentation, but moved tokens > between lines. Again, if it moves, but doesn't break the style - then it is going to happen only once while applying big-scary-patch. > >>>>>> @@ -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.) >>>> No, gain from human point of view >>>> But there is a gain that it is now formatted automatically. >>> See above: I'd first like to see a written, textual description for all these >>> requirements. After all it needs to be possible for a human to write code >>> that the tool then wouldn't try to re-arrange. Which in turn requires that >>> the restrictions / constraints on the layout are spelled out. >> Agree, the existing coding style document will require some extension: >> at least clarifications and addition of the rules not described yet. >>> I'm not looking >>> forward to pass all my patches through such a tool. I can write style- >>> conforming code pretty well, with - of course - occasional oversights, >> Which the tool will allow not to have for less accurate developers > I fear I don't understand this reply of yours. I mean that you can write such a well formatted code without any tool. But there are others who can't. Then the tool will help others to avoid code style violations. > >>> right >>> now. And that in multiple projects all with different styles. I expect to be >>> in the position to do so also going forward. This, imo, requires that there >>> be left some room for variations. Which in turn requires that the tool would >>> leave alone anything that is not in conflict with the written down or defacto >>> style. >> Not sure it is possible with any tool at all: it just makes the changes >> without distinguishing what can be skipped or not even if it does not >> violate the rules. It will always seek to improve or "improve" the >> code > Which by now I view as the core problem. It depends. If this is the change made once, then I don't see it as a lifetime problem. > >>>>>> @@ -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. >>>> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10] >>>> and is a binary option, which leaves no means to disable it as both true and >>>> false will re-format the code >>>> >>>> true:false: >>>> intf(){vs.intf(){ >>>> if(foo()){if(foo()){ >>>> label1:label1: >>>> bar();bar(); >>>> }} >>>> label2:label2: >>>> return1;return1; >>>> }} >>> If there was some indentation meant to be in that blob, it was all lost, >>> I'm afraid. >> Yes, sorry about that. The sample I was trying to put can be found at [2] > Funny, even with the setting "true" label2 there is unindented. We demand that > all labels be indented, even when - contextually - at function scope. (Nor do > we demand that labels be indented according to their - contextual - scope.) It seems that other projects (coding styles) see the alignment at function scope differently. From Xen. If it was not implemented before in clang-format could mean that either no other project use such an exception or there are not so many projects use clang-format and didn't obviously face such an issue > > Jan Thank you, Oleksandr
On 19.02.2025 16:40, Oleksandr Andrushchenko wrote: > On 19.02.25 16:05, Jan Beulich wrote: >> On 19.02.2025 14:52, Oleksandr Andrushchenko wrote: >>> On 19.02.25 15:18, Jan Beulich wrote: >>>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote: >>>>> On 17.02.25 12:20, Jan Beulich wrote: >>>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>>>>> @@ -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? >>>>> There are number of options that have influence on this formatting: >>>>> AllowShortBlocksOnASingleLine [4] >>>>> BreakBeforeTernaryOperators [5] >>>>> AlignOperands [6] >>>>> >>>>> I was not able to tweak these options to have the previous form. >>>> Right, sticking to the original form (with just the stray blanks zapped) >>>> would of course be best. Yet again - the tool is doing more transformations >>>> despite there not being any need. If, however, it does so, then one of my >>>> expectations would be that the ? and : are properly indented: >>>> >>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask) >>>> ? uart->fifo_size >>>> : 0; >>> This only differs from what the tool is doing by the fact it applies >>> the following rule: *IndentWidth: 4*, e.g. it has indented your construct >>> by 4 spaces, see [1]. Which, IMO, is acceptable change. >> I don't view this as acceptable. It falls in the same class then as >> >> ns_write_reg(uart, >> UART_FCR, >> UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | >> UART_FCR_TRG14); >> >> that I also commented on in my initial reply. > Ok, then how would you have it defined in the coding style as a rule? > Such a diversity in defining indentation? So, will you have a dedicated > rule for the ternary? Well, this feels like you're returning a request I made your way, elsewhere. Our present, unwritten rule for wrapping lines is to match the earlier line's indentation (or the start of the expression), plus accounting for any pending open parentheses, braces, or brackets. Hence why some consider this form ns_write_reg(uart, UART_FCR, (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14)); preferable, as some tools (iirc e.g. Andrew indicated his editor does) then are capable of inferring the intended indentation from the pending open parentheses. >>>> That's not overly neat wrapping, but in line with our style. If the other >>>> form was demanded going forward, I'd be curious how you'd verbally >>>> describe the requirement in ./CODING_STYLE. >>> I believe this can be stated around the fact that we need to indent, >>> e.g. apply the same rule as for other constructs already in use >> Except here the tool didn't merely adjust indentation, but moved tokens >> between lines. > Again, if it moves, but doesn't break the style - then it is going to happen > only once while applying big-scary-patch. As to that patch: To some degree I actually like the idea of following Linux in generally not allowing style-only patches. >>>>>>> @@ -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.) >>>>> No, gain from human point of view >>>>> But there is a gain that it is now formatted automatically. >>>> See above: I'd first like to see a written, textual description for all these >>>> requirements. After all it needs to be possible for a human to write code >>>> that the tool then wouldn't try to re-arrange. Which in turn requires that >>>> the restrictions / constraints on the layout are spelled out. >>> Agree, the existing coding style document will require some extension: >>> at least clarifications and addition of the rules not described yet. >>>> I'm not looking >>>> forward to pass all my patches through such a tool. I can write style- >>>> conforming code pretty well, with - of course - occasional oversights, >>> Which the tool will allow not to have for less accurate developers >> I fear I don't understand this reply of yours. > I mean that you can write such a well formatted code without any tool. > But there are others who can't. Then the tool will help others to avoid > code style violations. And it'll screw me up (and possibly others too). Jan
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index eaeb0e09d01e..0f605c98b036 100644 --- 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 #include <xen/console.h> @@ -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 */ int reg_width; /* Size of access to use, the registers * themselves are still bytes */ - char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */ + char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */ /* UART with IRQ line: interrupt-driven I/O. */ struct irqaction irqaction; u8 lsr_mask; @@ -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 */ unsigned int pb_bdf[3]; /* pci bridge BDF */ unsigned int ps_bdf[3]; /* pci serial port BDF */ u32 bar; @@ -80,6 +80,7 @@ static struct ns16550 { struct ns16550_config { u16 vendor_id; u16 dev_id; + enum { param_default, /* Must not be referenced by any table entry. */ param_trumanage, @@ -227,7 +228,7 @@ static void cf_check __ns16550_poll(const struct cpu_user_regs *regs) serial_rx_interrupt(port); } - if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask ) + if ( (ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask ) serial_tx_interrupt(port); out: @@ -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; } static void cf_check ns16550_putc(struct serial_port *port, char c) @@ -263,7 +265,7 @@ static int cf_check ns16550_getc(struct serial_port *port, char *pc) struct ns16550 *uart = port->uart; if ( ns16550_ioport_invalid(uart) || - !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) ) + !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) ) return 0; *pc = ns_read_reg(uart, UART_RBR); @@ -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; } @@ -285,26 +288,26 @@ static void pci_serial_early_init(struct ns16550 *uart) return; if ( uart->pb_bdf_enable ) - pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1], - uart->pb_bdf[2]), - PCI_IO_BASE, - (uart->io_base & 0xF000) | - ((uart->io_base & 0xF000) >> 8)); - - pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - PCI_BASE_ADDRESS_0, - uart->io_base | PCI_BASE_ADDRESS_SPACE_IO); - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - PCI_COMMAND, PCI_COMMAND_IO); + pci_conf_write16( + PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1], uart->pb_bdf[2]), + PCI_IO_BASE, + (uart->io_base & 0xF000) | ((uart->io_base & 0xF000) >> 8)); + + pci_conf_write32( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_BASE_ADDRESS_0, + uart->io_base | PCI_BASE_ADDRESS_SPACE_IO); + pci_conf_write16( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_COMMAND, + PCI_COMMAND_IO); #endif } static void ns16550_setup_preirq(struct ns16550 *uart) { unsigned char lcr; - unsigned int divisor; + unsigned int divisor; uart->intr_works = 0; @@ -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; if ( divisor ) uart->baud = uart->clock_hz / (divisor << 4); else - printk(XENLOG_ERR - "Automatic baud rate determination was requested," - " but a baud rate was not set up\n"); + printk( + XENLOG_ERR + "Automatic baud rate determination was requested," " but a baud rate was not set up\n"); } ns_write_reg(uart, UART_LCR, lcr); @@ -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); } static void __init cf_check ns16550_init_preirq(struct serial_port *port) @@ -399,7 +404,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart) { /* Master interrupt enable; also keep DTR/RTS asserted. */ ns_write_reg(uart, - UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS); + UART_MCR, + UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS); /* Enable receive interrupts. */ ns_write_reg(uart, UART_IER, UART_IER_ERDAI); @@ -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); #ifdef NS16550_PCI if ( uart->bar || uart->ps_bdf_enable ) { if ( uart->param && uart->param->mmio && - rangeset_add_range(mmio_ro_ranges, PFN_DOWN(uart->io_base), + rangeset_add_range(mmio_ro_ranges, + PFN_DOWN(uart->io_base), PFN_UP(uart->io_base + uart->io_size) - 1) ) - printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n"); + printk( + XENLOG_INFO + "Error while adding MMIO range of device to mmio_ro_ranges\n"); - if ( pci_ro_device(0, uart->ps_bdf[0], + if ( pci_ro_device(0, + uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) ) - printk(XENLOG_INFO "Could not mark config space of %02x:%02x.%u read-only.\n", - uart->ps_bdf[0], uart->ps_bdf[1], + printk(XENLOG_INFO + "Could not mark config space of %02x:%02x.%u read-only.\n", + uart->ps_bdf[0], + uart->ps_bdf[1], uart->ps_bdf[2]); if ( uart->msi ) { - struct msi_info msi = { - .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - .irq = uart->irq, - .entry_nr = 1 - }; + struct msi_info msi = { .sbdf = PCI_SBDF(0, + uart->ps_bdf[0], + uart->ps_bdf[1], + uart->ps_bdf[2]), + .irq = uart->irq, + .entry_nr = 1 }; rc = uart->irq; @@ -489,7 +501,10 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) if ( rc ) printk(XENLOG_WARNING "MSI setup failed (%d) for %02x:%02x.%o\n", - rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]); + rc, + uart->ps_bdf[0], + uart->ps_bdf[1], + uart->ps_bdf[2]); } } #endif @@ -497,8 +512,8 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) if ( uart->irq > 0 ) { uart->irqaction.handler = ns16550_interrupt; - uart->irqaction.name = "ns16550"; - uart->irqaction.dev_id = port; + uart->irqaction.name = "ns16550"; + uart->irqaction.dev_id = port; if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 ) printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq); } @@ -514,8 +529,9 @@ static void cf_check ns16550_suspend(struct serial_port *port) #ifdef NS16550_PCI if ( uart->bar ) - uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), PCI_COMMAND); + uart->cr = pci_conf_read16( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_COMMAND); #endif } @@ -526,19 +542,22 @@ static void _ns16550_resume(struct serial_port *port) if ( uart->bar ) { - pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar); + pci_conf_write32( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_BASE_ADDRESS_0 + uart->bar_idx * 4, + uart->bar); /* If 64 bit BAR, write higher 32 bits to BAR+4 */ if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) - pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64); - - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], - uart->ps_bdf[2]), - PCI_COMMAND, uart->cr); + pci_conf_write32( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_BASE_ADDRESS_0 + (uart->bar_idx + 1) * 4, + uart->bar64); + + pci_conf_write16( + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), + PCI_COMMAND, + uart->cr); } #endif @@ -547,6 +566,7 @@ static void _ns16550_resume(struct serial_port *port) } static int delayed_resume_tries; + static void cf_check ns16550_delayed_resume(void *data) { struct serial_port *port = data; @@ -634,32 +654,32 @@ static const struct vuart_info *ns16550_vuart_info(struct serial_port *port) #endif static struct uart_driver __read_mostly ns16550_driver = { - .init_preirq = ns16550_init_preirq, - .init_irq = ns16550_init_irq, + .init_preirq = ns16550_init_preirq, + .init_irq = ns16550_init_irq, .init_postirq = ns16550_init_postirq, - .endboot = ns16550_endboot, - .suspend = ns16550_suspend, - .resume = ns16550_resume, - .tx_ready = ns16550_tx_ready, - .putc = ns16550_putc, - .getc = ns16550_getc, - .irq = ns16550_irq, - .start_tx = ns16550_start_tx, - .stop_tx = ns16550_stop_tx, + .endboot = ns16550_endboot, + .suspend = ns16550_suspend, + .resume = ns16550_resume, + .tx_ready = ns16550_tx_ready, + .putc = ns16550_putc, + .getc = ns16550_getc, + .irq = ns16550_irq, + .start_tx = ns16550_start_tx, + .stop_tx = ns16550_stop_tx, #ifdef CONFIG_ARM - .vuart_info = ns16550_vuart_info, + .vuart_info = ns16550_vuart_info, #endif }; static void ns16550_init_common(struct ns16550 *uart) { - uart->clock_hz = UART_CLOCK_HZ; + uart->clock_hz = UART_CLOCK_HZ; /* Default is no transmit FIFO. */ uart->fifo_size = 1; /* Default lsr_mask = UART_LSR_THRE */ - uart->lsr_mask = UART_LSR_THRE; + uart->lsr_mask = UART_LSR_THRE; } #ifdef CONFIG_X86 @@ -670,13 +690,13 @@ static int __init parse_parity_char(int c) { case 'n': return UART_PARITY_NONE; - case 'o': + case 'o': return UART_PARITY_ODD; - case 'e': + case 'e': return UART_PARITY_EVEN; - case 'm': + case 'm': return UART_PARITY_MARK; - case 's': + case 's': return UART_PARITY_SPACE; } return 0; @@ -711,7 +731,7 @@ static int __init check_existence(struct ns16550 *uart) * 16C754B) allow only to modify them if an EFR bit is set. */ scratch2 = ns_read_reg(uart, UART_IER) & 0x0f; - ns_write_reg(uart,UART_IER, 0x0F); + ns_write_reg(uart, UART_IER, 0x0F); scratch3 = ns_read_reg(uart, UART_IER) & 0x0f; ns_write_reg(uart, UART_IER, scratch); if ( (scratch2 != 0) || (scratch3 != 0x0F) ) @@ -849,336 +869,293 @@ static const struct ns16550_config_param __initconst uart_param[] = { }, }; -static const struct ns16550_config __initconst uart_config[] = -{ +static const struct ns16550_config __initconst uart_config[] = { /* Broadcom TruManage device */ { - .vendor_id = PCI_VENDOR_ID_BROADCOM, - .dev_id = 0x160a, - .param = param_trumanage, - }, + .vendor_id = PCI_VENDOR_ID_BROADCOM, + .dev_id = 0x160a, + .param = param_trumanage, + }, /* OXPCIe952 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc11b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc11b, + .param = param_oxford, + }, /* OXPCIe952 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc11f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc11f, + .param = param_oxford, + }, /* OXPCIe952 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc138, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc138, + .param = param_oxford, + }, /* OXPCIe952 2 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc158, - .param = param_oxford_2port, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc158, + .param = param_oxford_2port, + }, /* OXPCIe952 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc13d, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc13d, + .param = param_oxford, + }, /* OXPCIe952 2 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc15d, - .param = param_oxford_2port, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc15d, + .param = param_oxford_2port, + }, /* OXPCIe952 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc40b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc40b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc40f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc40f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc41b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc41b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc41f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc41f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc42b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc42b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc42f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc42f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc43b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc43b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc43f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc43f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc44b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc44b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc44f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc44f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc45b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc45b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc45f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc45f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc46b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc46b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc46f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc46f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc47b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc47b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc47f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc47f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc48b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc48b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc48f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc48f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc49b, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc49b, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc49f, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc49f, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4ab, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4ab, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4af, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4af, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4bb, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4bb, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4bf, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4bf, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4cb, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4cb, + .param = param_oxford, + }, /* OXPCIe200 1 Native UART */ { - .vendor_id = PCI_VENDOR_ID_OXSEMI, - .dev_id = 0xc4cf, - .param = param_oxford, - }, + .vendor_id = PCI_VENDOR_ID_OXSEMI, + .dev_id = 0xc4cf, + .param = param_oxford, + }, /* Pericom PI7C9X7951 Uno UART */ - { - .vendor_id = PCI_VENDOR_ID_PERICOM, - .dev_id = 0x7951, - .param = param_pericom_1port - }, + { .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7951, + .param = param_pericom_1port }, /* Pericom PI7C9X7952 Duo UART */ - { - .vendor_id = PCI_VENDOR_ID_PERICOM, - .dev_id = 0x7952, - .param = param_pericom_2port - }, + { .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7952, + .param = param_pericom_2port }, /* Pericom PI7C9X7954 Quad UART */ - { - .vendor_id = PCI_VENDOR_ID_PERICOM, - .dev_id = 0x7954, - .param = param_pericom_4port - }, + { .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7954, + .param = param_pericom_4port }, /* Pericom PI7C9X7958 Octal UART */ - { - .vendor_id = PCI_VENDOR_ID_PERICOM, - .dev_id = 0x7958, - .param = param_pericom_8port - }, + { .vendor_id = PCI_VENDOR_ID_PERICOM, + .dev_id = 0x7958, + .param = param_pericom_8port }, /* Exar Corp. XR17V352 Dual PCIe UART */ - { - .vendor_id = PCI_VENDOR_ID_EXAR, - .dev_id = 0x0352, - .param = param_exar_xr17v352 - }, + { .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0352, + .param = param_exar_xr17v352 }, /* Exar Corp. XR17V354 Quad PCIe UART */ - { - .vendor_id = PCI_VENDOR_ID_EXAR, - .dev_id = 0x0354, - .param = param_exar_xr17v354 - }, + { .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0354, + .param = param_exar_xr17v354 }, /* Exar Corp. XR17V358 Octal PCIe UART */ - { - .vendor_id = PCI_VENDOR_ID_EXAR, - .dev_id = 0x0358, - .param = param_exar_xr17v358 - }, + { .vendor_id = PCI_VENDOR_ID_EXAR, + .dev_id = 0x0358, + .param = param_exar_xr17v358 }, /* Intel Corp. TGL-LP LPSS PCI UART #0 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0xa0a8, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0xa0a8, + .param = param_intel_lpss }, /* Intel Corp. TGL-LP LPSS PCI UART #1 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0xa0a9, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0xa0a9, + .param = param_intel_lpss }, /* Intel Corp. TGL-LP LPSS PCI UART #2 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0xa0c7, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0xa0c7, + .param = param_intel_lpss }, /* Intel Corp. TGL-H LPSS PCI UART #0 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x43a8, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x43a8, + .param = param_intel_lpss }, /* Intel Corp. TGL-H LPSS PCI UART #1 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x43a9, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x43a9, + .param = param_intel_lpss }, /* Intel Corp. TGL-H LPSS PCI UART #2 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x43a7, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x43a7, + .param = param_intel_lpss }, /* Intel Corp. ADL-P LPSS PCI UART #0 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x51a8, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x51a8, + .param = param_intel_lpss }, /* Intel Corp. ADL-P LPSS PCI UART #1 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x51a9, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x51a9, + .param = param_intel_lpss }, /* Intel Corp. ADL-P LPSS PCI UART #2 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x51c7, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x51c7, + .param = param_intel_lpss }, /* Intel Corp. ADL-P LPSS PCI UART #3 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x51da, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x51da, + .param = param_intel_lpss }, /* Intel Corp. ADL-S LPSS PCI UART #0 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x7aa8, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x7aa8, + .param = param_intel_lpss }, /* Intel Corp. ADL-S LPSS PCI UART #1 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x7aa9, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x7aa9, + .param = param_intel_lpss }, /* Intel Corp. ADL-S LPSS PCI UART #2 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x7afe, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x7afe, + .param = param_intel_lpss }, /* Intel Corp. ADL-S LPSS PCI UART #3 */ - { - .vendor_id = PCI_VENDOR_ID_INTEL, - .dev_id = 0x7adc, - .param = param_intel_lpss - }, + { .vendor_id = PCI_VENDOR_ID_INTEL, + .dev_id = 0x7adc, + .param = param_intel_lpss }, }; -static int __init -pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx) +static int __init pci_uart_config(struct ns16550 *uart, bool skip_amt, + unsigned int idx) { u64 orig_base = uart->io_base; unsigned int b, d, f, nextf, i; @@ -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; switch ( pci_conf_read16(PCI_SBDF(0, b, d, f), PCI_CLASS_DEVICE) ) @@ -1250,24 +1229,30 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx) if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) { pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); + PCI_BASE_ADDRESS_0 + bar_idx * 4, + ~0u); len = pci_conf_read32(PCI_SBDF(0, b, d, f), PCI_BASE_ADDRESS_0 + bar_idx * 4); pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + bar_idx*4, bar); + PCI_BASE_ADDRESS_0 + bar_idx * 4, + bar); /* Handle 64 bit BAR if found */ if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 ) { bar_64 = pci_conf_read32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4); + PCI_BASE_ADDRESS_0 + + (bar_idx + 1) * 4); pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u); + PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4, + ~0u); len_64 = pci_conf_read32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4); + PCI_BASE_ADDRESS_0 + + (bar_idx + 1) * 4); pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64); - size = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK; + PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4, + bar_64); + size = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK; size &= ((u64)len_64 << 32) | len; } else @@ -1280,11 +1265,13 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx) else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) ) { pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u); + PCI_BASE_ADDRESS_0 + bar_idx * 4, + ~0u); len = pci_conf_read32(PCI_SBDF(0, b, d, f), PCI_BASE_ADDRESS_0); pci_conf_write32(PCI_SBDF(0, b, d, f), - PCI_BASE_ADDRESS_0 + bar_idx*4, bar); + PCI_BASE_ADDRESS_0 + bar_idx * 4, + bar); size = len & PCI_BASE_ADDRESS_IO_MASK; uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO; @@ -1301,8 +1288,8 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx) * 8 bytes times (1 << reg_shift). */ if ( size < param->first_offset + - port_idx * param->uart_offset + - (8 << param->reg_shift) ) + port_idx * param->uart_offset + + (8 << param->reg_shift) ) continue; uart->param = param; @@ -1323,12 +1310,12 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx) uart->bar_idx = bar_idx; uart->bar = bar; uart->bar64 = bar_64; - uart->io_size = max(8U << param->reg_shift, - param->uart_offset); + uart->io_size = max(8U << param->reg_shift, param->uart_offset); uart->irq = pci_conf_read8(PCI_SBDF(0, b, d, f), - PCI_INTERRUPT_PIN) ? - pci_conf_read8(PCI_SBDF(0, b, d, f), - PCI_INTERRUPT_LINE) : 0; + PCI_INTERRUPT_PIN) + ? pci_conf_read8(PCI_SBDF(0, b, d, f), + PCI_INTERRUPT_LINE) + : 0; #ifdef CONFIG_X86 /* @@ -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 }; @@ -1476,7 +1463,6 @@ static enum __init serial_param_type get_token(char *token, char **value) return false; \ } while ( 0 ) - static bool __init parse_positional(struct ns16550 *uart, char **str) { int baud; @@ -1533,7 +1519,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) #ifdef CONFIG_HAS_PCI if ( strncmp(conf, "pci", 3) == 0 ) { - if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) ) + if ( pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com) ) return true; conf += 3; } @@ -1567,8 +1553,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) #ifdef CONFIG_HAS_PCI if ( *conf == ',' && *++conf != ',' ) { - conf = parse_pci(conf, NULL, &uart->ps_bdf[0], - &uart->ps_bdf[1], &uart->ps_bdf[2]); + conf = parse_pci(conf, + NULL, + &uart->ps_bdf[0], + &uart->ps_bdf[1], + &uart->ps_bdf[2]); if ( !conf ) PARSE_ERR_RET("Bad port PCI coordinates"); uart->ps_bdf_enable = true; @@ -1576,8 +1565,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) if ( *conf == ',' && *++conf != ',' ) { - if ( !parse_pci(conf, NULL, &uart->pb_bdf[0], - &uart->pb_bdf[1], &uart->pb_bdf[2]) ) + if ( !parse_pci(conf, + NULL, + &uart->pb_bdf[0], + &uart->pb_bdf[1], + &uart->pb_bdf[2]) ) PARSE_ERR_RET("Bad bridge PCI coordinates"); uart->pb_bdf_enable = true; } @@ -1648,7 +1640,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) case device: if ( strncmp(param_value, "pci", 3) == 0 ) { - pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com); + pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com); dev_set = true; } else if ( strncmp(param_value, "amt", 3) == 0 ) @@ -1659,15 +1651,21 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) break; case port_bdf: - if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0], - &uart->ps_bdf[1], &uart->ps_bdf[2]) ) + if ( !parse_pci(param_value, + NULL, + &uart->ps_bdf[0], + &uart->ps_bdf[1], + &uart->ps_bdf[2]) ) PARSE_ERR_RET("Bad port PCI coordinates\n"); uart->ps_bdf_enable = true; break; case bridge_bdf: - if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0], - &uart->pb_bdf[1], &uart->pb_bdf[2]) ) + if ( !parse_pci(param_value, + NULL, + &uart->pb_bdf[0], + &uart->pb_bdf[1], + &uart->pb_bdf[2]) ) PARSE_ERR_RET("Bad bridge PCI coordinates\n"); uart->pb_bdf_enable = true; break; @@ -1681,8 +1679,8 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) return true; } -static void __init ns16550_parse_port_config( - struct ns16550 *uart, const char *conf) +static void __init ns16550_parse_port_config(struct ns16550 *uart, + const char *conf) { char com_console_options[128]; char *str; @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config( if ( !parse_namevalue_pairs(str, uart) ) return; - config_parsed: +config_parsed: /* Sanity checks. */ if ( (uart->baud != BAUD_AUTO) && ((uart->baud < 1200) || (uart->baud > 115200)) ) @@ -1737,15 +1735,15 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults) ns16550_init_common(uart); - uart->baud = (defaults->baud ? : - console_has((index == 0) ? "com1" : "com2") - ? BAUD_AUTO : 0); + uart->baud = (defaults->baud + ?: console_has((index == 0) ? "com1" : "com2") ? BAUD_AUTO + : 0); uart->data_bits = defaults->data_bits; - uart->parity = parse_parity_char(defaults->parity); + uart->parity = parse_parity_char(defaults->parity); uart->stop_bits = defaults->stop_bits; - uart->irq = defaults->irq; - uart->io_base = defaults->io_base; - uart->io_size = 8; + uart->irq = defaults->irq; + uart->io_base = defaults->io_base; + uart->io_size = 8; uart->reg_width = 1; uart->reg_shift = 0; @@ -1766,9 +1764,9 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, ns16550_init_common(uart); - uart->baud = BAUD_AUTO; + uart->baud = BAUD_AUTO; uart->data_bits = 8; - uart->parity = UART_PARITY_NONE; + uart->parity = UART_PARITY_NONE; uart->stop_bits = 1; res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size); @@ -1797,7 +1795,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, } res = platform_get_irq(dev, 0); - if ( ! res ) + if ( !res ) return -EINVAL; uart->irq = res; @@ -1806,9 +1804,9 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, #ifdef CONFIG_ARM uart->vuart.base_addr = uart->io_base; uart->vuart.size = uart->io_size; - uart->vuart.data_off = UART_THR <<uart->reg_shift; - uart->vuart.status_off = UART_LSR<<uart->reg_shift; - uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT; + uart->vuart.data_off = UART_THR << uart->reg_shift; + uart->vuart.status_off = UART_LSR << uart->reg_shift; + uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT; #endif /* Register with generic serial driver. */ @@ -1819,8 +1817,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev, return 0; } -static const struct dt_device_match ns16550_dt_match[] __initconst = -{ +static const struct dt_device_match ns16550_dt_match[] __initconst = { DT_MATCH_COMPATIBLE("ns16550"), DT_MATCH_COMPATIBLE("ns16550a"), DT_MATCH_COMPATIBLE("snps,dw-apb-uart"), @@ -1829,8 +1826,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst = }; DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL) - .dt_match = ns16550_dt_match, - .init = ns16550_uart_dt_init, + .dt_match = ns16550_dt_match, .init = ns16550_uart_dt_init, DT_DEVICE_END #endif /* HAS_DEVICE_TREE */ @@ -1907,8 +1903,7 @@ static int __init ns16550_acpi_uart_init(const void *data) } ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL) - .class_type = ACPI_DBG2_16550_COMPATIBLE, - .init = ns16550_acpi_uart_init, + .class_type = ACPI_DBG2_16550_COMPATIBLE, .init = ns16550_acpi_uart_init, ACPI_DEVICE_END #endif /* CONFIG_ACPI && CONFIG_ARM */
Use .clang-format to format ns16550 driver. Signed-off-by: Oleksandr Andrushchenko <andr2000@gmail.com> --- xen/drivers/char/ns16550.c | 761 ++++++++++++++++++------------------- 1 file changed, 378 insertions(+), 383 deletions(-)