Re: [PATCH v1 1/4] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Mar 06, 2014 at 10:15:08AM +0000, Jan Beulich wrote:
> >>> On 05.03.14 at 18:25, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > @@ -325,11 +342,12 @@ static void __init ns16550_init_preirq(struct serial_port *port)
> >  #ifdef CONFIG_X86
> >          enum fixed_addresses idx = FIX_COM_BEGIN + (uart - ns16550_com);
> >  
> > -        set_fixmap_nocache(idx, uart->io_base);
> > +        set_fixmap_nocache(idx, uart->io_base + uart->offset);
> >          uart->remapped_io_base = (void __iomem *)fix_to_virt(idx);
> > -        uart->remapped_io_base += uart->io_base & ~PAGE_MASK;
> > +        uart->remapped_io_base += (uart->io_base + uart->offset) & ~PAGE_MASK;
> >  #else
> > -        uart->remapped_io_base = (char *)ioremap(uart->io_base, uart->io_size);
> > +        uart->remapped_io_base = (char *)ioremap(uart->io_base + uart->offset,
> > +                                                 uart->io_size);
> >  #endif
> 
> Looking at this again, I have a hard time seeing why you can't simply
> set uart->io_base to the full, final value ...

I was trying to make it obvious that it is not just io_base but also
an offset.
> 
> > @@ -701,6 +719,10 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >                          uart->lsr_mask = uart_config[i].lsr_mask;
> >                          uart->io_base = ((u64)bar_64 << 32) |
> >                                          (bar & PCI_BASE_ADDRESS_MEM_MASK);
> > +                        uart->offset = uart_config[i].first_offset;
> > +                        uart->offset += bar_idx * uart_config[i].uart_offset;
> 
> ... here, and drop the offset field again.

But on the other hand - why even do that - when all of that can nicely
be put in there with an explanation.

Will rebase! Thank you again for your review!
> 
> Jan
>