On Wed, Mar 12, 2025 at 4:30 AM Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx> wrote: Thank you, my comments below. > This patch introduces a configuration option that allows users to s/This patch introduces/Introduce/ > build the intel_pmc_ipc driver without ACPI support. This is useful > for systems where ACPI is not available or desired. > > Based on the discussion from the patch: https://patchwork.kernel.org/ > project/netdevbpf/patch/20250227121522.1802832-6- > yong.liang.choong@xxxxxxxxxxxxxxx/#26280764, it was necessary to > provide this option to accommodate specific use cases. Make it a Link tag, like "...from the patch [1], it was..." Link: https://.... [1] > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > Signed-off-by: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx> This is wrong as either it's a wrong tag (SoB --> Suggested-by?), or missing Co-developed-by, or wrong order (but in that case David should have sent the patch). ... > +#if CONFIG_ACPI Better to have #ifdef, but see below > static inline int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, struct pmc_ipc_rbuf *rbuf) > { > } > +#else > +static inline int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, struct pmc_ipc_rbuf *rbuf) > +{ return -ENODEV; } > +#endif /* CONFIG_ACPI */ Since it's already static inline, it might be more natural to have this inside the function. The current is usually used for the C impl. + static inline stub, like #ifdef FOO int foo(...); #else static inline int foo(...) { return ... } #endif But I'm not insisting, it's up to the PDx86 maintainers. -- With Best Regards, Andy Shevchenko
