Re: [PATCH v5 4/5] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 11/5/2025 4:33 AM, 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.
> 
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>   drivers/gpio/Kconfig          |  17 ++
>   drivers/gpio/Makefile         |   1 +
>   drivers/gpio/gpio-imx-rpmsg.c | 475 ++++++++++++++++++++++++++++++++++
>   3 files changed, 493 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..888c3fdeaa04
> --- /dev/null
> +++ b/drivers/gpio/gpio-imx-rpmsg.c
> @@ -0,0 +1,475 @@
> +// 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/completion.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/rpmsg.h>
> +#include <linux/rpmsg/imx_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,
> +	GPIO_RPMSG_DIRECTION_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;
> +};
> +

Hello Shenwei,
I'd like to go over a few aspects of this patch.


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

1.NULL pointer dereference here.

> +			"rpmsg channel doesn't exist, is remote core 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, "remote core replies an error: %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_setup_msg_header(struct imx_rpmsg_gpio_port *port,
> +						     unsigned int offset,
> +						     u8 cmd)
> +{
> +	struct gpio_rpmsg_data *msg = &port->gpio_pins[offset].msg;
> +
> +	memset(msg, 0, sizeof(struct gpio_rpmsg_data));
> +	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 = cmd;
> +	msg->pin_idx = offset;
> +	msg->port_idx = port->idx;
> +
> +	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_setup_msg_header(port, gpio, GPIO_RPMSG_INPUT_GET);
> +
> +	ret = gpio_send_message(port, msg, true);
> +	if (!ret)
> +		ret = !!port->gpio_pins[gpio].msg.in.value;
> +
> +	return ret;
> +}
> +
> +static int imx_rpmsg_gpio_get_direction(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_setup_msg_header(port, gpio, GPIO_RPMSG_DIRECTION_GET);
> +
> +	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_setup_msg_header(port, gpio, GPIO_RPMSG_INPUT_INIT);
> +
> +	return gpio_send_message(port, msg, true);
> +}
> +
> +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_setup_msg_header(port, gpio, GPIO_RPMSG_OUTPUT_INIT);
> +	msg->out.value = val;
> +
> +	return gpio_send_message(port, msg, true);
> +}
> +
> +static int imx_rpmsg_gpio_direction_output(struct gpio_chip *gc,
> +					unsigned int gpio, int val)
> +{
> +
> +	return imx_rpmsg_gpio_set(gc, gpio, val);
> +}
> +
> +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;
> +	}
> +

2.Unlocking port->info.lock when port is NULL will crash due to a NULL
pointer dereference. Please fix the logic as well.> +	/*
> +	 * 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_setup_msg_header(port, gpio_idx, GPIO_RPMSG_INPUT_INIT);
> +
> +	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;
> +
> +	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;
> +		generic_handle_domain_irq_safe(port->gc.irq.domain, msg->pin_idx);
> +	} 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] = NULL;
> +}
> +
> +static int imx_rpmsg_gpio_probe(struct platform_device *pdev)
> +{
> +	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
> +	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 = device_property_read_u32(&pdev->dev, "reg", &port->idx);
> +	if (ret)
> +		return ret;
> +
> +	if (port->idx > MAX_DEV_PER_CHANNEL)
> +		return -EINVAL;
> +
> +	ret = devm_mutex_init(&pdev->dev, &port->info.lock);
> +	if (ret)
> +		return ret;
> +
> +	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 = &pdev->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_direction = imx_rpmsg_gpio_get_direction;
> +	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);
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, imx_rpmsg_gpio_remove_action, port);
> +	if (ret)
> +		return ret;
> +
> +	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("Shenwei Wang <shenwei.wang@nxp.com>");
> +MODULE_DESCRIPTION("NXP i.MX SoC rpmsg gpio driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
> 
> 


-- 
Thx and BRs,
Zhongqiu Han