diff mbox series

[RFC,v7,1/6] dt-bindings: gpio: fix microchip,mpfs-gpio interrupt descriptions

Message ID 20240723-trash-issuing-e2bdd55b764e@wendy
State New
Headers show
Series PolarFire SoC GPIO support | expand

Commit Message

Conor Dooley July 23, 2024, 11:27 a.m. UTC
The microchip,mpfs-gpio binding suffered greatly due to being written
with a narrow minded view of the controller, and the interrupt bits
ended up incorrect. It was mistakenly assumed that the interrupt
configuration was set by platform firmware, based on the FPGA
configuration, and that the GPIO DT nodes were the only way to really
communicate interrupt configuration to software.

Instead, the mux should be a device in its own right, and the GPIO
controllers should be connected to it, rather than to the PLIC.
Now that a binding exists for that mux, try to fix the misconceptions
in the GPIO controller binding.

Firstly, it's not possible for this controller to have fewer than 14
GPIOs, and thus 14 interrupts also. There are three controllers, with
14, 24 & 32 GPIOs each.

The example is wacky too - it follows from the incorrect understanding
that the GPIO controllers are connected to the PLIC directly. They are
not however, with a mux sitting in between. Update the example to use
the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
the example - rather than the strange looking, repeated <53>.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/gpio/microchip,mpfs-gpio.yaml    | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

Comments

Krzysztof Kozlowski July 24, 2024, 1:25 p.m. UTC | #1
On 23/07/2024 13:27, Conor Dooley wrote:
> The microchip,mpfs-gpio binding suffered greatly due to being written
> with a narrow minded view of the controller, and the interrupt bits
> ended up incorrect. It was mistakenly assumed that the interrupt
> configuration was set by platform firmware, based on the FPGA
> configuration, and that the GPIO DT nodes were the only way to really
> communicate interrupt configuration to software.
> 
> Instead, the mux should be a device in its own right, and the GPIO
> controllers should be connected to it, rather than to the PLIC.
> Now that a binding exists for that mux, try to fix the misconceptions
> in the GPIO controller binding.
> 
> Firstly, it's not possible for this controller to have fewer than 14
> GPIOs, and thus 14 interrupts also. There are three controllers, with
> 14, 24 & 32 GPIOs each.
> 
> The example is wacky too - it follows from the incorrect understanding
> that the GPIO controllers are connected to the PLIC directly. They are
> not however, with a mux sitting in between. Update the example to use
> the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
> the example - rather than the strange looking, repeated <53>.
> 

You make ngpios required, which could be an ABI break except that there
is no Linux user for this, so there is no ABI break, right? If so, would
be nice to mention it. Rest looks good:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Conor Dooley July 24, 2024, 2:29 p.m. UTC | #2
On Wed, Jul 24, 2024 at 03:25:38PM +0200, Krzysztof Kozlowski wrote:
> On 23/07/2024 13:27, Conor Dooley wrote:
> > The microchip,mpfs-gpio binding suffered greatly due to being written
> > with a narrow minded view of the controller, and the interrupt bits
> > ended up incorrect. It was mistakenly assumed that the interrupt
> > configuration was set by platform firmware, based on the FPGA
> > configuration, and that the GPIO DT nodes were the only way to really
> > communicate interrupt configuration to software.
> > 
> > Instead, the mux should be a device in its own right, and the GPIO
> > controllers should be connected to it, rather than to the PLIC.
> > Now that a binding exists for that mux, try to fix the misconceptions
> > in the GPIO controller binding.
> > 
> > Firstly, it's not possible for this controller to have fewer than 14
> > GPIOs, and thus 14 interrupts also. There are three controllers, with
> > 14, 24 & 32 GPIOs each.
> > 
> > The example is wacky too - it follows from the incorrect understanding
> > that the GPIO controllers are connected to the PLIC directly. They are
> > not however, with a mux sitting in between. Update the example to use
> > the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
> > the example - rather than the strange looking, repeated <53>.
> > 
> 
> You make ngpios required, which could be an ABI break except that there
> is no Linux user for this, so there is no ABI break, right? If so, would
> be nice to mention it. Rest looks good:

No upstream user at least, and I don't believe that there are any
non-linux projects using the GPIO controllers via DT. I could, I
suppose, not make it required and use 32 as a default - but that could
cause problems with existing devicetrees where all 3 controllers omitted
the property, despite having differing numbers of GPIOs.

Now that I look again, the driver actually doesn't enforce the presence
of the property and I think it should fail to probe if not present.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml b/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
index d61569b3f15b2..eb7dbf1668285 100644
--- a/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
@@ -22,7 +22,7 @@  properties:
   interrupts:
     description:
       Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
-    minItems: 1
+    minItems: 14
     maxItems: 32
 
   interrupt-controller: true
@@ -39,9 +39,7 @@  properties:
   ngpios:
     description:
       The number of GPIOs available.
-    minimum: 1
-    maximum: 32
-    default: 32
+    enum: [14, 24, 32]
 
   gpio-controller: true
   gpio-line-names: true
@@ -81,6 +79,7 @@  required:
   - reg
   - "#gpio-cells"
   - gpio-controller
+  - ngpios
   - clocks
 
 additionalProperties: false
@@ -91,18 +90,19 @@  examples:
         compatible = "microchip,mpfs-gpio";
         reg = <0x20122000 0x1000>;
         clocks = <&clkcfg 25>;
-        interrupt-parent = <&plic>;
+        interrupt-parent = <&irqmux>;
         gpio-controller;
         #gpio-cells = <2>;
+        ngpios = <32>;
         interrupt-controller;
-        #interrupt-cells = <1>;
-        interrupts = <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>;
+        #interrupt-cells = <2>;
+        interrupts = <64>, <65>, <66>, <67>,
+                     <68>, <69>, <70>, <71>,
+                     <72>, <73>, <74>, <75>,
+                     <76>, <77>, <78>, <79>,
+                     <80>, <81>, <82>, <83>,
+                     <84>, <85>, <86>, <87>,
+                     <88>, <89>, <90>, <91>,
+                     <92>, <93>, <94>, <95>;
     };
 ...