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

Shenwei Wang posted 4 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Posted by Shenwei Wang 1 month, 2 weeks ago
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          |  11 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-imx-rpmsg.c | 559 ++++++++++++++++++++++++++++++++++
 3 files changed, 571 insertions(+)
 create mode 100644 drivers/gpio/gpio-imx-rpmsg.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a437fe652dbc..2ce4e9b5225e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -402,6 +402,17 @@ config GPIO_ICH
 
 	  If unsure, say N.
 
+config GPIO_IMX_RPMSG
+	tristate "NXP i.MX SoC RPMSG GPIO support"
+	depends on IMX_REMOTEPROC && RPMSG && GPIOLIB
+	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.
+
 config GPIO_IMX_SCU
        def_bool y
        depends on IMX_SCU
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..0f9c5ceec651
--- /dev/null
+++ b/drivers/gpio/gpio-imx-rpmsg.c
@@ -0,0 +1,559 @@
+// 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/kernel.h>
+#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/imx_rpmsg.h>
+#include <linux/init.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_qos.h>
+#include <linux/rpmsg.h>
+#include <linux/virtio.h>
+#include <linux/workqueue.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 pm_qos_request pm_qos_req;
+	struct completion cmd_complete;
+	struct mutex lock;
+	struct imx_rpmsg_gpio_port **port_store;
+};
+
+struct imx_rpmsg_gpio_port {
+	struct gpio_chip gc;
+	struct irq_chip chip;
+	struct device *dev;
+	struct irq_domain *domain;
+	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;
+	}
+
+	cpu_latency_qos_add_request(&info->pm_qos_req, 0);
+	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);
+		goto err_out;
+	}
+
+	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");
+			err = -ETIMEDOUT;
+			goto err_out;
+		}
+
+		if (info->reply_msg->out.retcode != 0) {
+			dev_err(&info->rpdev->dev, "rpmsg not ack %d!\n",
+				info->reply_msg->out.retcode);
+			err = -EINVAL;
+			goto err_out;
+		}
+
+		/* copy the reply message */
+		memcpy(&port->gpio_pins[info->reply_msg->pin_idx].msg,
+		       info->reply_msg, sizeof(*info->reply_msg));
+
+		err = 0;
+	}
+
+err_out:
+	cpu_latency_qos_remove_request(&info->pm_qos_req);
+
+	return err;
+}
+
+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;
+
+	mutex_lock(&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;
+
+	mutex_unlock(&port->info.lock);
+
+	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;
+	int ret;
+
+	mutex_lock(&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;
+
+	ret = gpio_send_message(port, msg, true);
+
+	mutex_unlock(&port->info.lock);
+
+	return ret;
+}
+
+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;
+	int ret;
+
+	mutex_lock(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
+	ret = gpio_send_message(port, msg, true);
+
+	mutex_unlock(&port->info.lock);
+
+	return ret;
+}
+
+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;
+	int ret;
+
+	mutex_lock(&port->info.lock);
+
+	msg = gpio_get_pin_msg(port, gpio);
+	imx_rpmsg_gpio_direction_output_init(gc, gpio, val, msg);
+	ret = gpio_send_message(port, msg, true);
+
+	mutex_unlock(&port->info.lock);
+
+	return ret;
+}
+
+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 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,
+};
+
+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;
+	unsigned long flags;
+	struct imx_rpmsg_gpio_port *port;
+	struct imx_rpmsg_driver_data *drvdata;
+
+	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->domain, msg->pin_idx);
+		local_irq_restore(flags);
+	} else
+		dev_err(&rpdev->dev, "wrong command type!\n");
+
+	return 0;
+}
+
+static int imx_rpmsg_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
+	int irq;
+
+	irq = irq_find_mapping(port->domain, gpio);
+	if (irq > 0) {
+		irq_set_chip_data(irq, port);
+		irq_set_chip_and_handler(irq, &port->chip, handle_level_irq);
+	}
+
+	return irq;
+}
+
+static void imx_rpmsg_gpio_remove_action(void *data)
+{
+	struct imx_rpmsg_gpio_port *port = data;
+
+	irq_domain_remove(port->domain);
+	port->info.port_store[port->idx] = 0;
+}
+
+static int imx_rpmsg_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_rpmsg_driver_data *pltdata = dev->platform_data;
+	struct device_node *np = dev->of_node;
+	struct imx_rpmsg_gpio_port *port;
+	struct gpio_chip *gc;
+	int irq_base;
+	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 = (struct imx_rpmsg_gpio_port **)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->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->to_irq = imx_rpmsg_gpio_to_irq;
+	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);
+	ret = devm_gpiochip_add_data(dev, gc, port);
+	if (ret < 0)
+		return ret;
+
+	devm_add_action_or_reset(&pdev->dev, imx_rpmsg_gpio_remove_action, port);
+
+	/* create an irq domain */
+	port->chip = imx_rpmsg_irq_chip;
+	port->chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
+					 pltdata->rproc_name, port->idx);
+	port->dev = &pdev->dev;
+
+	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, IMX_RPMSG_GPIO_PER_PORT,
+				   numa_node_id());
+	if (irq_base < 0) {
+		dev_err(&pdev->dev, "Failed to alloc irq_descs\n");
+		return irq_base;
+	}
+
+	port->domain = irq_domain_create_legacy(of_node_to_fwnode(np),
+						IMX_RPMSG_GPIO_PER_PORT,
+						irq_base, 0,
+						&irq_domain_simple_ops, port);
+	if (!port->domain) {
+		dev_err(&pdev->dev, "Failed to allocate IRQ domain\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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,
+};
+
+static int __init gpio_imx_rpmsg_init(void)
+{
+	return platform_driver_register(&imx_rpmsg_gpio_driver);
+}
+
+device_initcall(gpio_imx_rpmsg_init);
+
+MODULE_AUTHOR("NXP Semiconductor");
+MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0
Re: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Posted by kernel test robot 1 month, 1 week ago
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-rc2 next-20250820]
[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/20250819-044803
base:   https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
patch link:    https://lore.kernel.org/r/20250818204420.794554-4-shenwei.wang%40nxp.com
patch subject: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20250821/202508212119.gamkDcXG-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250821/202508212119.gamkDcXG-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/202508212119.gamkDcXG-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpio-imx-rpmsg.c:419:6: warning: variable 'port' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     419 |         if (msg)
         |             ^~~
   drivers/gpio/gpio-imx-rpmsg.c:421:7: note: uninitialized use occurs here
     421 |         if (!port)
         |              ^~~~
   drivers/gpio/gpio-imx-rpmsg.c:419:2: note: remove the 'if' if its condition is always true
     419 |         if (msg)
         |         ^~~~~~~~
     420 |                 port = drvdata->channel_devices[msg->port_idx];
   drivers/gpio/gpio-imx-rpmsg.c:415:34: note: initialize the variable 'port' to silence this warning
     415 |         struct imx_rpmsg_gpio_port *port;
         |                                         ^
         |                                          = NULL
   drivers/gpio/gpio-imx-rpmsg.c:503:10: error: incompatible function pointer types assigning to 'void (*)(struct gpio_chip *, unsigned int, int)' from 'int (struct gpio_chip *, unsigned int, int)' [-Wincompatible-function-pointer-types]
     503 |         gc->set = imx_rpmsg_gpio_set;
         |                 ^ ~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


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

   409	
   410	static int imx_rpmsg_gpio_callback(struct rpmsg_device *rpdev,
   411		void *data, int len, void *priv, u32 src)
   412	{
   413		struct gpio_rpmsg_data *msg = (struct gpio_rpmsg_data *)data;
   414		unsigned long flags;
   415		struct imx_rpmsg_gpio_port *port;
   416		struct imx_rpmsg_driver_data *drvdata;
   417	
   418		drvdata = dev_get_drvdata(&rpdev->dev);
 > 419		if (msg)
   420			port = drvdata->channel_devices[msg->port_idx];
   421		if (!port)
   422			return -ENODEV;
   423	
   424		if (msg->header.type == GPIO_RPMSG_REPLY) {
   425			port->info.reply_msg = msg;
   426			complete(&port->info.cmd_complete);
   427		} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
   428			port->info.notify_msg = msg;
   429			local_irq_save(flags);
   430			generic_handle_domain_irq(port->domain, msg->pin_idx);
   431			local_irq_restore(flags);
   432		} else
   433			dev_err(&rpdev->dev, "wrong command type!\n");
   434	
   435		return 0;
   436	}
   437	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Posted by Linus Walleij 1 month, 1 week ago
Hi Shenwei,

thanks for your patch!

On Mon, Aug 18, 2025 at 10:45 PM Shenwei Wang <shenwei.wang@nxp.com> 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.
>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>

Since this is a first RPMSG GPIO driver, I'd like if Björn and/or
Mathieu have a look at it so I'm sure it is RPMSG-proper!

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a437fe652dbc..2ce4e9b5225e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -402,6 +402,17 @@ config GPIO_ICH
>
>           If unsure, say N.
>
> +config GPIO_IMX_RPMSG
> +       tristate "NXP i.MX SoC RPMSG GPIO support"
> +       depends on IMX_REMOTEPROC && RPMSG && GPIOLIB
> +       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.

This is sorted under memory-mapped GPIO, but it isn't.

Create a new submenu:

menu "RPMSG GPIO drivers"
        depends on RPMSG

And put it here as the first such driver.

No need to have a dependency on RPMSG in the GPIO_IMX_RPMSG
Kconfig entry after this.

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>

bitops.h or just bits.h? Check which one you actually use.

> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/imx_rpmsg.h>
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>

Are you really using pm_qos?

> +#include <linux/rpmsg.h>
> +#include <linux/virtio.h>
> +#include <linux/workqueue.h>

(...)

> +struct imx_rpmsg_gpio_port {
> +       struct gpio_chip gc;
> +       struct irq_chip chip;

This irqchip doesn't look very immutable.

Look at other patches rewriting irqchips to be immutable
and break this out to a static const struct irq_chip with
IRQCHIP_IMMUTABLE set instead.

> +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;
> +
> +       mutex_lock(&port->info.lock);

Please use guards for all the mutexes:

#include <linux/cleanup.h>

guard(mutex)(&port->info.lock);

and it will be released as you exit the function.

> +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;
> +       int ret;
> +
> +       mutex_lock(&port->info.lock);

Dito for all these instances.
(Saves you a bunch of lines!)

> +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);
> +}

Here you need to keep the classic mutex_lock() though,
because of the irqchip locking abstraction helper.

> +static struct irq_chip imx_rpmsg_irq_chip = {

const

> +       .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,

probably also:

         GPIOCHIP_IRQ_RESOURCE_HELPERS,

?

I think you want to properly mark GPIO lines as used for
IRQs!

> +static int imx_rpmsg_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +       int irq;
> +
> +       irq = irq_find_mapping(port->domain, gpio);
> +       if (irq > 0) {
> +               irq_set_chip_data(irq, port);
> +               irq_set_chip_and_handler(irq, &port->chip, handle_level_irq);
> +       }
> +
> +       return irq;
> +}

Ugh we try to to use custom to_irq() if we can...

Do you have to?

Can't you use
select GPIOLIB_IRQCHIP
and be inspired by other chips using the irqchip
helper library?

We almost always use that these days.

> +       /* create an irq domain */
> +       port->chip = imx_rpmsg_irq_chip;
> +       port->chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> +                                        pltdata->rproc_name, port->idx);
> +       port->dev = &pdev->dev;
> +
> +       irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, IMX_RPMSG_GPIO_PER_PORT,
> +                                  numa_node_id());
> +       if (irq_base < 0) {
> +               dev_err(&pdev->dev, "Failed to alloc irq_descs\n");
> +               return irq_base;
> +       }
> +
> +       port->domain = irq_domain_create_legacy(of_node_to_fwnode(np),
> +                                               IMX_RPMSG_GPIO_PER_PORT,
> +                                               irq_base, 0,
> +                                               &irq_domain_simple_ops, port);
> +       if (!port->domain) {
> +               dev_err(&pdev->dev, "Failed to allocate IRQ domain\n");
> +               return -EINVAL;
> +       }

This also looks unnecessarily custom.

Try to use GPIOLIB_IRQCHIP.


> +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,
> +};
> +
> +static int __init gpio_imx_rpmsg_init(void)
> +{
> +       return platform_driver_register(&imx_rpmsg_gpio_driver);
> +}
> +
> +device_initcall(gpio_imx_rpmsg_init);

No please just do:

module_platform_driver(imx_rpmsg_gpio_driver);

Fix up  these things to begin with and then we can
look at details!

Yours,
Linus Walleij
Re: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver
Posted by Arnaud POULIQUEN 1 month ago
Hello,

On 8/21/25 11:01, Linus Walleij wrote:
> Hi Shenwei,
>
> thanks for your patch!
>
> On Mon, Aug 18, 2025 at 10:45 PM Shenwei Wang <shenwei.wang@nxp.com> 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.
>>
>> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> Since this is a first RPMSG GPIO driver, I'd like if Björn and/or
> Mathieu have a look at it so I'm sure it is RPMSG-proper!

Could this driver be generic (platform independent) ?
Perhaps i missed something, but it seems to me that there is no IMX
specific code.
Making it generic would allow other platforms to reuse it instead of
duplicating it.

Thanks,
Arnaud

>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index a437fe652dbc..2ce4e9b5225e 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -402,6 +402,17 @@ config GPIO_ICH
>>
>>            If unsure, say N.
>>
>> +config GPIO_IMX_RPMSG
>> +       tristate "NXP i.MX SoC RPMSG GPIO support"
>> +       depends on IMX_REMOTEPROC && RPMSG && GPIOLIB
>> +       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.
> This is sorted under memory-mapped GPIO, but it isn't.
>
> Create a new submenu:
>
> menu "RPMSG GPIO drivers"
>          depends on RPMSG
>
> And put it here as the first such driver.
>
> No need to have a dependency on RPMSG in the GPIO_IMX_RPMSG
> Kconfig entry after this.
>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/bitops.h>
> bitops.h or just bits.h? Check which one you actually use.
>
>> +#include <linux/err.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/imx_rpmsg.h>
>> +#include <linux/init.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_qos.h>
> Are you really using pm_qos?
>
>> +#include <linux/rpmsg.h>
>> +#include <linux/virtio.h>
>> +#include <linux/workqueue.h>
> (...)
>
>> +struct imx_rpmsg_gpio_port {
>> +       struct gpio_chip gc;
>> +       struct irq_chip chip;
> This irqchip doesn't look very immutable.
>
> Look at other patches rewriting irqchips to be immutable
> and break this out to a static const struct irq_chip with
> IRQCHIP_IMMUTABLE set instead.
>
>> +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;
>> +
>> +       mutex_lock(&port->info.lock);
> Please use guards for all the mutexes:
>
> #include <linux/cleanup.h>
>
> guard(mutex)(&port->info.lock);
>
> and it will be released as you exit the function.
>
>> +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;
>> +       int ret;
>> +
>> +       mutex_lock(&port->info.lock);
> Dito for all these instances.
> (Saves you a bunch of lines!)
>
>> +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);
>> +}
> Here you need to keep the classic mutex_lock() though,
> because of the irqchip locking abstraction helper.
>
>> +static struct irq_chip imx_rpmsg_irq_chip = {
> const
>
>> +       .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,
>
> probably also:
>
>           GPIOCHIP_IRQ_RESOURCE_HELPERS,
>
> ?
>
> I think you want to properly mark GPIO lines as used for
> IRQs!
>
>> +static int imx_rpmsg_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> +       struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
>> +       int irq;
>> +
>> +       irq = irq_find_mapping(port->domain, gpio);
>> +       if (irq > 0) {
>> +               irq_set_chip_data(irq, port);
>> +               irq_set_chip_and_handler(irq, &port->chip, handle_level_irq);
>> +       }
>> +
>> +       return irq;
>> +}
> Ugh we try to to use custom to_irq() if we can...
>
> Do you have to?
>
> Can't you use
> select GPIOLIB_IRQCHIP
> and be inspired by other chips using the irqchip
> helper library?
>
> We almost always use that these days.
>
>> +       /* create an irq domain */
>> +       port->chip = imx_rpmsg_irq_chip;
>> +       port->chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
>> +                                        pltdata->rproc_name, port->idx);
>> +       port->dev = &pdev->dev;
>> +
>> +       irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, IMX_RPMSG_GPIO_PER_PORT,
>> +                                  numa_node_id());
>> +       if (irq_base < 0) {
>> +               dev_err(&pdev->dev, "Failed to alloc irq_descs\n");
>> +               return irq_base;
>> +       }
>> +
>> +       port->domain = irq_domain_create_legacy(of_node_to_fwnode(np),
>> +                                               IMX_RPMSG_GPIO_PER_PORT,
>> +                                               irq_base, 0,
>> +                                               &irq_domain_simple_ops, port);
>> +       if (!port->domain) {
>> +               dev_err(&pdev->dev, "Failed to allocate IRQ domain\n");
>> +               return -EINVAL;
>> +       }
> This also looks unnecessarily custom.
>
> Try to use GPIOLIB_IRQCHIP.
>
>
>> +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,
>> +};
>> +
>> +static int __init gpio_imx_rpmsg_init(void)
>> +{
>> +       return platform_driver_register(&imx_rpmsg_gpio_driver);
>> +}
>> +
>> +device_initcall(gpio_imx_rpmsg_init);
> No please just do:
>
> module_platform_driver(imx_rpmsg_gpio_driver);
>
> Fix up  these things to begin with and then we can
> look at details!
>
> Yours,
> Linus Walleij
>