Re: [PATCH v1] platform/x86: intel_pmc_ipc: add option to build without ACPI

[Andy Shevchenko]

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