Re: [PATCH 4/4] platform/x86: pmc_atom: make the PMC driver actually unselectable

From: Hans de Goede
Date: Mon May 02 2022 - 10:45:58 EST


Hi,

On 4/28/22 08:24, Paul Gortmaker wrote:
> This caught my eye when I saw it was def_bool and hence largely
> pointless to have a Kconfig for it at all.
>
> Yet there is no reason why you shouldn't be able to opt out of Atom
> platform support if you only care about desktop and server class CPUs.
>
> It was introduced as def_bool, but there is no obvious reason as to why
> it was forcibly built-in for everyone, other than LPSS implicitly
> relying on it (which is now fixed). So here we fix up the Kconfig and
> open the door for people to opt out.
>
> Since putting "default y" on anything that isn't absolutely essential is
> generally frowned upon, I made the default be CONFIG_MATOM. People who
> use "make oldconfig" or similar won't notice any difference.
>
> The two "unchanged" lines for PCI and COMMON_CLK appear in the diff from
> fixing a whitespace issue that somehow managed to live on despite being
> moved between two different Kconfig files since its introduction.
>
> Cc: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Mark Gross <markgross@xxxxxxxxxx>
> Cc: platform-driver-x86@xxxxxxxxxxxxxxx
> Signed-off-by: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
> ---
> drivers/platform/x86/Kconfig | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f08ad85683cb..86459e99d831 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1163,6 +1163,11 @@ config WINMATE_FM07_KEYS
> endif # X86_PLATFORM_DEVICES
>
> config PMC_ATOM
> - def_bool y
> - depends on PCI
> - select COMMON_CLK
> + bool "Intel Atom SOC Power Management Controller driver"
> + default MATOM

Besides the remarks from Andy, this does seem like a weird default,
MATOM means that gcc is passed -march=atom nothing more and nothing
less. For a distro kernel which may e.g. set
CONFIG_GENERIC_CPU we still want this enabled. But as said it is
brought in by CONFIG_X86_INTEL_LPSS when that is set.

Thinking more about this I think it might be best to just do this
instead, replacing patch 2 + 4 of this set:

diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
index 1244c4e568ff..c2088b3c4081 100644
--- a/drivers/clk/x86/Makefile
+++ b/drivers/clk/x86/Makefile
@@ -1,6 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_PMC_ATOM) += clk-pmc-atom.o
obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE) += clk-fch.o
-clk-x86-lpss-y := clk-lpss-atom.o
-obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o
+obj-$(CONFIG_X86_INTEL_LPSS) += clk-lpss-atom.o clk-pmc-atom.o
obj-$(CONFIG_CLK_LGM_CGU) += clk-cgu.o clk-cgu-pll.o clk-lgm.o
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index f08ad85683cb..85c396a43048 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1161,8 +1161,3 @@ config WINMATE_FM07_KEYS
that delivers key events when these buttons are pressed.

endif # X86_PLATFORM_DEVICES
-
-config PMC_ATOM
- def_bool y
- depends on PCI
- select COMMON_CLK
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 4a59f47a46e2..cc2a74713313 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -126,7 +126,7 @@ obj-$(CONFIG_INTEL_SCU_PCI) += intel_scu_pcidrv.o
obj-$(CONFIG_INTEL_SCU_PLATFORM) += intel_scu_pltdrv.o
obj-$(CONFIG_INTEL_SCU_WDT) += intel_scu_wdt.o
obj-$(CONFIG_INTEL_SCU_IPC_UTIL) += intel_scu_ipcutil.o
-obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
+obj-$(CONFIG_X86_INTEL_LPSS) += pmc_atom.o

# Siemens Simatic Industrial PCs
obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o


This just folds the enabling of the pmc_atom code into the same
Kconfig option as the one used the the LPSS code, so this actually
simplify things; while at the same time making it possible to
not build the pmc_atom code by unselecting the CONFIG_X86_INTEL_LPSS
option.

Regards,

Hans