Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts

From: Marc Zyngier <maz@kernel.org>
To: Suman Anna <s-anna@ti.com>
Cc: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>,
	tglx@linutronix.de, jason@lakedaemon.net, robh+dt@kernel.org,
	lee.jones@linaro.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, david@lechnology.com,
	wmills@ti.com
Subject: Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
Date: Sat, 25 Jul 2020 17:27:57 +0100	[thread overview]
Message-ID: <1cd0b6c9bfe2dc42e9c6b69baacf4635@kernel.org> (raw)
In-Reply-To: <14a0978a-f38f-8cd7-3fee-b0e438513396@ti.com>

On 2020-07-25 16:57, Suman Anna wrote:

Suman,

> Hi Marc,

[...]

>>>>> @@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
>>>>> platform_device *pdev)
>>>>>           return -ENOMEM;
>>>>> 
>>>>>       for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
>>>>> +        if (intc->invalid_intr & BIT(i))
>>>>> +            continue;
>>>>> +
>>>>>           irq = platform_get_irq_byname(pdev, irq_names[i]);
>>>>>           if (irq <= 0) {
>>>>> +            if (intc->shared_intr & BIT(i))
>>>>> +                continue;
>>>> 
>>>> I don't really understand why you are treating these "shared" 
>>>> interrupts
>>>> differently from the invalid ones. In all cases, they shouldn't be 
>>>> used.
>>> 
>>> The behavior is the same in how we handle it, but the difference is
>>> that an "invalid" one is never even connected to the ARM interrupt
>>> controller, while the "shared" one is a choice. So, unless this
>>> interrupt is being used/handled by a different processor/entity, you
>>> would not see this skipped from the dts node.
>> 
>> And I'm saying that all that matters is that you are discarding these
>> interrupts. Whether they are flagged invalid or shared, they are not
>> available to Linux. So the difference in handling is pointless and
>> only makes it harder to understand what you are doing.
> 
> The primary reason for using two properties and this logic was to
> accurately describe the h/w and usage of these in the DT bindings to
> distinguish the "never connected" vs the "optionally can be skipped"
> interrupts rather than go by how these are handled in the driver. I
> feel we will loose this description and make it confusing for SoC
> product integration developers.

This logic makes zero difference to Linux, and I do not see what
you gain by having two code paths with separate list of unusable
interrupts. And why on Earth would a "Soc product integration
developer" have any business to mess with this driver code?
They should very much stay away from it and deal with their
precious value add.

If you want two properties or even twenty, go for it, and have fun.
Just don't make this driver even more unreadable than it already is.
Merge all these interrupts in *one* list of unusable interrupts,
and be done with it.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...