On 10/11/2022 15:13, Marc Zyngier wrote:
On Thu, 10 Nov 2022 13:00:50 +0000,
Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
On 10/11/2022 12:01, Marc Zyngier wrote:
On Thu, 10 Nov 2022 11:22:26 +0000,
Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
On 10/11/2022 08:02, Marc Zyngier wrote:
On Wed, 09 Nov 2022 16:53:28 +0000,
Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
The Cirrus Logic CS48L31/32/33 audio codecs contain a programmable
interrupt controller with a variety of interrupt sources, including
GPIOs that can be used as interrupt inputs.
This driver provides the handling for the interrupt controller. As the
codec is accessed via regmap, the generic regmap_irq functionality
is used to do most of the work.
I cannot spot a shred of interrupt controller code in there. This
It is providing support for handling an interrupt controller so that
other drivers can bind to those interrupts. It's just that regmap
provides a lot of generic implementation for SPI-connected interrupt
controllers so we don't need to open-code all that in the
irqchip driver.
And thus none of that code needs to live in drivers/irqchip.
belongs IMO to the MFD code.
We did once put interrupt support in MFD for an older product line but
the MFD maintainer doesn't like the MFD being a dumping-ground for
random other functionality that have their own subsystems.
I don't like this stuff either. All this code is a glorified set of
interrupt handlers and #defines that only hide the lack of a proper DT
binding to express the interrupt routing (it feels like looking at
board files from 10 years ago).
I didn't understand this. The whole purpose of this is to instantiate
Linux interrupts for the PIC interrupt sources so that other drivers
that want to use the interrupts from the CS48L32 PIC can use standard
kernel APIs or DT to bind against them.
There is zero standard APIs in this patch. Does cs48l32_request_irq()
look standard to you? This whole thing makes a mockery of the
interrupt model and of firmware-based interrupt description which we
spent years to build.
The four handlers registered within the driver are done here simply
because they don't belong to any particular child driver. Since they
are a fixed feature of the chip that we know we want to handle we may as
well just register them.
Again, they have no purpose in an interrupt controller driver.
If we put them in the MFD with DT definitions it would make a
circular dependency between MFD and its child, which is not a great
situation. If it's these handlers that are bothering you, we could move
them to the audio driver.
And what's left? Nothing.
Ah, I see. You've missed that the bulk of the implementation re-uses
existing library code from regmap. It does say this in the commit
message.
"the generic regmap_irq functionality is used to do most of the work."
and I've also said this in previous replies.
This is no way driver that does nothing. There's over 1000 lines of code
handling the PIC and dispatching its interrupts to other drivers that
want to bind to them. It's just that it makes no sense to duplicate 1300
lines of interrupt handling code from elsewhere when we can re-use that
by calling regmap_add_irq_chip(). That gives us all the interrupt-
controller-handling code in drivers/base/regmap/regmap-irq.c
Perhaps you could re-review this taking into account that
regmap_add_irq_chip() is significant.
