Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <7609be8e-6331-2b4b-3349-bf300bb6af15@redhat.com>
Date:   Mon, 2 May 2022 16:45:42 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 4/4] platform/x86: pmc_atom: make the PMC driver actually
 unselectable
Content-Language: en-US
To:     Paul Gortmaker <paul.gortmaker@windriver.com>,
        linux-kernel@vger.kernel.org
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Aubrey Li <aubrey.li@linux.intel.com>,
        Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org,
        Mark Gross <markgross@kernel.org>,
        platform-driver-x86@vger.kernel.org,
        "Rafael J. Wysocki" <rafael@kernel.org>
References: <20220428062430.31010-1-paul.gortmaker@windriver.com>
 <20220428062430.31010-5-paul.gortmaker@windriver.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220428062430.31010-5-paul.gortmaker@windriver.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Mark Gross <markgross@kernel.org>
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  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