Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 27 Nov 2020, Bertrand Marquis wrote:
> Hi Jan,
> 
> > On 27 Nov 2020, at 13:58, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > 
> > On 25.11.2020 19:16, Rahul Singh wrote:
> >> --- a/xen/drivers/char/ns16550.c
> >> +++ b/xen/drivers/char/ns16550.c
> >> @@ -16,7 +16,7 @@
> >> #include <xen/timer.h>
> >> #include <xen/serial.h>
> >> #include <xen/iocap.h>
> >> -#ifdef CONFIG_HAS_PCI
> >> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> >> #include <xen/pci.h>
> >> #include <xen/pci_regs.h>
> >> #include <xen/pci_ids.h>
> >> @@ -51,7 +51,7 @@ static struct ns16550 {
> >>     unsigned int timeout_ms;
> >>     bool_t intr_works;
> >>     bool_t dw_usr_bsy;
> >> -#ifdef CONFIG_HAS_PCI
> >> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> > 
> > I'm sorry to be picky, but this being a hack wants, imo, also calling
> > it so, by way of a code comment. Clearly this should go at one of the
> > first instances, yet neither of the two above are really suitable imo.
> > Hence I'm coming back to my prior suggestion of introducing a
> > consolidated #define without this becoming a Kconfig setting:
> > 
> > /*
> > * The PCI part of the code in this file currently is only known to
> > * work on x86. Undo this hack once the logic has been suitably
> > * abstracted.
> > */
> > #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> > # define NS16550_PCI
> > #endif
> > 
> > And then use NS16550_PCI everywhere. I'd be fine making this
> > adjustment while committing, if I knew that (a) you're okay with it
> > and (b) the R-b and A-b you've already got can be kept.
> > 
> 
> Sounds ok to me so you can keep my R-b if you go this way.

I am OK with that too