Re: [Xen-devel] [PATCH v8] ns16550: Add support for UART parameters to be specifed with name-value pairs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.17 at 22:07, <swapnil.paratey@xxxxxxx> 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.paratey@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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, &param_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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel