Re: [PATCH] irqchip: create a Kconfig menu for irqchip drivers

[Masahiro Yamada]

2017-07-26 22:50 GMT+09:00 Marc Zyngier <marc.zyngier@xxxxxxx>:
> On 26/07/17 14:14, Masahiro Yamada wrote:
>> 2017-07-26 19:37 GMT+09:00 Marc Zyngier <marc.zyngier@xxxxxxx>:
>>> On 26/07/17 11:18, Masahiro Yamada wrote:
>>>> Hi Marc,
>>>>
>>>>
>>>> 2017-07-26 17:04 GMT+09:00 Marc Zyngier <marc.zyngier@xxxxxxx>:
>>>>> 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@xxxxxxxxxxxxx>
>>>>>> ---
>>>>>>
>>>>>>  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