Re: [PATCH v1 3/8] irqchip: Add EcoNet EN751221 INTC

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Mar 21 2025 at 23:20, Caleb James DeLisle wrote:
> On 21/03/2025 21:26, Thomas Gleixner wrote:
>> Caleb!
>>
>> On Fri, Mar 21 2025 at 13:46, Caleb James DeLisle wrote:
>>> ---
>>> If CPU_MIPSR2_IRQ_EI / CPU_MIPSR2_IRQ_VI are enabled in the build, this
>>> device switches to sending all interrupts as vectored - which IRQ_MIPS_CPU
>>> is not prepared to handle. If anybody knows how to either disable this
>>> behavior, or handle vectored interrupts without ugly code that breaks
>>> cascading, please let me know and I will implement that and add
>>> MIPS_MT_SMP in a future patchset.
>> This must be addressed before this driver can be merged, but that's a
>> topic for the MIPS wizards and out of my area of expertise, except for
>> the obvious:
>>
>>      For a start you can exclude this platform from being enabled in
>>      Kconfig when the EI/VI muck is enabled. That's what 'depends on' is
>>      for,
>
> Maybe my message was misleading everything has been tested and works correctly
> on multiple SoCs because ECONET_SOC_EN751221 does not select EI/VI. Answering
> this question will allow me to enable them, thus also getting
> MIPS_MT_SMP.

It does not select it, but it can be enabled independently or through
some other magic config knob, right? And if it gets enabled, then it
does not work, right?

> I could look at forbidding them in the driver, but I'm not sure that's
> appropriate as this seems like more of an SoC issue than an INTC
> issue. But I'll follow your guidance.

What's not appropriate? If it does not work, then it's very appropriate
to do

   config ECONET
          depends on !EI && !VI

on the principle of least surprise, no?

>> So this patch clearly should have been tagged with 'RFC'.
>
> Given the patchset works correctly in testing, does this comment
> stand?

Until the EI/VI issue is resolved so that it either works or cannot
happen.

>>> +static int econet_intc_map(struct irq_domain *d, u32 irq, irq_hw_number_t hwirq)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (hwirq >= INTC_IRQ_COUNT) {
>>> +		pr_err("%s: hwirq %lu out of range\n", __func__, hwirq);
>>> +		return -EINVAL;
>>> +	} else if (econet_intc_rai.shadow_interrupts[hwirq] == INTC_IS_SHADOW) {
>>> +		pr_err("%s: can't map hwirq %lu, it is a shadow interrupt\n",
>>> +		       __func__, hwirq);
>> No newline
> If I understand correctly, you prefer:
> .....interrupt\n", __func__, hwirq);
> for a 96 char line?

You have 100 characters in drivers/irqchip/

>>> +	.domain_ops = {
>>> +		.xlate = irq_domain_xlate_onecell,
>>> +		.map = econet_intc_map,
>> See documention.
> I suppose this is tab alignment, but I will in any case make a point
> of reading it all carefully.

Yes. The aligned tabular view is way simpler to read and parse. Reading
is based on pattern recognition. Irregular patterns disturb the reading
flow, which means the focus is shifted from understanding to decoding
the irregular pattern.

> In case of any doubt, I wasn't trying to sneak bad code past you.

I did not assume malice here at all.

Thanks,

        tglx