Re: [Xen-devel] [PATCH RFC 3/8] ns16550: make usable on ARM

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

>  static char ns_read_reg(struct ns16550 *uart, int reg)
>  {
> +    void __iomem *addr = uart->remapped_io_base + (reg<<uart->reg_shift);

Blanks around <<.

> +#ifdef HAS_IOPORTS
>      if ( uart->remapped_io_base == NULL )
>          return inb(uart->io_base + reg);
> -    return readb(uart->remapped_io_base + reg);
> +#endif
> +    switch (uart->reg_width) {

Coding style.

> +    default: /* assume single byte */

Is that really a good idea? I'd recommend returning all 1s here, and
dropping writes below.

> +    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?

> +    }

>  static void ns_write_reg(struct ns16550 *uart, int reg, char c)

Same/similar comments go for this function.

> +#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).

> @@ -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?

> +static const char const *ns16550_dt_compat[] __initdata =

That ought to be __initconst now that committed that patch.

Jan


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