On 2016/2/2 2:09, Robert Richter wrote:
On 23.01.16 17:39:20, Hanjun Guo wrote:@@ -385,10 +386,8 @@ void __init arm64_numa_init(void) { int ret = -ENODEV; -#ifdef CONFIG_OF_NUMA if (!numa_off) - ret = numa_init(arm64_of_numa_init); -#endif + ret = numa_init(acpi_disabled ? arm64_of_numa_init : arm64_acpi_numa_init); if (ret) numa_init(dummy_numa_init);Ok, this style is mostly flavor, some people want #ifdefs (my preference), some not. In any case it must build with or without the config option set. But first some words why I like #ifdefs: * Code is easier to understand as you don't need to look at any other location whether it is enabled or not. * You can't break the build if the options are not set. Thus, you also don't need to check if the function is implemented for the unset case (valid for the coder and also the reviewer). This makes things a lot easier. * Total number of lines of code that needs to be implement is smaller. However, if we don't ifdef the code, we need empty functions stubs in the header file for them. Also, the conditional assignment does not reduce the complexity of the paths. It just concentrates everything in a single line. How about the following (similar to x86)? ---- if (!numa_off) { #ifdef CONFIG_ACPI_NUMA if (!numa_init(acpi_numa_init)) return 0; #endif #ifdef CONFIG_OF_NUMA if (!numa_init(of_numa_init)) return 0; #endif } return numa_init(dummy_numa_init); ---- Pretty straight and nice. Note: The !acpi_disabled check needs to be moved to the beginning of acpi_numa_init(). Variable ret can be removed.
Lorenzo suggested to remove it, Lorenzo, what's your opinion here? Thanks Hanjun
