Re: [Xen-devel] [PATCH v5 2/8] ACPI: add config for BIOS table scan

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.16 at 18:25, <jonathan.creekmore@xxxxxxxxx> wrote:
> Shannon Zhao writes:
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -5,6 +5,7 @@ config X86
>>      def_bool y
>>      select COMPAT
>>      select HAS_ACPI
>> +    select ACPI_LEGACY_TABLES_LOOKUP if HAS_ACPI
> 
> Since HAS_ACPI is selected right above this, it seems pointless to do
> the if HAS_ACPI here. Just select ACPI_LEGACY_TABLES_LOOKUP. Or, see below.

Indeed. But also beware of the alphabetical sorting here.

>> --- a/xen/drivers/acpi/Kconfig
>> +++ b/xen/drivers/acpi/Kconfig
>> @@ -2,3 +2,6 @@
>>  # Select HAS_ACPI if ACPI is supported
>>  config HAS_ACPI
>>      bool
>> +
>> +config ACPI_LEGACY_TABLES_LOOKUP
>> +    bool
> 
> Or, better, default the value of ACPI_LEGACY_TABLES_LOOKUP based on
> HAS_ACPI. That way, you only select HAS_ACPI to default this to on and,
> if another platform besides X86 ever enabled HAS_ACPI, it would turn on
> this option without you having to select it as well.

If you're thinking of "def_bool HAS_ACPI", then no, please don't:
This needlessly adds "# CONFIG_ACPI_LEGACY_TABLES_LOOKUP is
not set" to .config, which while only a cosmetic problem here in the
general case preventsprompts to show up once an option obtains a
prompt. And I'd like to avoid setting bad precedents.

>> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
>> index ce15470..a2fc8c4 100644
>> --- a/xen/drivers/acpi/osl.c
>> +++ b/xen/drivers/acpi/osl.c
>> @@ -75,12 +75,14 @@ acpi_physical_address __init 
>> acpi_os_get_root_pointer(void)
>>                             "System description tables not found\n");
>>                      return 0;
>>              }
>> -    } else {
>> +    } else if (IS_ENABLED(CONFIG_ACPI_LEGACY_TABLES_LOOKUP)) {
> 
> I would use an #ifdef CONFIG_ACPI_LEGACY_TABLES_LOOKUP instead of using
> the kconfig.h IS_ENABLED macro to keep from pulling that file in.

I don't think I objected to this part (and in fact I agree with Andrew
that the non-preprocessor variant, where it can be used without
breaking the build, is preferable). Iirc what I objected to was that
you didn't use Kconfig.

Jan


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