Re: [PATCH v2] dt-bindings: interrupt-controller: loongson,liointc: Fix warnings about liointc-2.0

[Krzysztof Kozlowski]

On 24/08/2023 13:32, Binbin Zhou wrote:
> Hi Krzysztof:
> 
> Thanks for your detailed reply.
> 
> On Tue, Aug 22, 2023 at 4:30 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>
>> On 22/08/2023 10:13, Binbin Zhou wrote:
>>> Hi Krzysztof:
>>>
>>> Thanks for your detailed reply.
>>>
>>> On Tue, Aug 22, 2023 at 1:44 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>>
>>>> On 21/08/2023 08:13, Binbin Zhou wrote:
>>>>> Since commit f4dee5d8e1fa ("dt-bindings: interrupt-controller: Add
>>>>> Loongson-2K1000 LIOINTC"), the loongson liointc supports configuring
>>>>> routes for 64-bit interrupt sources.
>>>>>
>>>>> For liointc-2.0, we need to define two liointc nodes in dts, one for
>>>>> "0-31" interrupt sources and the other for "32-63" interrupt sources.
>>>>> This applies to mips Loongson-2K1000.
>>>>>
>>>>> Unfortunately, there are some warnings about "loongson,liointc-2.0":
>>>>> 1. "interrupt-names" should be "required", the driver gets the parent
>>>>> interrupts through it.
>>>>
>>>> No, why? Parent? This does not make sense.
>>>
>>> This was noted in the v1 patch discussion. The liointc driver now gets
>>> the parent interrupt via of_irq_get_byname(), so I think the
>>> "interrupt-names" should be "required".
>>
>> of_irq_get_byname() does not give you parent interrupt, but the
>> interrupt. Why do you need parent interrupt and what is it?
>>
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-loongson-liointc.c?h=v6.5-rc6#n345
>>>
>>> static const char *const parent_names[] = {"int0", "int1", "int2", "int3"};
>>>
>>>         for (i = 0; i < LIOINTC_NUM_PARENT; i++) {
>>>                 parent_irq[i] = of_irq_get_byname(node, parent_names[i]);
>>>                 if (parent_irq[i] > 0)
>>>                         have_parent = TRUE;
>>>         }
>>>         if (!have_parent)
>>>                 return -ENODEV;
>>
>> How requiring parents interrupt is related to other changes in this
>> file? One logical change, one patch.
> 
> Yes, that was my mistake, whether or not the interrupt-names need to
> be "required" is another issue. It does not cause a check warning.
> I'll think about it some more.
>>
>> Anyway why did you do it and take it by names? Names here are basically
>> useless if they match indices, so just get interrupt by indices.
> 
> There is a match between interrupts, interrupt names and interrupt maps:
> 
> interrupt->interrupt name->interrupt map
> 2->int0->int_map[0]
> 3->int1->int_map[1]
> 4->int2->int_map[2]
> 5->int3->int_map[3]
> 
> As part of the 2k1000 liointc1 node:
> 
>                 liointc1: interrupt-controller@1fe11440 {
> ....
>                         interrupt-parent = <&cpuintc>;
>                         interrupts = <3>;
>                         interrupt-names = "int1";
> 
>                         loongson,parent_int_map = <0x00000000>, /* int0 */


How did you sneak this property? The version - v2 - which was reviewed
by Rob:
https://lore.kernel.org/all/20190905144316.12527-7-jiaxun.yang@xxxxxxxxxxx/
did not have it.

Now v3 suddenly appears with Rob's review and this property:
https://lore.kernel.org/all/20200112081416.722218-4-jiaxun.yang@xxxxxxxxxxx/

Please help me understand this property appeared there and how did you
get it reviewed?

>                                                 <0xffffffff>, /* int1 */
>                                                 <0x00000000>, /* int2 */
>                                                 <0x00000000>; /* int3 */

So now you will keep bringing more hacks for a hacky property. No, this
cannot go on.

Best regards,
Krzysztof