* [PATCH] irqchip: create a Kconfig menu for irqchip drivers @ 2017-07-26 4:03 Masahiro Yamada 2017-07-26 8:04 ` Marc Zyngier 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2017-07-26 4:03 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier; +Cc: Masahiro Yamada, linux-kernel 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.masahiro@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 -- 2.7.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 4:03 [PATCH] irqchip: create a Kconfig menu for irqchip drivers Masahiro Yamada @ 2017-07-26 8:04 ` Marc Zyngier 2017-07-26 10:18 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Marc Zyngier @ 2017-07-26 8:04 UTC (permalink / raw) To: Masahiro Yamada, Thomas Gleixner, Jason Cooper; +Cc: linux-kernel 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.masahiro@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 Do you have any example in mind where having a user-selectable interrupt controller actually makes sense on its own? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 8:04 ` Marc Zyngier @ 2017-07-26 10:18 ` Masahiro Yamada 2017-07-26 10:37 ` Marc Zyngier 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2017-07-26 10:18 UTC (permalink / raw) To: Marc Zyngier; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List Hi Marc, 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@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.masahiro@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. > 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(). Looks like irq-ts4800.c, irq-keystone.c are modules as well. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 10:18 ` Masahiro Yamada @ 2017-07-26 10:37 ` Marc Zyngier 2017-07-26 13:14 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Marc Zyngier @ 2017-07-26 10:37 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List On 26/07/17 11:18, Masahiro Yamada wrote: > Hi Marc, > > > 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@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.masahiro@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? > > >> 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? > Looks like irq-ts4800.c, irq-keystone.c are modules as well. They are directly selected by their respective defconfig. On arm64, which is what I expect you driver targets, you should simply select it in your platform entry. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 10:37 ` Marc Zyngier @ 2017-07-26 13:14 ` Masahiro Yamada 2017-07-26 13:50 ` Marc Zyngier 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2017-07-26 13:14 UTC (permalink / raw) To: Marc Zyngier; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List 2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>: > On 26/07/17 11:18, Masahiro Yamada wrote: >> Hi Marc, >> >> >> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@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.masahiro@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. >> >> >>> 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. >> 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 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. Users are allowed to disable the option from menuconfig. > 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. The prompts are pointless if the options are supposed by selected by others. 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? -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 13:14 ` Masahiro Yamada @ 2017-07-26 13:50 ` Marc Zyngier 2017-07-26 14:24 ` Thomas Gleixner 2017-07-26 14:27 ` Masahiro Yamada 0 siblings, 2 replies; 10+ messages in thread From: Marc Zyngier @ 2017-07-26 13:50 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List On 26/07/17 14:14, Masahiro Yamada wrote: > 2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>: >> On 26/07/17 11:18, Masahiro Yamada wrote: >>> Hi Marc, >>> >>> >>> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@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.masahiro@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. >>> >>> >>>> 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? > 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. > > 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*. >> 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. > The prompts are pointless if the options are supposed by selected by others. See above. > 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. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 13:50 ` Marc Zyngier @ 2017-07-26 14:24 ` Thomas Gleixner 2017-07-26 14:48 ` Masahiro Yamada 2017-07-26 14:27 ` Masahiro Yamada 1 sibling, 1 reply; 10+ messages in thread From: Thomas Gleixner @ 2017-07-26 14:24 UTC (permalink / raw) To: Marc Zyngier; +Cc: Masahiro Yamada, Jason Cooper, Linux Kernel Mailing List On Wed, 26 Jul 2017, Marc Zyngier wrote: > On 26/07/17 14:14, Masahiro Yamada wrote: > > 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. Masahiro is right. They are user selectable. Enable CONFIG_SOC_IMX51 and you'll get a prompt for TS-4800 IRQ controller in the middle of the 'Device Drivers' menu. Same for the other options, which have prompts. The irqchip Kconfig is included from drivers/Kconfig so there is menu context, just not a proper one. I agree that this is crap and these prompts should be removed and the irq thingy selected by the stuff which needs it. Thanks, tglx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 14:24 ` Thomas Gleixner @ 2017-07-26 14:48 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2017-07-26 14:48 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Marc Zyngier, Jason Cooper, Linux Kernel Mailing List 2017-07-26 23:24 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>: > On Wed, 26 Jul 2017, Marc Zyngier wrote: >> On 26/07/17 14:14, Masahiro Yamada wrote: >> > 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. > > Masahiro is right. They are user selectable. > > Enable CONFIG_SOC_IMX51 and you'll get a prompt for > > TS-4800 IRQ controller > > in the middle of the 'Device Drivers' menu. Same for the other options, > which have prompts. The irqchip Kconfig is included from drivers/Kconfig so > there is menu context, just not a proper one. > > I agree that this is crap and these prompts should be removed and the irq > thingy selected by the stuff which needs it. I could not find any corresponding platform that selects CONFIG_TS4800_IRQ. I am not familiar with this driver at all, but is this a corner case according to the commit message below? If the main SoC selects the FPGA IP, it is weird because this is really board-specific connection. commit d01f8633d52e4dac5ee598b87d49fd23346ccfd6 Author: Damien Riegel <damien.riegel@savoirfairelinux.com> Date: Mon Dec 21 15:11:23 2015 -0500 irqchip/ts4800: Add TS-4800 interrupt controller This commit adds support for the TS-4800 interrupt controller. This controller is instantiated in a companion FPGA, and multiplex interrupts for other FPGA IPs. As this component is external to the SoC, the SoC might need to reserve pins, so this controller is implemented as a platform driver and doesn't use the IRQCHIP_DECLARE construct. Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Rob Herring <robh+dt@kernel.org> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> Cc: Kumar Gala <galak@codeaurora.org> Cc: kernel@savoirfairelinux.com Link: http://lkml.kernel.org/r/1450728683-31416-2-git-send-email-damien.riegel@savoirfairelinux.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 13:50 ` Marc Zyngier 2017-07-26 14:24 ` Thomas Gleixner @ 2017-07-26 14:27 ` Masahiro Yamada 2017-07-26 14:58 ` Marc Zyngier 1 sibling, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2017-07-26 14:27 UTC (permalink / raw) To: Marc Zyngier; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List 2017-07-26 22:50 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>: > On 26/07/17 14:14, Masahiro Yamada wrote: >> 2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyngier@arm.com>: >>> On 26/07/17 11:18, Masahiro Yamada wrote: >>>> Hi Marc, >>>> >>>> >>>> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@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.masahiro@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers 2017-07-26 14:27 ` Masahiro Yamada @ 2017-07-26 14:58 ` Marc Zyngier 0 siblings, 0 replies; 10+ messages in thread From: Marc Zyngier @ 2017-07-26 14:58 UTC (permalink / raw) To: Masahiro Yamada; +Cc: Thomas Gleixner, Jason Cooper, Linux Kernel Mailing List On 26/07/17 15:27, Masahiro Yamada wrote: [snip lots of "Sigh"] > Before this, please understand Kconfig. I've so far been relatively polite and restrained. I don't expect anything less from you. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-07-26 14:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-26 4:03 [PATCH] irqchip: create a Kconfig menu for irqchip drivers Masahiro Yamada 2017-07-26 8:04 ` Marc Zyngier 2017-07-26 10:18 ` Masahiro Yamada 2017-07-26 10:37 ` Marc Zyngier 2017-07-26 13:14 ` Masahiro Yamada 2017-07-26 13:50 ` Marc Zyngier 2017-07-26 14:24 ` Thomas Gleixner 2017-07-26 14:48 ` Masahiro Yamada 2017-07-26 14:27 ` Masahiro Yamada 2017-07-26 14:58 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®