[PATCH v6 0/7] gpio: siul2-s32g2: add initial GPIO driver

[PATCH v6 0/7] gpio: siul2-s32g2: add initial GPIO driver

[Andrei Stefanescu]

This patch series adds support for basic GPIO
operations(set, get, direction_output/input, set_config).

There are two SIUL2 hardware modules: SIUL2_0 and SIUL2_1.
However, this driver exports both as a single GPIO driver.
This is because the interrupt registers are located only
in SIUL2_1, even for GPIOs that are part of SIUL2_0.

There are two gaps in the GPIO ranges:
- 102-111(inclusive) are invalid
- 123-143(inclusive) are invalid

These will be excluded via the `gpio-reserved-ranges`
property.

Writing and reading GPIO values is done via the PGPDO/PGPDI
registers(Parallel GPIO Pad Data Output/Input) which are
16 bit registers, each bit corresponding to a GPIO.

Note that the PGPDO order is similar to a big-endian grouping
of two registers:
PGPDO1, PGPDO0, PGPDO3, PGPDO2, PGPDO5, PGPDO4, gap, PGPDO6.

I have other patches for this driver:
- interrupt support
- power management callbacks

which I plan to upstream after this series gets merged
in order to simplify the review process.

v6 -> v5
- removed description for reg in the dt-bindings and added
  maxItems
- dropped label for example in the dt-bindings
- simplified the example in the dt-bindings
- changed dt-bindings filename to nxp,s32g2-siul2.yaml
- changed title in the dt-bindings
- dropped minItmes from gpio-ranges/gpio-reserved-ranges
  and added maxItems to gpio-reserved-ranges
- added required block for -grp[0-9]$ nodes
- switch to using "" as quotes
- kernel test robot: fixed frame sizes, added description
  for reg_name, fixed typo in gpio_configs_lock, removed
  uninitialized ret variable usage
- ordered includes in nxp-siul2.c, switched to dev-err-probe
  added a mention that other commits will add nvmem functionality
  to the mfd driver
- switched spin_lock_irqsave to scoped_guard statement
- switched dev_err to dev_err_probe in pinctrl-s32cc in places
  reached during the probing part

v5 -> v4
- fixed di_div error
- fixed dt-bindings error
- added Co-developed-by tags
- added new MFD driver nxp-siul2.c
- made the old pinctrl driver an MFD cell
- added the GPIO driver in the existing SIUL2 pinctrl one
- Switch from "devm_pinctrl_register" to
  "devm_pinctrl_register_and_init"

v4 -> v3
- removed useless parentheses
- added S32G3 fallback compatible
- fixed comment alignment
- fixed dt-bindings license
- fixed modpost: "__udivdi3"
- moved MAINTAINERS entry to have the new GPIO driver
  together with other files related to S32G

v3 -> v2
- fix dt-bindings schema id
- add maxItems to gpio-ranges
- removed gpio label from dt-bindings example
- added changelog for the MAINTAINERS commit and
  added separate entry for the SIUL2 GPIO driver
- added guard(raw_spinlock_irqsave) in
  'siul2_gpio_set_direction'
- updated the description for
  'devm_platform_get_and_ioremap_resource_byname'

v2 -> v1
dt-bindings:
- changed filename to match compatible
- fixed commit messages
- removed dt-bindings unnecessary properties descriptions
- added minItems for the interrupts property
driver:
- added depends on ARCH_S32 || COMPILE_TEST to Kconfig
- added select REGMAP_MMIO to Kconfig
- remove unnecessary include
- add of_node_put after `siul2_get_gpio_pinspec`
- removed inline from function definitions
- removed match data and moved the previous platdata
  definition to the top of the file to be visible
- replace bitmap_set/clear with __clear_bit/set_bit
  and devm_bitmap_zalloc with devm_kzalloc
- switched to gpiochip_generic_request/free/config
- fixed dev_err format for size_t reported by
  kernel test robot
- add platform_get_and_ioremap_resource_byname wrapper

Andrei Stefanescu (7):
  dt-bindings: mfd: add support for the NXP SIUL2 module
  mfd: nxp-siul2: add support for NXP SIUL2
  arm64: dts: s32g: make pinctrl part of mfd node
  pinctrl: s32: convert the driver into an mfd cell
  pinctrl: s32cc: change to "devm_pinctrl_register_and_init"
  pinctrl: s32cc: implement GPIO functionality
  MAINTAINERS: add MAINTAINER for NXP SIUL2 MFD driver

 .../bindings/mfd/nxp,s32g2-siul2.yaml         | 165 +++++
 MAINTAINERS                                   |   2 +
 arch/arm64/boot/dts/freescale/s32g2.dtsi      |  26 +-
 arch/arm64/boot/dts/freescale/s32g3.dtsi      |  26 +-
 drivers/mfd/Kconfig                           |  12 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/nxp-siul2.c                       | 410 +++++++++++++
 drivers/pinctrl/nxp/pinctrl-s32.h             |   3 +-
 drivers/pinctrl/nxp/pinctrl-s32cc.c           | 564 +++++++++++++-----
 drivers/pinctrl/nxp/pinctrl-s32g2.c           |  25 +-
 include/linux/mfd/nxp-siul2.h                 |  55 ++
 11 files changed, 1089 insertions(+), 200 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
 create mode 100644 drivers/mfd/nxp-siul2.c
 create mode 100644 include/linux/mfd/nxp-siul2.h

-- 
2.45.2

[PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Andrei Stefanescu]

Add the dt-bindings for the NXP SIUL2 module which is a multi
function device. It can export information about the SoC, configure
the pinmux&pinconf for pins and it is also a GPIO controller with
interrupt capability.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 .../bindings/mfd/nxp,s32g2-siul2.yaml         | 165 ++++++++++++++++++
 1 file changed, 165 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml

diff --git a/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml b/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
new file mode 100644
index 000000000000..a8edbea75bb6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
@@ -0,0 +1,165 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/nxp,s32g2-siul2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32 System Integration Unit Lite2 (SIUL2)
+
+maintainers:
+  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
+
+description: |
+  SIUL2 is a hardware block which implements pinmuxing,
+  pinconf, GPIOs (some with interrupt capability) and
+  registers which contain information about the SoC.
+  There are generally two SIUL2 modules whose functionality
+  is grouped together. For example interrupt configuration
+  registers are part of SIUL2_1 even though interrupts are
+  also available for SIUL2_0 pins.
+
+  The following register types are exported by SIUL2:
+    - MIDR (MCU ID Register) - information related to the SoC
+    - interrupt configuration registers
+    - MSCR (Multiplexed Signal Configuration Register) - pinmuxing and pinconf
+    - IMCR (Input Multiplexed Signal Configuration Register)- pinmuxing
+    - PGPDO (Parallel GPIO Pad Data Out Register) - GPIO output value
+    - PGPDI (Parallel GPIO Pad Data In Register) - GPIO input value
+
+  Most registers are 32bit wide with the exception of PGPDO/PGPDI which are
+  16bit wide.
+
+properties:
+  compatible:
+    enum:
+      - nxp,s32g2-siul2
+      - nxp,s32g3-siul2
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: siul20
+      - const: siul21
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges:
+    maxItems: 2
+
+  gpio-reserved-ranges:
+    maxItems: 2
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  nvmem-layout:
+    $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
+    description:
+      This container may reference an NVMEM layout parser.
+
+patternProperties:
+  "-hog(-[0-9]+)?$":
+    required:
+      - gpio-hog
+
+  "-pins$":
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      "-grp[0-9]$":
+        type: object
+        allOf:
+          - $ref: /schemas/pinctrl/pinmux-node.yaml#
+          - $ref: /schemas/pinctrl/pincfg-node.yaml#
+        description:
+          Pinctrl node's client devices specify pin muxes using subnodes,
+          which in turn use the standard properties below.
+
+        properties:
+          bias-disable: true
+          bias-high-impedance: true
+          bias-pull-up: true
+          bias-pull-down: true
+          drive-open-drain: true
+          input-enable: true
+          output-enable: true
+
+          pinmux:
+            description: |
+              An integer array for representing pinmux configurations of
+              a device. Each integer consists of a PIN_ID and a 4-bit
+              selected signal source(SSS) as IOMUX setting, which is
+              calculated as: pinmux = (PIN_ID << 4 | SSS)
+
+          slew-rate:
+            description: Supported slew rate based on Fmax values (MHz)
+            enum: [83, 133, 150, 166, 208]
+        required:
+          - pinmux
+
+        additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+  - gpio-reserved-ranges
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    siul2@4009c000 {
+      compatible = "nxp,s32g2-siul2";
+      reg = <0x4009c000 0x179c>,
+            <0x44010000 0x17b0>;
+      reg-names = "siul20", "siul21";
+      gpio-controller;
+      #gpio-cells = <2>;
+      gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
+      gpio-reserved-ranges = <102 10>, <123 21>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
+
+      jtag_pins: jtag-pins {
+        jtag-grp0 {
+          pinmux = <0x0>;
+          input-enable;
+          bias-pull-up;
+          slew-rate = <166>;
+        };
+      };
+
+      nvmem-layout {
+        compatible = "fixed-layout";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        soc-major@0 {
+          reg = <0 0x4>;
+        };
+      };
+    };
+...
-- 
2.45.2

Re: [PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Frank Li]

On Wed, Nov 13, 2024 at 12:10:53PM +0200, Andrei Stefanescu wrote:
> Add the dt-bindings for the NXP SIUL2 module which is a multi
> function device. It can export information about the SoC, configure
> the pinmux&pinconf for pins and it is also a GPIO controller with
> interrupt capability.
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> ---
>  .../bindings/mfd/nxp,s32g2-siul2.yaml         | 165 ++++++++++++++++++
>  1 file changed, 165 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml b/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
> new file mode 100644
> index 000000000000..a8edbea75bb6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/nxp,s32g2-siul2.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/nxp,s32g2-siul2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32 System Integration Unit Lite2 (SIUL2)
> +
> +maintainers:
> +  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> +
> +description: |
> +  SIUL2 is a hardware block which implements pinmuxing,
> +  pinconf, GPIOs (some with interrupt capability) and
> +  registers which contain information about the SoC.
> +  There are generally two SIUL2 modules whose functionality
> +  is grouped together. For example interrupt configuration
> +  registers are part of SIUL2_1 even though interrupts are
> +  also available for SIUL2_0 pins.
> +
> +  The following register types are exported by SIUL2:
> +    - MIDR (MCU ID Register) - information related to the SoC
> +    - interrupt configuration registers
> +    - MSCR (Multiplexed Signal Configuration Register) - pinmuxing and pinconf
> +    - IMCR (Input Multiplexed Signal Configuration Register)- pinmuxing
> +    - PGPDO (Parallel GPIO Pad Data Out Register) - GPIO output value
> +    - PGPDI (Parallel GPIO Pad Data In Register) - GPIO input value
> +
> +  Most registers are 32bit wide with the exception of PGPDO/PGPDI which are
> +  16bit wide.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g2-siul2
> +      - nxp,s32g3-siul2
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: siul20
> +      - const: siul21
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges:
> +    maxItems: 2
> +
> +  gpio-reserved-ranges:
> +    maxItems: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  nvmem-layout:
> +    $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
> +    description:
> +      This container may reference an NVMEM layout parser.
> +
> +patternProperties:
> +  "-hog(-[0-9]+)?$":
> +    required:
> +      - gpio-hog
> +
> +  "-pins$":
> +    type: object
> +    additionalProperties: false
> +
> +    patternProperties:
> +      "-grp[0-9]$":
> +        type: object
> +        allOf:
> +          - $ref: /schemas/pinctrl/pinmux-node.yaml#
> +          - $ref: /schemas/pinctrl/pincfg-node.yaml#
> +        description:
> +          Pinctrl node's client devices specify pin muxes using subnodes,
> +          which in turn use the standard properties below.
> +
> +        properties:
> +          bias-disable: true
> +          bias-high-impedance: true
> +          bias-pull-up: true
> +          bias-pull-down: true
> +          drive-open-drain: true
> +          input-enable: true
> +          output-enable: true
> +
> +          pinmux:
> +            description: |
> +              An integer array for representing pinmux configurations of
> +              a device. Each integer consists of a PIN_ID and a 4-bit
> +              selected signal source(SSS) as IOMUX setting, which is
> +              calculated as: pinmux = (PIN_ID << 4 | SSS)
> +
> +          slew-rate:
> +            description: Supported slew rate based on Fmax values (MHz)
> +            enum: [83, 133, 150, 166, 208]
> +        required:
> +          - pinmux
> +
> +        additionalProperties: false

There are $ref, need use  unevaluatedProperties: false

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +  - gpio-reserved-ranges
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    siul2@4009c000 {

generic node name, such as 'pinctrl'

> +      compatible = "nxp,s32g2-siul2";
> +      reg = <0x4009c000 0x179c>,
> +            <0x44010000 0x17b0>;
> +      reg-names = "siul20", "siul21";
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
> +      gpio-reserved-ranges = <102 10>, <123 21>;
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +      interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
> +
> +      jtag_pins: jtag-pins {

needn't lable 'jtag_pins' in example.

> +        jtag-grp0 {
> +          pinmux = <0x0>;
> +          input-enable;
> +          bias-pull-up;
> +          slew-rate = <166>;
> +        };
> +      };
> +
> +      nvmem-layout {
> +        compatible = "fixed-layout";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        soc-major@0 {
> +          reg = <0 0x4>;
> +        };
> +      };
> +    };
> +...
> --
> 2.45.2
>

Re: [PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Krzysztof Kozlowski]

On 13/11/2024 11:10, Andrei Stefanescu wrote:
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g2-siul2
> +      - nxp,s32g3-siul2

Not much improved. See other NXP bindings how they do this.

> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: siul20
> +      - const: siul21
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-ranges:
> +    maxItems: 2
> +
> +  gpio-reserved-ranges:
> +    maxItems: 2

That's odd to always require two reserved ranges. Does this mean all
devices have exactly the same reserved GPIOs? Then the driver should not
export them.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  nvmem-layout:
> +    $ref: /schemas/nvmem/layouts/nvmem-layout.yaml#
> +    description:
> +      This container may reference an NVMEM layout parser.
> +
> +patternProperties:
> +  "-hog(-[0-9]+)?$":
> +    required:
> +      - gpio-hog
> +
> +  "-pins$":
> +    type: object
> +    additionalProperties: false
> +
> +    patternProperties:
> +      "-grp[0-9]$":
> +        type: object
> +        allOf:
> +          - $ref: /schemas/pinctrl/pinmux-node.yaml#
> +          - $ref: /schemas/pinctrl/pincfg-node.yaml#
> +        description:
> +          Pinctrl node's client devices specify pin muxes using subnodes,
> +          which in turn use the standard properties below.
> +
> +        properties:
> +          bias-disable: true
> +          bias-high-impedance: true
> +          bias-pull-up: true
> +          bias-pull-down: true
> +          drive-open-drain: true
> +          input-enable: true
> +          output-enable: true
> +
> +          pinmux:
> +            description: |
> +              An integer array for representing pinmux configurations of
> +              a device. Each integer consists of a PIN_ID and a 4-bit
> +              selected signal source(SSS) as IOMUX setting, which is
> +              calculated as: pinmux = (PIN_ID << 4 | SSS)
> +
> +          slew-rate:
> +            description: Supported slew rate based on Fmax values (MHz)
> +            enum: [83, 133, 150, 166, 208]
> +        required:
> +          - pinmux
> +
> +        additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +  - gpio-reserved-ranges
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    siul2@4009c000 {

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>



Best regards,
Krzysztof

Re: [PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Andrei Stefanescu]

Hi Krzysztof,

Thank you for your review!

On 19/11/2024 11:21, Krzysztof Kozlowski wrote:
> On 13/11/2024 11:10, Andrei Stefanescu wrote:
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nxp,s32g2-siul2
>> +      - nxp,s32g3-siul2
> 
> Not much improved. See other NXP bindings how they do this.
> 

Do you mean to have the "nxp,s32g3-siul2" compatible fall back to the g2 one?

>> +
>> +  gpio-reserved-ranges:
>> +    maxItems: 2
> 
> That's odd to always require two reserved ranges. Does this mean all
> devices have exactly the same reserved GPIOs? Then the driver should not
> export them.

Yes, the driver exports GPIOs from two hardware modules because they are
tightly coupled. I export two gpio-ranges, each one corresponding to a
hardware module. If I were to export more gpio-ranges, thus avoiding
gpio-reserved-ranges, it would be hard to know to which hardware module
a gpio-range belongs. I would like to keep the current implementation
regarding this problem. Would that be ok?

> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>

Yes, sorry for this. I initially thought you were referring to the
label name. I now realize that I misread it. It will be changed
to pinctrl in the next version.
> 
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
Andrei

Re: [PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Krzysztof Kozlowski]

On Tue, Nov 19, 2024 at 11:44:23AM +0200, Andrei Stefanescu wrote:
> Hi Krzysztof,
> 
> Thank you for your review!
> 
> On 19/11/2024 11:21, Krzysztof Kozlowski wrote:
> > On 13/11/2024 11:10, Andrei Stefanescu wrote:
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - nxp,s32g2-siul2
> >> +      - nxp,s32g3-siul2
> > 
> > Not much improved. See other NXP bindings how they do this.
> > 
> 
> Do you mean to have the "nxp,s32g3-siul2" compatible fall back to the g2 one?

Yes, compatibility between devices means fallback.

> 
> >> +
> >> +  gpio-reserved-ranges:
> >> +    maxItems: 2
> > 
> > That's odd to always require two reserved ranges. Does this mean all
> > devices have exactly the same reserved GPIOs? Then the driver should not
> > export them.
> 
> Yes, the driver exports GPIOs from two hardware modules because they are
> tightly coupled. I export two gpio-ranges, each one corresponding to a
> hardware module. If I were to export more gpio-ranges, thus avoiding
> gpio-reserved-ranges, it would be hard to know to which hardware module
> a gpio-range belongs. I would like to keep the current implementation
> regarding this problem. Would that be ok?

I don't understand why this is needed then. If you always export same
set of GPIOs, why do you export something which is unusable/reserved?

Best regards,
Krzysztof

Re: [PATCH v6 1/7] dt-bindings: mfd: add support for the NXP SIUL2 module

[Andrei Stefanescu]

Hi Krzysztof,

>>>> +
>>>> +  gpio-reserved-ranges:
>>>> +    maxItems: 2

Would it be better if I removed the maxItems constraint? Looking at it now,
I think it should rather be minItems: 2 in order to allow the user to
further remove other GPIOs.

>>>
>>> That's odd to always require two reserved ranges. Does this mean all
>>> devices have exactly the same reserved GPIOs? Then the driver should not
>>> export them.
>>
>> Yes, the driver exports GPIOs from two hardware modules because they are
>> tightly coupled. I export two gpio-ranges, each one corresponding to a
>> hardware module. If I were to export more gpio-ranges, thus avoiding
>> gpio-reserved-ranges, it would be hard to know to which hardware module
>> a gpio-range belongs. I would like to keep the current implementation
>> regarding this problem. Would that be ok?
> 
> I don't understand why this is needed then. If you always export same
> set of GPIOs, why do you export something which is unusable/reserved?
>

I will detail a bit about SIUL2 GPIO ranges here:
SIUL2_0 exports GPIOs 0 - 101
SIUL2_1 exports GPIOs 112 - 122 and 144 - 190

Therefore, we have two gaps: 102 - 111 and 123 - 143. This applies for both
S32G2 and S32G3 SoCs.

AFAIK the only ways to exclude GPIOs from being exported are the following:
- split the gpio-ranges property so it doesn't include those GPIOs
  I thought about doing this but I think this would involve other vendor
  specific properties in order to know the memory region to use for each GPIO range.
  Currently, we have two GPIO ranges and each one maps to its respective
  SIUL2 module.
- using gpio-reserved-ranges which is my current approach because, in my opinion,
  it is the simplest approach.
- registering multiple gpio_chips
  I know this approach is used when dealing with multiple banks. However,
  I think GPIO banks would rather apply to SIUL2 modules and not to our
  valid GPIO ranges.

What are your thoughts about this?

Best regards,
Andrei

[PATCH v6 2/7] mfd: nxp-siul2: add support for NXP SIUL2

[Andrei Stefanescu]

SIUL2 (System Integration Unit Lite) is a hardware module which
implements various functionalities:
- reading SoC information
- pinctrl
- GPIO (including interrupts)

This commit only adds support for pinctrl&GPIO(one cell). Further
commits will add nvmem functionality(a second cell).

There are multiple register types in the SIUL2 module:
- MIDR (MCU ID Register)
	* contains information about the SoC.
- Interrupt related registers
	* There are 32 interrupts named EIRQ. An EIRQ
	  may be routed to one or more GPIOs. Not all
	  GPIOs have EIRQs associated with them
- MSCR (Multiplexed Signal Configuration Register)
	* handle pinmuxing and pinconf
- IMCR (Input Multiplexed Signal Configuration Register)
	* are part of pinmuxing
- PGPDO/PGPDI (Parallel GPIO Pad Data Out/In Register)
	* Write/Read the GPIO value

There are two SIUL2 modules in the S32G SoC. This driver handles
both because functionality is shared between them. For example:
some GPIOs in SIUL2_0 have interrupt capability but the registers
configuring this are in SIUL2_1.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/mfd/Kconfig           |  12 +
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/nxp-siul2.c       | 410 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/nxp-siul2.h |  55 +++++
 4 files changed, 478 insertions(+)
 create mode 100644 drivers/mfd/nxp-siul2.c
 create mode 100644 include/linux/mfd/nxp-siul2.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f9325bcce1b9..fc590789e8b3 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1098,6 +1098,18 @@ config MFD_NTXEC
 	  certain e-book readers designed by the original design manufacturer
 	  Netronix.
 
+config MFD_NXP_SIUL2
+	tristate "NXP SIUL2 MFD driver"
+	select MFD_CORE
+	select REGMAP_MMIO
+	depends on ARCH_S32 || COMPILE_TEST
+	help
+	  Select this to get support for the NXP SIUL2 (System Integration
+	  Unit Lite) module. This hardware block contains registers for
+	  SoC information, pinctrl and GPIO functionality. This will
+	  probe a MFD driver which will contain cells for a combined
+	  pinctrl&GPIO driver and nvmem drivers for the SoC information.
+
 config MFD_RETU
 	tristate "Nokia Retu and Tahvo multi-function device"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 2a9f91e81af8..7b19ea014221 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -226,6 +226,7 @@ obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
 obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
 obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
+obj-$(CONFIG_MFD_NXP_SIUL2) 	+= nxp-siul2.o
 obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
 obj-$(CONFIG_MFD_RK8XX)		+= rk8xx-core.o
 obj-$(CONFIG_MFD_RK8XX_I2C)	+= rk8xx-i2c.o
diff --git a/drivers/mfd/nxp-siul2.c b/drivers/mfd/nxp-siul2.c
new file mode 100644
index 000000000000..7751992e4df3
--- /dev/null
+++ b/drivers/mfd/nxp-siul2.c
@@ -0,0 +1,410 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * SIUL2(System Integration Unit Lite) MFD driver
+ *
+ * Copyright 2024 NXP
+ */
+#include <linux/init.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nxp-siul2.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#define S32G_NUM_SIUL2 2
+
+#define S32_REG_RANGE(start, end, name, access)		\
+	{						\
+		.reg_name = (name),			\
+		.reg_start_offset = (start),		\
+		.reg_end_offset = (end),		\
+		.reg_access = (access),			\
+		.valid = true,				\
+	}
+
+#define S32_INVALID_REG_RANGE		\
+	{				\
+		.reg_name = NULL,	\
+		.reg_access = NULL,	\
+		.valid = false,		\
+	}
+
+static const struct mfd_cell nxp_siul2_devs[] = {
+	{
+		.name = "s32g-siul2-pinctrl",
+	}
+};
+
+/**
+ * struct nxp_siul2_reg_range_info: a register range in SIUL2
+ * @reg_name: the name for the register range
+ * @reg_start_offset: the first valid register offset
+ * @reg_end_offset: the last valid register offset
+ * @reg_access: the read/write access tables if not NULL
+ * @valid: whether the register range is valid or not
+ */
+struct nxp_siul2_reg_range_info {
+	const char *reg_name;
+	unsigned int reg_start_offset;
+	unsigned int reg_end_offset;
+	const struct regmap_access_table *reg_access;
+	bool valid;
+};
+
+static const struct regmap_range s32g2_siul2_0_imcr_reg_ranges[] = {
+	/* IMCR0 - IMCR1 */
+	regmap_reg_range(0, 4),
+	/* IMCR3 - IMCR61 */
+	regmap_reg_range(0xC, 0xF4),
+	/* IMCR68 - IMCR83 */
+	regmap_reg_range(0x110, 0x14C)
+};
+
+static const struct regmap_access_table s32g2_siul2_0_imcr = {
+	.yes_ranges = s32g2_siul2_0_imcr_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_0_imcr_reg_ranges)
+};
+
+static const struct regmap_range s32g2_siul2_0_pgpd_reg_ranges[] = {
+	/* PGPD*0 - PGPD*5 */
+	regmap_reg_range(0, 0xA),
+	/* PGPD*6 - PGPD*6 */
+	regmap_reg_range(0xE, 0xE),
+};
+
+static const struct regmap_access_table s32g2_siul2_0_pgpd = {
+	.yes_ranges = s32g2_siul2_0_pgpd_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_0_pgpd_reg_ranges)
+};
+
+static const struct regmap_range s32g2_siul2_1_irq_reg_ranges[] = {
+	/* DISR0 */
+	regmap_reg_range(0x10, 0x10),
+	/* DIRER0 */
+	regmap_reg_range(0x18, 0x18),
+	/* DIRSR0 */
+	regmap_reg_range(0x20, 0x20),
+	/* IREER0 */
+	regmap_reg_range(0x28, 0x28),
+	/* IFEER0 */
+	regmap_reg_range(0x30, 0x30),
+};
+
+static const struct regmap_access_table s32g2_siul2_1_irq = {
+	.yes_ranges = s32g2_siul2_1_irq_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_irq_reg_ranges),
+};
+
+static const struct regmap_range s32g2_siul2_1_irq_volatile_reg_range[] = {
+	/* DISR0 */
+	regmap_reg_range(0x10, 0x10)
+};
+
+static const struct regmap_access_table s32g2_siul2_1_irq_volatile = {
+	.yes_ranges = s32g2_siul2_1_irq_volatile_reg_range,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_irq_volatile_reg_range),
+};
+
+static const struct regmap_range s32g2_siul2_1_mscr_reg_ranges[] = {
+	/* MSCR112 - MSCR122 */
+	regmap_reg_range(0, 0x28),
+	/* MSCR144 - MSCR190 */
+	regmap_reg_range(0x80, 0x138)
+};
+
+static const struct regmap_access_table s32g2_siul2_1_mscr = {
+	.yes_ranges = s32g2_siul2_1_mscr_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_mscr_reg_ranges),
+};
+
+static const struct regmap_range s32g2_siul2_1_imcr_reg_ranges[] = {
+	/* IMCR119 - IMCR121 */
+	regmap_reg_range(0, 8),
+	/* IMCR128 - IMCR129 */
+	regmap_reg_range(0x24, 0x28),
+	/* IMCR143 - IMCR151 */
+	regmap_reg_range(0x60, 0x80),
+	/* IMCR153 - IMCR161 */
+	regmap_reg_range(0x88, 0xA8),
+	/* IMCR205 - IMCR212 */
+	regmap_reg_range(0x158, 0x174),
+	/* IMCR224 - IMCR225 */
+	regmap_reg_range(0x1A4, 0x1A8),
+	/* IMCR233 - IMCR248 */
+	regmap_reg_range(0x1C8, 0x204),
+	/* IMCR273 - IMCR274 */
+	regmap_reg_range(0x268, 0x26C),
+	/* IMCR278 - IMCR281 */
+	regmap_reg_range(0x27C, 0x288),
+	/* IMCR283 - IMCR286 */
+	regmap_reg_range(0x290, 0x29C),
+	/* IMCR288 - IMCR294 */
+	regmap_reg_range(0x2A4, 0x2BC),
+	/* IMCR296 - IMCR302 */
+	regmap_reg_range(0x2C4, 0x2DC),
+	/* IMCR304 - IMCR310 */
+	regmap_reg_range(0x2E4, 0x2FC),
+	/* IMCR312 - IMCR314 */
+	regmap_reg_range(0x304, 0x30C),
+	/* IMCR316 */
+	regmap_reg_range(0x314, 0x314),
+	/* IMCR 318 */
+	regmap_reg_range(0x31C, 0x31C),
+	/* IMCR322 - IMCR340 */
+	regmap_reg_range(0x32C, 0x374),
+	/* IMCR343 - IMCR360 */
+	regmap_reg_range(0x380, 0x3C4),
+	/* IMCR363 - IMCR380 */
+	regmap_reg_range(0x3D0, 0x414),
+	/* IMCR383 - IMCR393 */
+	regmap_reg_range(0x420, 0x448),
+	/* IMCR398 - IMCR433 */
+	regmap_reg_range(0x45C, 0x4E8),
+	/* IMCR467 - IMCR470 */
+	regmap_reg_range(0x570, 0x57C),
+	/* IMCR473 - IMCR475 */
+	regmap_reg_range(0x588, 0x590),
+	/* IMCR478 - IMCR480*/
+	regmap_reg_range(0x59C, 0x5A4),
+	/* IMCR483 - IMCR485 */
+	regmap_reg_range(0x5B0, 0x5B8),
+	/* IMCR488 - IMCR490 */
+	regmap_reg_range(0x5C4, 0x5CC),
+	/* IMCR493 - IMCR495 */
+	regmap_reg_range(0x5D8, 0x5E0),
+};
+
+static const struct regmap_access_table s32g2_siul2_1_imcr = {
+	.yes_ranges = s32g2_siul2_1_imcr_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_imcr_reg_ranges)
+};
+
+static const struct regmap_range s32g2_siul2_1_pgpd_reg_ranges[] = {
+	/* PGPD*7 */
+	regmap_reg_range(0xC, 0xC),
+	/* PGPD*9 */
+	regmap_reg_range(0x10, 0x10),
+	/* PDPG*10 - PGPD*11 */
+	regmap_reg_range(0x14, 0x16),
+};
+
+static const struct regmap_access_table s32g2_siul2_1_pgpd = {
+	.yes_ranges = s32g2_siul2_1_pgpd_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_pgpd_reg_ranges)
+};
+
+static const struct nxp_siul2_reg_range_info
+s32g2_reg_ranges[S32G_NUM_SIUL2][SIUL2_NUM_REG_TYPES] = {
+	/* SIUL2_0 */
+	{
+		[SIUL2_MPIDR] = S32_REG_RANGE(4, 8, "SIUL2_0_MPIDR", NULL),
+		/* Interrupts are to be controlled from SIUL2_1 */
+		[SIUL2_IRQ] = S32_INVALID_REG_RANGE,
+		[SIUL2_MSCR] = S32_REG_RANGE(0x240, 0x3D4, "SIUL2_0_MSCR",
+					     NULL),
+		[SIUL2_IMCR] = S32_REG_RANGE(0xA40, 0xB8C, "SIUL2_0_IMCR",
+					     &s32g2_siul2_0_imcr),
+		[SIUL2_PGPDO] = S32_REG_RANGE(0x1700, 0x170E,
+					      "SIUL2_0_PGPDO",
+					      &s32g2_siul2_0_pgpd),
+		[SIUL2_PGPDI] = S32_REG_RANGE(0x1740, 0x174E,
+					      "SIUL2_0_PGPDI",
+					      &s32g2_siul2_0_pgpd),
+	},
+	/* SIUL2_1 */
+	{
+		[SIUL2_MPIDR] = S32_REG_RANGE(4, 8, "SIUL2_1_MPIDR", NULL),
+		[SIUL2_IRQ] = S32_REG_RANGE(0x10, 0xC0, "SIUL2_1_IRQ",
+					    &s32g2_siul2_1_irq),
+		[SIUL2_MSCR] = S32_REG_RANGE(0x400, 0x538, "SIUL2_1_MSCR",
+					     &s32g2_siul2_1_mscr),
+		[SIUL2_IMCR] = S32_REG_RANGE(0xC1C, 0x11FC, "SIUL2_1_IMCR",
+					     &s32g2_siul2_1_imcr),
+		[SIUL2_PGPDO] = S32_REG_RANGE(0x1700, 0x1716,
+					      "SIUL2_1_PGPDO",
+					      &s32g2_siul2_1_pgpd),
+		[SIUL2_PGPDI] = S32_REG_RANGE(0x1740, 0x1756,
+					      "SIUL2_1_PGPDI",
+					      &s32g2_siul2_1_pgpd),
+	},
+};
+
+static const struct regmap_config nxp_siul2_regmap_irq_conf = {
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.cache_type = REGCACHE_FLAT,
+	.use_raw_spinlock = true,
+	.volatile_table = &s32g2_siul2_1_irq_volatile,
+};
+
+static const struct regmap_config nxp_siul2_regmap_generic_conf = {
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.cache_type = REGCACHE_FLAT,
+	.use_raw_spinlock = true,
+};
+
+static const struct regmap_config nxp_siul2_regmap_pgpdo_conf = {
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.reg_bits = 32,
+	.reg_stride = 2,
+	.cache_type = REGCACHE_FLAT,
+	.use_raw_spinlock = true,
+};
+
+static const struct regmap_config nxp_siul2_regmap_pgpdi_conf = {
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.reg_bits = 32,
+	.reg_stride = 2,
+	.cache_type = REGCACHE_NONE,
+	.use_raw_spinlock = true,
+};
+
+static int nxp_siul2_init_regmap(struct platform_device *pdev,
+				 void __iomem *base, unsigned int siul)
+{
+	const struct regmap_config *regmap_configs[SIUL2_NUM_REG_TYPES] = {
+		[SIUL2_MPIDR]	= &nxp_siul2_regmap_generic_conf,
+		[SIUL2_IRQ]	= &nxp_siul2_regmap_irq_conf,
+		[SIUL2_MSCR]	= &nxp_siul2_regmap_generic_conf,
+		[SIUL2_IMCR]	= &nxp_siul2_regmap_generic_conf,
+		[SIUL2_PGPDO]	= &nxp_siul2_regmap_pgpdo_conf,
+		[SIUL2_PGPDI]	= &nxp_siul2_regmap_pgpdi_conf,
+	};
+	const struct nxp_siul2_reg_range_info *tmp_range;
+	struct regmap_config tmp_conf;
+	struct nxp_siul2_info *info;
+	struct nxp_siul2_mfd *priv;
+	void __iomem *reg_start;
+	int i;
+
+	priv = platform_get_drvdata(pdev);
+	info = &priv->siul2[siul];
+
+	for (i = 0; i < SIUL2_NUM_REG_TYPES; i++) {
+		if (!s32g2_reg_ranges[siul][i].valid)
+			continue;
+
+		tmp_range = &s32g2_reg_ranges[siul][i];
+		tmp_conf = *regmap_configs[i];
+		tmp_conf.name = tmp_range->reg_name;
+		tmp_conf.max_register =
+			tmp_range->reg_end_offset - tmp_range->reg_start_offset;
+
+		if (tmp_conf.cache_type != REGCACHE_NONE)
+			tmp_conf.num_reg_defaults_raw =
+				tmp_conf.max_register / tmp_conf.reg_stride;
+
+		if (tmp_range->reg_access) {
+			tmp_conf.wr_table = tmp_range->reg_access;
+			tmp_conf.rd_table = tmp_range->reg_access;
+		}
+
+		reg_start = base + tmp_range->reg_start_offset;
+		info->regmaps[i] = devm_regmap_init_mmio(&pdev->dev, reg_start,
+							 &tmp_conf);
+		if (IS_ERR(info->regmaps[i]))
+			return dev_err_probe(&pdev->dev,
+					     PTR_ERR(info->regmaps[i]),
+					     "regmap %d init failed: %ld\n", i,
+					     PTR_ERR(info->regmaps[i]));
+	}
+
+	return 0;
+}
+
+static int nxp_siul2_parse_dtb(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct of_phandle_args pinspec;
+	struct nxp_siul2_mfd *priv;
+	void __iomem *base;
+	char reg_name[16];
+	int i, ret;
+
+	priv = platform_get_drvdata(pdev);
+
+	for (i = 0; i < priv->num_siul2; i++) {
+		ret = snprintf(reg_name, ARRAY_SIZE(reg_name), "siul2%d", i);
+		if (ret < 0 || ret >= ARRAY_SIZE(reg_name))
+			return ret;
+
+		base = devm_platform_ioremap_resource_byname(pdev, reg_name);
+		if (IS_ERR(base))
+			return dev_err_probe(&pdev->dev, PTR_ERR(base),
+					     "Failed to get MEM resource: %s\n",
+					     reg_name);
+
+		ret = nxp_siul2_init_regmap(pdev, base, i);
+		if (ret)
+			return ret;
+
+		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+						       i, &pinspec);
+		if (ret)
+			return ret;
+
+		of_node_put(pinspec.np);
+
+		if (pinspec.args_count != 3)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+					     "Invalid pinspec count: %d\n",
+					     pinspec.args_count);
+
+		priv->siul2[i].gpio_base = pinspec.args[1];
+		priv->siul2[i].gpio_num = pinspec.args[2];
+	}
+
+	return 0;
+}
+
+static int nxp_siul2_probe(struct platform_device *pdev)
+{
+	struct nxp_siul2_mfd *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->num_siul2 = S32G_NUM_SIUL2;
+	priv->siul2 = devm_kcalloc(&pdev->dev, priv->num_siul2,
+				   sizeof(*priv->siul2), GFP_KERNEL);
+	if (!priv->siul2)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+	ret = nxp_siul2_parse_dtb(pdev);
+	if (ret)
+		return ret;
+
+	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
+				    nxp_siul2_devs, ARRAY_SIZE(nxp_siul2_devs),
+				    NULL, 0, NULL);
+}
+
+static const struct of_device_id nxp_siul2_dt_ids[] = {
+	{ .compatible = "nxp,s32g2-siul2" },
+	{ .compatible = "nxp,s32g3-siul2" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, nxp_siul2_dt_ids);
+
+static struct platform_driver nxp_siul2_mfd_driver = {
+	.driver = {
+		.name		= "nxp-siul2-mfd",
+		.of_match_table	= nxp_siul2_dt_ids,
+	},
+	.probe = nxp_siul2_probe,
+};
+
+module_platform_driver(nxp_siul2_mfd_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("NXP SIUL2 MFD driver");
+MODULE_AUTHOR("Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>");
diff --git a/include/linux/mfd/nxp-siul2.h b/include/linux/mfd/nxp-siul2.h
new file mode 100644
index 000000000000..238c812dba29
--- /dev/null
+++ b/include/linux/mfd/nxp-siul2.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * S32 SIUL2 core definitions
+ *
+ * Copyright 2024 NXP
+ */
+
+#ifndef __DRIVERS_MFD_NXP_SIUL2_H
+#define __DRIVERS_MFD_NXP_SIUL2_H
+
+#include <linux/regmap.h>
+
+/**
+ * enum nxp_siul2_reg_type - an enum for SIUL2 reg types
+ * @SIUL2_MPIDR - SoC info
+ * @SIUL2_IRQ - IRQ related registers, only valid in SIUL2_1
+ * @SIUL2_MSCR - used for pinmuxing and pinconf
+ * @SIUL2_IMCR - used for pinmuxing
+ * @SIUL2_PGPDO - writing the GPIO value
+ * @SIUL2_PGPDI - reading the GPIO value
+ */
+enum nxp_siul2_reg_type {
+	SIUL2_MPIDR,
+	SIUL2_IRQ,
+	SIUL2_MSCR,
+	SIUL2_IMCR,
+	SIUL2_PGPDO,
+	SIUL2_PGPDI,
+
+	SIUL2_NUM_REG_TYPES
+};
+
+/**
+ * struct nxp_siul2_info - details about one SIUL2 hardware instance
+ * @regmaps: the regmaps for each register type for a SIUL2 hardware instance
+ * @gpio_base: the first GPIO in this SIUL2 module
+ * @gpio_num: the number of GPIOs in this SIUL2 module
+ */
+struct nxp_siul2_info {
+	struct regmap *regmaps[SIUL2_NUM_REG_TYPES];
+	u32 gpio_base;
+	u32 gpio_num;
+};
+
+/**
+ * struct nxp_siul2_mfd - driver data
+ * @siul2: info about the SIUL2 modules present
+ * @num_siul2: number of siul2 modules
+ */
+struct nxp_siul2_mfd {
+	struct nxp_siul2_info *siul2;
+	u8 num_siul2;
+};
+
+#endif /* __DRIVERS_MFD_NXP_SIUL2_H */
-- 
2.45.2

Re: [PATCH v6 2/7] mfd: nxp-siul2: add support for NXP SIUL2

[Lee Jones]

Mark,

Seeing as the vast majority of this 400 line driver pertains to Regmap
handling (!), would you be kind enough to cast your expert eye over it
please?

On Wed, 13 Nov 2024, Andrei Stefanescu wrote:

> SIUL2 (System Integration Unit Lite) is a hardware module which
> implements various functionalities:
> - reading SoC information
> - pinctrl
> - GPIO (including interrupts)
> 
> This commit only adds support for pinctrl&GPIO(one cell). Further
> commits will add nvmem functionality(a second cell).

It's not an MFD until it has more than one device.

Please add that now or it cannot be accepted.

[pausing my review here for the time being]

> There are multiple register types in the SIUL2 module:
> - MIDR (MCU ID Register)
> 	* contains information about the SoC.
> - Interrupt related registers
> 	* There are 32 interrupts named EIRQ. An EIRQ
> 	  may be routed to one or more GPIOs. Not all
> 	  GPIOs have EIRQs associated with them
> - MSCR (Multiplexed Signal Configuration Register)
> 	* handle pinmuxing and pinconf
> - IMCR (Input Multiplexed Signal Configuration Register)
> 	* are part of pinmuxing
> - PGPDO/PGPDI (Parallel GPIO Pad Data Out/In Register)
> 	* Write/Read the GPIO value
> 
> There are two SIUL2 modules in the S32G SoC. This driver handles
> both because functionality is shared between them. For example:
> some GPIOs in SIUL2_0 have interrupt capability but the registers
> configuring this are in SIUL2_1.
> 
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> ---
>  drivers/mfd/Kconfig           |  12 +
>  drivers/mfd/Makefile          |   1 +
>  drivers/mfd/nxp-siul2.c       | 410 ++++++++++++++++++++++++++++++++++
>  include/linux/mfd/nxp-siul2.h |  55 +++++
>  4 files changed, 478 insertions(+)
>  create mode 100644 drivers/mfd/nxp-siul2.c
>  create mode 100644 include/linux/mfd/nxp-siul2.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f9325bcce1b9..fc590789e8b3 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1098,6 +1098,18 @@ config MFD_NTXEC
>  	  certain e-book readers designed by the original design manufacturer
>  	  Netronix.
>  
> +config MFD_NXP_SIUL2
> +	tristate "NXP SIUL2 MFD driver"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	depends on ARCH_S32 || COMPILE_TEST
> +	help
> +	  Select this to get support for the NXP SIUL2 (System Integration
> +	  Unit Lite) module. This hardware block contains registers for
> +	  SoC information, pinctrl and GPIO functionality. This will
> +	  probe a MFD driver which will contain cells for a combined
> +	  pinctrl&GPIO driver and nvmem drivers for the SoC information.
> +
>  config MFD_RETU
>  	tristate "Nokia Retu and Tahvo multi-function device"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2a9f91e81af8..7b19ea014221 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -226,6 +226,7 @@ obj-$(CONFIG_MFD_INTEL_PMC_BXT)	+= intel_pmc_bxt.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
>  obj-$(CONFIG_MFD_VIPERBOARD)    += viperboard.o
>  obj-$(CONFIG_MFD_NTXEC)		+= ntxec.o
> +obj-$(CONFIG_MFD_NXP_SIUL2) 	+= nxp-siul2.o
>  obj-$(CONFIG_MFD_RC5T583)	+= rc5t583.o rc5t583-irq.o
>  obj-$(CONFIG_MFD_RK8XX)		+= rk8xx-core.o
>  obj-$(CONFIG_MFD_RK8XX_I2C)	+= rk8xx-i2c.o
> diff --git a/drivers/mfd/nxp-siul2.c b/drivers/mfd/nxp-siul2.c
> new file mode 100644
> index 000000000000..7751992e4df3
> --- /dev/null
> +++ b/drivers/mfd/nxp-siul2.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * SIUL2(System Integration Unit Lite) MFD driver
> + *
> + * Copyright 2024 NXP
> + */
> +#include <linux/init.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/nxp-siul2.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#define S32G_NUM_SIUL2 2
> +
> +#define S32_REG_RANGE(start, end, name, access)		\
> +	{						\
> +		.reg_name = (name),			\
> +		.reg_start_offset = (start),		\
> +		.reg_end_offset = (end),		\
> +		.reg_access = (access),			\
> +		.valid = true,				\
> +	}
> +
> +#define S32_INVALID_REG_RANGE		\
> +	{				\
> +		.reg_name = NULL,	\
> +		.reg_access = NULL,	\
> +		.valid = false,		\
> +	}
> +
> +static const struct mfd_cell nxp_siul2_devs[] = {
> +	{
> +		.name = "s32g-siul2-pinctrl",
> +	}
> +};
> +
> +/**
> + * struct nxp_siul2_reg_range_info: a register range in SIUL2
> + * @reg_name: the name for the register range
> + * @reg_start_offset: the first valid register offset
> + * @reg_end_offset: the last valid register offset
> + * @reg_access: the read/write access tables if not NULL
> + * @valid: whether the register range is valid or not
> + */
> +struct nxp_siul2_reg_range_info {
> +	const char *reg_name;
> +	unsigned int reg_start_offset;
> +	unsigned int reg_end_offset;
> +	const struct regmap_access_table *reg_access;
> +	bool valid;
> +};
> +
> +static const struct regmap_range s32g2_siul2_0_imcr_reg_ranges[] = {
> +	/* IMCR0 - IMCR1 */
> +	regmap_reg_range(0, 4),
> +	/* IMCR3 - IMCR61 */
> +	regmap_reg_range(0xC, 0xF4),
> +	/* IMCR68 - IMCR83 */
> +	regmap_reg_range(0x110, 0x14C)
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_0_imcr = {
> +	.yes_ranges = s32g2_siul2_0_imcr_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_0_imcr_reg_ranges)
> +};
> +
> +static const struct regmap_range s32g2_siul2_0_pgpd_reg_ranges[] = {
> +	/* PGPD*0 - PGPD*5 */
> +	regmap_reg_range(0, 0xA),
> +	/* PGPD*6 - PGPD*6 */
> +	regmap_reg_range(0xE, 0xE),
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_0_pgpd = {
> +	.yes_ranges = s32g2_siul2_0_pgpd_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_0_pgpd_reg_ranges)
> +};
> +
> +static const struct regmap_range s32g2_siul2_1_irq_reg_ranges[] = {
> +	/* DISR0 */
> +	regmap_reg_range(0x10, 0x10),
> +	/* DIRER0 */
> +	regmap_reg_range(0x18, 0x18),
> +	/* DIRSR0 */
> +	regmap_reg_range(0x20, 0x20),
> +	/* IREER0 */
> +	regmap_reg_range(0x28, 0x28),
> +	/* IFEER0 */
> +	regmap_reg_range(0x30, 0x30),
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_1_irq = {
> +	.yes_ranges = s32g2_siul2_1_irq_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_irq_reg_ranges),
> +};
> +
> +static const struct regmap_range s32g2_siul2_1_irq_volatile_reg_range[] = {
> +	/* DISR0 */
> +	regmap_reg_range(0x10, 0x10)
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_1_irq_volatile = {
> +	.yes_ranges = s32g2_siul2_1_irq_volatile_reg_range,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_irq_volatile_reg_range),
> +};
> +
> +static const struct regmap_range s32g2_siul2_1_mscr_reg_ranges[] = {
> +	/* MSCR112 - MSCR122 */
> +	regmap_reg_range(0, 0x28),
> +	/* MSCR144 - MSCR190 */
> +	regmap_reg_range(0x80, 0x138)
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_1_mscr = {
> +	.yes_ranges = s32g2_siul2_1_mscr_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_mscr_reg_ranges),
> +};
> +
> +static const struct regmap_range s32g2_siul2_1_imcr_reg_ranges[] = {
> +	/* IMCR119 - IMCR121 */
> +	regmap_reg_range(0, 8),
> +	/* IMCR128 - IMCR129 */
> +	regmap_reg_range(0x24, 0x28),
> +	/* IMCR143 - IMCR151 */
> +	regmap_reg_range(0x60, 0x80),
> +	/* IMCR153 - IMCR161 */
> +	regmap_reg_range(0x88, 0xA8),
> +	/* IMCR205 - IMCR212 */
> +	regmap_reg_range(0x158, 0x174),
> +	/* IMCR224 - IMCR225 */
> +	regmap_reg_range(0x1A4, 0x1A8),
> +	/* IMCR233 - IMCR248 */
> +	regmap_reg_range(0x1C8, 0x204),
> +	/* IMCR273 - IMCR274 */
> +	regmap_reg_range(0x268, 0x26C),
> +	/* IMCR278 - IMCR281 */
> +	regmap_reg_range(0x27C, 0x288),
> +	/* IMCR283 - IMCR286 */
> +	regmap_reg_range(0x290, 0x29C),
> +	/* IMCR288 - IMCR294 */
> +	regmap_reg_range(0x2A4, 0x2BC),
> +	/* IMCR296 - IMCR302 */
> +	regmap_reg_range(0x2C4, 0x2DC),
> +	/* IMCR304 - IMCR310 */
> +	regmap_reg_range(0x2E4, 0x2FC),
> +	/* IMCR312 - IMCR314 */
> +	regmap_reg_range(0x304, 0x30C),
> +	/* IMCR316 */
> +	regmap_reg_range(0x314, 0x314),
> +	/* IMCR 318 */
> +	regmap_reg_range(0x31C, 0x31C),
> +	/* IMCR322 - IMCR340 */
> +	regmap_reg_range(0x32C, 0x374),
> +	/* IMCR343 - IMCR360 */
> +	regmap_reg_range(0x380, 0x3C4),
> +	/* IMCR363 - IMCR380 */
> +	regmap_reg_range(0x3D0, 0x414),
> +	/* IMCR383 - IMCR393 */
> +	regmap_reg_range(0x420, 0x448),
> +	/* IMCR398 - IMCR433 */
> +	regmap_reg_range(0x45C, 0x4E8),
> +	/* IMCR467 - IMCR470 */
> +	regmap_reg_range(0x570, 0x57C),
> +	/* IMCR473 - IMCR475 */
> +	regmap_reg_range(0x588, 0x590),
> +	/* IMCR478 - IMCR480*/
> +	regmap_reg_range(0x59C, 0x5A4),
> +	/* IMCR483 - IMCR485 */
> +	regmap_reg_range(0x5B0, 0x5B8),
> +	/* IMCR488 - IMCR490 */
> +	regmap_reg_range(0x5C4, 0x5CC),
> +	/* IMCR493 - IMCR495 */
> +	regmap_reg_range(0x5D8, 0x5E0),
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_1_imcr = {
> +	.yes_ranges = s32g2_siul2_1_imcr_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_imcr_reg_ranges)
> +};
> +
> +static const struct regmap_range s32g2_siul2_1_pgpd_reg_ranges[] = {
> +	/* PGPD*7 */
> +	regmap_reg_range(0xC, 0xC),
> +	/* PGPD*9 */
> +	regmap_reg_range(0x10, 0x10),
> +	/* PDPG*10 - PGPD*11 */
> +	regmap_reg_range(0x14, 0x16),
> +};
> +
> +static const struct regmap_access_table s32g2_siul2_1_pgpd = {
> +	.yes_ranges = s32g2_siul2_1_pgpd_reg_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(s32g2_siul2_1_pgpd_reg_ranges)
> +};
> +
> +static const struct nxp_siul2_reg_range_info
> +s32g2_reg_ranges[S32G_NUM_SIUL2][SIUL2_NUM_REG_TYPES] = {
> +	/* SIUL2_0 */
> +	{
> +		[SIUL2_MPIDR] = S32_REG_RANGE(4, 8, "SIUL2_0_MPIDR", NULL),
> +		/* Interrupts are to be controlled from SIUL2_1 */
> +		[SIUL2_IRQ] = S32_INVALID_REG_RANGE,
> +		[SIUL2_MSCR] = S32_REG_RANGE(0x240, 0x3D4, "SIUL2_0_MSCR",
> +					     NULL),
> +		[SIUL2_IMCR] = S32_REG_RANGE(0xA40, 0xB8C, "SIUL2_0_IMCR",
> +					     &s32g2_siul2_0_imcr),
> +		[SIUL2_PGPDO] = S32_REG_RANGE(0x1700, 0x170E,
> +					      "SIUL2_0_PGPDO",
> +					      &s32g2_siul2_0_pgpd),
> +		[SIUL2_PGPDI] = S32_REG_RANGE(0x1740, 0x174E,
> +					      "SIUL2_0_PGPDI",
> +					      &s32g2_siul2_0_pgpd),
> +	},
> +	/* SIUL2_1 */
> +	{
> +		[SIUL2_MPIDR] = S32_REG_RANGE(4, 8, "SIUL2_1_MPIDR", NULL),
> +		[SIUL2_IRQ] = S32_REG_RANGE(0x10, 0xC0, "SIUL2_1_IRQ",
> +					    &s32g2_siul2_1_irq),
> +		[SIUL2_MSCR] = S32_REG_RANGE(0x400, 0x538, "SIUL2_1_MSCR",
> +					     &s32g2_siul2_1_mscr),
> +		[SIUL2_IMCR] = S32_REG_RANGE(0xC1C, 0x11FC, "SIUL2_1_IMCR",
> +					     &s32g2_siul2_1_imcr),
> +		[SIUL2_PGPDO] = S32_REG_RANGE(0x1700, 0x1716,
> +					      "SIUL2_1_PGPDO",
> +					      &s32g2_siul2_1_pgpd),
> +		[SIUL2_PGPDI] = S32_REG_RANGE(0x1740, 0x1756,
> +					      "SIUL2_1_PGPDI",
> +					      &s32g2_siul2_1_pgpd),
> +	},
> +};
> +
> +static const struct regmap_config nxp_siul2_regmap_irq_conf = {
> +	.val_bits = 32,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.cache_type = REGCACHE_FLAT,
> +	.use_raw_spinlock = true,
> +	.volatile_table = &s32g2_siul2_1_irq_volatile,
> +};
> +
> +static const struct regmap_config nxp_siul2_regmap_generic_conf = {
> +	.val_bits = 32,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.cache_type = REGCACHE_FLAT,
> +	.use_raw_spinlock = true,
> +};
> +
> +static const struct regmap_config nxp_siul2_regmap_pgpdo_conf = {
> +	.val_bits = 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.reg_bits = 32,
> +	.reg_stride = 2,
> +	.cache_type = REGCACHE_FLAT,
> +	.use_raw_spinlock = true,
> +};
> +
> +static const struct regmap_config nxp_siul2_regmap_pgpdi_conf = {
> +	.val_bits = 16,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.reg_bits = 32,
> +	.reg_stride = 2,
> +	.cache_type = REGCACHE_NONE,
> +	.use_raw_spinlock = true,
> +};
> +
> +static int nxp_siul2_init_regmap(struct platform_device *pdev,
> +				 void __iomem *base, unsigned int siul)
> +{
> +	const struct regmap_config *regmap_configs[SIUL2_NUM_REG_TYPES] = {
> +		[SIUL2_MPIDR]	= &nxp_siul2_regmap_generic_conf,
> +		[SIUL2_IRQ]	= &nxp_siul2_regmap_irq_conf,
> +		[SIUL2_MSCR]	= &nxp_siul2_regmap_generic_conf,
> +		[SIUL2_IMCR]	= &nxp_siul2_regmap_generic_conf,
> +		[SIUL2_PGPDO]	= &nxp_siul2_regmap_pgpdo_conf,
> +		[SIUL2_PGPDI]	= &nxp_siul2_regmap_pgpdi_conf,
> +	};
> +	const struct nxp_siul2_reg_range_info *tmp_range;
> +	struct regmap_config tmp_conf;
> +	struct nxp_siul2_info *info;
> +	struct nxp_siul2_mfd *priv;
> +	void __iomem *reg_start;
> +	int i;
> +
> +	priv = platform_get_drvdata(pdev);
> +	info = &priv->siul2[siul];
> +
> +	for (i = 0; i < SIUL2_NUM_REG_TYPES; i++) {
> +		if (!s32g2_reg_ranges[siul][i].valid)
> +			continue;
> +
> +		tmp_range = &s32g2_reg_ranges[siul][i];
> +		tmp_conf = *regmap_configs[i];
> +		tmp_conf.name = tmp_range->reg_name;
> +		tmp_conf.max_register =
> +			tmp_range->reg_end_offset - tmp_range->reg_start_offset;
> +
> +		if (tmp_conf.cache_type != REGCACHE_NONE)
> +			tmp_conf.num_reg_defaults_raw =
> +				tmp_conf.max_register / tmp_conf.reg_stride;
> +
> +		if (tmp_range->reg_access) {
> +			tmp_conf.wr_table = tmp_range->reg_access;
> +			tmp_conf.rd_table = tmp_range->reg_access;
> +		}
> +
> +		reg_start = base + tmp_range->reg_start_offset;
> +		info->regmaps[i] = devm_regmap_init_mmio(&pdev->dev, reg_start,
> +							 &tmp_conf);
> +		if (IS_ERR(info->regmaps[i]))
> +			return dev_err_probe(&pdev->dev,
> +					     PTR_ERR(info->regmaps[i]),
> +					     "regmap %d init failed: %ld\n", i,
> +					     PTR_ERR(info->regmaps[i]));
> +	}
> +
> +	return 0;
> +}
> +
> +static int nxp_siul2_parse_dtb(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct of_phandle_args pinspec;
> +	struct nxp_siul2_mfd *priv;
> +	void __iomem *base;
> +	char reg_name[16];
> +	int i, ret;
> +
> +	priv = platform_get_drvdata(pdev);
> +
> +	for (i = 0; i < priv->num_siul2; i++) {
> +		ret = snprintf(reg_name, ARRAY_SIZE(reg_name), "siul2%d", i);
> +		if (ret < 0 || ret >= ARRAY_SIZE(reg_name))
> +			return ret;
> +
> +		base = devm_platform_ioremap_resource_byname(pdev, reg_name);
> +		if (IS_ERR(base))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(base),
> +					     "Failed to get MEM resource: %s\n",
> +					     reg_name);
> +
> +		ret = nxp_siul2_init_regmap(pdev, base, i);
> +		if (ret)
> +			return ret;
> +
> +		ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +						       i, &pinspec);
> +		if (ret)
> +			return ret;
> +
> +		of_node_put(pinspec.np);
> +
> +		if (pinspec.args_count != 3)
> +			return dev_err_probe(&pdev->dev, -EINVAL,
> +					     "Invalid pinspec count: %d\n",
> +					     pinspec.args_count);
> +
> +		priv->siul2[i].gpio_base = pinspec.args[1];
> +		priv->siul2[i].gpio_num = pinspec.args[2];
> +	}
> +
> +	return 0;
> +}
> +
> +static int nxp_siul2_probe(struct platform_device *pdev)
> +{
> +	struct nxp_siul2_mfd *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->num_siul2 = S32G_NUM_SIUL2;
> +	priv->siul2 = devm_kcalloc(&pdev->dev, priv->num_siul2,
> +				   sizeof(*priv->siul2), GFP_KERNEL);
> +	if (!priv->siul2)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +	ret = nxp_siul2_parse_dtb(pdev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> +				    nxp_siul2_devs, ARRAY_SIZE(nxp_siul2_devs),
> +				    NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id nxp_siul2_dt_ids[] = {
> +	{ .compatible = "nxp,s32g2-siul2" },
> +	{ .compatible = "nxp,s32g3-siul2" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, nxp_siul2_dt_ids);
> +
> +static struct platform_driver nxp_siul2_mfd_driver = {
> +	.driver = {
> +		.name		= "nxp-siul2-mfd",
> +		.of_match_table	= nxp_siul2_dt_ids,
> +	},
> +	.probe = nxp_siul2_probe,
> +};
> +
> +module_platform_driver(nxp_siul2_mfd_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("NXP SIUL2 MFD driver");
> +MODULE_AUTHOR("Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>");
> diff --git a/include/linux/mfd/nxp-siul2.h b/include/linux/mfd/nxp-siul2.h
> new file mode 100644
> index 000000000000..238c812dba29
> --- /dev/null
> +++ b/include/linux/mfd/nxp-siul2.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * S32 SIUL2 core definitions
> + *
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __DRIVERS_MFD_NXP_SIUL2_H
> +#define __DRIVERS_MFD_NXP_SIUL2_H
> +
> +#include <linux/regmap.h>
> +
> +/**
> + * enum nxp_siul2_reg_type - an enum for SIUL2 reg types
> + * @SIUL2_MPIDR - SoC info
> + * @SIUL2_IRQ - IRQ related registers, only valid in SIUL2_1
> + * @SIUL2_MSCR - used for pinmuxing and pinconf
> + * @SIUL2_IMCR - used for pinmuxing
> + * @SIUL2_PGPDO - writing the GPIO value
> + * @SIUL2_PGPDI - reading the GPIO value
> + */
> +enum nxp_siul2_reg_type {
> +	SIUL2_MPIDR,
> +	SIUL2_IRQ,
> +	SIUL2_MSCR,
> +	SIUL2_IMCR,
> +	SIUL2_PGPDO,
> +	SIUL2_PGPDI,
> +
> +	SIUL2_NUM_REG_TYPES
> +};
> +
> +/**
> + * struct nxp_siul2_info - details about one SIUL2 hardware instance
> + * @regmaps: the regmaps for each register type for a SIUL2 hardware instance
> + * @gpio_base: the first GPIO in this SIUL2 module
> + * @gpio_num: the number of GPIOs in this SIUL2 module
> + */
> +struct nxp_siul2_info {
> +	struct regmap *regmaps[SIUL2_NUM_REG_TYPES];
> +	u32 gpio_base;
> +	u32 gpio_num;
> +};
> +
> +/**
> + * struct nxp_siul2_mfd - driver data
> + * @siul2: info about the SIUL2 modules present
> + * @num_siul2: number of siul2 modules
> + */
> +struct nxp_siul2_mfd {
> +	struct nxp_siul2_info *siul2;
> +	u8 num_siul2;
> +};
> +
> +#endif /* __DRIVERS_MFD_NXP_SIUL2_H */
> -- 
> 2.45.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v6 2/7] mfd: nxp-siul2: add support for NXP SIUL2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 327 bytes --]

On Wed, Dec 11, 2024 at 12:45:56PM +0000, Lee Jones wrote:

> Seeing as the vast majority of this 400 line driver pertains to Regmap
> handling (!), would you be kind enough to cast your expert eye over it
> please?

Is there something specific you're concerned about there?  It looks like
it's just data which should be fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/7] mfd: nxp-siul2: add support for NXP SIUL2

[Lee Jones]

On Wed, 11 Dec 2024, Mark Brown wrote:

> On Wed, Dec 11, 2024 at 12:45:56PM +0000, Lee Jones wrote:
> 
> > Seeing as the vast majority of this 400 line driver pertains to Regmap
> > handling (!), would you be kind enough to cast your expert eye over it
> > please?
> 
> Is there something specific you're concerned about there?  It looks like
> it's just data which should be fine.

Just the mass of complex hoop-jumping to get all of these Regmaps
registered.  But if nothing stands out to you, then it's probably okay.

Still, I can't review / take this until it's a proper MFD.

-- 
Lee Jones [李琼斯]

Re: [PATCH v6 2/7] mfd: nxp-siul2: add support for NXP SIUL2

[Andrei Stefanescu]

Hi Mark, Lee,

Thank you for your review!

On 13/12/2024 19:15, Lee Jones wrote:
> On Wed, 11 Dec 2024, Mark Brown wrote:
> 
>> On Wed, Dec 11, 2024 at 12:45:56PM +0000, Lee Jones wrote:
>>
>>> Seeing as the vast majority of this 400 line driver pertains to Regmap
>>> handling (!), would you be kind enough to cast your expert eye over it
>>> please?
>>
>> Is there something specific you're concerned about there?  It looks like
>> it's just data which should be fine.
> 
> Just the mass of complex hoop-jumping to get all of these Regmaps
> registered.  But if nothing stands out to you, then it's probably okay.
> 
> Still, I can't review / take this until it's a proper MFD.
> 

I will add the SIUL2 NVMEM driver to this series in v7 then.

Best regards,
Andrei

[PATCH v6 3/7] arm64: dts: s32g: make pinctrl part of mfd node

[Andrei Stefanescu]

SIUL2 is now represented as an mfd device. Therefore, the old
pinctrl node is deprecated. Move the pinctrl related properties
inside the new "nxp-siul2" node. The latter one is now used
to represent the mfd device.

This change came as a result of upstream review in the following series:
https://lore.kernel.org/linux-gpio/a924bbb6-96ec-40be-9d82-a76b2ab73afd@oss.nxp.com/
https://lore.kernel.org/all/20240926143122.1385658-3-andrei.stefanescu@oss.nxp.com/

The SIUL2 module has multiple capabilities. It has support for reading
SoC information, pinctrl and GPIO. All of this functionality is part of
the same register space. The initial pinctrl driver treated the pinctrl
functionality as separate from the GPIO one. However, they do rely on
common registers and a long, detailed and specific register range list
would be required for pinctrl&GPIO (carving out the necessary memory
for each function). Moreover, in some cases this wouldn't be enough. For
example reading a GPIO's direction would require a read of the MSCR
register corresponding to that pin. This would not be possible in the
GPIO driver because all of the MSCR registers are referenced by the
pinctrl driver.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 arch/arm64/boot/dts/freescale/s32g2.dtsi | 26 +++++++++++-------------
 arch/arm64/boot/dts/freescale/s32g3.dtsi | 26 +++++++++++-------------
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/s32g2.dtsi b/arch/arm64/boot/dts/freescale/s32g2.dtsi
index fa054bfe7d5c..e14ce5503e1f 100644
--- a/arch/arm64/boot/dts/freescale/s32g2.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32g2.dtsi
@@ -114,20 +114,18 @@ soc@0 {
 		#size-cells = <1>;
 		ranges = <0 0 0 0x80000000>;
 
-		pinctrl: pinctrl@4009c240 {
-			compatible = "nxp,s32g2-siul2-pinctrl";
-				/* MSCR0-MSCR101 registers on siul2_0 */
-			reg = <0x4009c240 0x198>,
-				/* MSCR112-MSCR122 registers on siul2_1 */
-			      <0x44010400 0x2c>,
-				/* MSCR144-MSCR190 registers on siul2_1 */
-			      <0x44010480 0xbc>,
-				/* IMCR0-IMCR83 registers on siul2_0 */
-			      <0x4009ca40 0x150>,
-				/* IMCR119-IMCR397 registers on siul2_1 */
-			      <0x44010c1c 0x45c>,
-				/* IMCR430-IMCR495 registers on siul2_1 */
-			      <0x440110f8 0x108>;
+		siul2: siul2@4009c000 {
+			compatible = "nxp,s32g2-siul2";
+			reg = <0x4009c000 0x179c>,
+			      <0x44010000 0x17b0>;
+			reg-names = "siul20", "siul21";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
+			gpio-reserved-ranges = <102 10>, <123 21>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
 
 			jtag_pins: jtag-pins {
 				jtag-grp0 {
diff --git a/arch/arm64/boot/dts/freescale/s32g3.dtsi b/arch/arm64/boot/dts/freescale/s32g3.dtsi
index b4226a9143c8..fa43d036686f 100644
--- a/arch/arm64/boot/dts/freescale/s32g3.dtsi
+++ b/arch/arm64/boot/dts/freescale/s32g3.dtsi
@@ -171,20 +171,18 @@ soc@0 {
 		#size-cells = <1>;
 		ranges = <0 0 0 0x80000000>;
 
-		pinctrl: pinctrl@4009c240 {
-			compatible = "nxp,s32g2-siul2-pinctrl";
-				/* MSCR0-MSCR101 registers on siul2_0 */
-			reg = <0x4009c240 0x198>,
-				/* MSCR112-MSCR122 registers on siul2_1 */
-			      <0x44010400 0x2c>,
-				/* MSCR144-MSCR190 registers on siul2_1 */
-			      <0x44010480 0xbc>,
-				/* IMCR0-IMCR83 registers on siul2_0 */
-			      <0x4009ca40 0x150>,
-				/* IMCR119-IMCR397 registers on siul2_1 */
-			      <0x44010c1c 0x45c>,
-				/* IMCR430-IMCR495 registers on siul2_1 */
-			      <0x440110f8 0x108>;
+		siul2: siul2@4009c000 {
+			compatible = "nxp,s32g3-siul2";
+			reg = <0x4009c000 0x179c>,
+			      <0x44010000 0x17b0>;
+			reg-names = "siul20", "siul21";
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
+			gpio-reserved-ranges = <102 10>, <123 21>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
 
 			jtag_pins: jtag-pins {
 				jtag-grp0 {
-- 
2.45.2

[PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Andrei Stefanescu]

The SIUL2 module is now represented as an mfd device. The pinctrl driver
is now an mfd_cell. Therefore, remove its compatible and adjust its
probing in order to get the necessary information from its mfd parent.

This change came as a result of upstream review in the following series:
https://lore.kernel.org/linux-gpio/a924bbb6-96ec-40be-9d82-a76b2ab73afd@oss.nxp.com/
https://lore.kernel.org/all/20240926143122.1385658-3-andrei.stefanescu@oss.nxp.com/

The SIUL2 module has multiple capabilities. It has support for reading
SoC information, pinctrl and GPIO. All of this functionality is part of
the same register space. The initial pinctrl driver treated the pinctrl
functionality as separate from the GPIO one. However, they do rely on
common registers and a long, detailed and specific register range list
would be required for pinctrl&GPIO (carving out the necessary memory
for each function). Moreover, in some cases this wouldn't be enough. For
example reading a GPIO's direction would require a read of the MSCR
register corresponding to that pin. This would not be possible in the
GPIO driver because all of the MSCR registers are referenced by the
pinctrl driver.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/pinctrl/nxp/pinctrl-s32.h   |   3 +-
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 143 ++++++++++++----------------
 drivers/pinctrl/nxp/pinctrl-s32g2.c |  25 +----
 3 files changed, 66 insertions(+), 105 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32.h b/drivers/pinctrl/nxp/pinctrl-s32.h
index add3c77ddfed..d52c6f814de8 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32.h
+++ b/drivers/pinctrl/nxp/pinctrl-s32.h
@@ -2,7 +2,7 @@
  *
  * S32 pinmux core definitions
  *
- * Copyright 2016-2020, 2022 NXP
+ * Copyright 2016-2020, 2022, 2024 NXP
  * Copyright (C) 2022 SUSE LLC
  * Copyright 2015-2016 Freescale Semiconductor, Inc.
  * Copyright (C) 2012 Linaro Ltd.
@@ -38,6 +38,7 @@ struct s32_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
 	unsigned int npins;
 	const struct s32_pin_range *mem_pin_ranges;
+	const struct regmap **regmaps;
 	unsigned int mem_regions;
 };
 
diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 501eb296c760..bb2f8127c2b7 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -12,6 +12,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/mfd/nxp-siul2.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -44,12 +45,6 @@ enum s32_write_type {
 	S32_PINCONF_OVERWRITE,
 };
 
-static struct regmap_config s32_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-};
-
 static u32 get_pin_no(u32 pinmux)
 {
 	return (pinmux & S32_PIN_ID_MASK) >> S32_PIN_ID_SHIFT;
@@ -85,14 +80,15 @@ struct s32_pinctrl_context {
 	unsigned int *pads;
 };
 
-/*
+/**
+ * struct s32_pinctrl - private driver data
  * @dev: a pointer back to containing device
  * @pctl: a pointer to the pinctrl device structure
  * @regions: reserved memory regions with start/end pin
  * @info: structure containing information about the pin
  * @gpio_configs: Saved configurations for GPIO pins
- * @gpiop_configs_lock: lock for the `gpio_configs` list
- * @s32_pinctrl_context: Configuration saved over system sleep
+ * @gpio_configs_lock: lock for the `gpio_configs` list
+ * @saved_context: Configuration saved over system sleep
  */
 struct s32_pinctrl {
 	struct device *dev;
@@ -123,14 +119,13 @@ s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin)
 	return NULL;
 }
 
-static inline int s32_check_pin(struct pinctrl_dev *pctldev,
-				unsigned int pin)
+static int s32_check_pin(struct pinctrl_dev *pctldev, unsigned int pin)
 {
 	return s32_get_region(pctldev, pin) ? 0 : -EINVAL;
 }
 
-static inline int s32_regmap_read(struct pinctrl_dev *pctldev,
-			   unsigned int pin, unsigned int *val)
+static int s32_regmap_read(struct pinctrl_dev *pctldev, unsigned int pin,
+			   unsigned int *val)
 {
 	struct s32_pinctrl_mem_region *region;
 	unsigned int offset;
@@ -145,7 +140,7 @@ static inline int s32_regmap_read(struct pinctrl_dev *pctldev,
 	return regmap_read(region->map, offset, val);
 }
 
-static inline int s32_regmap_write(struct pinctrl_dev *pctldev,
+static int s32_regmap_write(struct pinctrl_dev *pctldev,
 			    unsigned int pin,
 			    unsigned int val)
 {
@@ -163,7 +158,7 @@ static inline int s32_regmap_write(struct pinctrl_dev *pctldev,
 
 }
 
-static inline int s32_regmap_update(struct pinctrl_dev *pctldev, unsigned int pin,
+static int s32_regmap_update(struct pinctrl_dev *pctldev, unsigned int pin,
 			     unsigned int mask, unsigned int val)
 {
 	struct s32_pinctrl_mem_region *region;
@@ -236,10 +231,10 @@ static int s32_dt_group_node_to_map(struct pinctrl_dev *pctldev,
 	}
 
 	ret = pinconf_generic_parse_dt_config(np, pctldev, &cfgs, &n_cfgs);
-	if (ret) {
-		dev_err(dev, "%pOF: could not parse node property\n", np);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "%pOF: could not parse node property\n",
+				     np);
 
 	if (n_cfgs)
 		reserve++;
@@ -321,7 +316,7 @@ static int s32_pmx_set(struct pinctrl_dev *pctldev, unsigned int selector,
 	/* Check beforehand so we don't have a partial config. */
 	for (i = 0; i < grp->data.npins; i++) {
 		if (s32_check_pin(pctldev, grp->data.pins[i]) != 0) {
-			dev_err(info->dev, "invalid pin: %u in group: %u\n",
+			dev_err(info->dev, "Invalid pin: %u in group: %u\n",
 				grp->data.pins[i], group);
 			return -EINVAL;
 		}
@@ -475,8 +470,8 @@ static int s32_get_slew_regval(int arg)
 	return -EINVAL;
 }
 
-static inline void s32_pin_set_pull(enum pin_config_param param,
-				   unsigned int *mask, unsigned int *config)
+static void s32_pin_set_pull(enum pin_config_param param,
+			     unsigned int *mask, unsigned int *config)
 {
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
@@ -762,15 +757,15 @@ static int s32_pinctrl_parse_groups(struct device_node *np,
 	grp->data.name = np->name;
 
 	npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
-	if (npins < 0) {
-		dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
-			grp->data.name);
-		return -EINVAL;
-	}
-	if (!npins) {
-		dev_err(dev, "The group %s has no pins.\n", grp->data.name);
-		return -EINVAL;
-	}
+	if (npins < 0)
+		return dev_err_probe(dev, -EINVAL,
+				     "Failed to read 'pinmux' in node %s\n",
+				     grp->data.name);
+
+	if (!npins)
+		return dev_err_probe(dev, -EINVAL,
+				     "The group %s has no pins\n",
+				     grp->data.name);
 
 	grp->data.npins = npins;
 
@@ -811,11 +806,9 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 	/* Initialise function */
 	func->name = np->name;
 	func->ngroups = of_get_child_count(np);
-	if (func->ngroups == 0) {
-		dev_err(info->dev, "no groups defined in %pOF\n", np);
-		return -EINVAL;
-	}
-
+	if (func->ngroups == 0)
+		return dev_err_probe(info->dev, -EINVAL,
+				     "No groups defined in %pOF\n", np);
 	groups = devm_kcalloc(info->dev, func->ngroups,
 				    sizeof(*func->groups), GFP_KERNEL);
 	if (!groups)
@@ -838,57 +831,42 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
 static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 				struct s32_pinctrl *ipctl)
 {
+	struct nxp_siul2_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
 	struct s32_pinctrl_soc_info *info = ipctl->info;
-	struct device_node *np = pdev->dev.of_node;
-	struct resource *res;
-	struct regmap *map;
-	void __iomem *base;
-	unsigned int mem_regions = info->soc_data->mem_regions;
+	unsigned int mem_regions;
+	struct device_node *np;
+	u32 nfuncs = 0, i = 0, j;
+	u8 regmap_type;
 	int ret;
-	u32 nfuncs = 0;
-	u32 i = 0;
 
+	np = pdev->dev.parent->of_node;
 	if (!np)
 		return -ENODEV;
 
-	if (mem_regions == 0 || mem_regions >= 10000) {
-		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
-		return -EINVAL;
-	}
+	/* one MSCR and one IMCR region per SIUL2 module */
+	mem_regions =  info->soc_data->mem_regions;
+	if (mem_regions != mfd->num_siul2 * 2)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Mem_regions is invalid: %u\n",
+				     mem_regions);
 
 	ipctl->regions = devm_kcalloc(&pdev->dev, mem_regions,
 				      sizeof(*ipctl->regions), GFP_KERNEL);
 	if (!ipctl->regions)
 		return -ENOMEM;
 
+	/* Order is MSCR regions first, then IMCR ones */
 	for (i = 0; i < mem_regions; i++) {
-		base = devm_platform_get_and_ioremap_resource(pdev, i, &res);
-		if (IS_ERR(base))
-			return PTR_ERR(base);
-
-		snprintf(ipctl->regions[i].name,
-			 sizeof(ipctl->regions[i].name), "map%u", i);
-
-		s32_regmap_config.name = ipctl->regions[i].name;
-		s32_regmap_config.max_register = resource_size(res) -
-						 s32_regmap_config.reg_stride;
-
-		map = devm_regmap_init_mmio(&pdev->dev, base,
-						&s32_regmap_config);
-		if (IS_ERR(map)) {
-			dev_err(&pdev->dev, "Failed to init regmap[%u]\n", i);
-			return PTR_ERR(map);
-		}
-
-		ipctl->regions[i].map = map;
+		regmap_type = i < mem_regions / 2 ? SIUL2_MSCR : SIUL2_IMCR;
+		j = i % mfd->num_siul2;
+		ipctl->regions[i].map = mfd->siul2[j].regmaps[regmap_type];
 		ipctl->regions[i].pin_range = &info->soc_data->mem_pin_ranges[i];
 	}
 
 	nfuncs = of_get_child_count(np);
-	if (nfuncs <= 0) {
-		dev_err(&pdev->dev, "no functions defined\n");
-		return -EINVAL;
-	}
+	if (nfuncs <= 0)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "No functions defined\n");
 
 	info->nfunctions = nfuncs;
 	info->functions = devm_kcalloc(&pdev->dev, nfuncs,
@@ -918,18 +896,17 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 int s32_pinctrl_probe(struct platform_device *pdev,
 		      const struct s32_pinctrl_soc_data *soc_data)
 {
-	struct s32_pinctrl *ipctl;
-	int ret;
-	struct pinctrl_desc *s32_pinctrl_desc;
-	struct s32_pinctrl_soc_info *info;
 #ifdef CONFIG_PM_SLEEP
 	struct s32_pinctrl_context *saved_context;
 #endif
+	struct pinctrl_desc *s32_pinctrl_desc;
+	struct s32_pinctrl_soc_info *info;
+	struct s32_pinctrl *ipctl;
+	int ret;
 
-	if (!soc_data || !soc_data->pins || !soc_data->npins) {
-		dev_err(&pdev->dev, "wrong pinctrl info\n");
-		return -EINVAL;
-	}
+	if (!soc_data || !soc_data->pins || !soc_data->npins)
+		return dev_err_probe(&pdev->dev, -EINVAL,
+				     "Wrong pinctrl info\n");
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -964,17 +941,15 @@ int s32_pinctrl_probe(struct platform_device *pdev,
 	s32_pinctrl_desc->owner = THIS_MODULE;
 
 	ret = s32_pinctrl_probe_dt(pdev, ipctl);
-	if (ret) {
-		dev_err(&pdev->dev, "fail to probe dt properties\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to probe dt properties\n");
 
 	ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
 					    ipctl);
 	if (IS_ERR(ipctl->pctl))
 		return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
-				     "could not register s32 pinctrl driver\n");
-
+				     "Could not register s32 pinctrl driver\n");
 #ifdef CONFIG_PM_SLEEP
 	saved_context = &ipctl->saved_context;
 	saved_context->pads =
diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c
index 440ff1879424..27669991c339 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32g2.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c
@@ -3,13 +3,14 @@
  * NXP S32G pinctrl driver
  *
  * Copyright 2015-2016 Freescale Semiconductor, Inc.
- * Copyright 2017-2018, 2020-2022 NXP
+ * Copyright 2017-2018, 2020-2022, 2024 NXP
  * Copyright (C) 2022 SUSE LLC
  */
 
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/mfd/nxp-siul2.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -713,12 +714,10 @@ static const struct pinctrl_pin_desc s32_pinctrl_pads_siul2[] = {
 static const struct s32_pin_range s32_pin_ranges_siul2[] = {
 	/* MSCR pin ID ranges */
 	S32_PIN_RANGE(0, 101),
-	S32_PIN_RANGE(112, 122),
-	S32_PIN_RANGE(144, 190),
+	S32_PIN_RANGE(112, 190),
 	/* IMCR pin ID ranges */
 	S32_PIN_RANGE(512, 595),
-	S32_PIN_RANGE(631, 909),
-	S32_PIN_RANGE(942, 1007),
+	S32_PIN_RANGE(631, 1007),
 };
 
 static const struct s32_pinctrl_soc_data s32_pinctrl_data = {
@@ -728,22 +727,9 @@ static const struct s32_pinctrl_soc_data s32_pinctrl_data = {
 	.mem_regions = ARRAY_SIZE(s32_pin_ranges_siul2),
 };
 
-static const struct of_device_id s32_pinctrl_of_match[] = {
-	{
-		.compatible = "nxp,s32g2-siul2-pinctrl",
-		.data = &s32_pinctrl_data,
-	},
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, s32_pinctrl_of_match);
-
 static int s32g_pinctrl_probe(struct platform_device *pdev)
 {
-	const struct s32_pinctrl_soc_data *soc_data;
-
-	soc_data = of_device_get_match_data(&pdev->dev);
-
-	return s32_pinctrl_probe(pdev, soc_data);
+	return s32_pinctrl_probe(pdev, &s32_pinctrl_data);
 }
 
 static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
@@ -753,7 +739,6 @@ static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
 static struct platform_driver s32g_pinctrl_driver = {
 	.driver = {
 		.name = "s32g-siul2-pinctrl",
-		.of_match_table = s32_pinctrl_of_match,
 		.pm = pm_sleep_ptr(&s32g_pinctrl_pm_ops),
 		.suppress_bind_attrs = true,
 	},
-- 
2.45.2

Re: [PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Krzysztof Kozlowski]

On 13/11/2024 11:10, Andrei Stefanescu wrote:
>  
> -static inline void s32_pin_set_pull(enum pin_config_param param,
> -				   unsigned int *mask, unsigned int *config)
> +static void s32_pin_set_pull(enum pin_config_param param,
> +			     unsigned int *mask, unsigned int *config)
>  {
>  	switch (param) {
>  	case PIN_CONFIG_BIAS_DISABLE:
> @@ -762,15 +757,15 @@ static int s32_pinctrl_parse_groups(struct device_node *np,
>  	grp->data.name = np->name;
>  
>  	npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
> -	if (npins < 0) {
> -		dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
> -			grp->data.name);
> -		return -EINVAL;
> -	}
> -	if (!npins) {
> -		dev_err(dev, "The group %s has no pins.\n", grp->data.name);
> -		return -EINVAL;
> -	}
> +	if (npins < 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Failed to read 'pinmux' in node %s\n",
> +				     grp->data.name);

I do not see how this change is related. Looks you are mixing cleanups
with refactoring into MFD cell. These are two different things.

> +
> +	if (!npins)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "The group %s has no pins\n",
> +				     grp->data.name);
>  
>  	grp->data.npins = npins;
>  
> @@ -811,11 +806,9 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
>  	/* Initialise function */
>  	func->name = np->name;
>  	func->ngroups = of_get_child_count(np);
> -	if (func->ngroups == 0) {
> -		dev_err(info->dev, "no groups defined in %pOF\n", np);
> -		return -EINVAL;
> -	}
> -
> +	if (func->ngroups == 0)
> +		return dev_err_probe(info->dev, -EINVAL,
> +				     "No groups defined in %pOF\n", np);
>  	groups = devm_kcalloc(info->dev, func->ngroups,
>  				    sizeof(*func->groups), GFP_KERNEL);
>  	if (!groups)
> @@ -838,57 +831,42 @@ static int s32_pinctrl_parse_functions(struct device_node *np,
>  static int s32_pinctrl_probe_dt(struct platform_device *pdev,
>  				struct s32_pinctrl *ipctl)
>  {
> +	struct nxp_siul2_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
>  	struct s32_pinctrl_soc_info *info = ipctl->info;
> -	struct device_node *np = pdev->dev.of_node;
> -	struct resource *res;
> -	struct regmap *map;
> -	void __iomem *base;
> -	unsigned int mem_regions = info->soc_data->mem_regions;
> +	unsigned int mem_regions;
> +	struct device_node *np;
> +	u32 nfuncs = 0, i = 0, j;
> +	u8 regmap_type;
>  	int ret;
> -	u32 nfuncs = 0;
> -	u32 i = 0;
>  
> +	np = pdev->dev.parent->of_node;
>  	if (!np)
>  		return -ENODEV;
>  
> -	if (mem_regions == 0 || mem_regions >= 10000) {
> -		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
> -		return -EINVAL;
> -	}
> +	/* one MSCR and one IMCR region per SIUL2 module */

How is this related to converion into MFD cell?

Still looks like an ABI break.


Best regards,
Krzysztof

Re: [PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Andrei Stefanescu]

Hi Krzysztof,

>> +	if (npins < 0)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Failed to read 'pinmux' in node %s\n",
>> +				     grp->data.name);
> 
> I do not see how this change is related. Looks you are mixing cleanups
> with refactoring into MFD cell. These are two different things.

Yes, I also included some small refactoring changes. I didn't think they were
important enough to include them in a separate commit. Would you like me to separate
them in another commit?

>> -	if (mem_regions == 0 || mem_regions >= 10000) {
>> -		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
>> -		return -EINVAL;
>> -	}
>> +	/* one MSCR and one IMCR region per SIUL2 module */
> 
> How is this related to converion into MFD cell?

We no longer parse the device tree to configure the regmaps, we instead
get them from the mfd driver. This is the main point of converting this
driver into an mfd cell. 

> 
> Still looks like an ABI break.
> 

Yes, the driver no longer adheres to the nxp,s32g2-siul2-pinctrl.yaml binding.

The intention is to deprecate that binding since it doesn't correctly describe
the hardware. I am not sure on how to do this. I thought that changing this
driver and removing the old compatible would be enough.

Would it be better to add another file which would contain the old probing
function(and match the old binding) so clients would be able to select the
old implementation?

> 
> Best regards,
> Krzysztof
> 

Best regards,
Andrei

Re: [PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Krzysztof Kozlowski]

On 19/11/2024 10:57, Andrei Stefanescu wrote:
> Hi Krzysztof,
> 
>>> +	if (npins < 0)
>>> +		return dev_err_probe(dev, -EINVAL,
>>> +				     "Failed to read 'pinmux' in node %s\n",
>>> +				     grp->data.name);
>>
>> I do not see how this change is related. Looks you are mixing cleanups
>> with refactoring into MFD cell. These are two different things.
> 
> Yes, I also included some small refactoring changes. I didn't think they were
> important enough to include them in a separate commit. Would you like me to separate
> them in another commit?

You cannot include such changes along other, meaningful changes. This
does not apply to this patch only but all contributions. There is a
clear policy how cleanups, bugs and new things are being upstreamed:
https://elixir.bootlin.com/linux/v6.12/source/Documentation/process/submitting-patches.rst#L168

Please read above document very carefully. This is v6 and we still
circle around absolute basics.

> 
>>> -	if (mem_regions == 0 || mem_regions >= 10000) {
>>> -		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
>>> -		return -EINVAL;
>>> -	}
>>> +	/* one MSCR and one IMCR region per SIUL2 module */
>>
>> How is this related to converion into MFD cell?
> 
> We no longer parse the device tree to configure the regmaps, we instead
> get them from the mfd driver. This is the main point of converting this
> driver into an mfd cell. 
> 
>>
>> Still looks like an ABI break.
>>
> 
> Yes, the driver no longer adheres to the nxp,s32g2-siul2-pinctrl.yaml binding.

I did not find in commit msg explanation that this is ABI break and why
it is allowed. I asked for it.

> 
> The intention is to deprecate that binding since it doesn't correctly describe
> the hardware. I am not sure on how to do this. I thought that changing this
> driver and removing the old compatible would be enough.

No, you cannot break the ABI. Either you deprecate this or just don't touch.

> 
> Would it be better to add another file which would contain the old probing
> function(and match the old binding) so clients would be able to select the
> old implementation?

I don't understand that. Your driver is supposed to keep ABI. Not
through some selection but just as is.

Best regards,
Krzysztof

Re: [PATCH v6 4/7] pinctrl: s32: convert the driver into an mfd cell

[Andrei Stefanescu]

Hi Krzysztof,

>>>> +	if (npins < 0)
>>>> +		return dev_err_probe(dev, -EINVAL,
>>>> +				     "Failed to read 'pinmux' in node %s\n",
>>>> +				     grp->data.name);
>>>
>>> I do not see how this change is related. Looks you are mixing cleanups
>>> with refactoring into MFD cell. These are two different things.
>>
>> Yes, I also included some small refactoring changes. I didn't think they were
>> important enough to include them in a separate commit. Would you like me to separate
>> them in another commit?
> 
> You cannot include such changes along other, meaningful changes. This
> does not apply to this patch only but all contributions. There is a
> clear policy how cleanups, bugs and new things are being upstreamed:
> https://elixir.bootlin.com/linux/v6.12/source/Documentation/process/submitting-patches.rst#L168
> 
> Please read above document very carefully. This is v6 and we still
> circle around absolute basics.

I will split these changes in two commits in v7.

> 
>>
>>>> -	if (mem_regions == 0 || mem_regions >= 10000) {
>>>> -		dev_err(&pdev->dev, "mem_regions is invalid: %u\n", mem_regions);
>>>> -		return -EINVAL;
>>>> -	}
>>>> +	/* one MSCR and one IMCR region per SIUL2 module */
>>>
>>> How is this related to converion into MFD cell?
>>
>> We no longer parse the device tree to configure the regmaps, we instead
>> get them from the mfd driver. This is the main point of converting this
>> driver into an mfd cell. 
>>
>>>
>>> Still looks like an ABI break.
>>>
>>
>> Yes, the driver no longer adheres to the nxp,s32g2-siul2-pinctrl.yaml binding.
> 
> I did not find in commit msg explanation that this is ABI break and why
> it is allowed. I asked for it.

I will explicitly mention this in v7.

> 
>>
>> The intention is to deprecate that binding since it doesn't correctly describe
>> the hardware. I am not sure on how to do this. I thought that changing this
>> driver and removing the old compatible would be enough.
> 
> No, you cannot break the ABI. Either you deprecate this or just don't touch.

Yes, I would like to deprecate the binding. I am not sure on how to do this.
I saw that some bindings have a "deprecated: true" property at the top-level.
Should I also add a commit which does that in v7 or are there other steps
for deprecating a binding?

Best regards,
Andrei

[PATCH v6 5/7] pinctrl: s32cc: change to "devm_pinctrl_register_and_init"

[Andrei Stefanescu]

Switch from "devm_pinctrl_register" to "devm_pinctrl_register_and_init"
and "pinctrl_enable" since this is the recommended way.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index bb2f8127c2b7..0b79c7445929 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -945,10 +945,10 @@ int s32_pinctrl_probe(struct platform_device *pdev,
 		return dev_err_probe(&pdev->dev, ret,
 				     "Failed to probe dt properties\n");
 
-	ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
-					    ipctl);
-	if (IS_ERR(ipctl->pctl))
-		return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
+	ret = devm_pinctrl_register_and_init(&pdev->dev, s32_pinctrl_desc,
+					     ipctl, &ipctl->pctl);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
 				     "Could not register s32 pinctrl driver\n");
 #ifdef CONFIG_PM_SLEEP
 	saved_context = &ipctl->saved_context;
@@ -960,7 +960,12 @@ int s32_pinctrl_probe(struct platform_device *pdev,
 		return -ENOMEM;
 #endif
 
-	dev_info(&pdev->dev, "initialized s32 pinctrl driver\n");
+	ret = pinctrl_enable(ipctl->pctl);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to enable pinctrl\n");
+
+	dev_info(&pdev->dev, "Initialized s32 pinctrl driver\n");
 
 	return 0;
 }
-- 
2.45.2

[PATCH v6 6/7] pinctrl: s32cc: implement GPIO functionality

[Andrei Stefanescu]

Add basic GPIO functionality (request, free, get, set) for the existing
pinctrl SIUL2 driver since the hardware for pinctrl&GPIO is tightly
coupled.

Also, remove pinmux_ops which are no longer needed.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 406 +++++++++++++++++++++++-----
 1 file changed, 344 insertions(+), 62 deletions(-)

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 0b79c7445929..e213a910d958 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -40,6 +40,14 @@
 #define S32_MSCR_ODE		BIT(20)
 #define S32_MSCR_OBE		BIT(21)
 
+/* PGPDOs are 16bit registers that come in big endian
+ * order if they are grouped in pairs of two.
+ *
+ * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
+ */
+#define S32_PGPD(N)		(((N) ^ 1) * 2)
+#define S32_PGPD_SIZE		16
+
 enum s32_write_type {
 	S32_PINCONF_UPDATE_ONLY,
 	S32_PINCONF_OVERWRITE,
@@ -84,6 +92,7 @@ struct s32_pinctrl_context {
  * struct s32_pinctrl - private driver data
  * @dev: a pointer back to containing device
  * @pctl: a pointer to the pinctrl device structure
+ * @gc: a pointer to the gpio_chip
  * @regions: reserved memory regions with start/end pin
  * @info: structure containing information about the pin
  * @gpio_configs: Saved configurations for GPIO pins
@@ -93,6 +102,7 @@ struct s32_pinctrl_context {
 struct s32_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
+	struct gpio_chip gc;
 	struct s32_pinctrl_mem_region *regions;
 	struct s32_pinctrl_soc_info *info;
 	struct list_head gpio_configs;
@@ -366,66 +376,6 @@ static int s32_pmx_get_groups(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
-				       struct pinctrl_gpio_range *range,
-				       unsigned int offset)
-{
-	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct gpio_pin_config *gpio_pin;
-	unsigned int config;
-	unsigned long flags;
-	int ret;
-
-	ret = s32_regmap_read(pctldev, offset, &config);
-	if (ret)
-		return ret;
-
-	/* Save current configuration */
-	gpio_pin = kmalloc(sizeof(*gpio_pin), GFP_KERNEL);
-	if (!gpio_pin)
-		return -ENOMEM;
-
-	gpio_pin->pin_id = offset;
-	gpio_pin->config = config;
-
-	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
-	list_add(&gpio_pin->list, &ipctl->gpio_configs);
-	spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
-
-	/* GPIO pin means SSS = 0 */
-	config &= ~S32_MSCR_SSS_MASK;
-
-	return s32_regmap_write(pctldev, offset, config);
-}
-
-static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
-				      struct pinctrl_gpio_range *range,
-				      unsigned int offset)
-{
-	struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
-	struct gpio_pin_config *gpio_pin, *tmp;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
-
-	list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {
-		if (gpio_pin->pin_id == offset) {
-			ret = s32_regmap_write(pctldev, gpio_pin->pin_id,
-						 gpio_pin->config);
-			if (ret != 0)
-				goto unlock;
-
-			list_del(&gpio_pin->list);
-			kfree(gpio_pin);
-			break;
-		}
-	}
-
-unlock:
-	spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
-}
-
 static int s32_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 				      struct pinctrl_gpio_range *range,
 				      unsigned int offset,
@@ -449,8 +399,6 @@ static const struct pinmux_ops s32_pmx_ops = {
 	.get_function_name = s32_pmx_get_func_name,
 	.get_function_groups = s32_pmx_get_groups,
 	.set_mux = s32_pmx_set,
-	.gpio_request_enable = s32_pmx_gpio_request_enable,
-	.gpio_disable_free = s32_pmx_gpio_disable_free,
 	.gpio_set_direction = s32_pmx_gpio_set_direction,
 };
 
@@ -669,6 +617,311 @@ static const struct pinconf_ops s32_pinconf_ops = {
 	.pin_config_group_dbg_show = s32_pinconf_group_dbg_show,
 };
 
+static struct s32_pinctrl *to_s32_pinctrl(struct gpio_chip *chip)
+{
+	return container_of(chip, struct s32_pinctrl, gc);
+}
+
+static struct regmap *s32_gpio_get_pgpd_regmap(struct gpio_chip *chip,
+					       unsigned int pin,
+					       bool output)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(chip);
+	struct nxp_siul2_mfd *mfd;
+	u32 base, num;
+	int i;
+
+	mfd = dev_get_drvdata(ipctl->dev->parent);
+
+	for (i = 0; i < mfd->num_siul2; i++) {
+		base = mfd->siul2[i].gpio_base;
+		num = mfd->siul2[i].gpio_num;
+
+		if (pin >= base && pin < base + num)
+			return output ? mfd->siul2[i].regmaps[SIUL2_PGPDO] :
+					mfd->siul2[i].regmaps[SIUL2_PGPDI];
+	}
+
+	return NULL;
+}
+
+static int s32_gpio_request(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(gc);
+	struct pinctrl_dev *pctldev = ipctl->pctl;
+	struct gpio_pin_config *gpio_pin;
+	unsigned int config;
+	int ret;
+
+	ret = s32_regmap_read(pctldev, gpio, &config);
+	if (ret)
+		return ret;
+
+	/* Save current configuration */
+	gpio_pin = kmalloc(sizeof(*gpio_pin), GFP_KERNEL);
+	if (!gpio_pin)
+		return -ENOMEM;
+
+	gpio_pin->pin_id = gpio;
+	gpio_pin->config = config;
+
+	scoped_guard(spinlock_irqsave, &ipctl->gpio_configs_lock)
+		list_add(&gpio_pin->list, &ipctl->gpio_configs);
+
+	/* GPIO pin means SSS = 0 */
+	config &= ~S32_MSCR_SSS_MASK;
+
+	return s32_regmap_write(pctldev, gpio, config);
+}
+
+static void s32_gpio_free(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(gc);
+	struct pinctrl_dev *pctldev = ipctl->pctl;
+	struct gpio_pin_config *gpio_pin, *tmp;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
+
+	list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {
+		if (gpio_pin->pin_id == gpio) {
+			ret = s32_regmap_write(pctldev, gpio_pin->pin_id,
+					       gpio_pin->config);
+			if (ret != 0)
+				goto unlock;
+
+			list_del(&gpio_pin->list);
+			kfree(gpio_pin);
+			break;
+		}
+	}
+
+unlock:
+	spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
+}
+
+static int s32_gpio_get_dir(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(chip);
+	unsigned int reg_value;
+	int ret;
+
+	ret = s32_regmap_read(ipctl->pctl, gpio, &reg_value);
+	if (ret)
+		return ret;
+
+	if (!(reg_value & S32_MSCR_IBE))
+		return -EINVAL;
+
+	return reg_value & S32_MSCR_OBE ? GPIO_LINE_DIRECTION_OUT :
+					  GPIO_LINE_DIRECTION_IN;
+}
+
+static unsigned int s32_pin2pad(unsigned int pin)
+{
+	return pin / S32_PGPD_SIZE;
+}
+
+static u16 s32_pin2mask(unsigned int pin)
+{
+	/**
+	 * From Reference manual :
+	 * PGPDOx[PPDOy] = GPDO(x × 16) + (15 - y)[PDO_(x × 16) + (15 - y)]
+	 */
+	return BIT(S32_PGPD_SIZE - 1 - pin % S32_PGPD_SIZE);
+}
+
+static struct regmap *s32_gpio_get_regmap_offset_mask(struct gpio_chip *chip,
+						      unsigned int gpio,
+						      unsigned int *reg_offset,
+						      u16 *mask,
+						      bool output)
+{
+	struct regmap *regmap;
+	unsigned int pad;
+
+	regmap = s32_gpio_get_pgpd_regmap(chip, gpio, output);
+	if (!regmap)
+		return NULL;
+
+	*mask = s32_pin2mask(gpio);
+	pad = s32_pin2pad(gpio);
+
+	*reg_offset = S32_PGPD(pad);
+
+	return regmap;
+}
+
+static void s32_gpio_set_val(struct gpio_chip *chip, unsigned int gpio,
+			     int value)
+{
+	unsigned int reg_offset;
+	struct regmap *regmap;
+	u16 mask;
+
+	regmap = s32_gpio_get_regmap_offset_mask(chip, gpio, &reg_offset,
+						 &mask, true);
+	if (!regmap)
+		return;
+
+	value = value ? mask : 0;
+
+	regmap_update_bits(regmap, reg_offset, mask, value);
+}
+
+static void s32_gpio_set(struct gpio_chip *chip, unsigned int gpio,
+			 int value)
+{
+	if (s32_gpio_get_dir(chip, gpio) != GPIO_LINE_DIRECTION_OUT)
+		return;
+
+	s32_gpio_set_val(chip, gpio, value);
+}
+
+static int s32_gpio_get(struct gpio_chip *chip, unsigned int gpio)
+{
+	unsigned int reg_offset, value;
+	struct regmap *regmap;
+	u16 mask;
+	int ret;
+
+	if (s32_gpio_get_dir(chip, gpio) != GPIO_LINE_DIRECTION_IN)
+		return -EINVAL;
+
+	regmap = s32_gpio_get_regmap_offset_mask(chip, gpio, &reg_offset,
+						 &mask, false);
+	if (!regmap)
+		return -EINVAL;
+
+	ret = regmap_read(regmap, reg_offset, &value);
+	if (ret)
+		return ret;
+
+	return !!(value & mask);
+}
+
+static int s32_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
+			    int val)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(chip);
+
+	s32_gpio_set_val(chip, gpio, val);
+
+	return s32_pmx_gpio_set_direction(ipctl->pctl, NULL, gpio, false);
+}
+
+static int s32_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct s32_pinctrl *ipctl = to_s32_pinctrl(chip);
+
+	return s32_pmx_gpio_set_direction(ipctl->pctl, NULL, gpio, true);
+}
+
+static int s32_gpio_gen_names(struct device *dev, unsigned int cnt, char **names,
+			      char *ch_index, unsigned int *num_index)
+{
+	unsigned int i;
+
+	for (i = 0; i < cnt; i++) {
+		if (i != 0 && !(*num_index % 16))
+			(*ch_index)++;
+
+		names[i] = devm_kasprintf(dev, GFP_KERNEL, "P%c_%02d",
+					  *ch_index, 0xFU & (*num_index)++);
+		if (!names[i])
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int s32_gpio_remove_reserved_names(struct device *dev,
+					  struct s32_pinctrl *ipctl,
+					  char **names)
+{
+	struct device_node *np = dev->of_node;
+	u32 base_gpio, num_gpio, tmp;
+	int num_ranges, i, j, ret;
+
+	/* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
+
+	num_ranges = of_property_count_u32_elems(dev->of_node,
+						 "gpio-reserved-ranges");
+
+	/* The "gpio-reserved-ranges" is optional. */
+	if (num_ranges < 0)
+		return 0;
+	num_ranges /= 2;
+
+	for (i = 0; i < num_ranges; i++) {
+		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+						 i * 2, &base_gpio);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Cannot parse start GPIO: %d\n",
+					     ret);
+
+		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+						 i * 2 + 1, &num_gpio);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Cannot parse num. GPIOs: %d\n",
+					     ret);
+
+		if (check_add_overflow(base_gpio, num_gpio, &tmp) ||
+		    base_gpio + num_gpio > ipctl->gc.ngpio)
+			return dev_err_probe(dev, -EINVAL,
+					     "Invalid reserved GPIOs\n");
+
+		/* Remove names set for reserved GPIOs. */
+		for (j = base_gpio; j < base_gpio + num_gpio; j++) {
+			devm_kfree(dev, names[j]);
+			names[j] = NULL;
+		}
+	}
+
+	return 0;
+}
+
+static int s32_gpio_populate_names(struct device *dev,
+				   struct s32_pinctrl *ipctl)
+{
+	struct nxp_siul2_mfd *mfd = dev_get_drvdata(ipctl->dev->parent);
+	unsigned int num_index = 0;
+	char ch_index = 'A';
+	char **names;
+	int i, ret;
+
+	names = devm_kcalloc(dev, ipctl->gc.ngpio, sizeof(*names),
+			     GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	for (i = 0; i < mfd->num_siul2; i++) {
+		if (mfd->siul2[i].gpio_base % 16 == 0)
+			num_index = 0;
+
+		ret = s32_gpio_gen_names(dev, mfd->siul2[i].gpio_num,
+					 names + mfd->siul2[i].gpio_base,
+					 &ch_index, &num_index);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Error setting SIUL2_%d names\n",
+					     i);
+
+		ch_index++;
+	}
+
+	ret = s32_gpio_remove_reserved_names(dev, ipctl, names);
+	if (ret)
+		return ret;
+
+	ipctl->gc.names = (const char *const *)names;
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static bool s32_pinctrl_should_save(struct s32_pinctrl *ipctl,
 				    unsigned int pin)
@@ -896,12 +1149,14 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
 int s32_pinctrl_probe(struct platform_device *pdev,
 		      const struct s32_pinctrl_soc_data *soc_data)
 {
+	struct nxp_siul2_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
 #ifdef CONFIG_PM_SLEEP
 	struct s32_pinctrl_context *saved_context;
 #endif
 	struct pinctrl_desc *s32_pinctrl_desc;
 	struct s32_pinctrl_soc_info *info;
 	struct s32_pinctrl *ipctl;
+	struct gpio_chip *gc;
 	int ret;
 
 	if (!soc_data || !soc_data->pins || !soc_data->npins)
@@ -967,5 +1222,32 @@ int s32_pinctrl_probe(struct platform_device *pdev,
 
 	dev_info(&pdev->dev, "Initialized s32 pinctrl driver\n");
 
+	gc = &ipctl->gc;
+	gc->parent = &pdev->dev;
+	gc->label = dev_name(&pdev->dev);
+	gc->base = -1;
+	/* In some cases, there is a gap between the SIUL GPIOs. */
+	gc->ngpio = mfd->siul2[mfd->num_siul2 - 1].gpio_base +
+		    mfd->siul2[mfd->num_siul2 - 1].gpio_num;
+	ret = s32_gpio_populate_names(&pdev->dev, ipctl);
+	if (ret)
+		return ret;
+
+	gc->set = s32_gpio_set;
+	gc->get = s32_gpio_get;
+	gc->set_config = gpiochip_generic_config;
+	gc->request = s32_gpio_request;
+	gc->free = s32_gpio_free;
+	gc->direction_output = s32_gpio_dir_out;
+	gc->direction_input = s32_gpio_dir_in;
+	gc->get_direction = s32_gpio_get_dir;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, gc, ipctl);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Unable to add gpiochip\n");
+
+	dev_info(&pdev->dev, "Initialized s32 GPIO functionality\n");
+
 	return 0;
 }
-- 
2.45.2

Re: [PATCH v6 6/7] pinctrl: s32cc: implement GPIO functionality

[Markus Elfring]

…
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
…
> +static void s32_gpio_free(struct gpio_chip *gc, unsigned int gpio)
> +{
…
> +	spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
> +
> +	list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {
…
> +	}
> +
> +unlock:
> +	spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
> +}

How do you think about to apply another call “scoped_guard(spinlock_irqsave, &ipctl->gpio_configs_lock)” here?

Regards,
Markus

[PATCH v6 7/7] MAINTAINERS: add MAINTAINER for NXP SIUL2 MFD driver

[Andrei Stefanescu]

Add the new MFD driver for the SIUL2 module under the NXP S32G existing
entry. This MFD driver currently has one cell for a combined pinctrl&GPIO
driver and will, in the future, contain another cell for an NVMEM driver.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bdae0faf000c..cfbaceea5f40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2787,7 +2787,9 @@ R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
 L:	NXP S32 Linux Team <s32@nxp.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+F:	Documentation/devicetree/bindings/mfd/nxp,siul2.yaml
 F:	arch/arm64/boot/dts/freescale/s32g*.dts*
+F:	drivers/mfd/nxp-siul2.c
 F:	drivers/pinctrl/nxp/
 
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
-- 
2.45.2

Re: [PATCH v6 7/7] MAINTAINERS: add MAINTAINER for NXP SIUL2 MFD driver

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next lee-mfd/for-mfd-next shawnguo/for-next linus/master v6.12-rc7 next-20241113]
[cannot apply to lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/dt-bindings-mfd-add-support-for-the-NXP-SIUL2-module/20241113-181639
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link:    https://lore.kernel.org/r/20241113101124.1279648-8-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v6 7/7] MAINTAINERS: add MAINTAINER for NXP SIUL2 MFD driver
reproduce: (https://download.01.org/0day-ci/archive/20241113/202411132227.SO6y3U3E-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411132227.SO6y3U3E-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Documentation/userspace-api/netlink/netlink-raw.rst: :doc:`tc<../../networking/netlink_spec/tc>`
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
   Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/mfd/nxp,siul2.yaml
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
   Using alabaster theme

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki