Re: [PATCH v2 11/12] soc: samsung: pm_domains: modularize EXYNOS_PM_DOMAINS

From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Will McVicker <willmcvicker@google.com>,
	Russell King <linux@armlinux.org.uk>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Sylwester Nawrocki <s.nawrocki@samsung.com>,
	Tomasz Figa <tomasz.figa@gmail.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	John Stultz <john.stultz@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>
Cc: Lee Jones <lee.jones@linaro.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Saravana Kannan <saravanak@google.com>,
	kernel-team@android.com, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
	linux-rtc@vger.kernel.org
Subject: Re: [PATCH v2 11/12] soc: samsung: pm_domains: modularize EXYNOS_PM_DOMAINS
Date: Wed, 29 Sep 2021 15:36:56 +0200	[thread overview]
Message-ID: <976e57f8-07ee-4698-b29d-dd854b79e570@canonical.com> (raw)
In-Reply-To: <20210928235635.1348330-12-willmcvicker@google.com>

On 29/09/2021 01:56, Will McVicker wrote:
> Convert the Exynos PM Domains driver into a module. This includes
> setting EXYNOS_PM_DOMAINS as tristate and removing it from being
> auto-selected by ARCH_EXYNOS. Instead, the config will use
> "default y if ARCH_EXYNOS" which allows us to set it to a module via the
> defconfig now.
> 
> Signed-off-by: Will McVicker <willmcvicker@google.com>
> ---
>  arch/arm/mach-exynos/Kconfig     |  1 -
>  arch/arm64/Kconfig.platforms     |  1 -
>  drivers/soc/samsung/Kconfig      |  3 ++-
>  drivers/soc/samsung/pm_domains.c | 12 +++++++-----
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index e97e1d8f7b00..2ad19a08bf06 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -15,7 +15,6 @@ menuconfig ARCH_EXYNOS
>  	select EXYNOS_THERMAL
>  	select EXYNOS_PMU_ARM
>  	select EXYNOS_SROM
> -	select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
>  	select GPIOLIB
>  	select HAVE_ARM_ARCH_TIMER if ARCH_EXYNOS5
>  	select HAVE_ARM_SCU if SMP
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index e5e4b9b2fb97..e44d5e9f5058 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -91,7 +91,6 @@ config ARCH_BRCMSTB
>  
>  config ARCH_EXYNOS
>  	bool "ARMv8 based Samsung Exynos SoC family"
> -	select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
>  	select HAVE_S3C_RTC if RTC_CLASS
>  	select PINCTRL
>  	select PM_GENERIC_DOMAINS if PM
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index fdf1162ec98b..e4743c29f73c 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -37,8 +37,9 @@ config EXYNOS_PMU_ARM
>  	depends on ARCH_EXYNOS || (ARM && COMPILE_TEST)
>  
>  config EXYNOS_PM_DOMAINS
> -	bool "Exynos PM domains" if COMPILE_TEST
> +	tristate "Exynos PM domains"

+Cc Arnd and Olof,

Unlike in clocks and soc drivers changes, you mentioned the removal of
"if", however it is not explained why you do it.

Why is the most important part of commit message, not "what". Because
"What" we can easily see. But "why" is sometimes trickier.

Please also explain why Exynos is so special that we deviate from the
policy for all SoC that critical SoC-related drivers have to be enabled
(built-in or as module).
https://lore.kernel.org/lkml/CAK8P3a1TY+XT1vF=wAh0XA5BXU5Z6Ab1d4DekXbVsN9aj3aL5w@mail.gmail.com/

We follow specific convention or policy and changing it requires some
discussion, not silently under the "modularize" hood. It really looks
like you want to sneak it in.

P.S. I recommend also to Cc Soc maintainers, because their point of view
here is crucial.

>  	depends on (ARCH_EXYNOS && PM_GENERIC_DOMAINS) || COMPILE_TEST
> +	default y if ARCH_EXYNOS

Best regards,
Krzysztof