[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 3/8] ns16550: make usable on ARM
On Tue, 2013-09-10 at 16:00 +0100, Jan Beulich wrote: > >>> On 10.09.13 at 16:18, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote: > > @@ -40,15 +45,22 @@ string_param("com2", opt_com2); > > > > static struct ns16550 { > > int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq; > > - unsigned long io_base; /* I/O port or memory-mapped I/O address. */ > > + u64 io_base; /* I/O port or memory-mapped I/O address. */ > > + u64 io_size; > > Does the size really need to be 64 bits? No, I just wrote 64 for both by mistake. [...] > > + default: /* assume single byte */ > > Is that really a good idea? I'd recommend returning all 1s here, and > dropping writes below. OK. > > + case 1: > > + return readb(addr); > > + case 4: > > + return readl(addr); > > This won't work without changing the return type of the function. > Or is this just mandating the access width, without the upper 24 > bits carrying meaningful data? The latter, the registers are still 8 bytes, it's just the accesses. Maybe I should mask. > > +#ifdef HAS_PCI > > static void pci_serial_early_init(struct ns16550 *uart) > > { > > if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 ) > > @@ -178,6 +215,9 @@ static void pci_serial_early_init(struct ns16550 *uart) > > pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2], > > PCI_COMMAND, PCI_COMMAND_IO); > > } > > +#else > > +static void pci_serial_early_init(struct ns16550 *uart) {} > > +#endif > > I'd prefer if #ifdef-s of this kind went inside the function body, thus > avoiding the duplication of the function "header". This particularly > reduces the churn on eventual future changes to the function type > (in particular people not building all architectures not noticing the > need to change more than one place). Me too normally, no idea why I did it this way! > > > @@ -278,21 +320,27 @@ static void __init ns16550_init_postirq(struct > > serial_port *port) > > bits = uart->data_bits + uart->stop_bits + !!uart->parity; > > uart->timeout_ms = max_t( > > unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud); > > - > > if ( uart->irq > 0 ) > > Anything wrong with the blank line above? A victim of some debugging code I inserted then removed I think. I'll but it back. > > > +static const char const *ns16550_dt_compat[] __initdata = > > That ought to be __initconst now that committed that patch. Indeed yes, thanks. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |