Re: [PATCH 07/10] irqchip/cs42l43: Add support for the cs42l43 IRQs

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

On Fri, May 12, 2023 at 05:07:45PM +0100, Marc Zyngier wrote:
> On Fri, 12 May 2023 16:39:33 +0100,
> Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > 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.
> 

Thank you this is helpful. This device has GPIOs that other
devices might want to use for IRQs, so the chip is capable
of providing IRQ services to other devices in the system not
just itself. This is commonly used where external boosted
amps have their IRQs hooked up to the CODEC.

I guess if Mark doesn't mind I think the only internal bit of the
device that uses the IRQs is the CODEC driver so I could move the
IRQ handling in there, it does seem a little odd to me, but I
guess I don't have any problems with it.

> > 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?

So we probably could do that for device tree systems, but getting
this into ACPI I think will be exceedingly difficult, and that is
likely the primary market for the device.

> 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?

Ah ok so I think I am starting to understand, if I might
paraphrase, your main objection here is that many of the IRQs are
fixed purpose signals originating inside the chip itself, rather
than external lines that can be hooked up for generic purposes.

I guess most "first class" IRQ controllers have a lot more
generic IRQs than they do fixed purpose ones. Where as we only
have the 3 GPIOs as generic purpose IRQ lines.

Thanks,
Charles