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:   Fri, 12 May 2023 17:07:45 +0100
Message-ID: <86mt29mt2m.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     <broonie@kernel.org>, <lee@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
In-Reply-To: <20230512153933.GH68926@ediswmail.ad.cirrus.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>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.

> 
> 2) driver is firmware-driven (DT or ACPI, pick your poison)
> 
> The irq chip has representation in firmware, in fact we have
> tested this on both ACPI and DT. Other devices can request
> IRQs from it through firmware, same as they can for any other
> IRQ chip.

That's not what I'm talking about.

> Is the objection here the table mapping the register fields that
> are provided as an IRQ on the device?

I'm referring to this sort of construct:

+	CS42L43_IRQ_REG(HP_STARTUP_DONE,			MSM),
+	CS42L43_IRQ_REG(HP_SHUTDOWN_DONE,			MSM),
+	CS42L43_IRQ_REG(HSDET_DONE,				MSM),
+	CS42L43_IRQ_REG(TIPSENSE_UNPLUG_DB,			MSM),
+	CS42L43_IRQ_REG(TIPSENSE_PLUG_DB,			MSM),
+	CS42L43_IRQ_REG(RINGSENSE_UNPLUG_DB,			MSM),
+	CS42L43_IRQ_REG(RINGSENSE_PLUG_DB,			MSM),
+	CS42L43_IRQ_REG(TIPSENSE_UNPLUG_PDET,			MSM),
+	CS42L43_IRQ_REG(TIPSENSE_PLUG_PDET,			MSM),
+	CS42L43_IRQ_REG(RINGSENSE_UNPLUG_PDET,			MSM),
+	CS42L43_IRQ_REG(RINGSENSE_PLUG_PDET,			MSM),

Why isn't this described in firmware tables? Why doesn't it need to be
carried as part of the driver? Is "CLASS_D_AMP" something an interrupt
controller driver should care about?

	M.

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