[PATCH v2 0/4] Enable Remote GPIO over RPMSG on i.MX Platform

[PATCH v2 0/4] Enable Remote GPIO over RPMSG on i.MX Platform

[Shenwei Wang]

Support the remote devices on the remote processor via the RPMSG bus on
i.MX platform.

The expected DTS layout structure is following:

    cm33: remoteproc-cm33 {
             compatible = "fsl,imx8ulp-cm33";

             rpmsg {
                     rpmsg-io-channel {
                             gpio@0 {
                                     compatible = "fsl,imx-rpmsg-gpio";
                                     reg = <0>;
                             };

                             gpio@1 {
                                     compatible = "fsl,imx-rpmsg-gpio";
                                     reg = <1>;
                             };

                             ...
                     };

                     rpmsg-i2c-channel {
                             i2c@0 {
                                     compatible = "fsl,imx-rpmsg-i2c";
                                     reg = <0>;
                             };
                     };

                     ...
             };
     };

Changes in v2:
 - re-implemented the gpio driver per Linus Walleij's feedback by using
   GPIOLIB_IRQCHIP helper library.
 - fix various format issue per Mathieu/Peng 's review comments.
 - update the yaml doc per Rob's feedback

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc: Linus Walleij <linus.walleij@linaro.org>

Shenwei Wang (4):
  dt-bindings: remoteproc: imx_rproc: Add "rpmsg" subnode support
  remoteproc: imx_rproc: Populate devices under "rpmsg" subnode
  gpio: imx-rpmsg: add imx-rpmsg GPIO driver
  arm64: dts: imx8ulp: Add rpmsg node under imx_rproc

 .../bindings/remoteproc/fsl,imx-rproc.yaml    | 129 +++++
 arch/arm64/boot/dts/freescale/imx8ulp.dtsi    |  27 +
 drivers/gpio/Kconfig                          |  17 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-imx-rpmsg.c                 | 488 ++++++++++++++++++
 drivers/remoteproc/imx_rproc.c                | 126 +++++
 include/linux/rpmsg/imx_rpmsg.h               |  48 ++
 7 files changed, 836 insertions(+)
 create mode 100644 drivers/gpio/gpio-imx-rpmsg.c
 create mode 100644 include/linux/rpmsg/imx_rpmsg.h

--
2.43.0

[PATCH v2 1/4] dt-bindings: remoteproc: imx_rproc: Add "rpmsg" subnode support

[Shenwei Wang]

Remote processors may announce multiple devices (e.g., I2C, GPIO) over
an RPMSG channel. These devices may require corresponding device tree
nodes, especially when acting as providers, to supply phandles for their
consumers.

Define an RPMSG node to work as a container for a group of RPMSG channels
under the imx_rproc node.

Each subnode within "rpmsg" represents an individual RPMSG channel. The
name of each subnode corresponds to the channel name as defined by the
remote processor.

All remote devices associated with a given channel are defined as child
nodes under the corresponding channel node.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 .../bindings/remoteproc/fsl,imx-rproc.yaml    | 129 ++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
index 57d75acb0b5e..16c7db4c5a79 100644
--- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
@@ -84,6 +84,98 @@ properties:
       This property is to specify the resource id of the remote processor in SoC
       which supports SCFW
 
+  rpmsg:
+    type: object
+    additionalProperties: false
+    description:
+      Present a group of RPMSG channel devices.
+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+      rpmsg-io-channel:
+        type: object
+        unevaluatedProperties: false
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+        patternProperties:
+          "gpio@[0-9a-f]+$":
+            type: object
+            unevaluatedProperties: false
+            properties:
+              compatible:
+                enum:
+                  - fsl,imx-rpmsg-gpio
+
+              reg:
+                maxItems: 1
+
+              "#gpio-cells":
+                const: 2
+
+              gpio-controller: true
+
+              interrupt-controller: true
+
+              "#interrupt-cells":
+                const: 2
+
+            required:
+              - compatible
+              - reg
+              - "#gpio-cells"
+              - "#interrupt-cells"
+
+            allOf:
+              - $ref: /schemas/gpio/gpio.yaml#
+              - $ref: /schemas/interrupt-controller.yaml#
+
+        required:
+          - '#address-cells'
+          - '#size-cells'
+
+      rpmsg-i2c-channel:
+        type: object
+        unevaluatedProperties: false
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+        patternProperties:
+          "i2c@[0-9a-f]+$":
+            type: object
+            unevaluatedProperties: false
+            properties:
+              compatible:
+                enum:
+                  - fsl,imx-rpmsg-i2c
+
+              reg:
+                maxItems: 1
+
+            required:
+              - compatible
+              - reg
+
+            allOf:
+              - $ref: /schemas/i2c/i2c-controller.yaml#
+
+        required:
+          - '#address-cells'
+          - '#size-cells'
+
 required:
   - compatible
 
@@ -146,5 +238,42 @@ examples:
                 &mu 3 1>;
       memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>, <&rsc_table>;
       syscon = <&src>;
+
+      rpmsg {
+        rpmsg-io-channel {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          gpio@0 {
+            compatible = "fsl,imx-rpmsg-gpio";
+            reg = <0>;
+            gpio-controller;
+            #gpio-cells = <2>;
+            #interrupt-cells = <2>;
+            interrupt-controller;
+            interrupt-parent = <&rpmsg_gpioa>;
+          };
+
+          gpio@1 {
+            compatible = "fsl,imx-rpmsg-gpio";
+            reg = <1>;
+            gpio-controller;
+            #gpio-cells = <2>;
+            #interrupt-cells = <2>;
+            interrupt-controller;
+            interrupt-parent = <&rpmsg_gpiob>;
+          };
+        };
+
+        rpmsg-i2c-channel {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          i2c@0 {
+            compatible = "fsl,imx-rpmsg-i2c";
+            reg = <0>;
+          };
+        };
+      };
     };
 ...
-- 
2.43.0

[PATCH v2 2/4] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode

[Shenwei Wang]

Register the RPMsg channel driver and populate remote devices defined
under the "rpmsg" subnode upon receiving their notification messages.

The following illustrates the expected DTS layout structure:

	cm33: remoteproc-cm33 {
		compatible = "fsl,imx8ulp-cm33";

		rpmsg {
			rpmsg-io-channel {
				gpio@0 {
					compatible = "fsl,imx-rpmsg-gpio";
					reg = <0>;
				};

				gpio@1 {
					compatible = "fsl,imx-rpmsg-gpio";
					reg = <1>;
				};

				...
			};

			rpmsg-i2c-channel {
				i2c@0 {
					compatible = "fsl,imx-rpmsg-i2c";
					reg = <0>;
				};
			};

			...
		};
	};

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/remoteproc/imx_rproc.c  | 126 ++++++++++++++++++++++++++++++++
 include/linux/rpmsg/imx_rpmsg.h |  48 ++++++++++++
 2 files changed, 174 insertions(+)
 create mode 100644 include/linux/rpmsg/imx_rpmsg.h

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index a6eef0080ca9..07ea6127e34e 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -8,6 +8,7 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/firmware/imx/sci.h>
+#include <linux/rpmsg/imx_rpmsg.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mailbox_client.h>
@@ -15,6 +16,8 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
@@ -22,6 +25,7 @@
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
+#include <linux/rpmsg.h>
 #include <linux/workqueue.h>
 
 #include "imx_rproc.h"
@@ -1084,6 +1088,126 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
 	return NOTIFY_DONE;
 }
 
+struct imx_rpmsg_driver {
+	struct rpmsg_driver rpdrv;
+	void *driver_data;
+};
+
+static char *channel_device_map[][2] = {
+	{"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
+	{"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},
+};
+
+static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev,
+	void *data, int len, void *priv, u32 src)
+{
+	struct imx_rpmsg_driver_data *drvdata;
+
+	drvdata = dev_get_drvdata(&rpdev->dev);
+	if (drvdata && drvdata->rx_callback)
+		return drvdata->rx_callback(rpdev, data, len, priv, src);
+
+	return 0;
+}
+
+static void imx_rpmsg_endpoint_remove(struct rpmsg_device *rpdev)
+{
+	of_platform_depopulate(&rpdev->dev);
+}
+
+static int imx_rpmsg_endpoint_probe(struct rpmsg_device *rpdev)
+{
+	struct imx_rpmsg_driver_data *drvdata;
+	struct imx_rpmsg_driver *imx_rpdrv;
+	struct device *dev = &rpdev->dev;
+	struct of_dev_auxdata *auxdata;
+	struct rpmsg_driver *rpdrv;
+	int i;
+
+	rpdrv = container_of(dev->driver, struct rpmsg_driver, drv);
+	imx_rpdrv = container_of(rpdrv, struct imx_rpmsg_driver, rpdrv);
+
+	if (!imx_rpdrv->driver_data)
+		return -EINVAL;
+
+	drvdata = devm_kmemdup(dev, imx_rpdrv->driver_data, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	i = drvdata->map_idx;
+	if (i >= ARRAY_SIZE(channel_device_map))
+		return -ENODEV;
+
+	auxdata = devm_kzalloc(dev, sizeof(*auxdata)*2, GFP_KERNEL);
+	if (!auxdata)
+		return -ENOMEM;
+
+	drvdata->rpdev = rpdev;
+	auxdata[0].compatible = channel_device_map[i][1];
+	auxdata[0].platform_data = drvdata;
+	dev_set_drvdata(dev, drvdata);
+
+	of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
+	of_node_put(drvdata->channel_node);
+
+	return 0;
+}
+
+static int imx_of_rpmsg_node_init(struct platform_device *pdev)
+{
+	struct device_node *np __free(device_node), *channel;
+	struct imx_rpmsg_driver_data *driver_data;
+	struct imx_rpmsg_driver *rp_driver;
+	struct rpmsg_device_id *rpdev_id;
+	int i, ret;
+
+	int count = ARRAY_SIZE(channel_device_map);
+	struct device *dev = &pdev->dev;
+
+	np = of_get_child_by_name(dev->of_node, "rpmsg");
+	if (!np)
+		return 0;
+
+	for (i = 0; i < count; i++) {
+		ret = -ENOMEM;
+		channel = of_get_child_by_name(np, channel_device_map[i][0]);
+		if (!channel)
+			continue;
+
+		rpdev_id = devm_kzalloc(dev, sizeof(*rpdev_id)*2, GFP_KERNEL);
+		if (!rpdev_id)
+			break;
+		strscpy(rpdev_id[0].name, channel_device_map[i][0], RPMSG_NAME_SIZE);
+
+		rp_driver = devm_kzalloc(dev, sizeof(*rp_driver), GFP_KERNEL);
+		if (!rp_driver)
+			break;
+
+		driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
+		if (!driver_data)
+			break;
+
+		ret = 0;
+		driver_data->rproc_name = dev->of_node->name;
+		driver_data->channel_node = channel;
+		driver_data->map_idx = i;
+
+		rp_driver->rpdrv.drv.name = channel_device_map[i][0];
+		rp_driver->rpdrv.id_table = rpdev_id;
+		rp_driver->rpdrv.probe = imx_rpmsg_endpoint_probe;
+		rp_driver->rpdrv.remove = imx_rpmsg_endpoint_remove;
+		rp_driver->rpdrv.callback = imx_rpmsg_endpoint_cb;
+		rp_driver->driver_data = driver_data;
+
+		register_rpmsg_driver(&rp_driver->rpdrv);
+	}
+
+	if ((ret < 0) && channel)
+		of_node_put(channel);
+
+	return ret;
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1177,6 +1301,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_clk;
 	}
 
+	imx_of_rpmsg_node_init(pdev);
+
 	return 0;
 
 err_put_clk:
diff --git a/include/linux/rpmsg/imx_rpmsg.h b/include/linux/rpmsg/imx_rpmsg.h
new file mode 100644
index 000000000000..04a5ad2d4a1d
--- /dev/null
+++ b/include/linux/rpmsg/imx_rpmsg.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2025 NXP */
+
+/*
+ * @file linux/imx_rpmsg.h
+ *
+ * @brief Global header file for iMX RPMSG
+ *
+ * @ingroup RPMSG
+ */
+#ifndef __LINUX_IMX_RPMSG_H__
+#define __LINUX_IMX_RPMSG_H__
+
+/* Category define */
+#define IMX_RMPSG_LIFECYCLE	1
+#define IMX_RPMSG_PMIC		2
+#define IMX_RPMSG_AUDIO		3
+#define IMX_RPMSG_KEY		4
+#define IMX_RPMSG_GPIO		5
+#define IMX_RPMSG_RTC		6
+#define IMX_RPMSG_SENSOR	7
+
+/* rpmsg version */
+#define IMX_RMPSG_MAJOR		1
+#define IMX_RMPSG_MINOR		0
+
+#define MAX_DEV_PER_CHANNEL	10
+
+struct imx_rpmsg_head {
+	u8 cate;	/* Category */
+	u8 major;	/* Major version */
+	u8 minor;	/* Minor version */
+	u8 type;	/* Message type */
+	u8 cmd;		/* Command code */
+	u8 reserved[5];
+} __packed;
+
+struct imx_rpmsg_driver_data {
+	int map_idx;
+	const char *rproc_name;
+	struct rpmsg_device *rpdev;
+	struct device_node *channel_node;
+	int (*rx_callback)(struct rpmsg_device *rpdev, void *data,
+			   int len, void *priv, u32 src);
+	void *channel_devices[MAX_DEV_PER_CHANNEL];
+};
+
+#endif /* __LINUX_IMX_RPMSG_H__ */
-- 
2.43.0

[PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Shenwei Wang]

On i.MX SoCs, the system may include two processors:
	- An MCU running an RTOS
	- An MPU running Linux

These processors communicate via the RPMSG protocol.
The driver implements the standard GPIO interface, allowing
the Linux side to control GPIO controllers which reside in
the remote processor via RPMSG protocol.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/gpio/Kconfig          |  17 ++
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-imx-rpmsg.c | 488 ++++++++++++++++++++++++++++++++++
 3 files changed, 506 insertions(+)
 create mode 100644 drivers/gpio/gpio-imx-rpmsg.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a437fe652dbc..97eda94b0ba1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1847,6 +1847,23 @@ config GPIO_SODAVILLE
 
 endmenu
 
+menu "RPMSG GPIO drivers"
+	depends on RPMSG
+
+config GPIO_IMX_RPMSG
+	tristate "NXP i.MX SoC RPMSG GPIO support"
+	depends on IMX_REMOTEPROC
+	select GPIOLIB_IRQCHIP
+	default IMX_REMOTEPROC
+	help
+	  Say yes here to support the RPMSG GPIO functions on i.MX SoC based
+	  platform.  Currently supported devices: i.MX7ULP, i.MX8ULP, i.MX8x,
+	  and i.MX9x.
+
+	  If unsure, say N.
+
+endmenu
+
 menu "SPI GPIO expanders"
 	depends on SPI_MASTER
 
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 379f55e9ed1e..e01465c03431 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_I8255)		+= gpio-i8255.o
 obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
 obj-$(CONFIG_GPIO_IDIO_16)		+= gpio-idio-16.o
 obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
+obj-$(CONFIG_GPIO_IMX_RPMSG)		+= gpio-imx-rpmsg.o
 obj-$(CONFIG_GPIO_IMX_SCU)		+= gpio-imx-scu.o
 obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
 obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
diff --git a/drivers/gpio/gpio-imx-rpmsg.c b/drivers/gpio/gpio-imx-rpmsg.c
new file mode 100644
index 000000000000..46b1b6b798c8
--- /dev/null
+++ b/drivers/gpio/gpio-imx-rpmsg.c
@@ -0,0 +1,488 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 NXP
+ *
+ * The driver exports a standard gpiochip interface to control
+ * the GPIO controllers via RPMSG on a remote processor.
+ */
+
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/rpmsg/imx_rpmsg.h>
+#include <linux/init.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rpmsg.h>
+
+#define IMX_RPMSG_GPIO_PER_PORT	32
+#define RPMSG_TIMEOUT	1000
+
+enum gpio_input_trigger_type {
+	GPIO_RPMSG_TRI_IGNORE,
+	GPIO_RPMSG_TRI_RISING,
+	GPIO_RPMSG_TRI_FALLING,
+	GPIO_RPMSG_TRI_BOTH_EDGE,
+	GPIO_RPMSG_TRI_LOW_LEVEL,
+	GPIO_RPMSG_TRI_HIGH_LEVEL,
+};
+
+enum gpio_rpmsg_header_type {
+	GPIO_RPMSG_SETUP,
+	GPIO_RPMSG_REPLY,
+	GPIO_RPMSG_NOTIFY,
+};
+
+enum gpio_rpmsg_header_cmd {
+	GPIO_RPMSG_INPUT_INIT,
+	GPIO_RPMSG_OUTPUT_INIT,
+	GPIO_RPMSG_INPUT_GET,
+};
+
+struct gpio_rpmsg_data {
+	struct imx_rpmsg_head header;
+	u8 pin_idx;
+	u8 port_idx;
+	union {
+		u8 event;
+		u8 retcode;
+		u8 value;
+	} out;
+	union {
+		u8 wakeup;
+		u8 value;
+	} in;
+} __packed __aligned(8);
+
+struct imx_rpmsg_gpio_pin {
+	u8 irq_shutdown;
+	u8 irq_unmask;
+	u8 irq_mask;
+	u32 irq_wake_enable;
+	u32 irq_type;
+	struct gpio_rpmsg_data msg;
+};
+
+struct imx_gpio_rpmsg_info {
+	struct rpmsg_device *rpdev;
+	struct gpio_rpmsg_data *notify_msg;
+	struct gpio_rpmsg_data *reply_msg;
+	struct completion cmd_complete;
+	struct mutex lock;
+	void **port_store;
+};
+
+struct imx_rpmsg_gpio_port {
+	struct gpio_chip gc;
+	struct imx_rpmsg_gpio_pin gpio_pins[IMX_RPMSG_GPIO_PER_PORT];
+	struct imx_gpio_rpmsg_info info;
+	int idx;
+};
+
+static int gpio_send_message(struct imx_rpmsg_gpio_port *port,
+			     struct gpio_rpmsg_data *msg,
+			     bool sync)
+{
+	struct imx_gpio_rpmsg_info *info = &port->info;
+	int err;
+
+	if (!info->rpdev) {
+		dev_dbg(&info->rpdev->dev,
+			"rpmsg channel not ready, m4 image ready?\n");
+		return -EINVAL;
+	}
+
+	reinit_completion(&info->cmd_complete);
+	err = rpmsg_send(info->rpdev->ept, (void *)msg,
+			 sizeof(struct gpio_rpmsg_data));
+	if (err) {
+		dev_err(&info->rpdev->dev, "rpmsg_send failed: %d\n", err);
+		return err;
+	}
+
+	if (sync) {
+		err = wait_for_completion_timeout(&info->cmd_complete,
+						  msecs_to_jiffies(RPMSG_TIMEOUT));
+		if (!err) {
+			dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
+			return -ETIMEDOUT;
+		}
+
+		if (info->reply_msg->out.retcode != 0) {
+			dev_err(&info->rpdev->dev, "rpmsg not ack %d!\n",
+				info->reply_msg->out.retcode);
+			return -EINVAL;
+		}
+
+		/* copy the reply message */
+		memcpy(&port->gpio_pins[info->reply_msg->pin_idx].msg,
+		       info->reply_msg, sizeof(*info->reply_msg));
+	}
+
+	return 0;
+}
+
+static struct gpio_rpmsg_data *gpio_get_pin_msg(struct imx_rpmsg_gpio_port *port,
+						unsigned int offset)
+{
+	struct gpio_rpmsg_data *msg = &port->gpio_pins[offset].msg;
+
+	memset(msg, 0, sizeof(struct gpio_rpmsg_data));
+
+	return msg;
+};
+
+static int imx_rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+	struct gpio_rpmsg_data *msg = NULL;
+	int ret;
+
+	guard(mutex)(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	msg->header.cate = IMX_RPMSG_GPIO;
+	msg->header.major = IMX_RMPSG_MAJOR;
+	msg->header.minor = IMX_RMPSG_MINOR;
+	msg->header.type = GPIO_RPMSG_SETUP;
+	msg->header.cmd = GPIO_RPMSG_INPUT_GET;
+	msg->pin_idx = gpio;
+	msg->port_idx = port->idx;
+
+	ret = gpio_send_message(port, msg, true);
+	if (!ret)
+		ret = !!port->gpio_pins[gpio].msg.in.value;
+
+	return ret;
+}
+
+static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
+					  unsigned int gpio)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+	struct gpio_rpmsg_data *msg = NULL;
+
+	guard(mutex)(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	msg->header.cate = IMX_RPMSG_GPIO;
+	msg->header.major = IMX_RMPSG_MAJOR;
+	msg->header.minor = IMX_RMPSG_MINOR;
+	msg->header.type = GPIO_RPMSG_SETUP;
+	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
+	msg->pin_idx = gpio;
+	msg->port_idx = port->idx;
+
+	msg->out.event = GPIO_RPMSG_TRI_IGNORE;
+	msg->in.wakeup = 0;
+
+	return gpio_send_message(port, msg, true);
+}
+
+static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
+		unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+
+	msg->header.cate = IMX_RPMSG_GPIO;
+	msg->header.major = IMX_RMPSG_MAJOR;
+	msg->header.minor = IMX_RMPSG_MINOR;
+	msg->header.type = GPIO_RPMSG_SETUP;
+	msg->header.cmd = GPIO_RPMSG_OUTPUT_INIT;
+	msg->pin_idx = gpio;
+	msg->port_idx = port->idx;
+	msg->out.value = val;
+}
+
+static int imx_rpmsg_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+	struct gpio_rpmsg_data *msg = NULL;
+
+	guard(mutex)(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
+
+	return gpio_send_message(port, msg, true);
+}
+
+static int imx_rpmsg_gpio_direction_output(struct gpio_chip *gc,
+					unsigned int gpio, int val)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+	struct gpio_rpmsg_data *msg = NULL;
+
+	guard(mutex)(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
+
+	return gpio_send_message(port, msg, true);
+}
+
+static int imx_rpmsg_irq_set_type(struct irq_data *d, u32 type)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 gpio_idx = d->hwirq;
+	int edge = 0;
+	int ret = 0;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		edge = GPIO_RPMSG_TRI_RISING;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		edge = GPIO_RPMSG_TRI_FALLING;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		edge = GPIO_RPMSG_TRI_BOTH_EDGE;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		edge = GPIO_RPMSG_TRI_LOW_LEVEL;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		edge = GPIO_RPMSG_TRI_HIGH_LEVEL;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	port->gpio_pins[gpio_idx].irq_type = edge;
+
+	return ret;
+}
+
+static int imx_rpmsg_irq_set_wake(struct irq_data *d, u32 enable)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 gpio_idx = d->hwirq;
+
+	port->gpio_pins[gpio_idx].irq_wake_enable = enable;
+
+	return 0;
+}
+
+/*
+ * This function will be called at:
+ *  - one interrupt setup.
+ *  - the end of one interrupt happened
+ * The gpio over rpmsg driver will not write the real register, so save
+ * all infos before this function and then send all infos to M core in this
+ * step.
+ */
+static void imx_rpmsg_unmask_irq(struct irq_data *d)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 gpio_idx = d->hwirq;
+
+	port->gpio_pins[gpio_idx].irq_unmask = 1;
+}
+
+static void imx_rpmsg_mask_irq(struct irq_data *d)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 gpio_idx = d->hwirq;
+	/*
+	 * No need to implement the callback at A core side.
+	 * M core will mask interrupt after a interrupt occurred, and then
+	 * sends a notify to A core.
+	 * After A core dealt with the notify, A core will send a rpmsg to
+	 * M core to unmask this interrupt again.
+	 */
+	port->gpio_pins[gpio_idx].irq_mask = 1;
+}
+
+static void imx_rpmsg_irq_shutdown(struct irq_data *d)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 gpio_idx = d->hwirq;
+
+	port->gpio_pins[gpio_idx].irq_shutdown = 1;
+}
+
+static void imx_rpmsg_irq_bus_lock(struct irq_data *d)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+
+	mutex_lock(&port->info.lock);
+}
+
+static void imx_rpmsg_irq_bus_sync_unlock(struct irq_data *d)
+{
+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
+	struct gpio_rpmsg_data *msg = NULL;
+	u32 gpio_idx = d->hwirq;
+
+	if (port == NULL) {
+		mutex_unlock(&port->info.lock);
+		return;
+	}
+
+	/*
+	 * For mask irq, do nothing here.
+	 * M core will mask interrupt after a interrupt occurred, and then
+	 * sends a notify to A core.
+	 * After A core dealt with the notify, A core will send a rpmsg to
+	 * M core to unmask this interrupt again.
+	 */
+
+	if (port->gpio_pins[gpio_idx].irq_mask && !port->gpio_pins[gpio_idx].irq_unmask) {
+		port->gpio_pins[gpio_idx].irq_mask = 0;
+		mutex_unlock(&port->info.lock);
+		return;
+	}
+
+	msg = gpio_get_pin_msg(port, gpio_idx);
+	msg->header.cate = IMX_RPMSG_GPIO;
+	msg->header.major = IMX_RMPSG_MAJOR;
+	msg->header.minor = IMX_RMPSG_MINOR;
+	msg->header.type = GPIO_RPMSG_SETUP;
+	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
+	msg->pin_idx = gpio_idx;
+	msg->port_idx = port->idx;
+
+	if (port->gpio_pins[gpio_idx].irq_shutdown) {
+		msg->out.event = GPIO_RPMSG_TRI_IGNORE;
+		msg->in.wakeup = 0;
+		port->gpio_pins[gpio_idx].irq_shutdown = 0;
+	} else {
+		 /* if not set irq type, then use low level as trigger type */
+		msg->out.event = port->gpio_pins[gpio_idx].irq_type;
+		if (!msg->out.event)
+			msg->out.event = GPIO_RPMSG_TRI_LOW_LEVEL;
+		if (port->gpio_pins[gpio_idx].irq_unmask) {
+			msg->in.wakeup = 0;
+			port->gpio_pins[gpio_idx].irq_unmask = 0;
+		} else /* irq set wake */
+			msg->in.wakeup = port->gpio_pins[gpio_idx].irq_wake_enable;
+	}
+
+	gpio_send_message(port, msg, false);
+	mutex_unlock(&port->info.lock);
+}
+
+static const struct irq_chip imx_rpmsg_irq_chip = {
+	.irq_mask = imx_rpmsg_mask_irq,
+	.irq_unmask = imx_rpmsg_unmask_irq,
+	.irq_set_wake = imx_rpmsg_irq_set_wake,
+	.irq_set_type = imx_rpmsg_irq_set_type,
+	.irq_shutdown = imx_rpmsg_irq_shutdown,
+	.irq_bus_lock = imx_rpmsg_irq_bus_lock,
+	.irq_bus_sync_unlock = imx_rpmsg_irq_bus_sync_unlock,
+	.flags = IRQCHIP_IMMUTABLE,
+};
+
+static int imx_rpmsg_gpio_callback(struct rpmsg_device *rpdev,
+	void *data, int len, void *priv, u32 src)
+{
+	struct gpio_rpmsg_data *msg = (struct gpio_rpmsg_data *)data;
+	struct imx_rpmsg_gpio_port *port = NULL;
+	struct imx_rpmsg_driver_data *drvdata;
+	unsigned long flags;
+
+	drvdata = dev_get_drvdata(&rpdev->dev);
+	if (msg)
+		port = drvdata->channel_devices[msg->port_idx];
+
+	if (!port)
+		return -ENODEV;
+
+	if (msg->header.type == GPIO_RPMSG_REPLY) {
+		port->info.reply_msg = msg;
+		complete(&port->info.cmd_complete);
+	} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
+		port->info.notify_msg = msg;
+		local_irq_save(flags);
+		generic_handle_domain_irq(port->gc.irq.domain, msg->pin_idx);
+		local_irq_restore(flags);
+	} else
+		dev_err(&rpdev->dev, "wrong command type!\n");
+
+	return 0;
+}
+
+static void imx_rpmsg_gpio_remove_action(void *data)
+{
+	struct imx_rpmsg_gpio_port *port = data;
+
+	port->info.port_store[port->idx] = 0;
+}
+
+static int imx_rpmsg_gpio_probe(struct platform_device *pdev)
+{
+	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
+	struct imx_rpmsg_gpio_port *port;
+	struct gpio_irq_chip *girq;
+	struct gpio_chip *gc;
+	int ret;
+
+	if (!pltdata)
+		return -EPROBE_DEFER;
+
+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "reg", &port->idx);
+	if (ret)
+		return ret;
+
+	if (port->idx > MAX_DEV_PER_CHANNEL)
+		return -EINVAL;
+
+	mutex_init(&port->info.lock);
+	init_completion(&port->info.cmd_complete);
+	port->info.rpdev = pltdata->rpdev;
+	port->info.port_store = pltdata->channel_devices;
+	port->info.port_store[port->idx] = port;
+	if (!pltdata->rx_callback)
+		pltdata->rx_callback = imx_rpmsg_gpio_callback;
+
+	gc = &port->gc;
+	gc->owner = THIS_MODULE;
+	gc->parent = &pltdata->rpdev->dev;
+	gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
+				   pltdata->rproc_name, port->idx);
+	gc->ngpio = IMX_RPMSG_GPIO_PER_PORT;
+	gc->base = -1;
+
+	gc->direction_input = imx_rpmsg_gpio_direction_input;
+	gc->direction_output = imx_rpmsg_gpio_direction_output;
+	gc->get = imx_rpmsg_gpio_get;
+	gc->set = imx_rpmsg_gpio_set;
+
+	platform_set_drvdata(pdev, port);
+	girq = &gc->irq;
+	gpio_irq_chip_set_chip(girq, &imx_rpmsg_irq_chip);
+	girq->parent_handler = NULL;
+	girq->num_parents = 0;
+	girq->parents = NULL;
+	girq->chip->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
+				 pltdata->rproc_name, port->idx);
+
+	devm_add_action_or_reset(&pdev->dev, imx_rpmsg_gpio_remove_action, port);
+
+	return devm_gpiochip_add_data(&pdev->dev, gc, port);
+}
+
+static const struct of_device_id imx_rpmsg_gpio_dt_ids[] = {
+	{ .compatible = "fsl,imx-rpmsg-gpio" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver imx_rpmsg_gpio_driver = {
+	.driver	= {
+		.name = "gpio-imx-rpmsg",
+		.of_match_table = imx_rpmsg_gpio_dt_ids,
+	},
+	.probe = imx_rpmsg_gpio_probe,
+};
+
+module_platform_driver(imx_rpmsg_gpio_driver);
+
+MODULE_AUTHOR("NXP Semiconductor");
+MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

[PATCH v2 4/4] arm64: dts: imx8ulp: Add rpmsg node under imx_rproc

[Shenwei Wang]

Add the RPMSG bus node along with its GPIO subnodes to the device
tree.

Enable remote device communication and GPIO control via RPMSG on
the i.MX platform.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8ulp.dtsi | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
index 13b01f3aa2a4..6ab1c12a3bc1 100644
--- a/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8ulp.dtsi
@@ -191,6 +191,33 @@ scmi_sensor: protocol@15 {
 	cm33: remoteproc-cm33 {
 		compatible = "fsl,imx8ulp-cm33";
 		status = "disabled";
+
+		rpmsg {
+			rpmsg-io-channel {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				rpmsg_gpioa: gpio@0 {
+					compatible = "fsl,imx-rpmsg-gpio";
+					reg = <0>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					#interrupt-cells = <2>;
+					interrupt-controller;
+					interrupt-parent = <&rpmsg_gpioa>;
+				};
+
+				rpmsg_gpiob: gpio@1 {
+					compatible = "fsl,imx-rpmsg-gpio";
+					reg = <1>;
+					gpio-controller;
+					#gpio-cells = <2>;
+					#interrupt-cells = <2>;
+					interrupt-controller;
+					interrupt-parent = <&rpmsg_gpiob>;
+				};
+			};
+		};
 	};
 
 	soc: soc@0 {
-- 
2.43.0

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[kernel test robot]

Hi Shenwei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on brgl/gpio/for-next shawnguo/for-next linus/master v6.17-rc7 next-20250923]
[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/Shenwei-Wang/dt-bindings-remoteproc-imx_rproc-Add-rpmsg-subnode-support/20250923-040844
base:   https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20250922200413.309707-4-shenwei.wang%40nxp.com
patch subject: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
config: arm64-randconfig-r121-20250924 (https://download.01.org/0day-ci/archive/20250925/202509250023.gNpm32hs-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250925/202509250023.gNpm32hs-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/202509250023.gNpm32hs-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpio/gpio-imx-rpmsg.c:410:44: sparse: sparse: Using plain integer as NULL pointer

vim +410 drivers/gpio/gpio-imx-rpmsg.c

   405	
   406	static void imx_rpmsg_gpio_remove_action(void *data)
   407	{
   408		struct imx_rpmsg_gpio_port *port = data;
   409	
 > 410		port->info.port_store[port->idx] = 0;
   411	}
   412	

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

Re: [PATCH v2 2/4] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode

[Peng Fan]

Hi Shenwei,

Most of the patch looks good to me. Only a few minor comments inline.

On Mon, Sep 22, 2025 at 03:04:11PM -0500, Shenwei Wang wrote:
>Register the RPMsg channel driver and populate remote devices defined
>under the "rpmsg" subnode upon receiving their notification messages.
>
> 
> #include "imx_rproc.h"
>@@ -1084,6 +1088,126 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
> 	return NOTIFY_DONE;
> }
> 
>+struct imx_rpmsg_driver {
>+	struct rpmsg_driver rpdrv;
>+	void *driver_data;
>+};
>+
>+static char *channel_device_map[][2] = {
>+	{"rpmsg-io-channel", "fsl,imx-rpmsg-gpio"},
>+	{"rpmsg-i2c-channel", "fsl,imx-rpmsg-i2c"},
>+};
>+
>+static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev,
>+	void *data, int len, void *priv, u32 src)

Alought checkpatch not report warning, still better align the parameter to '('
static int imx_rpmsg_endpoint_cb(struct rpmsg_device *rpdev, void *data,
				 int len, void *priv, u32 src)

>+{
>+	struct imx_rpmsg_driver_data *drvdata;
>+
>+	drvdata = dev_get_drvdata(&rpdev->dev);
>+	if (drvdata && drvdata->rx_callback)
>+		return drvdata->rx_callback(rpdev, data, len, priv, src);
>+
>+	return 0;
>+}
>+
>+static void imx_rpmsg_endpoint_remove(struct rpmsg_device *rpdev)
>+{
>+	of_platform_depopulate(&rpdev->dev);
>+}
>+
>+static int imx_rpmsg_endpoint_probe(struct rpmsg_device *rpdev)
>+{
>+	struct imx_rpmsg_driver_data *drvdata;
>+	struct imx_rpmsg_driver *imx_rpdrv;
>+	struct device *dev = &rpdev->dev;
>+	struct of_dev_auxdata *auxdata;
>+	struct rpmsg_driver *rpdrv;
>+	int i;
>+
>+	rpdrv = container_of(dev->driver, struct rpmsg_driver, drv);
>+	imx_rpdrv = container_of(rpdrv, struct imx_rpmsg_driver, rpdrv);
>+
>+	if (!imx_rpdrv->driver_data)
>+		return -EINVAL;
>+
>+	drvdata = devm_kmemdup(dev, imx_rpdrv->driver_data, sizeof(*drvdata), GFP_KERNEL);
>+	if (!drvdata)
>+		return -ENOMEM;
>+
>+	i = drvdata->map_idx;
>+	if (i >= ARRAY_SIZE(channel_device_map))
>+		return -ENODEV;
>+
>+	auxdata = devm_kzalloc(dev, sizeof(*auxdata)*2, GFP_KERNEL);

'*' -> ' * '

>+	if (!auxdata)
>+		return -ENOMEM;
>+
>+	drvdata->rpdev = rpdev;
>+	auxdata[0].compatible = channel_device_map[i][1];
>+	auxdata[0].platform_data = drvdata;
>+	dev_set_drvdata(dev, drvdata);
>+
>+	of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
>+	of_node_put(drvdata->channel_node);

of_platform_populate will decrement the reference to drvdata->channel_node
before it return. I not see other places increment the reference to
channel_node, of_node_put here could be removed.

>+
>+	return 0;
>+}
>+
>+static int imx_of_rpmsg_node_init(struct platform_device *pdev)
>+{
>+	struct device_node *np __free(device_node), *channel;
>+	struct imx_rpmsg_driver_data *driver_data;
>+	struct imx_rpmsg_driver *rp_driver;
>+	struct rpmsg_device_id *rpdev_id;
>+	int i, ret;
>+
>+	int count = ARRAY_SIZE(channel_device_map);
>+	struct device *dev = &pdev->dev;
>+
>+	np = of_get_child_by_name(dev->of_node, "rpmsg");
>+	if (!np)
>+		return 0;
>+
>+	for (i = 0; i < count; i++) {
>+		ret = -ENOMEM;
>+		channel = of_get_child_by_name(np, channel_device_map[i][0]);
>+		if (!channel)
>+			continue;
>+
>+		rpdev_id = devm_kzalloc(dev, sizeof(*rpdev_id)*2, GFP_KERNEL);
>+		if (!rpdev_id)
>+			break;
>+		strscpy(rpdev_id[0].name, channel_device_map[i][0], RPMSG_NAME_SIZE);
>+
>+		rp_driver = devm_kzalloc(dev, sizeof(*rp_driver), GFP_KERNEL);
>+		if (!rp_driver)
>+			break;
>+
>+		driver_data = devm_kzalloc(dev, sizeof(*driver_data), GFP_KERNEL);
>+		if (!driver_data)
>+			break;
>+
>+		ret = 0;
>+		driver_data->rproc_name = dev->of_node->name;
>+		driver_data->channel_node = channel;
>+		driver_data->map_idx = i;
>+
>+		rp_driver->rpdrv.drv.name = channel_device_map[i][0];
>+		rp_driver->rpdrv.id_table = rpdev_id;
>+		rp_driver->rpdrv.probe = imx_rpmsg_endpoint_probe;
>+		rp_driver->rpdrv.remove = imx_rpmsg_endpoint_remove;
>+		rp_driver->rpdrv.callback = imx_rpmsg_endpoint_cb;
>+		rp_driver->driver_data = driver_data;
>+
>+		register_rpmsg_driver(&rp_driver->rpdrv);
>+	}
>+
>+	if ((ret < 0) && channel)
>+		of_node_put(channel);
>+
>+	return ret;
>+}
>+
> static int imx_rproc_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
>@@ -1177,6 +1301,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
> 		goto err_put_clk;
> 	}
> 
>+	imx_of_rpmsg_node_init(pdev);

Need return value check.

>+
> 	return 0;
> 

Regards,
Peng

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Peng Fan]

Hi Shenwei,

On Mon, Sep 22, 2025 at 03:04:12PM -0500, Shenwei Wang wrote:
>On i.MX SoCs, the system may include two processors:
>	- An MCU running an RTOS
>	- An MPU running Linux
>
>These processors communicate via the RPMSG protocol.
>The driver implements the standard GPIO interface, allowing
>the Linux side to control GPIO controllers which reside in
>the remote processor via RPMSG protocol.
>+ */
...
>+
>+#include <linux/err.h>
>+#include <linux/gpio/driver.h>
>+#include <linux/rpmsg/imx_rpmsg.h>
>+#include <linux/init.h>
>+#include <linux/irqdomain.h>
>+#include <linux/of.h>
>+#include <linux/platform_device.h>
>+#include <linux/rpmsg.h>

The included headers should be sorted.

>+
>+#define IMX_RPMSG_GPIO_PER_PORT	32
>+#define RPMSG_TIMEOUT	1000
>+
>+enum gpio_input_trigger_type {
>+	GPIO_RPMSG_TRI_IGNORE,
>+	GPIO_RPMSG_TRI_RISING,
>+	GPIO_RPMSG_TRI_FALLING,
>+	GPIO_RPMSG_TRI_BOTH_EDGE,
>+	GPIO_RPMSG_TRI_LOW_LEVEL,
>+	GPIO_RPMSG_TRI_HIGH_LEVEL,
>+};
>+
>+enum gpio_rpmsg_header_type {
>+	GPIO_RPMSG_SETUP,
>+	GPIO_RPMSG_REPLY,
>+	GPIO_RPMSG_NOTIFY,
>+};
>+
>+enum gpio_rpmsg_header_cmd {
>+	GPIO_RPMSG_INPUT_INIT,
>+	GPIO_RPMSG_OUTPUT_INIT,
>+	GPIO_RPMSG_INPUT_GET,
>+};
>+
>+struct gpio_rpmsg_data {
>+	struct imx_rpmsg_head header;
>+	u8 pin_idx;
>+	u8 port_idx;
>+	union {
>+		u8 event;
>+		u8 retcode;
>+		u8 value;
>+	} out;
>+	union {
>+		u8 wakeup;
>+		u8 value;
>+	} in;
>+} __packed __aligned(8);
>+
>+struct imx_rpmsg_gpio_pin {
>+	u8 irq_shutdown;
>+	u8 irq_unmask;
>+	u8 irq_mask;
>+	u32 irq_wake_enable;
>+	u32 irq_type;
>+	struct gpio_rpmsg_data msg;
>+};
>+
>+struct imx_gpio_rpmsg_info {
>+	struct rpmsg_device *rpdev;
>+	struct gpio_rpmsg_data *notify_msg;
>+	struct gpio_rpmsg_data *reply_msg;
>+	struct completion cmd_complete;
>+	struct mutex lock;
>+	void **port_store;
>+};
>+
>+struct imx_rpmsg_gpio_port {
>+	struct gpio_chip gc;
>+	struct imx_rpmsg_gpio_pin gpio_pins[IMX_RPMSG_GPIO_PER_PORT];
>+	struct imx_gpio_rpmsg_info info;
>+	int idx;
>+};
>+
>+static int gpio_send_message(struct imx_rpmsg_gpio_port *port,
>+			     struct gpio_rpmsg_data *msg,
>+			     bool sync)
>+{
>+	struct imx_gpio_rpmsg_info *info = &port->info;
>+	int err;
>+
>+	if (!info->rpdev) {
>+		dev_dbg(&info->rpdev->dev,
>+			"rpmsg channel not ready, m4 image ready?\n");

"m4 image" -> "remote core" 

>+		return -EINVAL;
>+	}
>+
>+	reinit_completion(&info->cmd_complete);
>+	err = rpmsg_send(info->rpdev->ept, (void *)msg,
>+			 sizeof(struct gpio_rpmsg_data));
>+	if (err) {
>+		dev_err(&info->rpdev->dev, "rpmsg_send failed: %d\n", err);
>+		return err;
>+	}
>+
>+	if (sync) {
>+		err = wait_for_completion_timeout(&info->cmd_complete,
>+						  msecs_to_jiffies(RPMSG_TIMEOUT));
>+		if (!err) {
>+			dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
>+			return -ETIMEDOUT;
>+		}
>+
>+		if (info->reply_msg->out.retcode != 0) {
>+			dev_err(&info->rpdev->dev, "rpmsg not ack %d!\n",
>+				info->reply_msg->out.retcode);
>+			return -EINVAL;
>+		}
>+
>+		/* copy the reply message */
>+		memcpy(&port->gpio_pins[info->reply_msg->pin_idx].msg,
>+		       info->reply_msg, sizeof(*info->reply_msg));
>+	}
>+
>+	return 0;
>+}
>+
>+static struct gpio_rpmsg_data *gpio_get_pin_msg(struct imx_rpmsg_gpio_port *port,
>+						unsigned int offset)
>+{
>+	struct gpio_rpmsg_data *msg = &port->gpio_pins[offset].msg;
>+
>+	memset(msg, 0, sizeof(struct gpio_rpmsg_data));
>+
>+	return msg;
>+};
>+
>+static int imx_rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>+{
>+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>+	struct gpio_rpmsg_data *msg = NULL;
>+	int ret;
>+
>+	guard(mutex)(&port->info.lock);
>+
>+	msg = gpio_get_pin_msg(port, gpio);
>+	msg->header.cate = IMX_RPMSG_GPIO;
>+	msg->header.major = IMX_RMPSG_MAJOR;
>+	msg->header.minor = IMX_RMPSG_MINOR;
>+	msg->header.type = GPIO_RPMSG_SETUP;
>+	msg->header.cmd = GPIO_RPMSG_INPUT_GET;
>+	msg->pin_idx = gpio;
>+	msg->port_idx = port->idx;
>+
>+	ret = gpio_send_message(port, msg, true);
>+	if (!ret)
>+		ret = !!port->gpio_pins[gpio].msg.in.value;
>+
>+	return ret;
>+}
>+
>+static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
>+					  unsigned int gpio)
>+{
>+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>+	struct gpio_rpmsg_data *msg = NULL;
>+
>+	guard(mutex)(&port->info.lock);
>+
>+	msg = gpio_get_pin_msg(port, gpio);
>+	msg->header.cate = IMX_RPMSG_GPIO;
>+	msg->header.major = IMX_RMPSG_MAJOR;
>+	msg->header.minor = IMX_RMPSG_MINOR;
>+	msg->header.type = GPIO_RPMSG_SETUP;
>+	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
>+	msg->pin_idx = gpio;
>+	msg->port_idx = port->idx;
>+
>+	msg->out.event = GPIO_RPMSG_TRI_IGNORE;
>+	msg->in.wakeup = 0;
>+
>+	return gpio_send_message(port, msg, true);
>+}
>+
>+static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
>+		unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
>+{
>+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>+
>+	msg->header.cate = IMX_RPMSG_GPIO;
>+	msg->header.major = IMX_RMPSG_MAJOR;
>+	msg->header.minor = IMX_RMPSG_MINOR;
>+	msg->header.type = GPIO_RPMSG_SETUP;
>+	msg->header.cmd = GPIO_RPMSG_OUTPUT_INIT;
>+	msg->pin_idx = gpio;
>+	msg->port_idx = port->idx;
>+	msg->out.value = val;
>+}
>+
>+static int imx_rpmsg_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>+{
>+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>+	struct gpio_rpmsg_data *msg = NULL;
>+
>+	guard(mutex)(&port->info.lock);
>+
>+	msg = gpio_get_pin_msg(port, gpio);
>+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
>+
>+	return gpio_send_message(port, msg, true);
>+}
>+
>+static int imx_rpmsg_gpio_direction_output(struct gpio_chip *gc,
>+					unsigned int gpio, int val)
>+{
>+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>+	struct gpio_rpmsg_data *msg = NULL;
>+
>+	guard(mutex)(&port->info.lock);
>+
>+	msg = gpio_get_pin_msg(port, gpio);
>+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
>+
>+	return gpio_send_message(port, msg, true);
>+}
>+
>+static int imx_rpmsg_irq_set_type(struct irq_data *d, u32 type)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	u32 gpio_idx = d->hwirq;
>+	int edge = 0;
>+	int ret = 0;
>+
>+	switch (type) {
>+	case IRQ_TYPE_EDGE_RISING:
>+		edge = GPIO_RPMSG_TRI_RISING;
>+		break;
>+	case IRQ_TYPE_EDGE_FALLING:
>+		edge = GPIO_RPMSG_TRI_FALLING;
>+		break;
>+	case IRQ_TYPE_EDGE_BOTH:
>+		edge = GPIO_RPMSG_TRI_BOTH_EDGE;
>+		break;
>+	case IRQ_TYPE_LEVEL_LOW:
>+		edge = GPIO_RPMSG_TRI_LOW_LEVEL;
>+		break;
>+	case IRQ_TYPE_LEVEL_HIGH:
>+		edge = GPIO_RPMSG_TRI_HIGH_LEVEL;
>+		break;
>+	default:
>+		ret = -EINVAL;
>+		break;
>+	}
>+
>+	port->gpio_pins[gpio_idx].irq_type = edge;
>+
>+	return ret;
>+}
>+
>+static int imx_rpmsg_irq_set_wake(struct irq_data *d, u32 enable)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	u32 gpio_idx = d->hwirq;
>+
>+	port->gpio_pins[gpio_idx].irq_wake_enable = enable;
>+
>+	return 0;
>+}
>+
>+/*
>+ * This function will be called at:
>+ *  - one interrupt setup.
>+ *  - the end of one interrupt happened
>+ * The gpio over rpmsg driver will not write the real register, so save
>+ * all infos before this function and then send all infos to M core in this
>+ * step.
>+ */
>+static void imx_rpmsg_unmask_irq(struct irq_data *d)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	u32 gpio_idx = d->hwirq;
>+
>+	port->gpio_pins[gpio_idx].irq_unmask = 1;
>+}
>+
>+static void imx_rpmsg_mask_irq(struct irq_data *d)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	u32 gpio_idx = d->hwirq;
>+	/*
>+	 * No need to implement the callback at A core side.
>+	 * M core will mask interrupt after a interrupt occurred, and then
>+	 * sends a notify to A core.
>+	 * After A core dealt with the notify, A core will send a rpmsg to
>+	 * M core to unmask this interrupt again.
>+	 */
>+	port->gpio_pins[gpio_idx].irq_mask = 1;
>+}
>+
>+static void imx_rpmsg_irq_shutdown(struct irq_data *d)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	u32 gpio_idx = d->hwirq;
>+
>+	port->gpio_pins[gpio_idx].irq_shutdown = 1;
>+}
>+
>+static void imx_rpmsg_irq_bus_lock(struct irq_data *d)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+
>+	mutex_lock(&port->info.lock);
>+}
>+
>+static void imx_rpmsg_irq_bus_sync_unlock(struct irq_data *d)
>+{
>+	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>+	struct gpio_rpmsg_data *msg = NULL;
>+	u32 gpio_idx = d->hwirq;
>+
>+	if (port == NULL) {
>+		mutex_unlock(&port->info.lock);
>+		return;
>+	}
>+
>+	/*
>+	 * For mask irq, do nothing here.
>+	 * M core will mask interrupt after a interrupt occurred, and then
>+	 * sends a notify to A core.
>+	 * After A core dealt with the notify, A core will send a rpmsg to
>+	 * M core to unmask this interrupt again.
>+	 */
>+
>+	if (port->gpio_pins[gpio_idx].irq_mask && !port->gpio_pins[gpio_idx].irq_unmask) {
>+		port->gpio_pins[gpio_idx].irq_mask = 0;
>+		mutex_unlock(&port->info.lock);
>+		return;
>+	}
>+
>+	msg = gpio_get_pin_msg(port, gpio_idx);
>+	msg->header.cate = IMX_RPMSG_GPIO;
>+	msg->header.major = IMX_RMPSG_MAJOR;
>+	msg->header.minor = IMX_RMPSG_MINOR;
>+	msg->header.type = GPIO_RPMSG_SETUP;
>+	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
>+	msg->pin_idx = gpio_idx;
>+	msg->port_idx = port->idx;
>+
>+	if (port->gpio_pins[gpio_idx].irq_shutdown) {
>+		msg->out.event = GPIO_RPMSG_TRI_IGNORE;
>+		msg->in.wakeup = 0;
>+		port->gpio_pins[gpio_idx].irq_shutdown = 0;
>+	} else {
>+		 /* if not set irq type, then use low level as trigger type */
>+		msg->out.event = port->gpio_pins[gpio_idx].irq_type;
>+		if (!msg->out.event)
>+			msg->out.event = GPIO_RPMSG_TRI_LOW_LEVEL;
>+		if (port->gpio_pins[gpio_idx].irq_unmask) {
>+			msg->in.wakeup = 0;
>+			port->gpio_pins[gpio_idx].irq_unmask = 0;
>+		} else /* irq set wake */
>+			msg->in.wakeup = port->gpio_pins[gpio_idx].irq_wake_enable;
>+	}
>+
>+	gpio_send_message(port, msg, false);
>+	mutex_unlock(&port->info.lock);
>+}
>+
>+static const struct irq_chip imx_rpmsg_irq_chip = {
>+	.irq_mask = imx_rpmsg_mask_irq,
>+	.irq_unmask = imx_rpmsg_unmask_irq,
>+	.irq_set_wake = imx_rpmsg_irq_set_wake,
>+	.irq_set_type = imx_rpmsg_irq_set_type,
>+	.irq_shutdown = imx_rpmsg_irq_shutdown,
>+	.irq_bus_lock = imx_rpmsg_irq_bus_lock,
>+	.irq_bus_sync_unlock = imx_rpmsg_irq_bus_sync_unlock,
>+	.flags = IRQCHIP_IMMUTABLE,
>+};
>+
>+static int imx_rpmsg_gpio_callback(struct rpmsg_device *rpdev,
>+	void *data, int len, void *priv, u32 src)
>+{
>+	struct gpio_rpmsg_data *msg = (struct gpio_rpmsg_data *)data;
>+	struct imx_rpmsg_gpio_port *port = NULL;
>+	struct imx_rpmsg_driver_data *drvdata;
>+	unsigned long flags;
>+
>+	drvdata = dev_get_drvdata(&rpdev->dev);
>+	if (msg)
>+		port = drvdata->channel_devices[msg->port_idx];
>+
>+	if (!port)
>+		return -ENODEV;
>+
>+	if (msg->header.type == GPIO_RPMSG_REPLY) {
>+		port->info.reply_msg = msg;
>+		complete(&port->info.cmd_complete);
>+	} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
>+		port->info.notify_msg = msg;
>+		local_irq_save(flags);

Would you explain a bit on why need to disable IRQ on current core.

>+		generic_handle_domain_irq(port->gc.irq.domain, msg->pin_idx);
>+		local_irq_restore(flags);
>+	} else
>+		dev_err(&rpdev->dev, "wrong command type!\n");
>+
>+	return 0;
>+}
>+
>+static void imx_rpmsg_gpio_remove_action(void *data)
>+{
>+	struct imx_rpmsg_gpio_port *port = data;
>+
>+	port->info.port_store[port->idx] = 0;
>+}
>+
>+static int imx_rpmsg_gpio_probe(struct platform_device *pdev)
>+{
>+	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
>+	struct device_node *np = pdev->dev.of_node;
>+	struct imx_rpmsg_gpio_port *port;
>+	struct gpio_irq_chip *girq;
>+	struct gpio_chip *gc;
>+	int ret;
>+
>+	if (!pltdata)
>+		return -EPROBE_DEFER;
>+
>+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
>+	if (!port)
>+		return -ENOMEM;
>+
>+	ret = of_property_read_u32(np, "reg", &port->idx);

"device_property_read_u32" should be better.

>+	if (ret)
>+		return ret;
>+
>+	if (port->idx > MAX_DEV_PER_CHANNEL)
>+		return -EINVAL;
>+
>+	mutex_init(&port->info.lock);
>+	init_completion(&port->info.cmd_complete);
>+	port->info.rpdev = pltdata->rpdev;
>+	port->info.port_store = pltdata->channel_devices;
>+	port->info.port_store[port->idx] = port;
>+	if (!pltdata->rx_callback)
>+		pltdata->rx_callback = imx_rpmsg_gpio_callback;
>+
>+	gc = &port->gc;
>+	gc->owner = THIS_MODULE;
>+	gc->parent = &pltdata->rpdev->dev;
>+	gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
>+				   pltdata->rproc_name, port->idx);
>+	gc->ngpio = IMX_RPMSG_GPIO_PER_PORT;
>+	gc->base = -1;
>+
>+	gc->direction_input = imx_rpmsg_gpio_direction_input;
>+	gc->direction_output = imx_rpmsg_gpio_direction_output;
>+	gc->get = imx_rpmsg_gpio_get;
>+	gc->set = imx_rpmsg_gpio_set;
>+
>+	platform_set_drvdata(pdev, port);
>+	girq = &gc->irq;
>+	gpio_irq_chip_set_chip(girq, &imx_rpmsg_irq_chip);
>+	girq->parent_handler = NULL;
>+	girq->num_parents = 0;
>+	girq->parents = NULL;
>+	girq->chip->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
>+				 pltdata->rproc_name, port->idx);

Align pltdata->rproc_name with the upper line '('.

>+
>+	devm_add_action_or_reset(&pdev->dev, imx_rpmsg_gpio_remove_action, port);

return value should be checked.

>+
>+	return devm_gpiochip_add_data(&pdev->dev, gc, port);
>+}
>+

Thanks,
Peng

RE: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Shenwei Wang]

> -----Original Message-----
> From: Peng Fan (OSS) <peng.fan@oss.nxp.com>
> Sent: Sunday, September 28, 2025 10:37 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Bjorn Andersson <andersson@kernel.org>; Mathieu Poirier
> <mathieu.poirier@linaro.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Shawn
> Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Linus
> Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> >+	if (!port)
> >+		return -ENODEV;
> >+
> >+	if (msg->header.type == GPIO_RPMSG_REPLY) {
> >+		port->info.reply_msg = msg;
> >+		complete(&port->info.cmd_complete);
> >+	} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
> >+		port->info.notify_msg = msg;
> >+		local_irq_save(flags);
> 
> Would you explain a bit on why need to disable IRQ on current core.
> 

The generic_handle_domain_irq is required to be called in an IRQ context.  Seems it can 
be replaced with the generic_handle_domain_irq_safe here.

Thanks,
Shenwei

> >+		generic_handle_domain_irq(port->gc.irq.domain, msg->pin_idx);
> >+		local_irq_restore(flags);
> >+	} else
> >+		dev_err(&rpdev->dev, "wrong command type!\n");
> >+
> >+	return 0;
> >+}
> >+
> >+static void imx_rpmsg_gpio_remove_action(void *data) {
> >+	struct imx_rpmsg_gpio_port *port = data;
> >+
> >+	port->info.port_store[port->idx] = 0; }
> >+
> >+static int imx_rpmsg_gpio_probe(struct platform_device *pdev) {
> >+	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
> >+	struct device_node *np = pdev->dev.of_node;
> >+	struct imx_rpmsg_gpio_port *port;
> >+	struct gpio_irq_chip *girq;
> >+	struct gpio_chip *gc;
> >+	int ret;
> >+
> >+	if (!pltdata)
> >+		return -EPROBE_DEFER;
> >+
> >+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
> >+	if (!port)
> >+		return -ENOMEM;
> >+
> >+	ret = of_property_read_u32(np, "reg", &port->idx);
> 
> "device_property_read_u32" should be better.
> 
> >+	if (ret)
> >+		return ret;
> >+
> >+	if (port->idx > MAX_DEV_PER_CHANNEL)
> >+		return -EINVAL;
> >+
> >+	mutex_init(&port->info.lock);
> >+	init_completion(&port->info.cmd_complete);
> >+	port->info.rpdev = pltdata->rpdev;
> >+	port->info.port_store = pltdata->channel_devices;
> >+	port->info.port_store[port->idx] = port;
> >+	if (!pltdata->rx_callback)
> >+		pltdata->rx_callback = imx_rpmsg_gpio_callback;
> >+
> >+	gc = &port->gc;
> >+	gc->owner = THIS_MODULE;
> >+	gc->parent = &pltdata->rpdev->dev;
> >+	gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> >+				   pltdata->rproc_name, port->idx);
> >+	gc->ngpio = IMX_RPMSG_GPIO_PER_PORT;
> >+	gc->base = -1;
> >+
> >+	gc->direction_input = imx_rpmsg_gpio_direction_input;
> >+	gc->direction_output = imx_rpmsg_gpio_direction_output;
> >+	gc->get = imx_rpmsg_gpio_get;
> >+	gc->set = imx_rpmsg_gpio_set;
> >+
> >+	platform_set_drvdata(pdev, port);
> >+	girq = &gc->irq;
> >+	gpio_irq_chip_set_chip(girq, &imx_rpmsg_irq_chip);
> >+	girq->parent_handler = NULL;
> >+	girq->num_parents = 0;
> >+	girq->parents = NULL;
> >+	girq->chip->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-
> gpio%d",
> >+				 pltdata->rproc_name, port->idx);
> 
> Align pltdata->rproc_name with the upper line '('.
> 
> >+
> >+	devm_add_action_or_reset(&pdev->dev,
> imx_rpmsg_gpio_remove_action,
> >+port);
> 
> return value should be checked.
> 
> >+
> >+	return devm_gpiochip_add_data(&pdev->dev, gc, port); }
> >+
> 
> Thanks,
> Peng

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Linus Walleij]

Hi Shenwei,

thanks for your patch!

This patch was not sent to the linux-gpio@vger.kernel.org
mailing list, please include that or we can't merge the driver.

I think the OpenAMP project should show interest in this
patch series so I created an issue in their github:
https://github.com/OpenAMP/website/issues/122

After fixing the issues pointed out by Peng the driver
looks good to me.

Yours,
Linus Walleij

RE: [PATCH v2 2/4] remoteproc: imx_rproc: Populate devices under "rpmsg" subnode

[Shenwei Wang]

Hi Peng,

> -----Original Message-----
> From: Peng Fan (OSS) <peng.fan@oss.nxp.com>
> Sent: Sunday, September 28, 2025 3:48 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Bjorn Andersson <andersson@kernel.org>; Mathieu Poirier
> <mathieu.poirier@linaro.org>; Rob Herring <robh@kernel.org>; Krzysztof
> Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Shawn
> Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Linus
> Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
> >+
> >+	drvdata->rpdev = rpdev;
> >+	auxdata[0].compatible = channel_device_map[i][1];
> >+	auxdata[0].platform_data = drvdata;
> >+	dev_set_drvdata(dev, drvdata);
> >+
> >+	of_platform_populate(drvdata->channel_node, NULL, auxdata, dev);
> >+	of_node_put(drvdata->channel_node);
> 
> of_platform_populate will decrement the reference to drvdata->channel_node
> before it return. I not see other places increment the reference to channel_node,
> of_node_put here could be removed.
> 

The of_node_put is to balance the reference counter which is increased by of_get_child_by_name in imx_of_rpmsg_node_init function.

+static int imx_of_rpmsg_node_init(struct platform_device *pdev)
+{
...
+	for (i = 0; i < count; i++) {
+		ret = -ENOMEM;
+		channel = of_get_child_by_name(np, channel_device_map[i][0]);


Thanks,
Shenwei

> >+
> >+	return 0;
> >+}
> >+
> >+static int imx_of_rpmsg_node_init(struct platform_device *pdev) {
> >+	struct device_node *np __free(device_node), *channel;
> >+	struct imx_rpmsg_driver_data *driver_data;
> >+	struct imx_rpmsg_driver *rp_driver;
> >+	struct rpmsg_device_id *rpdev_id;
> >+	int i, ret;
> >+
> >+	int count = ARRAY_SIZE(channel_device_map);
> >+	struct device *dev = &pdev->dev;
> >+

Re: [PATCH v2 1/4] dt-bindings: remoteproc: imx_rproc: Add "rpmsg" subnode support

[Frank Li]

On Mon, Sep 22, 2025 at 03:04:10PM -0500, Shenwei Wang wrote:
> Remote processors may announce multiple devices (e.g., I2C, GPIO) over
> an RPMSG channel. These devices may require corresponding device tree
> nodes, especially when acting as providers, to supply phandles for their
> consumers.
>
> Define an RPMSG node to work as a container for a group of RPMSG channels
> under the imx_rproc node.
>
> Each subnode within "rpmsg" represents an individual RPMSG channel. The
> name of each subnode corresponds to the channel name as defined by the
> remote processor.
>
> All remote devices associated with a given channel are defined as child
> nodes under the corresponding channel node.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  .../bindings/remoteproc/fsl,imx-rproc.yaml    | 129 ++++++++++++++++++
>  1 file changed, 129 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> index 57d75acb0b5e..16c7db4c5a79 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -84,6 +84,98 @@ properties:
>        This property is to specify the resource id of the remote processor in SoC
>        which supports SCFW
>
> +  rpmsg:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      Present a group of RPMSG channel devices.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0

This is reduntant. child nodes have not address.

> +
> +      rpmsg-io-channel:
> +        type: object
> +        unevaluatedProperties: false

should be additionalProperties: false if no $ref

Frank
> +        properties:
> +          '#address-cells':
> +            const: 1
> +
> +          '#size-cells':
> +            const: 0
> +
> +        patternProperties:
> +          "gpio@[0-9a-f]+$":
> +            type: object
> +            unevaluatedProperties: false
> +            properties:
> +              compatible:
> +                enum:
> +                  - fsl,imx-rpmsg-gpio
> +
> +              reg:
> +                maxItems: 1
> +
> +              "#gpio-cells":
> +                const: 2
> +
> +              gpio-controller: true
> +
> +              interrupt-controller: true
> +
> +              "#interrupt-cells":
> +                const: 2
> +
> +            required:
> +              - compatible
> +              - reg
> +              - "#gpio-cells"
> +              - "#interrupt-cells"
> +
> +            allOf:
> +              - $ref: /schemas/gpio/gpio.yaml#
> +              - $ref: /schemas/interrupt-controller.yaml#
> +
> +        required:
> +          - '#address-cells'
> +          - '#size-cells'
> +
> +      rpmsg-i2c-channel:
> +        type: object
> +        unevaluatedProperties: false
> +        properties:
> +          '#address-cells':
> +            const: 1
> +
> +          '#size-cells':
> +            const: 0
> +
> +        patternProperties:
> +          "i2c@[0-9a-f]+$":
> +            type: object
> +            unevaluatedProperties: false
> +            properties:
> +              compatible:
> +                enum:
> +                  - fsl,imx-rpmsg-i2c
> +
> +              reg:
> +                maxItems: 1
> +
> +            required:
> +              - compatible
> +              - reg
> +
> +            allOf:
> +              - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +        required:
> +          - '#address-cells'
> +          - '#size-cells'
> +
>  required:
>    - compatible
>
> @@ -146,5 +238,42 @@ examples:
>                  &mu 3 1>;
>        memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>, <&rsc_table>;
>        syscon = <&src>;
> +
> +      rpmsg {
> +        rpmsg-io-channel {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          gpio@0 {
> +            compatible = "fsl,imx-rpmsg-gpio";
> +            reg = <0>;
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +            #interrupt-cells = <2>;
> +            interrupt-controller;
> +            interrupt-parent = <&rpmsg_gpioa>;
> +          };
> +
> +          gpio@1 {
> +            compatible = "fsl,imx-rpmsg-gpio";
> +            reg = <1>;
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +            #interrupt-cells = <2>;
> +            interrupt-controller;
> +            interrupt-parent = <&rpmsg_gpiob>;
> +          };
> +        };
> +
> +        rpmsg-i2c-channel {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          i2c@0 {
> +            compatible = "fsl,imx-rpmsg-i2c";
> +            reg = <0>;
> +          };
> +        };
> +      };
>      };
>  ...
> --
> 2.43.0
>

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN]

Hello Linus


On 10/1/25 08:57, Linus Walleij wrote:
> Hi Shenwei,
>
> thanks for your patch!
>
> This patch was not sent to the linux-gpio@vger.kernel.org
> mailing list, please include that or we can't merge the driver.
>
> I think the OpenAMP project should show interest in this
> patch series so I created an issue in their github:
> https://github.com/OpenAMP/website/issues/122

Thanks for pointing it out , i will do a review of this new version

Regards,
Arnaud
>
> After fixing the issues pointed out by Peng the driver
> looks good to me.
>
> Yours,
> Linus Walleij
>

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN]

Hello Shenwei,

On 9/22/25 22:04, Shenwei Wang wrote:
> On i.MX SoCs, the system may include two processors:
> 	- An MCU running an RTOS
> 	- An MPU running Linux
>
> These processors communicate via the RPMSG protocol.
> The driver implements the standard GPIO interface, allowing
> the Linux side to control GPIO controllers which reside in
> the remote processor via RPMSG protocol.

What about my request in previous version to make this driver generic?

In ST we have similar driver in downstream, we would be interested in
reusing it if generic. Indeed we need some rpmsg mechanism for
gpio-interrupt. Today we have a downstream rpmsg driver [1][2] that could
migrate on a generic rpmsg-gpio driver.

[1]Documentation/devicetree/bindings/interrupt-controller/rpmsg,intc.yaml
[2]https://github.com/STMicroelectronics/linux/blob/v6.6-stm32mp/drivers/irqchip/irq-rpmsg.c

My comment below is based on the assumption that it would become generic.

> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>   drivers/gpio/Kconfig          |  17 ++
>   drivers/gpio/Makefile         |   1 +
>   drivers/gpio/gpio-imx-rpmsg.c | 488 ++++++++++++++++++++++++++++++++++
>   3 files changed, 506 insertions(+)
>   create mode 100644 drivers/gpio/gpio-imx-rpmsg.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a437fe652dbc..97eda94b0ba1 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1847,6 +1847,23 @@ config GPIO_SODAVILLE
>   
>   endmenu
>   
> +menu "RPMSG GPIO drivers"
> +	depends on RPMSG
> +
> +config GPIO_IMX_RPMSG
> +	tristate "NXP i.MX SoC RPMSG GPIO support"
> +	depends on IMX_REMOTEPROC
> +	select GPIOLIB_IRQCHIP
> +	default IMX_REMOTEPROC
> +	help
> +	  Say yes here to support the RPMSG GPIO functions on i.MX SoC based
> +	  platform.  Currently supported devices: i.MX7ULP, i.MX8ULP, i.MX8x,
> +	  and i.MX9x.
> +
> +	  If unsure, say N.
> +
> +endmenu
> +
>   menu "SPI GPIO expanders"
>   	depends on SPI_MASTER
>   
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 379f55e9ed1e..e01465c03431 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_I8255)		+= gpio-i8255.o
>   obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
>   obj-$(CONFIG_GPIO_IDIO_16)		+= gpio-idio-16.o
>   obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
> +obj-$(CONFIG_GPIO_IMX_RPMSG)		+= gpio-imx-rpmsg.o
>   obj-$(CONFIG_GPIO_IMX_SCU)		+= gpio-imx-scu.o
>   obj-$(CONFIG_GPIO_IT87)			+= gpio-it87.o
>   obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
> diff --git a/drivers/gpio/gpio-imx-rpmsg.c b/drivers/gpio/gpio-imx-rpmsg.c
> new file mode 100644
> index 000000000000..46b1b6b798c8
> --- /dev/null
> +++ b/drivers/gpio/gpio-imx-rpmsg.c
> @@ -0,0 +1,488 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2025 NXP
> + *
> + * The driver exports a standard gpiochip interface to control
> + * the GPIO controllers via RPMSG on a remote processor.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/rpmsg/imx_rpmsg.h>
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rpmsg.h>
> +
> +#define IMX_RPMSG_GPIO_PER_PORT	32
> +#define RPMSG_TIMEOUT	1000
> +
> +enum gpio_input_trigger_type {
> +	GPIO_RPMSG_TRI_IGNORE,
> +	GPIO_RPMSG_TRI_RISING,
> +	GPIO_RPMSG_TRI_FALLING,
> +	GPIO_RPMSG_TRI_BOTH_EDGE,
> +	GPIO_RPMSG_TRI_LOW_LEVEL,
> +	GPIO_RPMSG_TRI_HIGH_LEVEL,
> +};
What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|?
For instance:
GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING | GPIO_RPMSG_TRI_RISING,
> +
> +enum gpio_rpmsg_header_type {
> +	GPIO_RPMSG_SETUP,
> +	GPIO_RPMSG_REPLY,
> +	GPIO_RPMSG_NOTIFY,
> +};
> +
> +enum gpio_rpmsg_header_cmd {
> +	GPIO_RPMSG_INPUT_INIT,
> +	GPIO_RPMSG_OUTPUT_INIT,
> +	GPIO_RPMSG_INPUT_GET,
> +};
> +
> +struct gpio_rpmsg_data {
> +	struct imx_rpmsg_head header;
> +	u8 pin_idx;
> +	u8 port_idx;
> +	union {
> +		u8 event;
> +		u8 retcode;
> +		u8 value;
> +	} out;
> +	union {
> +		u8 wakeup;
> +		u8 value;
nitpicking put "value" field out of union as common
> +	} in;
> +} __packed __aligned(8);

Any reason to pack it an align it?
This structure will be copied in the RPMSg payload, right?

I also wonder if that definition should not be in a header file
with double licensing that the DT. Indeed this need to be
common with the remote side implementation  that can
be non GPL implementation.

> +
> +struct imx_rpmsg_gpio_pin {
> +	u8 irq_shutdown;
> +	u8 irq_unmask;
> +	u8 irq_mask;
> +	u32 irq_wake_enable;
> +	u32 irq_type;
> +	struct gpio_rpmsg_data msg;
> +};
> +
> +struct imx_gpio_rpmsg_info {
> +	struct rpmsg_device *rpdev;
> +	struct gpio_rpmsg_data *notify_msg;
> +	struct gpio_rpmsg_data *reply_msg;
> +	struct completion cmd_complete;
> +	struct mutex lock;
> +	void **port_store;
> +};
> +
> +struct imx_rpmsg_gpio_port {
> +	struct gpio_chip gc;
> +	struct imx_rpmsg_gpio_pin gpio_pins[IMX_RPMSG_GPIO_PER_PORT];
> +	struct imx_gpio_rpmsg_info info;
> +	int idx;
> +};
> +
> +static int gpio_send_message(struct imx_rpmsg_gpio_port *port,
> +			     struct gpio_rpmsg_data *msg,
> +			     bool sync)
> +{
> +	struct imx_gpio_rpmsg_info *info = &port->info;
> +	int err;
> +
> +	if (!info->rpdev) {
> +		dev_dbg(&info->rpdev->dev,
> +			"rpmsg channel not ready, m4 image ready?\n");
> +		return -EINVAL;
> +	}
> +
> +	reinit_completion(&info->cmd_complete);
> +	err = rpmsg_send(info->rpdev->ept, (void *)msg,
> +			 sizeof(struct gpio_rpmsg_data));
> +	if (err) {
> +		dev_err(&info->rpdev->dev, "rpmsg_send failed: %d\n", err);
> +		return err;
> +	}
> +
> +	if (sync) {
> +		err = wait_for_completion_timeout(&info->cmd_complete,
> +						  msecs_to_jiffies(RPMSG_TIMEOUT));
> +		if (!err) {
> +			dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		if (info->reply_msg->out.retcode != 0) {
> +			dev_err(&info->rpdev->dev, "rpmsg not ack %d!\n",
> +				info->reply_msg->out.retcode);
> +			return -EINVAL;
> +		}
> +
> +		/* copy the reply message */
> +		memcpy(&port->gpio_pins[info->reply_msg->pin_idx].msg,
> +		       info->reply_msg, sizeof(*info->reply_msg));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct gpio_rpmsg_data *gpio_get_pin_msg(struct imx_rpmsg_gpio_port *port,
> +						unsigned int offset)
> +{
> +	struct gpio_rpmsg_data *msg = &port->gpio_pins[offset].msg;
> +
> +	memset(msg, 0, sizeof(struct gpio_rpmsg_data));
> +
> +	return msg;
> +};
> +
> +static int imx_rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +	struct gpio_rpmsg_data *msg = NULL;
> +	int ret;
> +
> +	guard(mutex)(&port->info.lock);
> +
> +	msg = gpio_get_pin_msg(port, gpio);
> +	msg->header.cate = IMX_RPMSG_GPIO;
> +	msg->header.major = IMX_RMPSG_MAJOR;
> +	msg->header.minor = IMX_RMPSG_MINOR;
> +	msg->header.type = GPIO_RPMSG_SETUP;
> +	msg->header.cmd = GPIO_RPMSG_INPUT_GET;
> +	msg->pin_idx = gpio;
> +	msg->port_idx = port->idx;
> +
> +	ret = gpio_send_message(port, msg, true);
> +	if (!ret)
> +		ret = !!port->gpio_pins[gpio].msg.in.value;
Does this code is save?  !! return a boolean right?
why not force to 1 if  greater that 1?

> +
> +	return ret;
> +}
> +
> +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
> +					  unsigned int gpio)
> +{
> +	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +	struct gpio_rpmsg_data *msg = NULL;
> +
> +	guard(mutex)(&port->info.lock);
> +
> +	msg = gpio_get_pin_msg(port, gpio);
> +	msg->header.cate = IMX_RPMSG_GPIO;
Do you use a single rpmsg channel for several feature?
Any reason to not use one rpmsg channel per feature category?

> +	msg->header.major = IMX_RMPSG_MAJOR;
> +	msg->header.minor = IMX_RMPSG_MINOR;
> +	msg->header.type = GPIO_RPMSG_SETUP;
> +	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
> +	msg->pin_idx = gpio;
> +	msg->port_idx = port->idx;
> +
> +	msg->out.event = GPIO_RPMSG_TRI_IGNORE;
> +	msg->in.wakeup = 0;
> +
> +	return gpio_send_message(port, msg, true);
> +}
> +
> +static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
> +		unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
> +{
> +	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +
> +	msg->header.cate = IMX_RPMSG_GPIO;
> +	msg->header.major = IMX_RMPSG_MAJOR;
> +	msg->header.minor = IMX_RMPSG_MINOR;
> +	msg->header.type = GPIO_RPMSG_SETUP;
> +	msg->header.cmd = GPIO_RPMSG_OUTPUT_INIT;
> +	msg->pin_idx = gpio;
> +	msg->port_idx = port->idx;
> +	msg->out.value = val;
> +}
> +
> +static int imx_rpmsg_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> +	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +	struct gpio_rpmsg_data *msg = NULL;
> +
> +	guard(mutex)(&port->info.lock);
> +
> +	msg = gpio_get_pin_msg(port, gpio);
> +	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
> +
> +	return gpio_send_message(port, msg, true);
> +}
> +
> +static int imx_rpmsg_gpio_direction_output(struct gpio_chip *gc,
> +					unsigned int gpio, int val)
> +{
> +	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +	struct gpio_rpmsg_data *msg = NULL;
> +
> +	guard(mutex)(&port->info.lock);
> +
> +	msg = gpio_get_pin_msg(port, gpio);
> +	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
> +
> +	return gpio_send_message(port, msg, true);
> +}
> +
> +static int imx_rpmsg_irq_set_type(struct irq_data *d, u32 type)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 gpio_idx = d->hwirq;
> +	int edge = 0;
> +	int ret = 0;
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		edge = GPIO_RPMSG_TRI_RISING;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		edge = GPIO_RPMSG_TRI_FALLING;
> +		break;
> +	case IRQ_TYPE_EDGE_BOTH:
> +		edge = GPIO_RPMSG_TRI_BOTH_EDGE;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		edge = GPIO_RPMSG_TRI_LOW_LEVEL;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		edge = GPIO_RPMSG_TRI_HIGH_LEVEL;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	port->gpio_pins[gpio_idx].irq_type = edge;
> +
> +	return ret;
> +}
> +
> +static int imx_rpmsg_irq_set_wake(struct irq_data *d, u32 enable)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 gpio_idx = d->hwirq;
> +
> +	port->gpio_pins[gpio_idx].irq_wake_enable = enable;
> +
> +	return 0;
> +}
> +
> +/*
> + * This function will be called at:
> + *  - one interrupt setup.
> + *  - the end of one interrupt happened
> + * The gpio over rpmsg driver will not write the real register, so save
> + * all infos before this function and then send all infos to M core in this
> + * step.
> + */
> +static void imx_rpmsg_unmask_irq(struct irq_data *d)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 gpio_idx = d->hwirq;
> +
> +	port->gpio_pins[gpio_idx].irq_unmask = 1;
> +}
> +
> +static void imx_rpmsg_mask_irq(struct irq_data *d)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 gpio_idx = d->hwirq;
> +	/*
> +	 * No need to implement the callback at A core side.
> +	 * M core will mask interrupt after a interrupt occurred, and then
> +	 * sends a notify to A core.
> +	 * After A core dealt with the notify, A core will send a rpmsg to
> +	 * M core to unmask this interrupt again.
> +	 */
> +	port->gpio_pins[gpio_idx].irq_mask = 1;
> +}
> +
> +static void imx_rpmsg_irq_shutdown(struct irq_data *d)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 gpio_idx = d->hwirq;
> +
> +	port->gpio_pins[gpio_idx].irq_shutdown = 1;
> +}
> +
> +static void imx_rpmsg_irq_bus_lock(struct irq_data *d)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +
> +	mutex_lock(&port->info.lock);
> +}
> +
> +static void imx_rpmsg_irq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	struct gpio_rpmsg_data *msg = NULL;
> +	u32 gpio_idx = d->hwirq;
> +
> +	if (port == NULL) {
> +		mutex_unlock(&port->info.lock);
> +		return;
> +	}
> +
> +	/*
> +	 * For mask irq, do nothing here.
> +	 * M core will mask interrupt after a interrupt occurred, and then
> +	 * sends a notify to A core.
> +	 * After A core dealt with the notify, A core will send a rpmsg to
> +	 * M core to unmask this interrupt again.
> +	 */
> +
> +	if (port->gpio_pins[gpio_idx].irq_mask && !port->gpio_pins[gpio_idx].irq_unmask) {
> +		port->gpio_pins[gpio_idx].irq_mask = 0;
> +		mutex_unlock(&port->info.lock);
> +		return;
> +	}
> +
> +	msg = gpio_get_pin_msg(port, gpio_idx);
> +	msg->header.cate = IMX_RPMSG_GPIO;
> +	msg->header.major = IMX_RMPSG_MAJOR;
> +	msg->header.minor = IMX_RMPSG_MINOR;
> +	msg->header.type = GPIO_RPMSG_SETUP;
> +	msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
> +	msg->pin_idx = gpio_idx;
> +	msg->port_idx = port->idx;
> +
> +	if (port->gpio_pins[gpio_idx].irq_shutdown) {
> +		msg->out.event = GPIO_RPMSG_TRI_IGNORE;
> +		msg->in.wakeup = 0;
> +		port->gpio_pins[gpio_idx].irq_shutdown = 0;
> +	} else {
> +		 /* if not set irq type, then use low level as trigger type */
> +		msg->out.event = port->gpio_pins[gpio_idx].irq_type;
> +		if (!msg->out.event)
> +			msg->out.event = GPIO_RPMSG_TRI_LOW_LEVEL;
> +		if (port->gpio_pins[gpio_idx].irq_unmask) {
> +			msg->in.wakeup = 0;
> +			port->gpio_pins[gpio_idx].irq_unmask = 0;
> +		} else /* irq set wake */
> +			msg->in.wakeup = port->gpio_pins[gpio_idx].irq_wake_enable;
> +	}
> +
> +	gpio_send_message(port, msg, false);
> +	mutex_unlock(&port->info.lock);
> +}
> +
> +static const struct irq_chip imx_rpmsg_irq_chip = {
> +	.irq_mask = imx_rpmsg_mask_irq,
> +	.irq_unmask = imx_rpmsg_unmask_irq,
> +	.irq_set_wake = imx_rpmsg_irq_set_wake,
> +	.irq_set_type = imx_rpmsg_irq_set_type,
> +	.irq_shutdown = imx_rpmsg_irq_shutdown,
> +	.irq_bus_lock = imx_rpmsg_irq_bus_lock,
> +	.irq_bus_sync_unlock = imx_rpmsg_irq_bus_sync_unlock,
> +	.flags = IRQCHIP_IMMUTABLE,
> +};
> +
> +static int imx_rpmsg_gpio_callback(struct rpmsg_device *rpdev,
> +	void *data, int len, void *priv, u32 src)
> +{
> +	struct gpio_rpmsg_data *msg = (struct gpio_rpmsg_data *)data;
> +	struct imx_rpmsg_gpio_port *port = NULL;
> +	struct imx_rpmsg_driver_data *drvdata;
> +	unsigned long flags;
> +
> +	drvdata = dev_get_drvdata(&rpdev->dev);
> +	if (msg)
> +		port = drvdata->channel_devices[msg->port_idx];
> +
> +	if (!port)
> +		return -ENODEV;
> +
> +	if (msg->header.type == GPIO_RPMSG_REPLY) {
> +		port->info.reply_msg = msg;
> +		complete(&port->info.cmd_complete);
> +	} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
> +		port->info.notify_msg = msg;
> +		local_irq_save(flags);
> +		generic_handle_domain_irq(port->gc.irq.domain, msg->pin_idx);
> +		local_irq_restore(flags);
> +	} else
> +		dev_err(&rpdev->dev, "wrong command type!\n");
> +
> +	return 0;
> +}
> +
> +static void imx_rpmsg_gpio_remove_action(void *data)
> +{
> +	struct imx_rpmsg_gpio_port *port = data;
> +
> +	port->info.port_store[port->idx] = 0;
> +}
> +
> +static int imx_rpmsg_gpio_probe(struct platform_device *pdev)
> +{
> +	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct imx_rpmsg_gpio_port *port;
> +	struct gpio_irq_chip *girq;
> +	struct gpio_chip *gc;
> +	int ret;
> +
> +	if (!pltdata)
> +		return -EPROBE_DEFER;
> +
> +	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(np, "reg", &port->idx);
> +	if (ret)
> +		return ret;
> +
> +	if (port->idx > MAX_DEV_PER_CHANNEL)
> +		return -EINVAL;
> +
> +	mutex_init(&port->info.lock);
> +	init_completion(&port->info.cmd_complete);
> +	port->info.rpdev = pltdata->rpdev;
> +	port->info.port_store = pltdata->channel_devices;
> +	port->info.port_store[port->idx] = port;
> +	if (!pltdata->rx_callback)
> +		pltdata->rx_callback = imx_rpmsg_gpio_callback;
> +
> +	gc = &port->gc;
> +	gc->owner = THIS_MODULE;
> +	gc->parent = &pltdata->rpdev->dev;
> +	gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> +				   pltdata->rproc_name, port->idx);
> +	gc->ngpio = IMX_RPMSG_GPIO_PER_PORT;
> +	gc->base = -1;
> +
> +	gc->direction_input = imx_rpmsg_gpio_direction_input;
> +	gc->direction_output = imx_rpmsg_gpio_direction_output;
> +	gc->get = imx_rpmsg_gpio_get;
> +	gc->set = imx_rpmsg_gpio_set;
> +
> +	platform_set_drvdata(pdev, port);
> +	girq = &gc->irq;
> +	gpio_irq_chip_set_chip(girq, &imx_rpmsg_irq_chip);
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->chip->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> +				 pltdata->rproc_name, port->idx);
> +
> +	devm_add_action_or_reset(&pdev->dev, imx_rpmsg_gpio_remove_action, port);
> +
> +	return devm_gpiochip_add_data(&pdev->dev, gc, port);
> +}
> +
> +static const struct of_device_id imx_rpmsg_gpio_dt_ids[] = {
> +	{ .compatible = "fsl,imx-rpmsg-gpio" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver imx_rpmsg_gpio_driver = {
> +	.driver	= {
> +		.name = "gpio-imx-rpmsg",
> +		.of_match_table = imx_rpmsg_gpio_dt_ids,
> +	},
> +	.probe = imx_rpmsg_gpio_probe,
> +};
> +
> +module_platform_driver(imx_rpmsg_gpio_driver);
This implementation seems strange to me.
You have a platform driver, but your RPMsg driver appears split between 
this driver
and the remoteproc driver, especially regarding the 
imx_rpmsg_endpoint_probe() function.

 From my point of view, this driver should declare both a 
platform_driver and an
rpmsg_driver structures.

Your imx_rpmsg_gpio_driver platform driver should be probed by your 
remoteproc platform.
This platform driver would be responsible for:
- Parsing the device tree node
- Registering the RPMsg driver

Then, the RPMsg device should be probed either by the remote processor 
using the name service
announcement mechanism or if not possible by your remoteproc driver.

To better understand my proposal you can have a look to [1]and [2].
Here is another example for an rpmsg_i2c( ST downstream implementation):
https://github.com/STMicroelectronics/linux/blob/v6.6-stm32mp/drivers/i2c/busses/i2c-rpmsg.c
https://github.com/STMicroelectronics/linux/blob/v6.6-stm32mp/Documentation/devicetree/bindings/i2c/i2c-rpmsg.yaml

Thanks and Regards,
Arnaud

> +
> +MODULE_AUTHOR("NXP Semiconductor");
> +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Shenwei Wang]

Hi Arnaud,

> -----Original Message-----
> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
> Sent: Friday, October 3, 2025 2:40 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Bjorn Andersson
> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Rob
> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> Dooley <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Linus Walleij <linus.walleij@linaro.org>;
> Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
> > These processors communicate via the RPMSG protocol.
> > The driver implements the standard GPIO interface, allowing the Linux
> > side to control GPIO controllers which reside in the remote processor
> > via RPMSG protocol.
>
> What about my request in previous version to make this driver generic?
>

The only platform-dependent part of this driver is the layout of the message packet, which defines the
communication protocol between the host and the remote processor. It would be challenging to require
other vendors to follow our protocol and conform to the expected behavior.

> In ST we have similar driver in downstream, we would be interested in reusing it if
> generic. Indeed we need some rpmsg mechanism for gpio-interrupt. Today we
> have a downstream rpmsg driver [1][2] that could migrate on a generic rpmsg-
> gpio driver.
>
> > +
> > +#include <linux/err.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/rpmsg/imx_rpmsg.h>
> > +#include <linux/init.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/rpmsg.h>
> > +
> > +#define IMX_RPMSG_GPIO_PER_PORT      32
> > +#define RPMSG_TIMEOUT        1000
> > +
> > +enum gpio_input_trigger_type {
> > +     GPIO_RPMSG_TRI_IGNORE,
> > +     GPIO_RPMSG_TRI_RISING,
> > +     GPIO_RPMSG_TRI_FALLING,
> > +     GPIO_RPMSG_TRI_BOTH_EDGE,
> > +     GPIO_RPMSG_TRI_LOW_LEVEL,
> > +     GPIO_RPMSG_TRI_HIGH_LEVEL,
> > +};
> What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|?
> For instance:
> GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING |
> GPIO_RPMSG_TRI_RISING,

Yes, the suggestion is better.

> > +
> > +enum gpio_rpmsg_header_type {
> > +     GPIO_RPMSG_SETUP,
> > +     GPIO_RPMSG_REPLY,
> > +     GPIO_RPMSG_NOTIFY,
> > +};
> > +
> > +enum gpio_rpmsg_header_cmd {
> > +     GPIO_RPMSG_INPUT_INIT,
> > +     GPIO_RPMSG_OUTPUT_INIT,
> > +     GPIO_RPMSG_INPUT_GET,
> > +};
> > +
> > +struct gpio_rpmsg_data {
> > +     struct imx_rpmsg_head header;
> > +     u8 pin_idx;
> > +     u8 port_idx;
> > +     union {
> > +             u8 event;
> > +             u8 retcode;
> > +             u8 value;
> > +     } out;
> > +     union {
> > +             u8 wakeup;
> > +             u8 value;
> nitpicking put "value" field out of union as common

I'm afraid we can't make it common, as the two 'value' fields serve different purposes-one is used for outgoing messages and
the other for incoming messages-and they are located in different parts of the packet.

> > +     } in;
> > +} __packed __aligned(8);
>
> Any reason to pack it an align it?
> This structure will be copied in the RPMSg payload, right?
>

Yes. The message will then be transmitted via the MU hardware to the remote processor, so proper alignment is required.

> I also wonder if that definition should not be in a header file with double licensing
> that the DT. Indeed this need to be common with the remote side
> implementation  that can be non GPL implementation.
>
> > +
> > +struct imx_rpmsg_gpio_pin {
> > +     u8 irq_shutdown;
> > +     u8 irq_unmask;
> > +     u8 irq_mask;
> > +     u32 irq_wake_enable;
> > +     u32 irq_type;
> > +     struct gpio_rpmsg_data msg;
> > +};
> > +
> > +struct imx_gpio_rpmsg_info {
> > +     struct rpmsg_device *rpdev;
> > +     struct gpio_rpmsg_data *notify_msg;
> > +     struct gpio_rpmsg_data *reply_msg;
> > +     struct completion cmd_complete;
> > +     struct mutex lock;
> > +     msg->pin_idx = gpio;
> > +     msg->port_idx = port->idx;
> > +
> > +     ret = gpio_send_message(port, msg, true);
> > +     if (!ret)
> > +             ret = !!port->gpio_pins[gpio].msg.in.value;
> Does this code is save?  !! return a boolean right?
> why not force to 1 if  greater that 1?
>

This approach is intended to simplify the implementation. Forcing  to 1 would introduce an additional condition check.
I'm open to changing it if that's considered standard practice.

> > +
> > +     return ret;
> > +}
> > +
> > +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
> > +                                       unsigned int gpio) {
> > +     struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> > +     struct gpio_rpmsg_data *msg = NULL;
> > +
> > +     guard(mutex)(&port->info.lock);
> > +
> > +     msg = gpio_get_pin_msg(port, gpio);
> > +     msg->header.cate = IMX_RPMSG_GPIO;
> Do you use a single rpmsg channel for several feature?
> Any reason to not use one rpmsg channel per feature category?
>

The current implementation on the remote side uses a single channel to handle all GPIO controllers.
However, this driver itself does not have that limitation.

> > +     msg->header.major = IMX_RMPSG_MAJOR;
> > +     msg->header.minor = IMX_RMPSG_MINOR;
> > +     msg->header.type = GPIO_RPMSG_SETUP;
> > +     msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
> > +     msg->pin_idx = gpio;
> > +     msg->port_idx = port->idx;
> > +
> > +     msg->out.event = GPIO_RPMSG_TRI_IGNORE;
> > +     msg->in.wakeup = 0;
> > +
> > +     return gpio_send_message(port, msg, true); }
> > +
> > +static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
> > +             unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
> > +
> > +static struct platform_driver imx_rpmsg_gpio_driver = {
> > +     .driver = {
> > +             .name = "gpio-imx-rpmsg",
> > +             .of_match_table = imx_rpmsg_gpio_dt_ids,
> > +     },
> > +     .probe = imx_rpmsg_gpio_probe,
> > +};
> > +
> > +module_platform_driver(imx_rpmsg_gpio_driver);
> This implementation seems strange to me.
> You have a platform driver, but your RPMsg driver appears split between this
> driver and the remoteproc driver, especially regarding the
> imx_rpmsg_endpoint_probe() function.
>

See my reply below.

>  From my point of view, this driver should declare both a platform_driver and an
> rpmsg_driver structures.
>
> Your imx_rpmsg_gpio_driver platform driver should be probed by your
> remoteproc platform.
> This platform driver would be responsible for:
> - Parsing the device tree node
> - Registering the RPMsg driver
>
> Then, the RPMsg device should be probed either by the remote processor using
> the name service announcement mechanism or if not possible by your
> remoteproc driver.
>

The idea is to probe the GPIO driver successfully only after the remote processor is online and has sent the name service announcement.
Until then, the GPIO driver will remain in a deferred state, ensuring that all consumers of the associated GPIOs are also deferred.
The implementation you provided below does not guarantee that the related consumers will be properly deferred. This is the most
important behavior for a GPIO/I2C controllers.

Thanks,
Shenwei

> To better understand my proposal you can have a look to [1]and [2].
> Here is another example for an rpmsg_i2c( ST downstream implementation):
> https://github.co/
> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
> stm32mp%2Fdrivers%2Fi2c%2Fbusses%2Fi2c-
> rpmsg.c&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b474e
> 391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 8950740622597592%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
> 3D%7C0%7C%7C%7C&sdata=6lCk20Qhb%2F0MTw0NFtto7tj7EFYwZ%2BlOR1F3
> Qk7kQn8%3D&reserved=0
> https://github.co/
> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
> stm32mp%2FDocumentation%2Fdevicetree%2Fbindings%2Fi2c%2Fi2c-
> rpmsg.yaml&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b4
> 74e391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C638950740622612512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
> ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
> %3D%7C0%7C%7C%7C&sdata=4Gva%2FpqP2u8T57XDxSDaoHhvDeJ%2Fo5HtAB
> L9TY5gbDI%3D&reserved=0
>
> Thanks and Regards,
> Arnaud
>
> > +
> > +MODULE_AUTHOR("NXP Semiconductor");
> > +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
> > +MODULE_LICENSE("GPL");

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN]

On 10/3/25 20:41, Shenwei Wang wrote:
> Hi Arnaud,
>
>> -----Original Message-----
>> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
>> Sent: Friday, October 3, 2025 2:40 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Bjorn Andersson
>> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Rob
>> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
>> Dooley <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Linus Walleij <linus.walleij@linaro.org>;
>> Bartosz Golaszewski <brgl@bgdev.pl>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
>> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
>>> These processors communicate via the RPMSG protocol.
>>> The driver implements the standard GPIO interface, allowing the Linux
>>> side to control GPIO controllers which reside in the remote processor
>>> via RPMSG protocol.
>> What about my request in previous version to make this driver generic?
>>
> The only platform-dependent part of this driver is the layout of the message packet, which defines the
> communication protocol between the host and the remote processor. It would be challenging to require
> other vendors to follow our protocol and conform to the expected behavior.
>
>> In ST we have similar driver in downstream, we would be interested in reusing it if
>> generic. Indeed we need some rpmsg mechanism for gpio-interrupt. Today we
>> have a downstream rpmsg driver [1][2] that could migrate on a generic rpmsg-
>> gpio driver.
>>
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/gpio/driver.h>
>>> +#include <linux/rpmsg/imx_rpmsg.h>
>>> +#include <linux/init.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/rpmsg.h>
>>> +
>>> +#define IMX_RPMSG_GPIO_PER_PORT      32
>>> +#define RPMSG_TIMEOUT        1000
>>> +
>>> +enum gpio_input_trigger_type {
>>> +     GPIO_RPMSG_TRI_IGNORE,
>>> +     GPIO_RPMSG_TRI_RISING,
>>> +     GPIO_RPMSG_TRI_FALLING,
>>> +     GPIO_RPMSG_TRI_BOTH_EDGE,
>>> +     GPIO_RPMSG_TRI_LOW_LEVEL,
>>> +     GPIO_RPMSG_TRI_HIGH_LEVEL,
>>> +};
>> What about taking inspiration from the|IRQ_TYPE|bitfield defined in|irq.h|?
>> For instance:
>> GPIO_RPMSG_TRI_BOTH_EDGE = GPIO_RPMSG_TRI_FALLING |
>> GPIO_RPMSG_TRI_RISING,
> Yes, the suggestion is better.
>
>>> +
>>> +enum gpio_rpmsg_header_type {
>>> +     GPIO_RPMSG_SETUP,
>>> +     GPIO_RPMSG_REPLY,
>>> +     GPIO_RPMSG_NOTIFY,
>>> +};
>>> +
>>> +enum gpio_rpmsg_header_cmd {
>>> +     GPIO_RPMSG_INPUT_INIT,
>>> +     GPIO_RPMSG_OUTPUT_INIT,
>>> +     GPIO_RPMSG_INPUT_GET,
>>> +};
>>> +
>>> +struct gpio_rpmsg_data {
>>> +     struct imx_rpmsg_head header;
>>> +     u8 pin_idx;
>>> +     u8 port_idx;
>>> +     union {
>>> +             u8 event;
>>> +             u8 retcode;
>>> +             u8 value;
>>> +     } out;
>>> +     union {
>>> +             u8 wakeup;
>>> +             u8 value;
>> nitpicking put "value" field out of union as common
> I'm afraid we can't make it common, as the two 'value' fields serve different purposes-one is used for outgoing messages and
> the other for incoming messages-and they are located in different parts of the packet.
>
>>> +     } in;
>>> +} __packed __aligned(8);
>> Any reason to pack it an align it?
>> This structure will be copied in the RPMSg payload, right?
>>
> Yes. The message will then be transmitted via the MU hardware to the remote processor, so proper alignment is required.
>
>> I also wonder if that definition should not be in a header file with double licensing
>> that the DT. Indeed this need to be common with the remote side
>> implementation  that can be non GPL implementation.
>>
>>> +
>>> +struct imx_rpmsg_gpio_pin {
>>> +     u8 irq_shutdown;
>>> +     u8 irq_unmask;
>>> +     u8 irq_mask;
>>> +     u32 irq_wake_enable;
>>> +     u32 irq_type;
>>> +     struct gpio_rpmsg_data msg;
>>> +};
>>> +
>>> +struct imx_gpio_rpmsg_info {
>>> +     struct rpmsg_device *rpdev;
>>> +     struct gpio_rpmsg_data *notify_msg;
>>> +     struct gpio_rpmsg_data *reply_msg;
>>> +     struct completion cmd_complete;
>>> +     struct mutex lock;
>>> +     msg->pin_idx = gpio;
>>> +     msg->port_idx = port->idx;
>>> +
>>> +     ret = gpio_send_message(port, msg, true);
>>> +     if (!ret)
>>> +             ret = !!port->gpio_pins[gpio].msg.in.value;
>> Does this code is save?  !! return a boolean right?
>> why not force to 1 if  greater that 1?
>>
> This approach is intended to simplify the implementation. Forcing  to 1 would introduce an additional condition check.
> I'm open to changing it if that's considered standard practice.
>
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
>>> +                                       unsigned int gpio) {
>>> +     struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> +     struct gpio_rpmsg_data *msg = NULL;
>>> +
>>> +     guard(mutex)(&port->info.lock);
>>> +
>>> +     msg = gpio_get_pin_msg(port, gpio);
>>> +     msg->header.cate = IMX_RPMSG_GPIO;
>> Do you use a single rpmsg channel for several feature?
>> Any reason to not use one rpmsg channel per feature category?
>>
> The current implementation on the remote side uses a single channel to handle all GPIO controllers.
> However, this driver itself does not have that limitation.
>
>>> +     msg->header.major = IMX_RMPSG_MAJOR;
>>> +     msg->header.minor = IMX_RMPSG_MINOR;
>>> +     msg->header.type = GPIO_RPMSG_SETUP;
>>> +     msg->header.cmd = GPIO_RPMSG_INPUT_INIT;
>>> +     msg->pin_idx = gpio;
>>> +     msg->port_idx = port->idx;
>>> +
>>> +     msg->out.event = GPIO_RPMSG_TRI_IGNORE;
>>> +     msg->in.wakeup = 0;
>>> +
>>> +     return gpio_send_message(port, msg, true); }
>>> +
>>> +static inline void imx_rpmsg_gpio_direction_output_init(struct gpio_chip *gc,
>>> +             unsigned int gpio, int val, struct gpio_rpmsg_data *msg)
>>> +
>>> +static struct platform_driver imx_rpmsg_gpio_driver = {
>>> +     .driver = {
>>> +             .name = "gpio-imx-rpmsg",
>>> +             .of_match_table = imx_rpmsg_gpio_dt_ids,
>>> +     },
>>> +     .probe = imx_rpmsg_gpio_probe,
>>> +};
>>> +
>>> +module_platform_driver(imx_rpmsg_gpio_driver);
>> This implementation seems strange to me.
>> You have a platform driver, but your RPMsg driver appears split between this
>> driver and the remoteproc driver, especially regarding the
>> imx_rpmsg_endpoint_probe() function.
>>
> See my reply below.
>
>>   From my point of view, this driver should declare both a platform_driver and an
>> rpmsg_driver structures.
>>
>> Your imx_rpmsg_gpio_driver platform driver should be probed by your
>> remoteproc platform.
>> This platform driver would be responsible for:
>> - Parsing the device tree node
>> - Registering the RPMsg driver
>>
>> Then, the RPMsg device should be probed either by the remote processor using
>> the name service announcement mechanism or if not possible by your
>> remoteproc driver.
>>
> The idea is to probe the GPIO driver successfully only after the remote processor is online and has sent the name service announcement.
> Until then, the GPIO driver will remain in a deferred state, ensuring that all consumers of the associated GPIOs are also deferred.
> The implementation you provided below does not guarantee that the related consumers will be properly deferred. This is the most
> important behavior for a GPIO/I2C controllers.


As long as you keep the GPIO/I2C device as a child of the remote 
processor node,
you should not have deferred probe issues. 
The|of_platform_populate()|function ensures
that the I2C/GPIO devices are probed when the remote processor is started.
Calling|devm_gpiochip_add_data|in the RPMsg driver probe should also 
prevent such issues.

Regards,
Arnaud

>
> Thanks,
> Shenwei
>
>> To better understand my proposal you can have a look to [1]and [2].
>> Here is another example for an rpmsg_i2c( ST downstream implementation):
>> https://github.co/
>> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
>> stm32mp%2Fdrivers%2Fi2c%2Fbusses%2Fi2c-
>> rpmsg.c&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b474e
>> 391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
>> 8950740622597592%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRyd
>> WUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%
>> 3D%7C0%7C%7C%7C&sdata=6lCk20Qhb%2F0MTw0NFtto7tj7EFYwZ%2BlOR1F3
>> Qk7kQn8%3D&reserved=0
>> https://github.co/
>> m%2FSTMicroelectronics%2Flinux%2Fblob%2Fv6.6-
>> stm32mp%2FDocumentation%2Fdevicetree%2Fbindings%2Fi2c%2Fi2c-
>> rpmsg.yaml&data=05%7C02%7Cshenwei.wang%40nxp.com%7C22a9c88be60b4
>> 74e391008de02502ec7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
>> C638950740622612512%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnR
>> ydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D
>> %3D%7C0%7C%7C%7C&sdata=4Gva%2FpqP2u8T57XDxSDaoHhvDeJ%2Fo5HtAB
>> L9TY5gbDI%3D&reserved=0
>>
>> Thanks and Regards,
>> Arnaud
>>
>>> +
>>> +MODULE_AUTHOR("NXP Semiconductor");
>>> +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
>>> +MODULE_LICENSE("GPL");

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Shenwei Wang]

Hi Arnaud,

> -----Original Message-----
> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
> Sent: Monday, October 6, 2025 4:53 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Bjorn Andersson
> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Rob
> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> Dooley <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Linus Walleij <linus.walleij@linaro.org>;
> Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; openamp-rp@lists.openampproject.org
> Subject: [EXT] Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
> >> Then, the RPMsg device should be probed either by the remote
> >> processor using the name service announcement mechanism or if not
> >> possible by your remoteproc driver.
> >>
> > The idea is to probe the GPIO driver successfully only after the remote
> processor is online and has sent the name service announcement.
> > Until then, the GPIO driver will remain in a deferred state, ensuring that all
> consumers of the associated GPIOs are also deferred.
> > The implementation you provided below does not guarantee that the
> > related consumers will be properly deferred. This is the most important
> behavior for a GPIO/I2C controllers.
> 
> 
> As long as you keep the GPIO/I2C device as a child of the remote processor node,
> you should not have deferred probe issues.
> The|of_platform_populate()|function ensures
> that the I2C/GPIO devices are probed when the remote processor is started.
> Calling|devm_gpiochip_add_data|in the RPMsg driver probe should also
> prevent such issues.
> 

Here, deferred probing is not an issue -it's an intentional feature. We need to ensure that all consumers of the GPIO/I2C controllers remain
in the deferred state until the remote processor is fully online.

For instance, consider a regulator node that references a GPIO line from the RPMSG GPIO controller. The regulator will stay in the deferred state 
until the remote processor comes online and its services are announced and received.

Thanks,
Shenwei

> Regards,
> Arnaud
> 
> >
> > Thanks,
> > Shenwei
> >
> >> To better understand my proposal you can have a look to [1]and [2].
> >> Here is another example for an rpmsg_i2c( ST downstream implementation):
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit

Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN]

Hello Shenwei,

On 10/6/25 16:33, Shenwei Wang wrote:
> 
> Hi Arnaud,
> 
>> -----Original Message-----
>> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
>> Sent: Monday, October 6, 2025 4:53 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Bjorn Andersson
>> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Rob
>> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
>> Dooley <conor+dt@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
>> Hauer <s.hauer@pengutronix.de>; Linus Walleij <linus.walleij@linaro.org>;
>> Bartosz Golaszewski <brgl@bgdev.pl>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
>> <festevam@gmail.com>; Peng Fan <peng.fan@nxp.com>; linux-
>> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; imx@lists.linux.dev;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx
>> <linux-imx@nxp.com>; openamp-rp@lists.openampproject.org
>> Subject: [EXT] Re: [PATCH v2 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
>>>> Then, the RPMsg device should be probed either by the remote
>>>> processor using the name service announcement mechanism or if not
>>>> possible by your remoteproc driver.
>>>>
>>> The idea is to probe the GPIO driver successfully only after the remote
>> processor is online and has sent the name service announcement.
>>> Until then, the GPIO driver will remain in a deferred state, ensuring that all
>> consumers of the associated GPIOs are also deferred.
>>> The implementation you provided below does not guarantee that the
>>> related consumers will be properly deferred. This is the most important
>> behavior for a GPIO/I2C controllers.
>>
>>
>> As long as you keep the GPIO/I2C device as a child of the remote processor node,
>> you should not have deferred probe issues.
>> The|of_platform_populate()|function ensures
>> that the I2C/GPIO devices are probed when the remote processor is started.
>> Calling|devm_gpiochip_add_data|in the RPMsg driver probe should also
>> prevent such issues.
>>
> 
> Here, deferred probing is not an issue -it's an intentional feature. We need to ensure that all consumers of the GPIO/I2C controllers remain
> in the deferred state until the remote processor is fully online.
> 
> For instance, consider a regulator node that references a GPIO line from the RPMSG GPIO controller. The regulator will stay in the deferred state
> until the remote processor comes online and its services are announced and received.

I think there is a misunderstanding. My intention was just to mention
that in my proposal, the deferred mechanism should also work as
expected. This is the case for the rpmsg_i2c I mentioned as an example.
Anyway the main point here is to break the dependency between your 
remoteproc driver and the rpmsg GPIO driver. In your remoteproc
driver, you should just call of_platform_populate, and let's the 
compatible mechanism find the associated independent driver defined
in the rpmsg_gpio.c


 From my perspective the sequence should be
1) the remoteproc driver starts the remote processor
2) the remoteproc driver parses the child node with of_platform_populate
3) the rpmsg_gpio platform driver is probed by the of_platform_populate
     - parse the DT node and store configuration in data structure
     - register an rpmsg_gpio driver
4) the rpmsg gpio driver is probed (by the rpmsg bus).
     - register the GPIO and irq chips

Until step 4, the users should be automatically deferred

That said, regarding your implementation, the fact that you have created 
a single rpmsg endpoint for several rpmsg services complexify this 
approach , creating a dependency not only between rpmsg and remoteproc 
but also among rpmsg devices. Having a single rpmsg endpoint associated 
with a single rpmsg service would simplify things.

Of course, I am just sharing my opinion and expectations here. For the 
next steps, I will let the maintainers, Mathieu and Bjorn, provide their 
advice and guidance.

Thanks,
Arnaud

> 
> Thanks,
> Shenwei
> 
>> Regards,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Shenwei
>>>
>>>> To better understand my proposal you can have a look to [1]and [2].
>>>> Here is another example for an rpmsg_i2c( ST downstream implementation):
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit