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

[PATCH 0/3] 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.

Andrei Stefanescu (3):
  dt-bindings: gpio: add schema for NXP S32G2/S32G3 SoCs
  drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
  MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver

 .../bindings/gpio/nxp,gpio-siul2-s32g2.yaml   | 134 ++++
 MAINTAINERS                                   |   2 +
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-siul2-s32g2.c               | 607 ++++++++++++++++++
 5 files changed, 752 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml
 create mode 100644 drivers/gpio/gpio-siul2-s32g2.c

-- 
2.45.2

[PATCH 1/3] dt-bindings: gpio: add schema for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

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

Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 .../bindings/gpio/nxp,gpio-siul2-s32g2.yaml   | 134 ++++++++++++++++++
 1 file changed, 134 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml

diff --git a/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml b/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml
new file mode 100644
index 000000000000..fba41a18d4c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml
@@ -0,0 +1,134 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/nxp,gpio-siul2-s32g2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32G2 SIUL2 GPIO controller
+
+maintainers:
+  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
+  - Larisa Grigore <larisa.grigore@nxp.com>
+  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
+
+description: |
+  Support for the SIUL2 GPIOs found on the S32G2 and S32G3
+  chips. It includes an IRQ controller for all pins which have
+  an EIRQ associated.
+
+properties:
+  compatible:
+    items:
+      - const: nxp,s32g2-siul2-gpio
+
+  reg:
+    description: |
+      A list of register regions for configuring interrupts,
+      GPIO output values and reading GPIO input values.
+    items:
+      - description: PGPDO (output value) registers for SIUL2_0
+      - description: PGPDO (output value) registers for SIUL2_1
+      - description: PGPDI (input value) registers for SIUL2_0
+      - description: PGPDI (input value) registers for SIUL2_1
+      - description: EIRQ (interrupt) configuration registers from SIUL2_1
+      - description: EIRQ IMCR registers for interrupt muxing between pads
+
+  reg-names:
+    items:
+      - const: opads0
+      - const: opads1
+      - const: ipads0
+      - const: ipads1
+      - const: eirqs
+      - const: eirq-imcrs
+
+  interrupts:
+    description:
+      The port interrupt shared by all 32 EIRQs.
+
+  gpio-controller:
+    description:
+      Marks the device node as a gpio controller
+
+  "#gpio-cells":
+    description: |
+      Should be two. The first cell is the pin number and
+      the second cell is used to specify the gpio polarity
+      0 = active high
+      1 = active low
+
+  interrupt-controller:
+    description:
+      Marks the device node as an interrupt controller
+
+  "#interrupt-cells":
+    const: 2
+    description:
+      Refer to ../interrupt-controller/interrupts.txt for more details.
+
+  gpio-ranges:
+    description:
+      Interaction with the PINCTRL subsystem
+      Refer to gpio.txt for more details.
+
+  gpio-reserved-ranges:
+    description:
+      A list of start GPIO and number of GPIO pairs which are unusable.
+      Refer to gpio.txt for more details.
+
+patternProperties:
+  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
+    additionalProperties: false
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      input: true
+      output-high: true
+      output-low: true
+      line-name: true
+    required:
+      - gpio-hog
+      - gpios
+
+required:
+  - compatible
+  - interrupts
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+  - interrupt-controller
+  - "#interrupt-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gpio: siul2-gpio@4009d700 {
+        compatible = "nxp,s32g2-siul2-gpio";
+        reg = <0x4009d700 0x10>,
+              <0x44011700 0x18>,
+              <0x4009d740 0x10>,
+              <0x44011740 0x18>,
+              <0x44010010 0xb4>,
+              <0x44011078 0x80>;
+        reg-names = "opads0", "opads1", "ipads0",
+                    "ipads1", "eirqs", "eirq-imcrs";
+        gpio-controller;
+        #gpio-cells = <2>;
+                      /* GPIO 0-101 */
+        gpio-ranges = <&pinctrl 0   0   102>,
+                      /* GPIO 112-190 */
+                      <&pinctrl 112 112 79>;
+        gpio-reserved-ranges = <102 10>, <123 21>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
+        status = "okay";
+    };
-- 
2.45.2

[PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
(System Integration Unit Lite2) hardware module. There are two
SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
SIUL2_1 for the rest.

The GPIOs are not fully contiguous, there are some gaps:
- GPIO102 up to GPIO111(inclusive) are invalid
- GPIO123 up to GPIO143(inclusive) are invalid

Some GPIOs are input only(i.e. GPI182) though this restriction
is not yet enforced in code.

This patch adds basic GPIO functionality(no interrupts, no
suspend/resume functions).

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/gpio/Kconfig            |   8 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-siul2-s32g2.c | 607 ++++++++++++++++++++++++++++++++
 3 files changed, 616 insertions(+)
 create mode 100644 drivers/gpio/gpio-siul2-s32g2.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 58f43bcced7c..0c3c94daab0f 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -643,6 +643,14 @@ config GPIO_SIOX
 	  Say yes here to support SIOX I/O devices. These are units connected
 	  via a SIOX bus and have a number of fixed-direction I/O lines.
 
+config GPIO_SIUL2_S32G2
+        tristate "GPIO driver for S32G2/S32G3"
+        depends on OF_GPIO
+        help
+          This enables support for the SIUL2 GPIOs found on the S32G2/S32G3
+          chips. Say yes here to enable the SIUL2 to be used as an GPIO
+          controller for S32G2/S32G3 platforms.
+
 config GPIO_SNPS_CREG
 	bool "Synopsys GPIO via CREG (Control REGisters) driver"
 	depends on ARC || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 64dd6d9d730d..fb6e770a64b9 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -149,6 +149,7 @@ obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
+obj-$(CONFIG_GPIO_SIUL2_S32G2)		+= gpio-siul2-s32g2.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
diff --git a/drivers/gpio/gpio-siul2-s32g2.c b/drivers/gpio/gpio-siul2-s32g2.c
new file mode 100644
index 000000000000..07df16299237
--- /dev/null
+++ b/drivers/gpio/gpio-siul2-s32g2.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * SIUL2 GPIO support.
+ *
+ * Copyright (c) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2018-2024 NXP
+  */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/gpio.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/bitmap.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+/* 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 SIUL2_PGPDO(N)		(((N) ^ 1) * 2)
+#define S32G2_SIUL2_NUM		2
+#define S32G2_PADS_DTS_TAG_LEN	(7)
+
+#define SIUL2_GPIO_16_PAD_SIZE		16
+
+/**
+ * struct siul2_device_data  - platform data attached to the compatible.
+ * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
+ * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
+ */
+struct siul2_device_data {
+	const struct regmap_access_table **pad_access;
+	const bool reset_cnt;
+};
+
+/**
+ * struct siul2_desc - describes a SIUL2 hw module.
+ * @pad_access: array of valid I/O pads.
+ * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
+ * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
+ * @gpio_base: the first GPIO pin.
+ * @gpio_num: the number of GPIO pins.
+ */
+struct siul2_desc {
+	const struct regmap_access_table *pad_access;
+	struct regmap *opadmap;
+	struct regmap *ipadmap;
+	u32 gpio_base;
+	u32 gpio_num;
+};
+
+/**
+ * struct siul2_gpio_dev - describes a group of GPIO pins.
+ * @platdata: the platform data.
+ * @siul2: SIUL2_0 and SIUL2_1 modules information.
+ * @pin_dir_bitmap: the bitmap with pin directions.
+ * @gc: the GPIO chip.
+ * @lock: mutual access to bitmaps.
+ */
+struct siul2_gpio_dev {
+	const struct siul2_device_data *platdata;
+	struct siul2_desc siul2[S32G2_SIUL2_NUM];
+	unsigned long *pin_dir_bitmap;
+	struct gpio_chip gc;
+	raw_spinlock_t lock;
+};
+
+static inline int siul2_get_gpio_pinspec(struct platform_device *pdev,
+					 struct of_phandle_args *pinspec,
+					 unsigned int range_index)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+						range_index, pinspec);
+}
+
+static inline struct regmap *siul2_offset_to_regmap(struct siul2_gpio_dev *dev,
+						    unsigned int offset,
+						    bool input)
+{
+	struct siul2_desc *siul2;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(dev->siul2); i++) {
+		siul2 = &dev->siul2[i];
+		if (offset >= siul2->gpio_base &&
+		    offset - siul2->gpio_base < siul2->gpio_num)
+			return input ? siul2->ipadmap : siul2->opadmap;
+	}
+
+	return NULL;
+}
+
+static inline void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
+					    unsigned int gpio, int dir)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&dev->lock, flags);
+
+	if (dir == GPIO_LINE_DIRECTION_IN)
+		bitmap_clear(dev->pin_dir_bitmap, gpio, 1);
+	else
+		bitmap_set(dev->pin_dir_bitmap, gpio, 1);
+
+	raw_spin_unlock_irqrestore(&dev->lock, flags);
+}
+
+static inline int siul2_get_direction(struct siul2_gpio_dev *dev,
+				      unsigned int gpio)
+{
+	return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
+						     GPIO_LINE_DIRECTION_IN;
+}
+
+static inline struct siul2_gpio_dev *to_siul2_gpio_dev(struct gpio_chip *chip)
+{
+	return container_of(chip, struct siul2_gpio_dev, gc);
+}
+
+static int siul2_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	int ret = 0;
+
+	ret = pinctrl_gpio_direction_input(chip, gpio);
+	if (ret)
+		return ret;
+
+	gpio_dev = to_siul2_gpio_dev(chip);
+	siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_IN);
+
+	return 0;
+}
+
+static int siul2_gpio_get_dir(struct gpio_chip *chip, unsigned int gpio)
+{
+	return siul2_get_direction(to_siul2_gpio_dev(chip), gpio);
+}
+
+static unsigned int siul2_pin2pad(unsigned int pin)
+{
+	return pin / SIUL2_GPIO_16_PAD_SIZE;
+}
+
+static u16 siul2_pin2mask(unsigned int pin)
+{
+	/**
+	 * From Reference manual :
+	 * PGPDOx[PPDOy] = GPDO(x × 16) + (15 - y)[PDO_(x × 16) + (15 - y)]
+	 */
+	return BIT(SIUL2_GPIO_16_PAD_SIZE - 1 - pin % SIUL2_GPIO_16_PAD_SIZE);
+}
+
+static inline u32 siul2_get_pad_offset(unsigned int pad)
+{
+	return SIUL2_PGPDO(pad);
+}
+
+static void siul2_gpio_set_val(struct gpio_chip *chip, unsigned int offset,
+			       int value)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+	unsigned int pad, reg_offset;
+	struct regmap *regmap;
+	u16 mask;
+
+	mask = siul2_pin2mask(offset);
+	pad = siul2_pin2pad(offset);
+
+	reg_offset = siul2_get_pad_offset(pad);
+	regmap = siul2_offset_to_regmap(gpio_dev, offset, false);
+	if (!regmap)
+		return;
+
+	value = value ? mask : 0;
+
+	regmap_update_bits(regmap, reg_offset, mask, value);
+}
+
+static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
+			      int val)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	int ret = 0;
+
+	gpio_dev = to_siul2_gpio_dev(chip);
+	siul2_gpio_set_val(chip, gpio, val);
+
+	ret = pinctrl_gpio_direction_output(chip, gpio);
+	if (ret)
+		return ret;
+
+	siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_OUT);
+
+	return 0;
+}
+
+static int siul2_set_config(struct gpio_chip *chip, unsigned int offset,
+			    unsigned long config)
+{
+	return pinctrl_gpio_set_config(chip, offset, config);
+}
+
+static int siul2_gpio_request(struct gpio_chip *chip, unsigned int gpio)
+{
+	return pinctrl_gpio_request(chip, gpio);
+}
+
+static void siul2_gpio_free(struct gpio_chip *chip, unsigned int gpio)
+{
+	pinctrl_gpio_free(chip, gpio);
+}
+
+static void siul2_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+
+	if (!gpio_dev)
+		return;
+
+	if (siul2_get_direction(gpio_dev, offset) == GPIO_LINE_DIRECTION_IN)
+		return;
+
+	siul2_gpio_set_val(chip, offset, value);
+}
+
+static int siul2_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+	unsigned int mask, pad, reg_offset, data = 0;
+	struct regmap *regmap;
+
+	mask = siul2_pin2mask(offset);
+	pad = siul2_pin2pad(offset);
+
+	reg_offset = siul2_get_pad_offset(pad);
+	regmap = siul2_offset_to_regmap(gpio_dev, offset, true);
+	if (!regmap)
+		return -EINVAL;
+
+	regmap_read(regmap, reg_offset, &data);
+
+	return !!(data & mask);
+}
+
+static const struct regmap_config siul2_regmap_conf = {
+	.val_bits = 32,
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static struct regmap *common_regmap_init(struct platform_device *pdev,
+					 struct regmap_config *conf,
+					 const char *name)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	resource_size_t size;
+	void __iomem *base;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return ERR_PTR(-ENOMEM);
+
+	size = resource_size(res);
+	conf->val_bits = conf->reg_stride * 8;
+	conf->max_register = size - conf->reg_stride;
+	conf->name = name;
+	conf->use_raw_spinlock = true;
+
+	if (conf->cache_type != REGCACHE_NONE)
+		conf->num_reg_defaults_raw = size / conf->reg_stride;
+
+	return devm_regmap_init_mmio(dev, base, conf);
+}
+
+static bool not_writable(__always_unused struct device *dev,
+			 __always_unused unsigned int reg)
+{
+	return false;
+}
+
+static struct regmap *init_padregmap(struct platform_device *pdev,
+				     struct siul2_gpio_dev *gpio_dev,
+				     int selector, bool input)
+{
+	const struct siul2_device_data *platdata = gpio_dev->platdata;
+	struct regmap_config regmap_conf = siul2_regmap_conf;
+	char dts_tag[S32G2_PADS_DTS_TAG_LEN];
+	int err;
+
+	regmap_conf.reg_stride = 2;
+
+	if (selector != 0 && selector != 1)
+		return ERR_PTR(-EINVAL);
+
+	regmap_conf.rd_table = platdata->pad_access[selector];
+
+	err = snprintf(dts_tag, ARRAY_SIZE(dts_tag),  "%cpads%d",
+		       input ? 'i' : 'o', selector);
+	if (err < 0)
+		return ERR_PTR(-EINVAL);
+
+	if (input) {
+		regmap_conf.writeable_reg = not_writable;
+		regmap_conf.cache_type = REGCACHE_NONE;
+	} else {
+		regmap_conf.wr_table = platdata->pad_access[selector];
+	}
+
+	return common_regmap_init(pdev, &regmap_conf, dts_tag);
+}
+
+static int siul2_gpio_pads_init(struct platform_device *pdev,
+				struct siul2_gpio_dev *gpio_dev)
+{
+	struct device *dev = &pdev->dev;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+		gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
+							    false);
+		if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
+			dev_err(dev,
+				"Failed to initialize opad2%lu regmap config\n",
+				i);
+			return PTR_ERR(gpio_dev->siul2[i].opadmap);
+		}
+
+		gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
+							    true);
+		if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
+			dev_err(dev,
+				"Failed to initialize ipad2%lu regmap config\n",
+				i);
+			return PTR_ERR(gpio_dev->siul2[i].ipadmap);
+		}
+	}
+
+	return 0;
+}
+
+static int siul2_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 siul2_gpio_remove_reserved_names(struct device *dev,
+					    struct siul2_gpio_dev *gpio_dev,
+					    char **names)
+{
+	struct device_node *np = dev->of_node;
+	int num_ranges, i, j, ret;
+	u32 base_gpio, num_gpio;
+
+	/* 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) {
+			dev_err(dev, "Could not parse the start GPIO: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+						 i * 2 + 1, &num_gpio);
+		if (ret) {
+			dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
+			return ret;
+		}
+
+		if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
+			dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
+			return -EINVAL;
+		}
+
+		/* 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 siul2_gpio_populate_names(struct device *dev,
+				     struct siul2_gpio_dev *gpio_dev)
+{
+	unsigned int num_index = 0;
+	char ch_index = 'A';
+	char **names;
+	int i, ret;
+
+	names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
+			     GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	for (i = 0; i < S32G2_SIUL2_NUM; i++) {
+		ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
+				      names + gpio_dev->siul2[i].gpio_base,
+				      &ch_index, &num_index);
+		if (ret) {
+			dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
+				i);
+			return ret;
+		}
+
+		if (gpio_dev->platdata->reset_cnt)
+			num_index = 0;
+
+		ch_index++;
+	}
+
+	ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
+	if (ret)
+		return ret;
+
+	gpio_dev->gc.names = (const char *const *)names;
+
+	return 0;
+}
+
+static int siul2_gpio_probe(struct platform_device *pdev)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	struct device *dev = &pdev->dev;
+	struct of_phandle_args pinspec;
+	struct gpio_chip *gc;
+	size_t bitmap_size;
+	int ret = 0;
+	size_t i;
+
+	gpio_dev = devm_kzalloc(dev, sizeof(*gpio_dev), GFP_KERNEL);
+	if (!gpio_dev)
+		return -ENOMEM;
+
+	gpio_dev->platdata = of_device_get_match_data(dev);
+
+	for (i = 0; i < S32G2_SIUL2_NUM; i++)
+		gpio_dev->siul2[i].pad_access =
+			gpio_dev->platdata->pad_access[i];
+
+	ret = siul2_gpio_pads_init(pdev, gpio_dev);
+	if (ret)
+		return ret;
+
+	gc = &gpio_dev->gc;
+
+	platform_set_drvdata(pdev, gpio_dev);
+
+	raw_spin_lock_init(&gpio_dev->lock);
+
+	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+		ret = siul2_get_gpio_pinspec(pdev, &pinspec, i);
+		if (ret) {
+			dev_err(dev,
+				"unable to get pinspec %lu from device tree\n",
+				i);
+			return -EINVAL;
+		}
+
+		if (pinspec.args_count != 3) {
+			dev_err(dev, "Invalid pinspec count: %d\n",
+				pinspec.args_count);
+			return -EINVAL;
+		}
+
+		gpio_dev->siul2[i].gpio_base = pinspec.args[1];
+		gpio_dev->siul2[i].gpio_num = pinspec.args[2];
+	}
+
+	gc->base = -1;
+
+	/* In some cases, there is a gap between the SIUL GPIOs. */
+	gc->ngpio = gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_base +
+		    gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_num;
+
+	ret = siul2_gpio_populate_names(&pdev->dev, gpio_dev);
+	if (ret)
+		return ret;
+
+	bitmap_size = gc->ngpio * sizeof(*gpio_dev->pin_dir_bitmap);
+	gpio_dev->pin_dir_bitmap = devm_bitmap_zalloc(dev, bitmap_size,
+						      GFP_KERNEL);
+	if (!gpio_dev->pin_dir_bitmap)
+		return -ENOMEM;
+
+	gc->parent = dev;
+	gc->label = dev_name(dev);
+
+	gc->set = siul2_gpio_set;
+	gc->get = siul2_gpio_get;
+	gc->set_config = siul2_set_config;
+	gc->request = siul2_gpio_request;
+	gc->free = siul2_gpio_free;
+	gc->direction_output = siul2_gpio_dir_out;
+	gc->direction_input = siul2_gpio_dir_in;
+	gc->get_direction = siul2_gpio_get_dir;
+	gc->owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(dev, gc, gpio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "unable to add gpiochip\n");
+
+	return 0;
+}
+
+static const struct regmap_range s32g2_siul20_pad_yes_ranges[] = {
+	regmap_reg_range(SIUL2_PGPDO(0), SIUL2_PGPDO(0)),
+	regmap_reg_range(SIUL2_PGPDO(1), SIUL2_PGPDO(1)),
+	regmap_reg_range(SIUL2_PGPDO(2), SIUL2_PGPDO(2)),
+	regmap_reg_range(SIUL2_PGPDO(3), SIUL2_PGPDO(3)),
+	regmap_reg_range(SIUL2_PGPDO(4), SIUL2_PGPDO(4)),
+	regmap_reg_range(SIUL2_PGPDO(5), SIUL2_PGPDO(5)),
+	regmap_reg_range(SIUL2_PGPDO(6), SIUL2_PGPDO(6)),
+};
+
+static const struct regmap_access_table s32g2_siul20_pad_access_table = {
+	.yes_ranges	= s32g2_siul20_pad_yes_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul20_pad_yes_ranges),
+};
+
+static const struct regmap_range s32g2_siul21_pad_yes_ranges[] = {
+	regmap_reg_range(SIUL2_PGPDO(7), SIUL2_PGPDO(7)),
+	regmap_reg_range(SIUL2_PGPDO(9), SIUL2_PGPDO(9)),
+	regmap_reg_range(SIUL2_PGPDO(10), SIUL2_PGPDO(10)),
+	regmap_reg_range(SIUL2_PGPDO(11), SIUL2_PGPDO(11)),
+};
+
+static const struct regmap_access_table s32g2_siul21_pad_access_table = {
+	.yes_ranges	= s32g2_siul21_pad_yes_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul21_pad_yes_ranges),
+};
+
+static const struct regmap_access_table *s32g2_pad_access_table[] = {
+	&s32g2_siul20_pad_access_table,
+	&s32g2_siul21_pad_access_table
+};
+
+static_assert(ARRAY_SIZE(s32g2_pad_access_table) == S32G2_SIUL2_NUM);
+
+static const struct siul2_device_data s32g2_device_data = {
+	.pad_access	= s32g2_pad_access_table,
+	.reset_cnt	= true,
+};
+
+static const struct of_device_id siul2_gpio_dt_ids[] = {
+	{ .compatible = "nxp,s32g2-siul2-gpio", .data = &s32g2_device_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, siul2_gpio_dt_ids);
+
+static struct platform_driver siul2_gpio_driver = {
+	.driver			= {
+		.name		= "s32g2-siul2-gpio",
+		.owner		= THIS_MODULE,
+		.of_match_table = siul2_gpio_dt_ids,
+	},
+	.probe			= siul2_gpio_probe,
+};
+
+module_platform_driver(siul2_gpio_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP SIUL2 GPIO");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH 3/3] MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver

[Andrei Stefanescu]

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 8766f3e5e87e..b961cb2d041f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2686,10 +2686,12 @@ ARM/NXP S32G ARCHITECTURE
 R:	Chester Lin <chester62515@gmail.com>
 R:	Matthias Brugger <mbrugger@suse.com>
 R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
+R: 	Andrei Stefanescu <andrei.stefanescu@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:	arch/arm64/boot/dts/freescale/s32g*.dts*
+F: 	drivers/gpio/gpio-siul2-s32g2.c
 F:	drivers/pinctrl/nxp/
 
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
-- 
2.45.2

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Krzysztof Kozlowski]

On 26/08/2024 10:42, Andrei Stefanescu wrote:
> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
> (System Integration Unit Lite2) hardware module. There are two
> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
> SIUL2_1 for the rest.
> 
> The GPIOs are not fully contiguous, there are some gaps:
> - GPIO102 up to GPIO111(inclusive) are invalid
> - GPIO123 up to GPIO143(inclusive) are invalid
> 
> Some GPIOs are input only(i.e. GPI182) though this restriction
> is not yet enforced in code.
> 
> This patch adds basic GPIO functionality(no interrupts, no
> suspend/resume functions).
> 
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> ---
>  drivers/gpio/Kconfig            |   8 +
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-siul2-s32g2.c | 607 ++++++++++++++++++++++++++++++++
>  3 files changed, 616 insertions(+)
>  create mode 100644 drivers/gpio/gpio-siul2-s32g2.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 58f43bcced7c..0c3c94daab0f 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -643,6 +643,14 @@ config GPIO_SIOX
>  	  Say yes here to support SIOX I/O devices. These are units connected
>  	  via a SIOX bus and have a number of fixed-direction I/O lines.
>  
> +config GPIO_SIUL2_S32G2
> +        tristate "GPIO driver for S32G2/S32G3"
> +        depends on OF_GPIO

depends on ARCH || COMPILE_TEST?
(at least that's my preference)

> +        help
> +          This enables support for the SIUL2 GPIOs found on the S32G2/S32G3
> +          chips. Say yes here to enable the SIUL2 to be used as an GPIO
> +          controller for S32G2/S32G3 platforms.
> +
>  config GPIO_SNPS_CREG
>  	bool "Synopsys GPIO via CREG (Control REGisters) driver"
>  	depends on ARC || COMPILE_TEST
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 64dd6d9d730d..fb6e770a64b9 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -149,6 +149,7 @@ obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
>  obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
>  obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
>  obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
> +obj-$(CONFIG_GPIO_SIUL2_S32G2)		+= gpio-siul2-s32g2.o
>  obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
>  obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
>  obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
> diff --git a/drivers/gpio/gpio-siul2-s32g2.c b/drivers/gpio/gpio-siul2-s32g2.c
> new file mode 100644
> index 000000000000..07df16299237
> --- /dev/null
> +++ b/drivers/gpio/gpio-siul2-s32g2.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * SIUL2 GPIO support.
> + *
> + * Copyright (c) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2018-2024 NXP
> +  */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/bitmap.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +/* 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 SIUL2_PGPDO(N)		(((N) ^ 1) * 2)
> +#define S32G2_SIUL2_NUM		2
> +#define S32G2_PADS_DTS_TAG_LEN	(7)
> +
> +#define SIUL2_GPIO_16_PAD_SIZE		16
> +
> +/**
> + * struct siul2_device_data  - platform data attached to the compatible.
> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
> + */
> +struct siul2_device_data {
> +	const struct regmap_access_table **pad_access;
> +	const bool reset_cnt;
> +};
> +
> +/**
> + * struct siul2_desc - describes a SIUL2 hw module.
> + * @pad_access: array of valid I/O pads.
> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
> + * @gpio_base: the first GPIO pin.
> + * @gpio_num: the number of GPIO pins.
> + */
> +struct siul2_desc {
> +	const struct regmap_access_table *pad_access;
> +	struct regmap *opadmap;
> +	struct regmap *ipadmap;
> +	u32 gpio_base;
> +	u32 gpio_num;
> +};
> +
> +/**
> + * struct siul2_gpio_dev - describes a group of GPIO pins.
> + * @platdata: the platform data.
> + * @siul2: SIUL2_0 and SIUL2_1 modules information.
> + * @pin_dir_bitmap: the bitmap with pin directions.
> + * @gc: the GPIO chip.
> + * @lock: mutual access to bitmaps.
> + */
> +struct siul2_gpio_dev {
> +	const struct siul2_device_data *platdata;
> +	struct siul2_desc siul2[S32G2_SIUL2_NUM];
> +	unsigned long *pin_dir_bitmap;
> +	struct gpio_chip gc;
> +	raw_spinlock_t lock;
> +};
> +
> +static inline int siul2_get_gpio_pinspec(struct platform_device *pdev,
> +					 struct of_phandle_args *pinspec,
> +					 unsigned int range_index)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +						range_index, pinspec);

Where do you drop the ref?

> +}
> +
> +static inline struct regmap *siul2_offset_to_regmap(struct siul2_gpio_dev *dev,
> +						    unsigned int offset,
> +						    bool input)
> +{
> +	struct siul2_desc *siul2;
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dev->siul2); i++) {
> +		siul2 = &dev->siul2[i];
> +		if (offset >= siul2->gpio_base &&
> +		    offset - siul2->gpio_base < siul2->gpio_num)
> +			return input ? siul2->ipadmap : siul2->opadmap;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
> +					    unsigned int gpio, int dir)

Drop inline from all above. No point.

> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&dev->lock, flags);
> +
> +	if (dir == GPIO_LINE_DIRECTION_IN)
> +		bitmap_clear(dev->pin_dir_bitmap, gpio, 1);
> +	else
> +		bitmap_set(dev->pin_dir_bitmap, gpio, 1);
> +
> +	raw_spin_unlock_irqrestore(&dev->lock, flags);
> +}
> +
> +static inline int siul2_get_direction(struct siul2_gpio_dev *dev,
> +				      unsigned int gpio)
> +{
> +	return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
> +						     GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static inline struct siul2_gpio_dev *to_siul2_gpio_dev(struct gpio_chip *chip)
> +{
> +	return container_of(chip, struct siul2_gpio_dev, gc);
> +}
> +
> +static int siul2_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
> +{
> +	struct siul2_gpio_dev *gpio_dev;
> +	int ret = 0;
> +
> +	ret = pinctrl_gpio_direction_input(chip, gpio);
> +	if (ret)
> +		return ret;
> +
> +	gpio_dev = to_siul2_gpio_dev(chip);
> +	siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_IN);
> +
> +	return 0;
> +}
> +
> +static int siul2_gpio_get_dir(struct gpio_chip *chip, unsigned int gpio)
> +{
> +	return siul2_get_direction(to_siul2_gpio_dev(chip), gpio);
> +}
> +
> +static unsigned int siul2_pin2pad(unsigned int pin)
> +{
> +	return pin / SIUL2_GPIO_16_PAD_SIZE;
> +}
> +
> +static u16 siul2_pin2mask(unsigned int pin)
> +{
> +	/**
> +	 * From Reference manual :
> +	 * PGPDOx[PPDOy] = GPDO(x × 16) + (15 - y)[PDO_(x × 16) + (15 - y)]
> +	 */
> +	return BIT(SIUL2_GPIO_16_PAD_SIZE - 1 - pin % SIUL2_GPIO_16_PAD_SIZE);
> +}
> +
> +static inline u32 siul2_get_pad_offset(unsigned int pad)
> +{
> +	return SIUL2_PGPDO(pad);
> +}
> +


...

> +
> +static struct regmap *common_regmap_init(struct platform_device *pdev,
> +					 struct regmap_config *conf,
> +					 const char *name)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	resource_size_t size;
> +	void __iomem *base;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	base = devm_ioremap_resource(dev, res);

There is a wrapper for both calls above, so use it.

> +	if (IS_ERR(base))
> +		return ERR_PTR(-ENOMEM);
> +
> +	size = resource_size(res);
> +	conf->val_bits = conf->reg_stride * 8;
> +	conf->max_register = size - conf->reg_stride;
> +	conf->name = name;
> +	conf->use_raw_spinlock = true;
> +
> +	if (conf->cache_type != REGCACHE_NONE)
> +		conf->num_reg_defaults_raw = size / conf->reg_stride;
> +
> +	return devm_regmap_init_mmio(dev, base, conf);
> +}
> +
> +static bool not_writable(__always_unused struct device *dev,
> +			 __always_unused unsigned int reg)
> +{
> +	return false;
> +}
> +
> +static struct regmap *init_padregmap(struct platform_device *pdev,
> +				     struct siul2_gpio_dev *gpio_dev,
> +				     int selector, bool input)
> +{
> +	const struct siul2_device_data *platdata = gpio_dev->platdata;
> +	struct regmap_config regmap_conf = siul2_regmap_conf;
> +	char dts_tag[S32G2_PADS_DTS_TAG_LEN];
> +	int err;
> +
> +	regmap_conf.reg_stride = 2;
> +
> +	if (selector != 0 && selector != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	regmap_conf.rd_table = platdata->pad_access[selector];
> +
> +	err = snprintf(dts_tag, ARRAY_SIZE(dts_tag),  "%cpads%d",
> +		       input ? 'i' : 'o', selector);
> +	if (err < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (input) {
> +		regmap_conf.writeable_reg = not_writable;
> +		regmap_conf.cache_type = REGCACHE_NONE;
> +	} else {
> +		regmap_conf.wr_table = platdata->pad_access[selector];
> +	}
> +
> +	return common_regmap_init(pdev, &regmap_conf, dts_tag);
> +}
> +
> +static int siul2_gpio_pads_init(struct platform_device *pdev,
> +				struct siul2_gpio_dev *gpio_dev)
> +{
> +	struct device *dev = &pdev->dev;
> +	size_t i;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
> +		gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
> +							    false);
> +		if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
> +			dev_err(dev,
> +				"Failed to initialize opad2%lu regmap config\n",
> +				i);
> +			return PTR_ERR(gpio_dev->siul2[i].opadmap);
> +		}
> +
> +		gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
> +							    true);
> +		if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
> +			dev_err(dev,
> +				"Failed to initialize ipad2%lu regmap config\n",
> +				i);
> +			return PTR_ERR(gpio_dev->siul2[i].ipadmap);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int siul2_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 siul2_gpio_remove_reserved_names(struct device *dev,
> +					    struct siul2_gpio_dev *gpio_dev,
> +					    char **names)
> +{
> +	struct device_node *np = dev->of_node;
> +	int num_ranges, i, j, ret;
> +	u32 base_gpio, num_gpio;
> +
> +	/* 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) {
> +			dev_err(dev, "Could not parse the start GPIO: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> +						 i * 2 + 1, &num_gpio);
> +		if (ret) {
> +			dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
> +			dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
> +			return -EINVAL;
> +		}
> +
> +		/* 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 siul2_gpio_populate_names(struct device *dev,
> +				     struct siul2_gpio_dev *gpio_dev)
> +{
> +	unsigned int num_index = 0;
> +	char ch_index = 'A';
> +	char **names;
> +	int i, ret;
> +
> +	names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
> +			     GFP_KERNEL);
> +	if (!names)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < S32G2_SIUL2_NUM; i++) {
> +		ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
> +				      names + gpio_dev->siul2[i].gpio_base,
> +				      &ch_index, &num_index);
> +		if (ret) {
> +			dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
> +				i);
> +			return ret;
> +		}
> +
> +		if (gpio_dev->platdata->reset_cnt)
> +			num_index = 0;
> +
> +		ch_index++;
> +	}
> +
> +	ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
> +	if (ret)
> +		return ret;
> +
> +	gpio_dev->gc.names = (const char *const *)names;
> +
> +	return 0;
> +}
> +
> +static int siul2_gpio_probe(struct platform_device *pdev)
> +{
> +	struct siul2_gpio_dev *gpio_dev;
> +	struct device *dev = &pdev->dev;
> +	struct of_phandle_args pinspec;
> +	struct gpio_chip *gc;
> +	size_t bitmap_size;
> +	int ret = 0;
> +	size_t i;
> +
> +	gpio_dev = devm_kzalloc(dev, sizeof(*gpio_dev), GFP_KERNEL);
> +	if (!gpio_dev)
> +		return -ENOMEM;
> +
> +	gpio_dev->platdata = of_device_get_match_data(dev);
> +
> +	for (i = 0; i < S32G2_SIUL2_NUM; i++)
> +		gpio_dev->siul2[i].pad_access =
> +			gpio_dev->platdata->pad_access[i];
> +
> +	ret = siul2_gpio_pads_init(pdev, gpio_dev);
> +	if (ret)
> +		return ret;
> +
> +	gc = &gpio_dev->gc;
> +
> +	platform_set_drvdata(pdev, gpio_dev);
> +
> +	raw_spin_lock_init(&gpio_dev->lock);

Why do you use raw spin? Are you sure you need it (some people just
replace it thinking this will help them in PREEMPT_RT without actually
thinking if it is needed). IOW, do you have here irqchip anywhere?

> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
> +		ret = siul2_get_gpio_pinspec(pdev, &pinspec, i);
> +		if (ret) {
> +			dev_err(dev,
> +				"unable to get pinspec %lu from device tree\n",
> +				i);
> +			return -EINVAL;
> +		}
> +
> +		if (pinspec.args_count != 3) {
> +			dev_err(dev, "Invalid pinspec count: %d\n",
> +				pinspec.args_count);
> +			return -EINVAL;
> +		}
> +
> +		gpio_dev->siul2[i].gpio_base = pinspec.args[1];
> +		gpio_dev->siul2[i].gpio_num = pinspec.args[2];
> +	}
> +
> +	gc->base = -1;
> +
> +	/* In some cases, there is a gap between the SIUL GPIOs. */
> +	gc->ngpio = gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_base +
> +		    gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_num;
> +
> +	ret = siul2_gpio_populate_names(&pdev->dev, gpio_dev);
> +	if (ret)
> +		return ret;
> +
> +	bitmap_size = gc->ngpio * sizeof(*gpio_dev->pin_dir_bitmap);
> +	gpio_dev->pin_dir_bitmap = devm_bitmap_zalloc(dev, bitmap_size,
> +						      GFP_KERNEL);
> +	if (!gpio_dev->pin_dir_bitmap)
> +		return -ENOMEM;
> +
> +	gc->parent = dev;
> +	gc->label = dev_name(dev);
> +
> +	gc->set = siul2_gpio_set;
> +	gc->get = siul2_gpio_get;
> +	gc->set_config = siul2_set_config;
> +	gc->request = siul2_gpio_request;
> +	gc->free = siul2_gpio_free;
> +	gc->direction_output = siul2_gpio_dir_out;
> +	gc->direction_input = siul2_gpio_dir_in;
> +	gc->get_direction = siul2_gpio_get_dir;
> +	gc->owner = THIS_MODULE;
> +
> +	ret = devm_gpiochip_add_data(dev, gc, gpio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to add gpiochip\n");
> +
> +	return 0;
> +}
> +
> +static const struct regmap_range s32g2_siul20_pad_yes_ranges[] = {
> +	regmap_reg_range(SIUL2_PGPDO(0), SIUL2_PGPDO(0)),
> +	regmap_reg_range(SIUL2_PGPDO(1), SIUL2_PGPDO(1)),
> +	regmap_reg_range(SIUL2_PGPDO(2), SIUL2_PGPDO(2)),
> +	regmap_reg_range(SIUL2_PGPDO(3), SIUL2_PGPDO(3)),
> +	regmap_reg_range(SIUL2_PGPDO(4), SIUL2_PGPDO(4)),
> +	regmap_reg_range(SIUL2_PGPDO(5), SIUL2_PGPDO(5)),
> +	regmap_reg_range(SIUL2_PGPDO(6), SIUL2_PGPDO(6)),
> +};
> +
> +static const struct regmap_access_table s32g2_siul20_pad_access_table = {
> +	.yes_ranges	= s32g2_siul20_pad_yes_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul20_pad_yes_ranges),
> +};
> +
> +static const struct regmap_range s32g2_siul21_pad_yes_ranges[] = {
> +	regmap_reg_range(SIUL2_PGPDO(7), SIUL2_PGPDO(7)),
> +	regmap_reg_range(SIUL2_PGPDO(9), SIUL2_PGPDO(9)),
> +	regmap_reg_range(SIUL2_PGPDO(10), SIUL2_PGPDO(10)),
> +	regmap_reg_range(SIUL2_PGPDO(11), SIUL2_PGPDO(11)),
> +};
> +
> +static const struct regmap_access_table s32g2_siul21_pad_access_table = {
> +	.yes_ranges	= s32g2_siul21_pad_yes_ranges,
> +	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul21_pad_yes_ranges),
> +};
> +
> +static const struct regmap_access_table *s32g2_pad_access_table[] = {
> +	&s32g2_siul20_pad_access_table,
> +	&s32g2_siul21_pad_access_table
> +};
> +
> +static_assert(ARRAY_SIZE(s32g2_pad_access_table) == S32G2_SIUL2_NUM);
> +
> +static const struct siul2_device_data s32g2_device_data = {
> +	.pad_access	= s32g2_pad_access_table,
> +	.reset_cnt	= true,
> +};
> +
> +static const struct of_device_id siul2_gpio_dt_ids[] = {
> +	{ .compatible = "nxp,s32g2-siul2-gpio", .data = &s32g2_device_data },

Why do you have match data? There are no other variants.

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, siul2_gpio_dt_ids);
> +
> +static struct platform_driver siul2_gpio_driver = {
> +	.driver			= {
> +		.name		= "s32g2-siul2-gpio",
> +		.owner		= THIS_MODULE,

Oh.... It's still dissappointing to see people trying to usptream their
10 or 15 year old drivers. Drop. Start from NEW DRIVER when writing new
driver. Not from 10-15 year old downstream driver.



Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: gpio: add schema for NXP S32G2/S32G3 SoCs

[Krzysztof Kozlowski]

On 26/08/2024 10:42, Andrei Stefanescu wrote:
> Add the DT schema for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> 

A nit, subject: drop second/last, redundant "schema". The "dt-bindings"
prefix is already stating that these are bindings/schema.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> ---
>  .../bindings/gpio/nxp,gpio-siul2-s32g2.yaml   | 134 ++++++++++++++++++
>  1 file changed, 134 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml b/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml
> new file mode 100644
> index 000000000000..fba41a18d4c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/nxp,gpio-siul2-s32g2.yaml

Filename matching compatible.

> @@ -0,0 +1,134 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/nxp,gpio-siul2-s32g2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2 SIUL2 GPIO controller
> +
> +maintainers:
> +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> +  - Larisa Grigore <larisa.grigore@nxp.com>
> +  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.


> +  Support for the SIUL2 GPIOs found on the S32G2 and S32G3
> +  chips. It includes an IRQ controller for all pins which have
> +  an EIRQ associated.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: nxp,s32g2-siul2-gpio
> +
> +  reg:
> +    description: |
> +      A list of register regions for configuring interrupts,
> +      GPIO output values and reading GPIO input values.

Drop description, obvious. It cannot be anything else.

> +    items:
> +      - description: PGPDO (output value) registers for SIUL2_0
> +      - description: PGPDO (output value) registers for SIUL2_1
> +      - description: PGPDI (input value) registers for SIUL2_0
> +      - description: PGPDI (input value) registers for SIUL2_1
> +      - description: EIRQ (interrupt) configuration registers from SIUL2_1
> +      - description: EIRQ IMCR registers for interrupt muxing between pads
> +
> +  reg-names:
> +    items:
> +      - const: opads0
> +      - const: opads1
> +      - const: ipads0
> +      - const: ipads1
> +      - const: eirqs
> +      - const: eirq-imcrs
> +
> +  interrupts:
> +    description:
> +      The port interrupt shared by all 32 EIRQs.

Missing items. Look at existing examples. There is no code like this.

> +
> +  gpio-controller:
> +    description:
> +      Marks the device node as a gpio controller

Drop description, obvious.

> +
> +  "#gpio-cells":
> +    description: |
> +      Should be two. The first cell is the pin number and
> +      the second cell is used to specify the gpio polarity
> +      0 = active high
> +      1 = active low

This binding is nowhere near what we have in kernel. Please do not
re-invent stuff. Take recent binding and customize it.

Don't repeat constraints in free form text.

> +
> +  interrupt-controller:
> +    description:
> +      Marks the device node as an interrupt controller

Really? Drop.

> +
> +  "#interrupt-cells":
> +    const: 2
> +    description:
> +      Refer to ../interrupt-controller/interrupts.txt for more details.

Drop description, useless.

> +
> +  gpio-ranges:
> +    description:
> +      Interaction with the PINCTRL subsystem
> +      Refer to gpio.txt for more details.

Oh... drop. Missing constraints.

> +
> +  gpio-reserved-ranges:
> +    description:
> +      A list of start GPIO and number of GPIO pairs which are unusable.
> +      Refer to gpio.txt for more details.

Drop. Missing constraints.

> +
> +patternProperties:
> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> +    additionalProperties: false
> +    type: object
> +    properties:
> +      gpio-hog: true
> +      gpios: true
> +      input: true
> +      output-high: true
> +      output-low: true
> +      line-name: true
> +    required:
> +      - gpio-hog
> +      - gpios

Drop all this and use simplified form.
https://elixir.bootlin.com/linux/v6.9-rc3/source/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.yaml#L45

This applies to all your patches in the future as well.

> +
> +required:
> +  - compatible
> +  - interrupts
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    gpio: siul2-gpio@4009d700 {
> +        compatible = "nxp,s32g2-siul2-gpio";
> +        reg = <0x4009d700 0x10>,
> +              <0x44011700 0x18>,
> +              <0x4009d740 0x10>,
> +              <0x44011740 0x18>,
> +              <0x44010010 0xb4>,
> +              <0x44011078 0x80>;
> +        reg-names = "opads0", "opads1", "ipads0",
> +                    "ipads1", "eirqs", "eirq-imcrs";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +                      /* GPIO 0-101 */
> +        gpio-ranges = <&pinctrl 0   0   102>,
> +                      /* GPIO 112-190 */
> +                      <&pinctrl 112 112 79>;
> +        gpio-reserved-ranges = <102 10>, <123 21>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
> +        status = "okay";

Drop.

> +    };

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: gpio: add schema for NXP S32G2/S32G3 SoCs

[Krzysztof Kozlowski]

On 26/08/2024 10:42, Andrei Stefanescu wrote:
> Add the DT schema for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> 
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>

BTW, these emails bounce. What is the intention of having them here? How
did they contribute to the SCHEMA file? We all are going to get bounces
from replying to you...

Best regards,
Krzysztof

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Linus Walleij]

Hi Andrei,

thanks for your patch!

On Mon, Aug 26, 2024 at 10:43 AM Andrei Stefanescu
<andrei.stefanescu@oss.nxp.com> wrote:

> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
> (System Integration Unit Lite2) hardware module. There are two
> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
> SIUL2_1 for the rest.
>
> The GPIOs are not fully contiguous, there are some gaps:
> - GPIO102 up to GPIO111(inclusive) are invalid
> - GPIO123 up to GPIO143(inclusive) are invalid
>
> Some GPIOs are input only(i.e. GPI182) though this restriction
> is not yet enforced in code.
>
> This patch adds basic GPIO functionality(no interrupts, no
> suspend/resume functions).
>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

As Krzysztof points out: the driver is based on something really old and
needs an overhaul. Luckily GPIO drivers are not that big so it should be
pretty straight-forward.

> +config GPIO_SIUL2_S32G2
> +        tristate "GPIO driver for S32G2/S32G3"
> +        depends on OF_GPIO

select REGMAP?

You are using it.

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>

Drop this include.

> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/bitmap.h>

Really?

> +       raw_spin_lock_irqsave(&dev->lock, flags);
> +
> +       if (dir == GPIO_LINE_DIRECTION_IN)
> +               bitmap_clear(dev->pin_dir_bitmap, gpio, 1);
> +       else
> +               bitmap_set(dev->pin_dir_bitmap, gpio, 1);

This is just an unsigned long, just use the nonatomic
__clear_bit() and __set_bit()
from <linux/bitops.h>.

> +       gc->set = siul2_gpio_set;
> +       gc->get = siul2_gpio_get;
> +       gc->set_config = siul2_set_config;
> +       gc->request = siul2_gpio_request;
> +       gc->free = siul2_gpio_free;
> +       gc->direction_output = siul2_gpio_dir_out;
> +       gc->direction_input = siul2_gpio_dir_in;
> +       gc->get_direction = siul2_gpio_get_dir;

Since it is backed by proper pin control I would expect
a generic .set_config() implementation, but no hurry with that
I suppose.

Yours,
Linus Walleij

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Hi Krzysztof,

On 26/08/2024 12:10, Krzysztof Kozlowski wrote:
> On 26/08/2024 10:42, Andrei Stefanescu wrote:

Thank you for the quick review!

>> +	raw_spin_lock_init(&gpio_dev->lock);
> 
> Why do you use raw spin? Are you sure you need it (some people just
> replace it thinking this will help them in PREEMPT_RT without actually
> thinking if it is needed). IOW, do you have here irqchip anywhere?

I don't have an irqchip in this current patch series. There are, however,
other patches which add support for interrupts and implementations for
power management callbacks. I thought it would be easier for review if I
sent those after the current series gets merged.

>> +
>> +static const struct of_device_id siul2_gpio_dt_ids[] = {
>> +	{ .compatible = "nxp,s32g2-siul2-gpio", .data = &s32g2_device_data },
> 
> Why do you have match data? There are no other variants.


We do have another match data in our downstream version. Could I keep it
here or should I remove it?


I will fix the other comments in v2.

Best regards,
Andrei

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Hi Linus,

On 26/08/2024 12:42, Linus Walleij wrote:
> Hi Andrei,
> 
> thanks for your patch!
> 
> On Mon, Aug 26, 2024 at 10:43 AM Andrei Stefanescu
> <andrei.stefanescu@oss.nxp.com> wrote:
> 

Thank you for the quick review!

> 
> As Krzysztof points out: the driver is based on something really old and
> needs an overhaul. Luckily GPIO drivers are not that big so it should be
> pretty straight-forward.
> 

Are there other changes, apart from the ones in this email and those already
suggested by Krzysztof, that I should consider for v2?

I will fix the other comments in v2.

Best regards,
Andrei

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Krzysztof Kozlowski]

On 26/08/2024 16:03, Andrei Stefanescu wrote:
> Hi Krzysztof,
> 
> On 26/08/2024 12:10, Krzysztof Kozlowski wrote:
>> On 26/08/2024 10:42, Andrei Stefanescu wrote:
> 
> Thank you for the quick review!
> 
>>> +	raw_spin_lock_init(&gpio_dev->lock);
>>
>> Why do you use raw spin? Are you sure you need it (some people just
>> replace it thinking this will help them in PREEMPT_RT without actually
>> thinking if it is needed). IOW, do you have here irqchip anywhere?
> 
> I don't have an irqchip in this current patch series. There are, however,
> other patches which add support for interrupts and implementations for
> power management callbacks. I thought it would be easier for review if I
> sent those after the current series gets merged.

power management callbacks do not need raw spinlock.

> 
>>> +
>>> +static const struct of_device_id siul2_gpio_dt_ids[] = {
>>> +	{ .compatible = "nxp,s32g2-siul2-gpio", .data = &s32g2_device_data },
>>
>> Why do you have match data? There are no other variants.
> 
> 
> We do have another match data in our downstream version. Could I keep it
> here or should I remove it?

If you already work on new version and you are going to send it soon,
then it is fine. Otherwise you will add matchdata once there is such need.

Best regards,
Krzysztof

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrew Lunn]

> +static inline int siul2_get_gpio_pinspec(struct platform_device *pdev,
> +					 struct of_phandle_args *pinspec,
> +					 unsigned int range_index)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +						range_index, pinspec);
> +}

In general, inline should be avoided. The compiler is better at
deciding than humans.

	Andrew

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on robh/for-next linus/master v6.11-rc5 next-20240826]
[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-gpio-add-schema-for-NXP-S32G2-S32G3-SoCs/20240826-164853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240826084214.2368673-3-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240827/202408271011.hpcNokNu-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 08e5a1de8227512d4774a534b91cb2353cef6284)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408271011.hpcNokNu-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/202408271011.hpcNokNu-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/gpio/gpio-siul2-s32g2.c:342:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
     341 |                                 "Failed to initialize opad2%lu regmap config\n",
         |                                                            ~~~
         |                                                            %zu
     342 |                                 i);
         |                                 ^
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:351:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
     350 |                                 "Failed to initialize ipad2%lu regmap config\n",
         |                                                            ~~~
         |                                                            %zu
     351 |                                 i);
         |                                 ^
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:499:5: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat]
     498 |                                 "unable to get pinspec %lu from device tree\n",
         |                                                        ~~~
         |                                                        %zu
     499 |                                 i);
         |                                 ^
   include/linux/dev_printk.h:154:65: note: expanded from macro 'dev_err'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                                ~~~     ^~~~~~~~~~~
   include/linux/dev_printk.h:110:23: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ~~~    ^~~~~~~~~~~
   9 warnings generated.


vim +342 drivers/gpio/gpio-siul2-s32g2.c

   329	
   330	static int siul2_gpio_pads_init(struct platform_device *pdev,
   331					struct siul2_gpio_dev *gpio_dev)
   332	{
   333		struct device *dev = &pdev->dev;
   334		size_t i;
   335	
   336		for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
   337			gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
   338								    false);
   339			if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
   340				dev_err(dev,
   341					"Failed to initialize opad2%lu regmap config\n",
 > 342					i);
   343				return PTR_ERR(gpio_dev->siul2[i].opadmap);
   344			}
   345	
   346			gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
   347								    true);
   348			if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
   349				dev_err(dev,
   350					"Failed to initialize ipad2%lu regmap config\n",
   351					i);
   352				return PTR_ERR(gpio_dev->siul2[i].ipadmap);
   353			}
   354		}
   355	
   356		return 0;
   357	}
   358	

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

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on robh/for-next linus/master v6.11-rc5 next-20240826]
[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-gpio-add-schema-for-NXP-S32G2-S32G3-SoCs/20240826-164853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240826084214.2368673-3-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20240827/202408271250.W4HQp7ZZ-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240827/202408271250.W4HQp7ZZ-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/202408271250.W4HQp7ZZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/platform_device.h:13,
                    from drivers/gpio/gpio-siul2-s32g2.c:13:
   drivers/gpio/gpio-siul2-s32g2.c: In function 'siul2_gpio_pads_init':
>> drivers/gpio/gpio-siul2-s32g2.c:341:33: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     341 |                                 "Failed to initialize opad2%lu regmap config\n",
         |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:154:56: note: in expansion of macro 'dev_fmt'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:340:25: note: in expansion of macro 'dev_err'
     340 |                         dev_err(dev,
         |                         ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:341:62: note: format string is defined here
     341 |                                 "Failed to initialize opad2%lu regmap config\n",
         |                                                            ~~^
         |                                                              |
         |                                                              long unsigned int
         |                                                            %u
   drivers/gpio/gpio-siul2-s32g2.c:350:33: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     350 |                                 "Failed to initialize ipad2%lu regmap config\n",
         |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:154:56: note: in expansion of macro 'dev_fmt'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:349:25: note: in expansion of macro 'dev_err'
     349 |                         dev_err(dev,
         |                         ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:350:62: note: format string is defined here
     350 |                                 "Failed to initialize ipad2%lu regmap config\n",
         |                                                            ~~^
         |                                                              |
         |                                                              long unsigned int
         |                                                            %u
   drivers/gpio/gpio-siul2-s32g2.c: In function 'siul2_gpio_probe':
   drivers/gpio/gpio-siul2-s32g2.c:498:33: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     498 |                                 "unable to get pinspec %lu from device tree\n",
         |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:154:56: note: in expansion of macro 'dev_fmt'
     154 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:497:25: note: in expansion of macro 'dev_err'
     497 |                         dev_err(dev,
         |                         ^~~~~~~
   drivers/gpio/gpio-siul2-s32g2.c:498:58: note: format string is defined here
     498 |                                 "unable to get pinspec %lu from device tree\n",
         |                                                        ~~^
         |                                                          |
         |                                                          long unsigned int
         |                                                        %u


vim +341 drivers/gpio/gpio-siul2-s32g2.c

   329	
   330	static int siul2_gpio_pads_init(struct platform_device *pdev,
   331					struct siul2_gpio_dev *gpio_dev)
   332	{
   333		struct device *dev = &pdev->dev;
   334		size_t i;
   335	
   336		for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
   337			gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
   338								    false);
   339			if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
   340				dev_err(dev,
 > 341					"Failed to initialize opad2%lu regmap config\n",
   342					i);
   343				return PTR_ERR(gpio_dev->siul2[i].opadmap);
   344			}
   345	
   346			gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
   347								    true);
   348			if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
   349				dev_err(dev,
   350					"Failed to initialize ipad2%lu regmap config\n",
   351					i);
   352				return PTR_ERR(gpio_dev->siul2[i].ipadmap);
   353			}
   354		}
   355	
   356		return 0;
   357	}
   358	

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

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Hi Krzysztof,


>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>> +					 struct regmap_config *conf,
>> +					 const char *name)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	resource_size_t size;
>> +	void __iomem *base;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	base = devm_ioremap_resource(dev, res);
> 
> There is a wrapper for both calls above, so use it.

I am not sure I can change this because I also use the `resource_size`
call below in order to initialize the regmap_config. 
Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
the resource via a pointer.

I saw the `devm_platform_get_and_ioremap_resource` function but that one
retrieves the resource based on the index. I would like to keep identifying
the resource by its name instead of its index.

Would you agree to keep the existing implementation in this case?

> 
>> +	if (IS_ERR(base))
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	size = resource_size(res);
>> +	conf->val_bits = conf->reg_stride * 8;
>> +	conf->max_register = size - conf->reg_stride;
>> +	conf->name = name;
>> +	conf->use_raw_spinlock = true;
>> +
>> +	if (conf->cache_type != REGCACHE_NONE)
>> +		conf->num_reg_defaults_raw = size / conf->reg_stride;
>> +
>> +	return devm_regmap_init_mmio(dev, base, conf);

Best regards,
Andrei

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Krzysztof Kozlowski]

On 06/09/2024 10:43, Andrei Stefanescu wrote:
> Hi Krzysztof,
> 
> 
>>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>>> +					 struct regmap_config *conf,
>>> +					 const char *name)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct resource *res;
>>> +	resource_size_t size;
>>> +	void __iomem *base;
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>> +	if (!res) {
>>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>> +	base = devm_ioremap_resource(dev, res);
>>
>> There is a wrapper for both calls above, so use it.
> 
> I am not sure I can change this because I also use the `resource_size`
> call below in order to initialize the regmap_config. 
> Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
> the resource via a pointer.
> 
> I saw the `devm_platform_get_and_ioremap_resource` function but that one
> retrieves the resource based on the index. I would like to keep identifying
> the resource by its name instead of its index.

So add the wrapper. Or explain what's wrong with indices?

> 
> Would you agree to keep the existing implementation in this case?
Best regards,
Krzysztof

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

On 06/09/2024 12:39, Krzysztof Kozlowski wrote:
> On 06/09/2024 10:43, Andrei Stefanescu wrote:
>> Hi Krzysztof,
>>
>>
>>>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>>>> +					 struct regmap_config *conf,
>>>> +					 const char *name)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct resource *res;
>>>> +	resource_size_t size;
>>>> +	void __iomem *base;
>>>> +
>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>>> +	if (!res) {
>>>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>>>> +		return ERR_PTR(-EINVAL);
>>>> +	}
>>>> +
>>>> +	base = devm_ioremap_resource(dev, res);
>>>
>>> There is a wrapper for both calls above, so use it.
>>
>> I am not sure I can change this because I also use the `resource_size`
>> call below in order to initialize the regmap_config. 
>> Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
>> the resource via a pointer.
>>
>> I saw the `devm_platform_get_and_ioremap_resource` function but that one
>> retrieves the resource based on the index. I would like to keep identifying
>> the resource by its name instead of its index.
> 
> So add the wrapper. Or explain what's wrong with indices?

There's nothing wrong but I prefer to not force an order. I will
add a wrapper then.

Best regards,
Andrei

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Krzysztof Kozlowski]

On 06/09/2024 11:45, Andrei Stefanescu wrote:
> On 06/09/2024 12:39, Krzysztof Kozlowski wrote:
>> On 06/09/2024 10:43, Andrei Stefanescu wrote:
>>> Hi Krzysztof,
>>>
>>>
>>>>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>>>>> +					 struct regmap_config *conf,
>>>>> +					 const char *name)
>>>>> +{
>>>>> +	struct device *dev = &pdev->dev;
>>>>> +	struct resource *res;
>>>>> +	resource_size_t size;
>>>>> +	void __iomem *base;
>>>>> +
>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>>>> +	if (!res) {
>>>>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>>>>> +		return ERR_PTR(-EINVAL);
>>>>> +	}
>>>>> +
>>>>> +	base = devm_ioremap_resource(dev, res);
>>>>
>>>> There is a wrapper for both calls above, so use it.
>>>
>>> I am not sure I can change this because I also use the `resource_size`
>>> call below in order to initialize the regmap_config. 
>>> Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
>>> the resource via a pointer.
>>>
>>> I saw the `devm_platform_get_and_ioremap_resource` function but that one
>>> retrieves the resource based on the index. I would like to keep identifying
>>> the resource by its name instead of its index.
>>
>> So add the wrapper. Or explain what's wrong with indices?
> 
> There's nothing wrong but I prefer to not force an order. I will
> add a wrapper then.

Order is forced. You cannot change it. I don't understand your rationale.

Best regards,
Krzysztof

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

On 06/09/2024 12:53, Krzysztof Kozlowski wrote:
> On 06/09/2024 11:45, Andrei Stefanescu wrote:
>> On 06/09/2024 12:39, Krzysztof Kozlowski wrote:
>>> On 06/09/2024 10:43, Andrei Stefanescu wrote:
>>>> Hi Krzysztof,
>>>>
>>>>
>>>>>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>>>>>> +					 struct regmap_config *conf,
>>>>>> +					 const char *name)
>>>>>> +{
>>>>>> +	struct device *dev = &pdev->dev;
>>>>>> +	struct resource *res;
>>>>>> +	resource_size_t size;
>>>>>> +	void __iomem *base;
>>>>>> +
>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>>>>> +	if (!res) {
>>>>>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>> +	}
>>>>>> +
>>>>>> +	base = devm_ioremap_resource(dev, res);
>>>>>
>>>>> There is a wrapper for both calls above, so use it.
>>>>
>>>> I am not sure I can change this because I also use the `resource_size`
>>>> call below in order to initialize the regmap_config. 
>>>> Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
>>>> the resource via a pointer.
>>>>
>>>> I saw the `devm_platform_get_and_ioremap_resource` function but that one
>>>> retrieves the resource based on the index. I would like to keep identifying
>>>> the resource by its name instead of its index.
>>>
>>> So add the wrapper. Or explain what's wrong with indices?
>>
>> There's nothing wrong but I prefer to not force an order. I will
>> add a wrapper then.
> 
> Order is forced. You cannot change it. I don't understand your rationale.
> 
> Best regards,
> Krzysztof
> 

By order I mean the order in which the memory region descriptions are laid out
in the reg property. Currently, there is no issue if we, let's say, swap the order
of opads0 and opads1(as long as we keep the same change for the reg-names).

Just to double check, this would imply adding a new wrapper named
`devm_platform_get_and_ioremap_resource_byname` which would basically be
very similar to `devm_platform_get_and_ioremap_resource`. Is that ok?

Best regards,
Andrei

Re: [PATCH 2/3] drivers: gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Krzysztof Kozlowski]

On 06/09/2024 13:50, Andrei Stefanescu wrote:
> On 06/09/2024 12:53, Krzysztof Kozlowski wrote:
>> On 06/09/2024 11:45, Andrei Stefanescu wrote:
>>> On 06/09/2024 12:39, Krzysztof Kozlowski wrote:
>>>> On 06/09/2024 10:43, Andrei Stefanescu wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>>
>>>>>>> +static struct regmap *common_regmap_init(struct platform_device *pdev,
>>>>>>> +					 struct regmap_config *conf,
>>>>>>> +					 const char *name)
>>>>>>> +{
>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>> +	struct resource *res;
>>>>>>> +	resource_size_t size;
>>>>>>> +	void __iomem *base;
>>>>>>> +
>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
>>>>>>> +	if (!res) {
>>>>>>> +		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
>>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	base = devm_ioremap_resource(dev, res);
>>>>>>
>>>>>> There is a wrapper for both calls above, so use it.
>>>>>
>>>>> I am not sure I can change this because I also use the `resource_size`
>>>>> call below in order to initialize the regmap_config. 
>>>>> Unfortunately, `devm_platform_ioremap_resource_byname` doesn't also retrieve
>>>>> the resource via a pointer.
>>>>>
>>>>> I saw the `devm_platform_get_and_ioremap_resource` function but that one
>>>>> retrieves the resource based on the index. I would like to keep identifying
>>>>> the resource by its name instead of its index.
>>>>
>>>> So add the wrapper. Or explain what's wrong with indices?
>>>
>>> There's nothing wrong but I prefer to not force an order. I will
>>> add a wrapper then.
>>
>> Order is forced. You cannot change it. I don't understand your rationale.
>>
>> Best regards,
>> Krzysztof
>>
> 
> By order I mean the order in which the memory region descriptions are laid out
> in the reg property. Currently, there is no issue if we, let's say, swap the order
> of opads0 and opads1(as long as we keep the same change for the reg-names).

You cannot swap them. Order of items is fixed.

> 
> Just to double check, this would imply adding a new wrapper named
> `devm_platform_get_and_ioremap_resource_byname` which would basically be
> very similar to `devm_platform_get_and_ioremap_resource`. Is that ok?


That's ok for me.

Best regards,
Krzysztof