Re: [PATCH] ACPI: rename acpi_arm_init to acpi_arch_init

[Sunil V L <sunilvl@ventanamicro.com>]

On Sat, Jul 27, 2024 at 12:39:03AM +0800, Miao Wang wrote:
> Hi,
> 
> Thanks for your quick reply.
> 
> > 2024年7月27日 00:05，Sudeep Holla <sudeep.holla@arm.com> 写道：
> > 
> > On Fri, Jul 26, 2024 at 11:03:01PM +0800, Miao Wang via B4 Relay wrote:
> >> From: Miao Wang <shankerwangmiao@gmail.com>
> >> 
> >> So that we avoid arch-specific code in general ACPI initialization flow.
> >> Other architectures can also have chance to define their own
> >> arch-specific acpi initialization process if necessary.
> >> 
> > 
> > Nice, but I assume you are adding something similar to another arch(riscv
> > or loongarch ?). It would be nice to have those changes as well together to
> > make it easy to understand the intention much quicker.
> 
> Yes, you are right about it. I'm trying to add some codes for loongarch,
> after DSDT is loaded and namespace is created, before the devices are
> enumerated, so I'll have chance to add a _DEP method to one of the device
> using acpi_install_method to provide compatibility for some early loongarch
> devices which are produced before the loongarch related ACPI standard is
> finalized.
> 
I have arch-specific initialization need for RISC-V as well. So, good to
see this patch!.

> > 
> >> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
> >> ---
> >> arch/arm64/include/asm/acpi.h | 2 ++
> >> drivers/acpi/arm64/init.c     | 2 +-
> >> drivers/acpi/bus.c            | 2 +-
> >> include/linux/acpi.h          | 6 +++---
> >> 4 files changed, 7 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index a407f9cd549e..0d24e920e143 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -188,4 +188,6 @@ static inline void acpi_map_cpus_to_nodes(void) { }
> >> 
> >> #define ACPI_TABLE_UPGRADE_MAX_PHYS MEMBLOCK_ALLOC_ACCESSIBLE
> >> 
> >> +#define ACPI_HAVE_ARCH_INIT
> >> +
> > 
> > There is nothing core arm66 arch specific in acpi_arm_init() and hence it
> > is in drivers/acpi/arm64. I would like to avoid adding anything in arch/arm64
> > if possible. Also I don't think we need to define this ACPI_HAVE_ARCH_INIT
> > 
> >> #endif /*_ASM_ACPI_H*/
> >> diff --git a/drivers/acpi/arm64/init.c b/drivers/acpi/arm64/init.c
> >> index d0c8aed90fd1..7a47d8095a7d 100644
> >> --- a/drivers/acpi/arm64/init.c
> >> +++ b/drivers/acpi/arm64/init.c
> >> @@ -2,7 +2,7 @@
> >> #include <linux/acpi.h>
> >> #include "init.h"
> >> 
> >> -void __init acpi_arm_init(void)
> >> +void __init acpi_arch_init(void)
> > 
> > Keep the name acpi_arm_init as is.
> > 
> >> {
> >> if (IS_ENABLED(CONFIG_ACPI_AGDI))
> >> acpi_agdi_init();
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index 284bc2e03580..662f69e379ef 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -1458,7 +1458,7 @@ static int __init acpi_init(void)
> >> acpi_viot_early_init();
> >> acpi_hest_init();
> >> acpi_ghes_init();
> >> - acpi_arm_init();
> >> + acpi_arch_init();
> > 
> > Here we need acpi_arch_init() like you have changed.
> > 
> >> acpi_scan_init();
> >> acpi_ec_init();
> >> acpi_debugfs_init();
> >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> >> index f0b95c76c707..3c3a83499c2d 100644
> >> --- a/include/linux/acpi.h
> >> +++ b/include/linux/acpi.h
> >> @@ -1517,10 +1517,10 @@ static inline int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
> >> }
> >> #endif
> >> 
> >> -#ifdef CONFIG_ARM64
> >> -void acpi_arm_init(void);
> >> +#ifdef ACPI_HAVE_ARCH_INIT
> >> +void acpi_arch_init(void);
> > 
> > This is bit inconsistent. The Makefile is still conditional on
> > CONFIG_ARM64 while here you move to ACPI_HAVE_ARCH_INIT.
> > So while not just undefine and redefine acpi_arch_init to acpi_arm_init.
> > Something like this must work ?
> > 
> > #define acpi_arch_init() do { }while(0)
> > 
> > #ifdef CONFIG_ARM64
> > #undef acpi_arch_init
> > #define acpi_arch_init() acpi_arm_init()
> > #endif
> 
> It will work. However I can see the pattern in other parts, where
> the definition of a macro named HAVE_xxx is checked, and define an
> inline static function with empty body if such macro is not defined
> or define a function prototype with the same name otherwise, like
> acpi_arch_set_root_pointer. I'm just trying to follow this pattern.
> 
I was thinking to make it weak function similar to cpc_read_ffh().
Wouldn't it be better than ifdefery?

Thanks
Sunil