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

[Rahul Singh <Rahul.Singh@arm.com>]

Hello Jan,

> On 27 Nov 2020, at 1:58 pm, Jan Beulich <jbeulich@suse.com> 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

Thanks for reviewing the code.I am ok with it.

> (b) the R-b and A-b you've already got can be kept.
> 
> Jan
>