[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V6] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips
>>> On 02.12.13 at 19:32, Aravind Gopalakrishnan >>> <Aravind.Gopalakrishnan@xxxxxxx> wrote: > @@ -434,7 +477,21 @@ static void __init ns16550_endboot(struct serial_port > *port) > struct ns16550 *uart = port->uart; > > if ( uart->remapped_io_base ) > + { > + if ( uart->enable_ro ) { > + if ( rangeset_add_range(mmio_ro_ranges, > + uart->io_base, > + uart->io_base + uart->io_size - 1) ) > + WARN(); > + > + if ( pci_ro_device(0, uart->ps_bdf[0], > + PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) ) > + WARN(); > + Stray blank line. Also, for neither of the two WARN()s above the resulting stack trace is really meaningful. A simple printk() would therefore suffice. But, more importantly, did you overlook the use of pci_hide_device() in ns16550_init_postirq(): The hiding should be done in one place. And with pci_ro_device() implicitly hiding the device, you should probably make sure you call just one of the two. > + /* > + * Set enable_ro flag to 1 to > + * make device and MMIO region read only > + */ > + uart->enable_ro = 1; > + break; So in the comment with the field declaration you say this is optional, as if the user had a choice. Since the selection is made internally, I don't think you should comment it that way. Furthermore, with the way you use it the field should clearly be bool_t. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |