On Mon, Sep 30, 2024 at 04:00:57PM +0100, Conor Dooley wrote:

On Fri, Sep 27, 2024 at 10:13:54AM +0300, Andrei Stefanescu wrote:

quoted

Hi Conor,

Thank you very much for the prompt review!

On 26/09/2024 18:38, Conor Dooley wrote:

quoted

On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:

quoted

Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.

Signed-off-by: Phu Luu An <redacted>
Signed-off-by: Larisa Grigore <redacted>
Signed-off-by: Ghennadi Procopciuc <redacted>
Signed-off-by: Andrei Stefanescu <redacted>

What's up with this SoB chain? You're the author what did
the other 3 people do? Are they missing co-developed-by tags?

Yes, thank you for suggesting it! I will also add Co-developed-by tags
for them. In the end it should look like this:

Co-developed-by: Phu Luu An <redacted>
Signed-off-by: Phu Luu An <redacted>
Co-developed-by: Larisa Grigore <redacted>
Signed-off-by: Larisa Grigore <redacted>
Co-developed-by: Ghennadi Procopciuc <redacted>
Signed-off-by: Ghennadi Procopciuc <redacted>
Signed-off-by: Andrei Stefanescu <redacted>

quoted

quoted

+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gpio@4009d700 {
+        compatible = "nxp,s32g2-siul2-gpio";
+        reg = <0x4009d700 0x10>,
+              <0x44011700 0x18>,
+              <0x4009d740 0x10>,
+              <0x44011740 0x18>,
+              <0x44010010 0xb4>,
+              <0x44011078 0x80>;

Huh, I only noticed this now. Are you sure that this is a correct
representation of this device, and it is not really part of some syscon?
The "random" nature of the addresses  and the tiny sizes of the
reservations make it seem that way. What other devices are in these
regions?

Thanks for your answer to my second question, but I think you missed this
part here ^^^

Reading it again, I think you might have answered my first question,
though not explicitly. The regions in question do both pinctrl and gpio,
but you have chosen to represent it has lots of mini register regions,
rather than as a simple-mfd type device - which I think would be the
correct representation. .

Cheers,
Conor.

quoted

quoted

Additionally, it looks like "opads0" and "ipads0" are in a different
region to their "1" equivalents. Should this really be represented as
two disctint GPIO controllers?

I will add a bit more context regarding the hardware.

The hardware module which implements pinctrl & GPIO is called SIUL2.
For both S32G2 and S32G3 we have the same version of the module and 
it is integrated in the same way. Each SoC has two SIUL2 instances which
mostly have the same register types and only differ in the number
of pads associated to them:

- SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
- SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190

There are multiple registers for the SIUL2 modules which are important
for pinctrl & GPIO:

- MSCR (Multiplexed Signal Configuration Register)
  It configures the function of a pin and some
  pinconf properties:
    - input buffer
    - output buffer
    - open-drain
    - pull-up/pull-down
    - slew rate
  Function 0 means the pin is to be used as a GPIO.

- IMCR (Input Multiplexed Signal and Configuration Register)
  If the signal on this pad is to be read by another hardware
  module, this register is similar to a multiplexer, its value
  configures which pad the hardware will link to the module.
  As an example let's consider the I2C0 SDA line. It has one
  IMCR associated to it. Below are its possible pins and
  corresponding IMCR values:
    pin 16 <- 2
    pin 24 <- 7
    pin 31 <- 3
    pin 122 <- 4 
      (Note that MSCR122 is part of SIUL2_1 but the IMCR for
       I2C0_SDA is part of SIUL2_0)
    pin 153 <- 5
    pin 161 <- 6
  The IMCR values should be aligned with the function bits in the
  MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
  the function bits in MSCR122 and write the value 4 to the I2C0_SDA
  IMCR. 

- PGPDO/PGPDI Parallel GPIO Pad Data Out/In
  16 bit registers where each bit(besides some gaps) represents
  a GPIO's output/input value

- DISR0, DIRER0, IREER0, IFEER0
  These registers are used for: status, enable, rising/falling edge
  configuration for interrupts. We have 32 interrupts called EIRQ and
  each interrupt has one or more pads associated with it (controlled
  by an IMCR register per EIRQ).

  However, one important thing to note is that even though there are
  EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
  are only present in SIUL2_1.

Because of mixed pins (I2C0_SDA in the example above with the MSCR
in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
configuration registers in SIUL2_1 we decided to have a single
driver instance.

quoted

Cheers,
Conor.

Best regards,
Andrei