Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 16 May 2023 11:09:36 +0100
From:   Lee Jones <lee@kernel.org>
To:     Marc Zyngier <maz@kernel.org>
Cc:     Charles Keepax <ckeepax@opensource.cirrus.com>, broonie@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, tglx@linutronix.de, linus.walleij@linaro.org,
        vkoul@kernel.org, lgirdwood@gmail.com,
        yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
        pierre-louis.bossart@linux.intel.com, alsa-devel@alsa-project.org,
        patches@opensource.cirrus.com, devicetree@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 07/10] irqchip/cs42l43: Add support for the cs42l43 IRQs
Message-ID: <20230516100936.GF10825@google.com>
References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com>
 <20230512122838.243002-8-ckeepax@opensource.cirrus.com>
 <86o7mpmvqq.wl-maz@kernel.org>
 <20230512153933.GH68926@ediswmail.ad.cirrus.com>
 <86mt29mt2m.wl-maz@kernel.org>
 <20230515112554.GA10825@google.com>
 <86h6scmzf7.wl-maz@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <86h6scmzf7.wl-maz@kernel.org>
Precedence: bulk

On Tue, 16 May 2023, Marc Zyngier wrote:

> On Mon, 15 May 2023 12:25:54 +0100,
> Lee Jones <lee@kernel.org> wrote:
> > 
> > On Fri, 12 May 2023, Marc Zyngier wrote:
> > 
> > > On Fri, 12 May 2023 16:39:33 +0100,
> > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> > > > 
> > > > On Fri, May 12, 2023 at 04:10:05PM +0100, Marc Zyngier wrote:
> > > > > On Fri, 12 May 2023 13:28:35 +0100,
> > > > > Charles Keepax <ckeepax@opensource.cirrus.com> wrote:
> > > > > > 
> > > > > > The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> > > > > > (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> > > > > > for portable applications. It provides a high dynamic range, stereo
> > > > > > DAC for headphone output, two integrated Class D amplifiers for
> > > > > > loudspeakers, and two ADCs for wired headset microphone input or
> > > > > > stereo line input. PDM inputs are provided for digital microphones.
> > > > > > 
> > > > > > The IRQ chip provides IRQ functionality both to other parts of the
> > > > > > cs42l43 device and to external devices that wish to use its IRQs.
> > > > > 
> > > > > Sorry, but this isn't much of an interrupt controller driver. A modern
> > > > > interrupt controller driver is firmware-driven (DT or ACPI, pick your
> > > > > poison), uses irq domains, and uses the irqchip API.
> > > > > 
> > > > 
> > > > Apologies but I really need a little help clarifying the issues
> > > > here. I am totally happy to fix things up but might need a couple
> > > > pointers.
> > > > 
> > > > 1) uses the irqchip API / uses irq domains
> > > > 
> > > > The driver does use both the irqchip API and domains, what
> > > > part of the IRQ API are we not using that we should be?
> > > > 
> > > > The driver registers an irq domain using
> > > > irq_domain_create_linear.  It requests its parent IRQ using
> > > > request_threaded_irq. It passes IRQs onto the devices requesting
> > > > IRQs from it using handle_nested_irq and irq_find_mapping.
> > > > 
> > > > Is the objection here that regmap is making these calls for us,
> > > > rather than them being hard coded into this driver?
> > > 
> > > That's one of the reasons. Look at the existing irqchip drivers: they
> > > have nothing in common with yours. The regmap irqchip abstraction may
> > > be convenient for what you are doing, but the result isn't really an
> > > irqchip driver. It is something that is a small bit of a larger device
> > > and not an interrupt controller driver on its own. The irqchip
> > > subsystem is there for "first class" interrupt controllers.
> > 
> > I'm not aware of another subsystem that deals with !IRQChip level IRQ
> > controllers.  Where do simple or "second class" interrupt controllers
> > go?
> 
> This isn't an interrupt controller. This is internal signalling, local
> to a single component that has been artificially broken into discrete
> bits, including an interrupt controller. The only *real* interrupts
> here are the GPIOs.
> 
> I'm happy to see an interrupt controller for the GPIOs. But the rest
> is just internal muck that doesn't really belong here. Where should it

You should have been a poet! =;-)

> go? Together with the rest of the stuff that manages the block as a
> whole. Which looks like the MFD subsystem to me.

Very well.  Let's see this "muck" in a patch please!

-- 
Lee Jones [李琼斯]