Re: [Xen-devel] [PATCH v2] ns16550: Add command line parsing adjustments

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.17 at 17:24, <swapnil.paratey@xxxxxxx> wrote:
> On 01/06/2017 08:58 AM, Jan Beulich wrote:
>>>>> On 05.01.17 at 23:39, <swapnil.paratey@xxxxxxx> wrote:
>>> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>>>           uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>>>       }
>>>   
>>> +    if ( *conf == '/' )
>>> +    {
>>> +        conf++;
>>> +        if ( *conf != '/' && *conf != ',' )
>>> +            uart->reg_width = simple_strtol(conf, &conf, 0);
>>> +    }
>>> +
>>> +    if ( *conf == '/' )
>>> +    {
>>> +        conf++;
>>> +        if ( *conf != '/' && *conf != ',' )
>>> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
>>> +    }
>> Putting the new override settings here is not only undesirable
>> because the / separator so far really separates two similar
>> things (while you now make it also separate dissimilar ones),
>> but it also means one won't be able to override anything
>> coming out of pci_uart_config(). Therefore I think you need
>> to move this further down (and use , as the separator), in
>> turn requiring an adjustment to the doc change.
> 
> Just to understand this correctly, is this the expected syntax?
> <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>]
> [,[<bridge-bdf>][,[<reg_width>][,[<reg_shift>]]]]]]]]
> 
> Can I add these override settings (reg_width and reg_shift) just before
> the sanity checks?

Well, as you may have seen, things are getting complicated here:
The two currently-last elements are permitted only with
CONFIG_HAS_PCI, so by adding the new fields to the end we'd
end up having two syntaxes (one with and one without PCI
support). I therefore have to modify my original proposal, and
ask for the addition to be done earlier, perhaps - using a
separator other than comma (maybe colon or semicolon) - with
the [<io-base>|pci|amt] element (as that's really the item
(possibly implicitly) specifying the I/O range, which you mean to
amend.

> Should I have sanity checks for reg_width and reg_shift?
> If so, should reg_width just check for values 1 and 4?
> And should reg_shift have a max shift value as a sanity check?

Yeah, considering the other consistency checks, it would probably
be a good idea to have this.

Jan


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