Message ID | 20250312022955.1418234-1-yong.liang.choong@linux.intel.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [v1] platform/x86: intel_pmc_ipc: add option to build without ACPI | expand |
On Wed, Mar 12, 2025 at 4:30 AM Choong Yong Liang <yong.liang.choong@linux.intel.com> 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@linux.intel.com/#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@linux.intel.com> > Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> 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.
On 12/3/2025 3:54 pm, Andy Shevchenko wrote: > On Wed, Mar 12, 2025 at 4:30 AM Choong Yong Liang > <yong.liang.choong@linux.intel.com> 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@linux.intel.com/#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] > > Hi Andy, Thank you for your detailed feedback and suggestions. I'll make the necessary adjustments to the patch based on your comments above. >> Signed-off-by: David E. Box <david.e.box@linux.intel.com> >> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > 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). > I believe the sequence is still correct, as the solution was provided by David, and he should be the main author. I'm just the submitter, so my sign-off should be placed last. > ... > >> +#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. > Sure, let's wait for more feedback.
On Wed, Mar 12, 2025 at 1:23 PM Choong Yong Liang <yong.liang.choong@linux.intel.com> wrote: > On 12/3/2025 3:54 pm, Andy Shevchenko wrote: > > On Wed, Mar 12, 2025 at 4:30 AM Choong Yong Liang > > <yong.liang.choong@linux.intel.com> wrote: ... > Thank you for your detailed feedback and suggestions. I'll make the > necessary adjustments to the patch based on your comments above. > > >> Signed-off-by: David E. Box <david.e.box@linux.intel.com> > >> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > > > 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). > > > I believe the sequence is still correct, as the solution was provided by > David, and he should be the main author. I'm just the submitter, so my > sign-off should be placed last. You are the submitter *and* the author in this case. As I said, that SoB chain is wrong in its current form.
On 12/3/2025 10:00 pm, Andy Shevchenko wrote: > On Wed, Mar 12, 2025 at 1:23 PM Choong Yong Liang > <yong.liang.choong@linux.intel.com> wrote: >> On 12/3/2025 3:54 pm, Andy Shevchenko wrote: >>> On Wed, Mar 12, 2025 at 4:30 AM Choong Yong Liang >>> <yong.liang.choong@linux.intel.com> wrote: > > ... > >> Thank you for your detailed feedback and suggestions. I'll make the >> necessary adjustments to the patch based on your comments above. >> >>>> Signed-off-by: David E. Box <david.e.box@linux.intel.com> >>>> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> >>> >>> 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). >>> >> I believe the sequence is still correct, as the solution was provided by >> David, and he should be the main author. I'm just the submitter, so my >> sign-off should be placed last. > > You are the submitter *and* the author in this case. As I said, that > SoB chain is wrong in its current form. > Hi Andy, I believe this time I have correctly attributed the authorship by following the guide from: https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by --- From: David E. Box <david.e.box@linux.intel.com> <changelog> Signed-off-by: David E. Box <david.e.box@linux.intel.com> Co-developed-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> ---
diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h b/include/linux/platform_data/x86/intel_pmc_ipc.h index 6e603a8c075f..a852f1a02532 100644 --- a/include/linux/platform_data/x86/intel_pmc_ipc.h +++ b/include/linux/platform_data/x86/intel_pmc_ipc.h @@ -34,6 +34,7 @@ struct pmc_ipc_rbuf { * * Return: 0 on success. Non-zero on mailbox error */ +#if CONFIG_ACPI static inline int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, struct pmc_ipc_rbuf *rbuf) { struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; @@ -90,5 +91,9 @@ static inline int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, struct pmc_ipc_rbuf return 0; } +#else +static inline int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, struct pmc_ipc_rbuf *rbuf) +{ return -ENODEV; } +#endif /* CONFIG_ACPI */ #endif /* INTEL_PMC_IPC_H */