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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.17 at 18:07, <swapnil.paratey@xxxxxxx> wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -38,11 +38,28 @@
>   * can be specified in place of a numeric baud rate. Polled mode is specified
>   * by requesting irq 0.
>   */
> -static char __initdata opt_com1[30] = "";
> -static char __initdata opt_com2[30] = "";
> +static char __initdata opt_com1[128] = "";
> +static char __initdata opt_com2[128] = "";
>  string_param("com1", opt_com1);
>  string_param("com2", opt_com2);
>  
> +typedef enum e_serial_param_type {
> +    baud,
> +    bridge_bdf,
> +    clock_hz,
> +    data_bits,
> +    device,
> +    io_base,
> +    irq,
> +    parity,
> +    port_bdf,
> +    reg_shift,
> +    reg_width,
> +    stop_bits,
> +    /* list all parameters before this line */
> +    max_serial_params

Bad name: If "max", it should be "max_serial_param" and equal the
highest valid one. Otherwise you mean "num_serial_params". Also
no need for a typedef here.

> @@ -77,6 +94,31 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +struct serial_param_var
> +{
> +    const char *sp_name;

I think you would save storage (and even more so runtime one) if
you used name[12] here.

> +    serial_param_type sp_type;

For both of them, not need for a K&R (or even older) style "sp_"
prefix.

> +static bool __init parse_positional(struct ns16550 *uart, char **str)
>  {
>      int baud;
> +    const char *conf;
> +    char *name_val_pos;
>  
> -    /* No user-specified configuration? */
> -    if ( (conf == NULL) || (*conf == '\0') )
> +    conf = *str;
> +    name_val_pos = strchr(conf, '=');
> +
> +    /* finding the end of the positional parameters */
> +    if ( name_val_pos )

Is this really needed? The while() below looks to be sufficient.

>      {
> -        /* Some platforms may automatically probe the UART configuartion. */
> -        if ( uart->baud != 0 )
> -            goto config_parsed;
> -        return;
> +        while (name_val_pos > *str)

Coding style (more elsewhere - please take the time and look over
your patch yourself to eliminate all such issues).

> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +    char *token, *start = str;
> +    char param_value[16];
> +    bool dev_set = false;
> +
> +    if ( (str == NULL) || (*str == '\0') )
> +        return true;
> +
> +    /* strsep gives NULL when there are no tokens found */
> +    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;
> +        case bridge_bdf:

Please have blank lines between non-fall-through case blocks.

> +        if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],

Indentation.

> +                        &uart->ps_bdf[1], &uart->ps_bdf[2]))
> +                PARSE_ERR_RET("Bad port PCI coordinates\n");
> +            uart->ps_bdf_enable = 1;
> +            break;
> +        case clock_hz:
> +            uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);
> +            break;
> +        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;
> +        case io_base:
> +            if ( dev_set == true )

Pointless comparison of a bool value with "true".

> +static void __init ns16550_parse_port_config(
> +    struct ns16550 *uart, const char *conf)
> +{
> +    char cmdline[128];

Please don't use misleading variable names - this isn't intended to hold
an entire command line.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel