From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751251AbdJBLVN (ORCPT ); Mon, 2 Oct 2017 07:21:13 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:33064 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751173AbdJBLVM (ORCPT ); Mon, 2 Oct 2017 07:21:12 -0400 X-Google-Smtp-Source: AOwi7QABkxOpzESA0iklaoM2OSuH0Z8acDf6TBH/6yiLTPQ4RxjxGY2oON2RLuBznM7GHupinLKYkDMdYekh1GnqNCQ= MIME-Version: 1.0 In-Reply-To: <20170926080022.215f7b7b@endymion> References: <6c1696cf9f860cc8151a906ecbc799121b5aeeab.1505984256.git.sean.wang@mediatek.com> <20170926080022.215f7b7b@endymion> From: Arnd Bergmann Date: Mon, 2 Oct 2017 13:21:10 +0200 X-Google-Sender-Auth: iMEsHFgvSJpX-j9iPN2yVYTdQu4 Message-ID: Subject: Re: [PATCH] soc: mediatek: turn MTK_PMIC_WRAP into visible symbols To: Jean Delvare Cc: Sean Wang , Matthias Brugger , "moderated list:ARM/Mediatek SoC..." , Linux ARM , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 26, 2017 at 8:00 AM, Jean Delvare wrote: > On Thu, 21 Sep 2017 17:01:05 +0800, sean.wang@mediatek.com wrote: >> From: Sean Wang >> >> MTK_PMIC_WRAP is the basic and required configuration for those various >> MediaTek PMICs, so turning MTK_PMIC_WRAP into visible symbols easily >> allows users tending to have the enablement for those PMICs. > > I can't really make sense of the sentence above, sorry, and can't see > how it matches the change below anyway. MTK_PMIC_WRAP is already a > visible symbol before this change. The change is probably good in > itself, but please try to come up with a better description. > >> Signed-off-by: Sean Wang >> --- >> drivers/soc/mediatek/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> index a2fcd7f..d513629 100644 >> --- a/drivers/soc/mediatek/Kconfig >> +++ b/drivers/soc/mediatek/Kconfig >> @@ -15,7 +15,7 @@ config MTK_INFRACFG >> config MTK_PMIC_WRAP >> tristate "MediaTek PMIC Wrapper Support" >> depends on ARCH_MEDIATEK >> - depends on RESET_CONTROLLER >> + select RESET_CONTROLLER >> select REGMAP Your other patch now removes the ARCH_MEDIATEK dependency and allows enabling the driver for COMPILE_TEST on architectures that might not have ARCH_HAS_RESET_CONTROLLER set, so you cannot 'select' the symbol in general. I think a better solution is to leave the 'depends on RESET_CONTROLLER' present here, but instead add 'select RESET_CONTROLLER' to the ARCH_MEDIATEK definition. Generally speaking, we don't want to mix 'select' and 'depends on' statements for the same symbol, but if we do, it should be done consistently in a very clear hierarchy. In case of RESET_CONTROLLER, the rule seems to be to 'select' it from architectures and platforms, while using 'depends on' from device drivers that absolutely require it. Note that for compile-testing, it should be fine to rely on the fallback definitions in include/linux/reset.h, so you might be able to just remove the 'depends on' statement completely. In any case, please try to be clearer about what the patch actually tries to achieve. Did you run into a build error without it? If so, please copy the exact build error message into the patch description. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH] soc: mediatek: turn MTK_PMIC_WRAP into visible symbols Date: Mon, 2 Oct 2017 13:21:10 +0200 Message-ID: References: <6c1696cf9f860cc8151a906ecbc799121b5aeeab.1505984256.git.sean.wang@mediatek.com> <20170926080022.215f7b7b@endymion> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20170926080022.215f7b7b@endymion> Sender: linux-kernel-owner@vger.kernel.org To: Jean Delvare Cc: Sean Wang , Matthias Brugger , "moderated list:ARM/Mediatek SoC..." , Linux ARM , Linux Kernel Mailing List List-Id: linux-mediatek@lists.infradead.org On Tue, Sep 26, 2017 at 8:00 AM, Jean Delvare wrote: > On Thu, 21 Sep 2017 17:01:05 +0800, sean.wang@mediatek.com wrote: >> From: Sean Wang >> >> MTK_PMIC_WRAP is the basic and required configuration for those various >> MediaTek PMICs, so turning MTK_PMIC_WRAP into visible symbols easily >> allows users tending to have the enablement for those PMICs. > > I can't really make sense of the sentence above, sorry, and can't see > how it matches the change below anyway. MTK_PMIC_WRAP is already a > visible symbol before this change. The change is probably good in > itself, but please try to come up with a better description. > >> Signed-off-by: Sean Wang >> --- >> drivers/soc/mediatek/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> index a2fcd7f..d513629 100644 >> --- a/drivers/soc/mediatek/Kconfig >> +++ b/drivers/soc/mediatek/Kconfig >> @@ -15,7 +15,7 @@ config MTK_INFRACFG >> config MTK_PMIC_WRAP >> tristate "MediaTek PMIC Wrapper Support" >> depends on ARCH_MEDIATEK >> - depends on RESET_CONTROLLER >> + select RESET_CONTROLLER >> select REGMAP Your other patch now removes the ARCH_MEDIATEK dependency and allows enabling the driver for COMPILE_TEST on architectures that might not have ARCH_HAS_RESET_CONTROLLER set, so you cannot 'select' the symbol in general. I think a better solution is to leave the 'depends on RESET_CONTROLLER' present here, but instead add 'select RESET_CONTROLLER' to the ARCH_MEDIATEK definition. Generally speaking, we don't want to mix 'select' and 'depends on' statements for the same symbol, but if we do, it should be done consistently in a very clear hierarchy. In case of RESET_CONTROLLER, the rule seems to be to 'select' it from architectures and platforms, while using 'depends on' from device drivers that absolutely require it. Note that for compile-testing, it should be fine to rely on the fallback definitions in include/linux/reset.h, so you might be able to just remove the 'depends on' statement completely. In any case, please try to be clearer about what the patch actually tries to achieve. Did you run into a build error without it? If so, please copy the exact build error message into the patch description. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Mon, 2 Oct 2017 13:21:10 +0200 Subject: [PATCH] soc: mediatek: turn MTK_PMIC_WRAP into visible symbols In-Reply-To: <20170926080022.215f7b7b@endymion> References: <6c1696cf9f860cc8151a906ecbc799121b5aeeab.1505984256.git.sean.wang@mediatek.com> <20170926080022.215f7b7b@endymion> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Sep 26, 2017 at 8:00 AM, Jean Delvare wrote: > On Thu, 21 Sep 2017 17:01:05 +0800, sean.wang at mediatek.com wrote: >> From: Sean Wang >> >> MTK_PMIC_WRAP is the basic and required configuration for those various >> MediaTek PMICs, so turning MTK_PMIC_WRAP into visible symbols easily >> allows users tending to have the enablement for those PMICs. > > I can't really make sense of the sentence above, sorry, and can't see > how it matches the change below anyway. MTK_PMIC_WRAP is already a > visible symbol before this change. The change is probably good in > itself, but please try to come up with a better description. > >> Signed-off-by: Sean Wang >> --- >> drivers/soc/mediatek/Kconfig | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> index a2fcd7f..d513629 100644 >> --- a/drivers/soc/mediatek/Kconfig >> +++ b/drivers/soc/mediatek/Kconfig >> @@ -15,7 +15,7 @@ config MTK_INFRACFG >> config MTK_PMIC_WRAP >> tristate "MediaTek PMIC Wrapper Support" >> depends on ARCH_MEDIATEK >> - depends on RESET_CONTROLLER >> + select RESET_CONTROLLER >> select REGMAP Your other patch now removes the ARCH_MEDIATEK dependency and allows enabling the driver for COMPILE_TEST on architectures that might not have ARCH_HAS_RESET_CONTROLLER set, so you cannot 'select' the symbol in general. I think a better solution is to leave the 'depends on RESET_CONTROLLER' present here, but instead add 'select RESET_CONTROLLER' to the ARCH_MEDIATEK definition. Generally speaking, we don't want to mix 'select' and 'depends on' statements for the same symbol, but if we do, it should be done consistently in a very clear hierarchy. In case of RESET_CONTROLLER, the rule seems to be to 'select' it from architectures and platforms, while using 'depends on' from device drivers that absolutely require it. Note that for compile-testing, it should be fine to rely on the fallback definitions in include/linux/reset.h, so you might be able to just remove the 'depends on' statement completely. In any case, please try to be clearer about what the patch actually tries to achieve. Did you run into a build error without it? If so, please copy the exact build error message into the patch description. Arnd