Re: [PATCH/RFC] drivers/irqchip: add irq-inverter

From: Marc Zyngier <maz@kernel.org>
To: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <magnus.damm@gmail.com>,
	linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC] drivers/irqchip: add irq-inverter
Date: Tue, 28 Dec 2021 18:18:59 +0000	[thread overview]
Message-ID: <87h7aszj70.wl-maz@kernel.org> (raw)
In-Reply-To: <20211228165642.2514766-1-nikita.yoush@cogentembedded.com>

On Tue, 28 Dec 2021 16:56:43 +0000,
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:
> 
> Interrupt trigger type is typically used to configure interrupt
> controller to properly interpret interrupt signal sent from a device.
> 
> However, some devices have configureable interrupt outputs, and drivers
> tend to use interrupt trigger type also to configure device interrupt
> output.
> 
> This works well when device interrupt output is connected directly to
> interrupt controller input. However, this is not always the case.
> Sometimes the interrupt signal gets inverted between the device
> producing it and the controller consuming it. Combined with both sides
> using the same interrupt trigger type to configure the signal, this
> results into non-working setup regardless of what interrupt trigger type
> is configured.

Regardless? Surely there is a canonical, working configuration.

> 
> Irq-inverer is a solution for this case. It is a virtual irqchip that
> provides additional virq number that behaves exactly as existing one,
> but with inverted trigger type reported via irq_get_trigger_type() API.
> 
> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc,
> that has WiFi interrupt delivered over inverting level-shifter:
> 
> / {
> 	wlcore_interrupt: inverter {
> 		compatible = "linux,irq-inverter";
> 		interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_RISING>;
> 		interrupt-controller;
> 		#interrupt-cells = <0>;
> 	};
> };
>
> &wlcore {
> 	interrupts-extended = <&wlcore_interrupt>;
> };

So you don't describe the trigger at the endpoint level, but at the
pseudo-interrupt controller level? /me feels mildly sick.

And by the way: "An interrupt specifier is one or more cells of data
(as specified by #interrupt-cells) ...". Ergo, #interrupt-cells cannot
be 0 when the interrupt controller can be an interrupt-parent.

> 
> Then, wl18xx driver gets IRQ_TYPE_EDGE_FALLING return from
> irq_get_trigger_type() call, and configures interrupt output for that.
> Then the signal is delivered inverted to the GPIO module, and handled
> correctly, because GPIO is configured for IRQ_TYPE_EDGE_RISING.

So this is only to avoid writing the correct device tree?

> 
> Implementation notes:
> 
> - why platform_driver and not IRQCHIP_DECLARE()?
> - because IRQCHIP_DECLARE() does not process EPROBE_DEFER properly

More importantly, IRQCHIP_DECLARE() is for root interrupt controllers
that need to be probed long before we have a device model up and
running.

> - why not using hierarchial irq_domain?
> - because with hierarchial irq_domain, same interrupt gets the same virq
>   number at all levels, and trigger type is tied to virq number, so need
>   different virq numbers for reporting different trigger types

Why would you have different interrupt numbers? A given line has one
configuration at any given point, and only one.

> 
> - why using request_irq() for parent irq, instead of setting up chained
>   interrupt in irqchips?
> - because this way code is much simpler, and shall work for all cases
>   (such as normal/threaded parent irq, normal/threaded child irq,
>   different parent interrupt chips, etc)

And that's a NAK for deliberately violating the irqchip API.

> 
> - why just not introducing separate API for consumer-side and
>   produced-side trigger type?
> - because with the chosen approach, no changes are needed to any cases
>   that don't suffer from inverted interrupt routing

The right way to do it is to use the existing API by exposing the
inverter (there are existing examples in the tree, using the
hierarchical model). It isn't rocket science, and not much more code
than the pile of hacks^W^W^Wcreative approach you have.

	M.

-- 
Without deviation from the norm, progress is not possible.