2017-07-26 22:50 GMT+09:00 Marc Zyngier <marc.zyng...@arm.com>: > On 26/07/17 14:14, Masahiro Yamada wrote: >> 2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyng...@arm.com>: >>> On 26/07/17 11:18, Masahiro Yamada wrote: >>>> Hi Marc, >>>> >>>> >>>> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyng...@arm.com>: >>>>> On 26/07/17 05:03, Masahiro Yamada wrote: >>>>>> Some irqchip drivers have a Kconfig prompt. When we run menuconfig >>>>>> or friends, those drivers are directly listed in the "Device Drivers" >>>>>> menu level. This does not look nice. Create a sub-system level menu. >>>>>> >>>>>> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com> >>>>>> --- >>>>>> >>>>>> drivers/irqchip/Kconfig | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >>>>>> index f1fd5f44d1d4..7b66313a2952 100644 >>>>>> --- a/drivers/irqchip/Kconfig >>>>>> +++ b/drivers/irqchip/Kconfig >>>>>> @@ -1,3 +1,5 @@ >>>>>> +menu "IRQ chip support" >>>>>> + >>>>>> config IRQCHIP >>>>>> def_bool y >>>>>> depends on OF_IRQ >>>>>> @@ -306,3 +308,5 @@ config QCOM_IRQ_COMBINER >>>>>> help >>>>>> Say yes here to add support for the IRQ combiner devices embedded >>>>>> in Qualcomm Technologies chips. >>>>>> + >>>>>> +endmenu >>>>>> >>>>> >>>>> I'm very reluctant to introduce this. IMHO, interrupt controllers are >>>>> way too low level a thing to let them be selected by the user. They >>>>> really should be selected by the platform that needs them >>>> >>>> This is true for the root irqchip. >>>> Not necessarily true for child irqchips. >>> >>> I dispute that argument. We've been able to make this work so far >>> *without* exposing yet another menu maze to the user. What has changed? >> >> >> The irqchip maintainers applied drivers >> with user-configurable Kconfig entries. > > They are *not* user-selectable, since there is *NO* menu entry. *You* > are making them user-selectable, and I'm objecting to that.
Sigh, how many times do I have to say this. They are user-selectable. Just try this. I am using Linus' tree. masahiro@grover:~/workspace/linux$ git describe v4.13-rc2-22-gfd2b2c57ec20 masahiro@grover:~/workspace/linux$ make ARCH=arm defconfig HOSTCC scripts/basic/fixdep HOSTCC scripts/basic/bin2c HOSTCC scripts/kconfig/conf.o SHIPPED scripts/kconfig/zconf.tab.c SHIPPED scripts/kconfig/zconf.lex.c SHIPPED scripts/kconfig/zconf.hash.c HOSTCC scripts/kconfig/zconf.tab.o HOSTLD scripts/kconfig/conf *** Default configuration is based on 'multi_v7_defconfig' # # configuration written to .config # masahiro@grover:~/workspace/linux$ make ARCH=arm menuconfig Visit the "Device Drivers" menu. I can toggle TS-4800 IRQ controller Keystone 2 IRQ controller IP Huh? -*- Memory Controller drivers ---> <*> Industrial I/O support ---> < > Non-Transparent Bridge support ---- [ ] VME bridge support ---- [*] Pulse-Width Modulation (PWM) Support ---> < > TS-4800 IRQ controller <*> Keystone 2 IRQ controller IP < > IndustryPack bus support ---- -*- Reset Controller Support ---> < > FMC support ---- PHY Subsystem ---> >>>> >>>> >>>>> Do you have any example in mind where having a user-selectable interrupt >>>>> controller actually makes sense on its own? >>>> >>>> Yes. >>>> >>>> I see some user-selectable drivers in drivers/irqchip/Kconfig >>>> and I'd like to add one more for my SoCs. >>>> >>>> >>>> This patch: >>>> https://github.com/uniphier/linux/commit/f39efdf0ce34f77ae9e324d9ec6c7f486f43a0ed >>>> >>>> This is really optional, so >>>> I intentionally implemented it as a platform driver >>>> instead of IRQCHIP_DECLARE(). >>> >>> I really cannot see how this could be optional. It means that you could >>> end-up in a situation where the drivers for the devices being this >>> irqchip could have been compiled in, but not their interrupt controller. >>> How useful is that? >> >> In my case, the assumed irq consumer is GPIO. >> >> If the irq consumer is probed before the irqchip, >> it will be tried later by -EPROBE_DEFER. > >> If the irqchip is not compiled at all, right, the irq consumer will not work. >> One possible (and general) solution is to specify "depends on" correctly >> between the provider and the consumer. > > Exactly. It has to be selected either by the platform Kconfig, or > whatever is wired onto this irqchip. > >>>> Looks like irq-ts4800.c, irq-keystone.c are modules as well. >>> >>> They are directly selected by their respective defconfig. >> >> >> Are you sure? >> >> As far as I see, they are not selected by anyone. >> >> >> $ git grep 'TS4800_IRQ\|KEYSTONE_IRQ' >> arch/arm/configs/keystone_defconfig:CONFIG_KEYSTONE_IRQ=y >> arch/arm/configs/multi_v7_defconfig:CONFIG_KEYSTONE_IRQ=y > > And what is that if not a selection? This is not a selection. Again, it is just a default value. Users can change the configuration as they like starting from the reasonable default setting. In this discussion with you, my impression is you really do not understand Kconfig. >> drivers/irqchip/Kconfig:config TS4800_IRQ >> drivers/irqchip/Kconfig:config KEYSTONE_IRQ >> drivers/irqchip/Makefile:obj-$(CONFIG_TS4800_IRQ) += >> irq-ts4800.o >> drivers/irqchip/Makefile:obj-$(CONFIG_KEYSTONE_IRQ) += >> irq-keystone.o >> >> >> >> defconfig just provides a default value. > > That's the platform maintainer's problem, not mine. Surely your problem. Adding a menu prompt makes the entry vision in menuconfig etc. >> >> Users are allowed to disable the option from menuconfig. > > No. They are allowed to change what makes sense, and leaving them in > control of the irqchips doesn't make any sense *at all*. I just explained how Kconfig works. If you do not understand this, I recommend you to play around with it for a while. >>> On arm64, >>> which is what I expect you driver targets, you should simply select it >>> in your platform entry. >> >> OK, assuming your clain is correct, >> we have 5 suspicious entries in drivers/irqchip/Kconfig. >> >> >> config JCORE_AIC >> bool "J-Core integrated AIC" if COMPILE_TEST >> >> config TS4800_IRQ >> tristate "TS-4800 IRQ controller" >> >> config KEYSTONE_IRQ >> tristate "Keystone 2 IRQ controller IP" >> >> config EZNPS_GIC >> bool "NPS400 Global Interrupt Manager (GIM)" >> >> config QCOM_IRQ_COMBINER >> bool "QCOM IRQ combiner support" >> >> >> >> The prompt strings make the entries visible in menuconfig. >> So, they should be removed. > > Not at all. The help string is extremely useful (use the '/' key i9n > menuconfig and search for an entry...), and act as documentation. Sigh, I am not talking about the "help". I am talking about the strings after bool/tristate. >> The prompts are pointless if the options are supposed by selected by others. > > See above. I really recommend you to play with a simple Kconfig example. Add and remove prompts and see how the behavior will change. >> Also, tristate is pointless. >> If they are supposed to be selected by platforms, >> they have no chance to be a module. >> They should be turned into bool (without prompt) >> >> Is this what you mean? > > Among other things, yes. Before this, please understand Kconfig. -- Best Regards Masahiro Yamada
