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