>>> On 02.06.17 at 22:07, <swapnil.para...@amd.com> wrote: > Add name=value parsing options for com1 and com2 to add flexibility > in setting register values for MMIO UART devices. > > Maintain backward compatibility with previous positional parameter > specfications. > > eg. com1=115200,8n1,0x3f8,4 > eg. com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2 > eg. com1=baud=115200,parity=n,reg_width=4,reg_shift=2,irq=4 > > Signed-off-by: Swapnil Paratey <swapnil.para...@amd.com> > Reviewed-by: Jan Beulich <jbeul...@suse.com>
For the future, please drop prior tags when fixing bugs or doing any other not purely mechanical changes. > +enum serial_param_type { > + baud, > + clock_hz, > + data_bits, > + io_base, > + irq, > + parity, > + reg_shift, > + reg_width, > + stop_bits, > +#ifdef CONFIG_HAS_PCI > + bridge_bdf, > + device, > + port_bdf, > +#endif > + /* List all parameters before this line. */ > + num_serial_params > +}; Just like you've done here and ... > @@ -77,6 +96,32 @@ static struct ns16550 { > #endif > } ns16550_com[2] = { { 0 } }; > > +struct serial_param_var { > + char name[12]; > + enum serial_param_type type; > +}; > + > +/* > + * Enum struct keeping a table of all accepted parameter names for parsing > + * com_console_options for serial port com1 and com2. > + */ > +static const struct serial_param_var __initconst sp_vars[] = { > + {"baud", baud}, > + {"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}, > +#endif > +}; ... here I would also have expected you to group PCI things ... > +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) > +{ > + char *token, *start = str; > + char *param_value = NULL; > + bool dev_set = false; > + > + if ( (str == NULL) || (*str == '\0') ) > + return true; > + > + do > + { > + /* When no tokens are found, start will be NULL */ > + token = strsep(&start, ","); > + > + switch ( get_token(token, ¶m_value) ) > + { > + case baud: > + uart->baud = simple_strtoul(param_value, NULL, 0); > + break; > +#ifdef CONFIG_HAS_PCI > + case bridge_bdf: > + 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; > +#endif > + case clock_hz: > + uart->clock_hz = simple_strtoul(param_value, NULL, 0) << 4; > + break; > +#ifdef CONFIG_HAS_PCI > + case device: > + if ( strncmp(param_value, "pci", 3) == 0 ) > + { > + pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com); > + dev_set = true; > + } > + else if ( strncmp(param_value, "amt", 3) == 0 ) > + { > + pci_uart_config(uart, 0, uart - ns16550_com); > + dev_set = true; > + } > + break; > +#endif > + case io_base: > + if ( dev_set ) > + { > + printk(XENLOG_WARNING > + "Can't use io_base with dev=pci or dev=amt > options\n"); > + break; > + } > + uart->io_base = simple_strtoul(param_value, NULL, 0); > + break; > + > + case irq: > + uart->irq = simple_strtoul(param_value, NULL, 0); > + break; > + > + case data_bits: > + uart->data_bits = simple_strtoul(param_value, NULL, 0); > + break; > + > + case parity: > + uart->parity = parse_parity_char(*param_value); > + break; > +#ifdef CONFIG_HAS_PCI > + case port_bdf: > + if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0], > + &uart->pb_bdf[1], &uart->pb_bdf[2]) ) > + PARSE_ERR_RET("Bad port PCI coordinates\n"); > + uart->pb_bdf_enable = true; > + break; > +#endif > + case stop_bits: > + uart->stop_bits = simple_strtoul(param_value, NULL, 0); > + break; > + > + case reg_shift: > + uart->reg_shift = simple_strtoul(param_value, NULL, 0); > + break; > + > + case reg_width: > + uart->reg_width = simple_strtoul(param_value, NULL, 0); > + break; > + > + default: > + PARSE_ERR_RET("Invalid parameter: %s\n", token); > + > + } > + } while ( start != NULL ); > + > + return true; > +} ... here. Furthermore please retain the blank lines you had between individual case blocks (instead of replacing them by #if-s/#else-s). And please get rid of the stray blank line at the bottom of above switch(). With all of this you may then retain my R-b. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel