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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Fri, Nov 11, 2022 at 08:00:10AM +0000, Marc Zyngier wrote:
> On Thu, 10 Nov 2022 20:36:16 +0000,
> Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Nov 10, 2022 at 06:47:20PM +0000, Marc Zyngier wrote:

Apologies this ended up getting quite long. Cirrus has no trouble
changing how the IRQ driver works I just think we are struggling a
little to understand exactly what parts of the code need reworking
in what way, we appreciate your patience in helping us through.

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

I think this is part of the crossed wires here, the code does not
mandate the use of its own home grown API, although it does
provide one. For example our CODECs often provide GPIO/IRQ
services for other devices such as say speaker amps attached
along side them.

Here is a DT example from one of my dev systems with GPIO1 on
cs47l35 (a madera CODEC) handling the IRQ for cs35l35 (a speaker
amp):

cs35l35_left: cs35l35@40 {
	compatible = "cirrus,cs35l35";
	reg = <0x40>;

	#sound-dai-cells = <1>;

	reset-gpios = <&axi_gpio0 0 0>;

	interrupt-parent = <&cs47l35>;
	interrupts = <57 0>;
};

No special code is required in the cs35l35 driver (it is fully
upstreamed sound/soc/codecs/cs35l35.c). So if we are missing some
actual interrupt controller API we need to be supporting that we
are not please point us at it and we will happily add support?

So I think your objections are mostly regarding the
cs48l32_request_irq function (and friends) that are being
used by the other parts of the MFD.  I don't think it would be super
hard to remove these functions and move the IRQ into DT if that is
the preferred way.

> > ACPI gets to be a lot of fun here, it's just not idiomatic to describe
> > the internals of these devices in firmware there and a lot of the
> > systems shipping this stuff are targeted at other OSs and system
> > integrators are therefore not in the least worried about Linux
> > preferences.

I would echo Mark's statement that going the way of moving this
into DT/ACPI will actually likely necessitate the addition of a
lot of "board file" stuff in the future. If the part gets used in
any ACPI systems (granted support is not in yet but this is not a
super unlikely addition in the future for cs48l32) we will need to
support the laptops containing the part in Linux and the vendors are
extremely unlikely to put internal CODEC IRQs into the ACPI tables.

But that aside I guess my main question about this approach would
be what the DT binding would look like for the CODEC. Currently
our devices use a single DT node for the device. Again pulling a
Madera example from my dev setup, this is what the DT binding for
one of our CODECs currently looks vaguely like:

cs47l35: cs47l35@1 {
	compatible = "cirrus,cs47l35";
	reg = <0x1>;

	spi-max-frequency = <11000000>;

	interrupt-controller;
	#interrupt-cells = <2>;
	interrupt-parent = <&gpio0>;
	interrupts = <56 8>;

	gpio-controller;
	#gpio-cells = <2>;

	#sound-dai-cells = <1>;

	AVDD-supply = <&lochnagar_vdd1v8>;
	DBVDD1-supply = <&lochnagar_vdd1v8>;
	DBVDD2-supply = <&lochnagar_vdd1v8>;
	CPVDD1-supply = <&lochnagar_vdd1v8>;
	CPVDD2-supply = <&lochnagar_vddcore>;
	DCVDD-supply = <&lochnagar_vddcore>;
	SPKVDD-supply = <&wallvdd>;

	reset-gpios = <&lochnagar_pin 0 0>;

	clocks = <&lochnagar_clk LOCHNAGAR_CDC_MCLK1>, <&lochnagar_clk LOCHNAGAR_CDC_MCLK2>;
	clock-names = "mclk1", "mclk2";

	pinctrl-names = "default";
	pinctrl-0 = <&cs47l35_defaults>;
};

The interrupt-parent points at who our IRQ is connected to, and
we are an interrupt-controller so people can use our IRQs. I
think it is not currently supported to have more than a single
interrupt-parent for a device so with the current binding is it
actually possible for the device to refer to its own IRQs in DT?

An alternative approach would be to actually represent the MFD in
device tree, I think this would allow things to work and look
something like (totally not tested just for discussion):

cs47l35: cs47l35@1 {
	compatible = "cirrus,cs47l35";
	reg = <0x1>;

	spi-max-frequency = <11000000>;

	irq: madera-irq {
		compatible = "cirrus,madera-irq";

		interrupt-controller;
		#interrupt-cells = <2>;
		interrupt-parent = <&gpio0>;
		interrupts = <56 8>;
	};

	gpio: madera-gpio {
		compatible = "cirrus,madera-gpio";
		gpio-controller;
		#gpio-cells = <2>;
	};

	sound: madera-sound {
		compatible = "cirrus,cs47l35-sound";

		interrupt-parent = <&madera-irq>;
		interrupts = <55 0>, <56 0>;
		#sound-dai-cells = <1>;
	};
};

Historically I believe we have been discouraged (by upstream, not
from our customers) from explicitly representing the parts of the
MFD in device tree separately, as it was viewed that this is just
an external SPI CODEC and one node mapped much more logically to the
hardware, which is what DT should be describing. However, are you
saying this would be a preferrable approach from your side?  Or am
I missing some alternative solution?

Again thank you kindly for you time looking at this.

Thanks,
Charles