On 12/06/2017 23:12, Arnd Bergmann wrote: > On Mon, Jun 12, 2017 at 11:38 AM, Daniel Lezcano > <daniel.lezc...@linaro.org> wrote: >> On Fri, Jun 09, 2017 at 10:48:13PM +0200, Arnd Bergmann wrote: >>> On Fri, Jun 9, 2017 at 10:15 PM, John Stultz <john.stu...@linaro.org> wrote: >>>> On Fri, Jun 9, 2017 at 1:06 PM, Arnd Bergmann <a...@arndb.de> wrote: >>>>> On Fri, Jun 9, 2017 at 5:46 PM, Daniel Lezcano >>>>> <daniel.lezc...@linaro.org> wrote: >>>>> >>>>> Yes, but I'm not sure this is the right patch either. We tend to not >>>>> use 'select' for user-visible drivers, and most hisilicon platforms >>>>> won't need this driver. >>>>> >>>>> I think it would be more consistent to add this to the defconfig >>>>> and regard it as a user error when the driver is disabled on a >>>>> machine that needs it. >>>> >>>> Maybe the select is not exactly in the right place, but I don't really >>>> feel like a pmic on an SoC is a "user-visible driver". I deal with the >>>> board often and when the new dependency was made on the clk, I would >>>> have never have found it on my own w/o Ulf and Daniel pointing out >>>> what I needed to enable. >>> >>> What I meant is that the Kconfig option is user-visible. On a very high >>> level, this is a result of arch/arm64/Kconfig.platforms listing only >>> very broad categories of SoCs, in many cases only the manufacturers >>> of very different chip families, which then control the visibility of the >>> individual Kconfig items for things like pinctrl or clk. >>> >>> I now see that MFD_HI655X_PMIC is the top-level driver that you >>> have to select before enabling COMMON_CLK_HI655X, so the >>> patch is actually broken unless it actually selects both. >>> >>> How about simply adding a 'default MFD_HI655X_PMIC' to >>> COMMON_CLK_HI655X to enable it unless it is explicitly >>> turned off? >> >> Actually, I share John's opinion. >> >> Ideally when we choose a platform, all the relevants devices configuration >> options should be selected automatically from a single topmost node of a tree >> (platform selection) to all the nodes corresponding to the devices, leaving >> the >> user to select one simple option without knowledge of the SoC hardware >> internals. >> >> If the user is expert in the platform and knows exactly what he does, then he >> can select an _EXPERT_ like option and be able to disable some drivers. >> >> It is how I tend to write the Kconfig options, so the 'default >> MFD_HI655X_PMIC' >> is confusing for me. Wouldn't make sense to select COMMON_CLK_HI655X when >> MFD_HI655X_PMIC is enabled? > > I don't think it's that easy. When you do that, MFD_HI655X_PMIC gains > a dependency on COMMON_CLK and will again cause a warning on > machines that disable that during compile testing.
This issue is related to the missing stubs in the includes. > Using 'select' for user-selectable options generally leads to problems, > and you are better off avoiding it. If you want to make the symbol impossible > to turn off for non-EXPERT configurations, you can write it like > > config COMMON_CLK_HI655X > tristate "Clock driver for Hi655x" if EXPERT > depends on (MFD_HI655X_PMIC || COMPILE_TEST) > depends on REGMAP > default MFD_HI655X_PMIC > > That way the option is completely hidden for non-EXPERT, > but still has the right default otherwise, and the dependencies > are tracked right for compile-testing. Ok. Thanks! -- Daniel -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
