Re: [PATCH 09/12] irqchip: cirrus: Add driver for Cirrus Logic CS48L31/32/33 codecs

[Marc Zyngier]

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.

> 
> > None of that belongs in the irqchip code.
> > 
> 
> I don't really understand here what the criteria is that makes this not
> a irqchip driver but it was ok for madera. We have a PIC and we need to
> handle that and export those interrupts so other drivers can bind
> against them. Is the problem that the PIC is on a SPI bus and irqchip is
> only for memory-mapped PICs? Or is it that we have re-used existing
> library code instead of open-coding it, so you aren't seeing the actual
> handling code?

An irqchip driver uses the irq_chip structure, uses irq domains to
abstract the device-specific interrupt numbering from clients, and
doesn't force the use of an esoteric API on these clients.

What I see here is the exact opposite.

Was it OK for madera? No. A moment of weakness, I presume. Do I want
to repeat the same mistake? Neither.

> As Lee has already objected in the past to having the interrupt
> controller implementation in MFD I don't want to move it there without
> Lee's agreement that it's ok to put the PIC IRQ implementation in MFD
> for CS48L32.

If you were implementing an actual interrupt controller driver, I'd
take it without any question. The fact that this code mandates the use
of its own homegrown API rules it out.

Thanks,

	M.

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