[PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
Arnaud POULIQUEN
arnaud.pouliquen at foss.st.com
Fri Feb 20 01:12:58 PST 2026
On 2/19/26 21:04, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Arnaud POULIQUEN <arnaud.pouliquen at foss.st.com>
>> Sent: Thursday, February 19, 2026 3:21 AM
>> To: Shenwei Wang <shenwei.wang at nxp.com>; Linus Walleij
>> <linusw at kernel.org>; Bartosz Golaszewski <brgl at kernel.org>; Jonathan Corbet
>> <corbet at lwn.net>; Rob Herring <robh at kernel.org>; Krzysztof Kozlowski
>> <krzk+dt at kernel.org>; Conor Dooley <conor+dt at kernel.org>; Bjorn Andersson
>> <andersson at kernel.org>; Mathieu Poirier <mathieu.poirier at linaro.org>; Frank Li
>> <frank.li at nxp.com>; Sascha Hauer <s.hauer at pengutronix.de>
>> Cc: Shuah Khan <skhan at linuxfoundation.org>; linux-gpio at vger.kernel.org; linux-
>> doc at vger.kernel.org; linux-kernel at vger.kernel.org; Pengutronix Kernel Team
>> <kernel at pengutronix.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan
>> <peng.fan at nxp.com>; devicetree at vger.kernel.org; linux-
>> remoteproc at vger.kernel.org; imx at lists.linux.dev; linux-arm-
>> kernel at lists.infradead.org; dl-linux-imx <linux-imx at nxp.com>; Bartosz
>> Golaszewski <brgl at bgdev.pl>; Andrew Lunn <andrew at lunn.ch>
>> Subject: [EXT] Re: [PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
>>
>> Hello Shenwei,
>>
>> This versiob seems much more generic, thanks!
>>
>> On 2/12/26 22:36, Shenwei Wang wrote:
>>> On an AMP platform, 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 at bgdev.pl>
>>> Cc: Andrew Lunn <andrew at lunn.ch>
>>> Signed-off-by: Shenwei Wang <shenwei.wang at nxp.com>
>>> ---
>>> drivers/gpio/Kconfig | 17 ++
>>> drivers/gpio/Makefile | 1 +
>>> drivers/gpio/gpio-rpmsg.c | 588
>> ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 606 insertions(+)
>>> create mode 100644 drivers/gpio/gpio-rpmsg.c
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
>>> b45fb799e36c..3179a54f0634 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -1892,6 +1892,23 @@ config GPIO_SODAVILLE
>>>
>>> endmenu
>>>
>>> +menu "RPMSG GPIO drivers"
>>> + depends on RPMSG
>>> +
>>> +config GPIO_RPMSG
>>> + tristate "Generic RPMSG GPIO support"
>>> + depends on REMOTEPROC
>>> + select GPIOLIB_IRQCHIP
>>> + default REMOTEPROC
>>> + help
>>> + Say yes here to support the generic GPIO functions over the RPMSG
>>> + bus. 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
>>> c05f7d795c43..501aba56ad68 100644
>>> --- a/drivers/gpio/Makefile
>>> +++ b/drivers/gpio/Makefile
>>> @@ -158,6 +158,7 @@ obj-$(CONFIG_GPIO_RDC321X) += gpio-
>> rdc321x.o
>>> obj-$(CONFIG_GPIO_REALTEK_OTTO) += gpio-realtek-otto.o
>>> obj-$(CONFIG_GPIO_REG) += gpio-reg.o
>>> obj-$(CONFIG_GPIO_ROCKCHIP) += gpio-rockchip.o
>>> +obj-$(CONFIG_GPIO_RPMSG) += gpio-rpmsg.o
>>> obj-$(CONFIG_GPIO_RTD) += gpio-rtd.o
>>> obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o
>>> obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o
>>> diff --git a/drivers/gpio/gpio-rpmsg.c b/drivers/gpio/gpio-rpmsg.c new
>>> file mode 100644 index 000000000000..163f51fd45b5
>>> --- /dev/null
>>> +++ b/drivers/gpio/gpio-rpmsg.c
>>> @@ -0,0 +1,588 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright 2026 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/of_device.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/remoteproc.h>
>>> +#include <linux/rpmsg.h>
>>> +
>>> +#define RPMSG_GPIO_ID 5
>>> +#define RPMSG_VENDOR 1
>>> +#define RPMSG_VERSION 0
>>> +
>>> +#define GPIOS_PER_PORT_DEFAULT 32
>>> +#define RPMSG_TIMEOUT 1000
>>> +
>>> +/* GPIO RPMSG header type */
>>> +#define GPIO_RPMSG_SETUP 0
>>> +#define GPIO_RPMSG_REPLY 1
>>> +#define GPIO_RPMSG_NOTIFY 2
>>> +
>>> +/* GPIO Interrupt trigger type */
>>> +#define GPIO_RPMSG_TRI_IGNORE 0
>>> +#define GPIO_RPMSG_TRI_RISING 1
>>> +#define GPIO_RPMSG_TRI_FALLING 2
>>> +#define GPIO_RPMSG_TRI_BOTH_EDGE 3
>>> +#define GPIO_RPMSG_TRI_LOW_LEVEL 4
>>> +#define GPIO_RPMSG_TRI_HIGH_LEVEL 5
>>> +
>>> +/* GPIO RPMSG commands */
>>> +#define GPIO_RPMSG_INPUT_INIT 0
>>> +#define GPIO_RPMSG_OUTPUT_INIT 1
>>> +#define GPIO_RPMSG_INPUT_GET 2
>>> +#define GPIO_RPMSG_DIRECTION_GET 3
>>> +
>>> +#define MAX_PORT_PER_CHANNEL 10
>>> +
>>> +/*
>>> + * @rproc_name: the name of the remote proc.
>>> + * @channel_devices: an array of the devices related to the rpdev.
>>> + */
>>> +struct rpdev_drvdata {
>>> + const char *rproc_name;
>>> + void *channel_devices[MAX_PORT_PER_CHANNEL];
>>> +};
>>> +
>>> +struct gpio_rpmsg_head {
>>
>> Sometime the prefix is gpio_rpmsg, sometime rpmsg_gpio or just gpio, could you
>> use "rpmsg_gpio" prefix in the whole driver?
>>
>
> All the types use prefix gpio_rpmsg. All the functions use rpmsg_gpio.
>
>>> + u8 id; /* Message ID Code */
>>> + u8 vendor; /* Vendor ID number */
>>
>> Does this fields above are mandatory, seems that it is just some constant values
>> that are useless.
>>
>>> + u8 version; /* Vendor-specific version number */
>>
>> Why it is vendor specific? the version should represent the rpmsg-tty protocol
>> version.
>>
>>> + u8 type; /* Message type */
>>> + u8 cmd; /* Command code */
>>> + u8 reserved[5];
>>
>> What is the purpose of this reserved field?
>>
>>> +} __packed;
>>> +
>>> +struct gpio_rpmsg_packet {
>>> + struct gpio_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);
>>
>> Any reason to use __packed and alignement here?
>> This structure will be copied in a vring buffer right?
>>
>
> Using __packed together with an explicit alignment is a common pattern for defining
> communication packets. The goal is to ensure a stable and predictable layout across
> different architectures and compilers. Even though this structure is copied into a vring
> buffer, enforcing the layout avoids potential ABI or padding differences that could lead
> to compatibility issues when the data is parsed on the other side.
>
Please could you give a concrete example for this rpmsg, I can not see a
situation where adding padding at the end of the RPMsg is usefull?
>>> +
>>> +struct gpio_rpmsg_pin {
>>> + u8 irq_shutdown;
>>> + u8 irq_unmask;
>>> + u8 irq_mask;
>>> + u32 irq_wake_enable;
>>> + u32 irq_type;
>>> + struct gpio_rpmsg_packet msg;
>>> +};
>>> +
>>> +struct gpio_rpmsg_info {
>>> + struct rpmsg_device *rpdev;
>>> + struct gpio_rpmsg_packet *reply_msg;
>>> + struct completion cmd_complete;
>>> + struct mutex lock;
>>> + void **port_store;
>>> +};
>>> +
>>> +struct rpmsg_gpio_port {
>>> + struct gpio_chip gc;
>>> + struct gpio_rpmsg_pin gpio_pins[GPIOS_PER_PORT_DEFAULT];
>>> + struct gpio_rpmsg_info info;
>>> + u32 ngpios;
>>> + u32 idx;
>>> +};
>>> +
>>> +static int gpio_send_message(struct rpmsg_gpio_port *port,
>>
>> s/gpio_send_message/rpmsg_gpio_send_message
>>
>>> + struct gpio_rpmsg_packet *msg,
>>> + bool sync)
>>> +{
>>> + struct gpio_rpmsg_info *info = &port->info;
>>> + int err;
>>> +
>>> + reinit_completion(&info->cmd_complete);
>>> + err = rpmsg_send(info->rpdev->ept, msg, sizeof(struct gpio_rpmsg_packet));
>>> + 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 == 0) {
>>> + dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
>>> + return -ETIMEDOUT;
>>
>> strange condition you return an error if err == 0, for redability use 'ret' variable or
>> simply:
>>
>
> Agree. Changing to "ret" is clearer here.
>
>> if(!wait_for_completion_timeout(&info->cmd_complete,
>> msecs_to_jiffies(RPMSG_TIMEOUT)) {
>> 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_packet *gpio_setup_msg_common(struct
>> rpmsg_gpio_port *port,
>>> + unsigned int offset,
>>> + u8 cmd) {
>>> + struct gpio_rpmsg_packet *msg = &port->gpio_pins[offset].msg;
>>> +
>>> + memset(msg, 0, sizeof(struct gpio_rpmsg_packet));
>>> + msg->header.id = RPMSG_GPIO_ID;
>>> + msg->header.vendor = RPMSG_VENDOR;
>>> + msg->header.version = RPMSG_VERSION;
>>> + msg->header.type = GPIO_RPMSG_SETUP;
>>> + msg->header.cmd = cmd;
>>> + msg->pin_idx = offset;
>>> + msg->port_idx = port->idx;
>>> +
>>> + return msg;
>>> +}
>>> +
>>> +static int rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio) {
>>> + struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> + struct gpio_rpmsg_packet *msg;
>>> + int ret;
>>> +
>>> + guard(mutex)(&port->info.lock);
>>> +
>>> + msg = gpio_setup_msg_common(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 rpmsg_gpio_get_direction(struct gpio_chip *gc, unsigned
>>> +int gpio) {
>>> + struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> + struct gpio_rpmsg_packet *msg;
>>> + int ret;
>>> +
>>> + guard(mutex)(&port->info.lock);
>>> +
>>> + msg = gpio_setup_msg_common(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 rpmsg_gpio_direction_input(struct gpio_chip *gc, unsigned
>>> +int gpio) {
>>> + struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> + struct gpio_rpmsg_packet *msg;
>>> +
>>> + guard(mutex)(&port->info.lock);
>>> +
>>> + msg = gpio_setup_msg_common(port, gpio, GPIO_RPMSG_INPUT_INIT);
>>> +
>>> + return gpio_send_message(port, msg, true); }
>>> +
>>> +static int rpmsg_gpio_set(struct gpio_chip *gc, unsigned int gpio,
>>> +int val) {
>>> + struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
>>> + struct gpio_rpmsg_packet *msg;
>>> +
>>> + guard(mutex)(&port->info.lock);
>>> +
>>> + msg = gpio_setup_msg_common(port, gpio, GPIO_RPMSG_OUTPUT_INIT);
>>> + msg->out.value = val;
>>> +
>>> + return gpio_send_message(port, msg, true); }
>>> +
>>> +static int rpmsg_gpio_direction_output(struct gpio_chip *gc,
>>> + unsigned int gpio,
>>> + int val) {
>>> + return rpmsg_gpio_set(gc, gpio, val); }
>>> +
>>> +static int gpio_rpmsg_irq_set_type(struct irq_data *d, u32 type) {
>>> + struct 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;
>>> + irq_set_handler_locked(d, handle_simple_irq);
>>> + break;
>>> + case IRQ_TYPE_EDGE_FALLING:
>>> + edge = GPIO_RPMSG_TRI_FALLING;
>>> + irq_set_handler_locked(d, handle_simple_irq);
>>> + break;
>>> + case IRQ_TYPE_EDGE_BOTH:
>>> + edge = GPIO_RPMSG_TRI_BOTH_EDGE;
>>> + irq_set_handler_locked(d, handle_simple_irq);
>>> + break;
>>> + case IRQ_TYPE_LEVEL_LOW:
>>> + edge = GPIO_RPMSG_TRI_LOW_LEVEL;
>>> + irq_set_handler_locked(d, handle_level_irq);
>>> + break;
>>> + case IRQ_TYPE_LEVEL_HIGH:
>>> + edge = GPIO_RPMSG_TRI_HIGH_LEVEL;
>>> + irq_set_handler_locked(d, handle_level_irq);
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + irq_set_handler_locked(d, handle_bad_irq);
>>> + break;
>>> + }
>>> +
>>> + port->gpio_pins[gpio_idx].irq_type = edge;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int gpio_rpmsg_irq_set_wake(struct irq_data *d, u32 enable) {
>>> + struct 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 unmask/mask function is invoked in two situations:
>>> + * - when an interrupt is being set up, and
>>> + * - after an interrupt has occurred.
>>> + *
>>> + * The GPIO driver does not access hardware registers directly.
>>> + * Instead, it caches all relevant information locally, and then
>>> +sends
>>> + * the accumulated state to the remote system at this stage.
>>> + */
>>> +static void gpio_rpmsg_unmask_irq(struct irq_data *d) {
>>> + struct 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 gpio_rpmsg_mask_irq(struct irq_data *d) {
>>> + struct rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>>> + u32 gpio_idx = d->hwirq;
>>> +
>>> + /*
>>> + * When an interrupt occurs, the remote system masks the interrupt
>>> + * and then sends a notification to Linux. After Linux processes
>>> + * that notification, it sends an RPMsg command back to the remote
>>> + * system to unmask the interrupt again.
>>> + */
>>> + port->gpio_pins[gpio_idx].irq_mask = 1; }
>>> +
>>> +static void gpio_rpmsg_irq_shutdown(struct irq_data *d) {
>>> + struct 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 gpio_rpmsg_irq_bus_lock(struct irq_data *d) {
>>> + struct rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>>> +
>>> + mutex_lock(&port->info.lock);
>>> +}
>>> +
>>> +static void gpio_rpmsg_irq_bus_sync_unlock(struct irq_data *d) {
>>> + struct rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
>>> + struct gpio_rpmsg_packet *msg = NULL;
>>> + u32 gpio_idx = d->hwirq;
>>> +
>>> + /*
>>> + * For mask irq, do nothing here.
>>> + * The remote system will mask interrupt after an interrupt occurs,
>>> + * and then send a notify to Linux system.
>>> + * After Linux system dealt with the notify, it will send an rpmsg to
>>> + * the remote system 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_common(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 gpio_rpmsg_irq_chip = {
>>> + .irq_mask = gpio_rpmsg_mask_irq,
>>> + .irq_unmask = gpio_rpmsg_unmask_irq,
>>> + .irq_set_wake = gpio_rpmsg_irq_set_wake,
>>> + .irq_set_type = gpio_rpmsg_irq_set_type,
>>> + .irq_shutdown = gpio_rpmsg_irq_shutdown,
>>> + .irq_bus_lock = gpio_rpmsg_irq_bus_lock,
>>> + .irq_bus_sync_unlock = gpio_rpmsg_irq_bus_sync_unlock,
>>> + .flags = IRQCHIP_IMMUTABLE,
>>> +};
>>> +
>>> +static void rpmsg_gpio_remove_action(void *data) {
>>> + struct rpmsg_gpio_port *port = data;
>>> +
>>> + port->info.port_store[port->idx] = NULL; }
>>> +
>>> +static int rpmsg_gpiochip_register(struct rpmsg_device *rpdev, struct
>>> +device_node *np) {
>>> + struct rpdev_drvdata *drvdata = dev_get_drvdata(&rpdev->dev);
>>> + struct rpmsg_gpio_port *port;
>>> + struct gpio_irq_chip *girq;
>>> + struct gpio_chip *gc;
>>> + int ret;
>>> +
>>> + port = devm_kzalloc(&rpdev->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_PORT_PER_CHANNEL)
>>> + return -EINVAL;
>>> +
>>> + ret = devm_mutex_init(&rpdev->dev, &port->info.lock);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = of_property_read_u32(np, "ngpios", &port->ngpios);
>>> + if (ret)
>>> + port->ngpios = GPIOS_PER_PORT_DEFAULT;
>>> +
>>> + init_completion(&port->info.cmd_complete);
>>> + port->info.reply_msg = devm_kzalloc(&rpdev->dev,
>>> + sizeof(struct gpio_rpmsg_packet),
>>> + GFP_KERNEL);
>>> + port->info.port_store = drvdata->channel_devices;
>>> + port->info.port_store[port->idx] = port;
>>> + port->info.rpdev = rpdev;
>>> +
>>> + gc = &port->gc;
>>> + gc->owner = THIS_MODULE;
>>> + gc->parent = &rpdev->dev;
>>> + gc->fwnode = of_fwnode_handle(np);
>>> + gc->ngpio = port->ngpios;
>>> + gc->base = -1;
>>> + gc->label = devm_kasprintf(&rpdev->dev, GFP_KERNEL, "%s-gpio%d",
>>> + drvdata->rproc_name, port->idx);
>>> +
>>> + gc->direction_input = rpmsg_gpio_direction_input;
>>> + gc->direction_output = rpmsg_gpio_direction_output;
>>> + gc->get_direction = rpmsg_gpio_get_direction;
>>> + gc->get = rpmsg_gpio_get;
>>> + gc->set = rpmsg_gpio_set;
>>> +
>>> + girq = &gc->irq;
>>> + gpio_irq_chip_set_chip(girq, &gpio_rpmsg_irq_chip);
>>> + girq->parent_handler = NULL;
>>> + girq->num_parents = 0;
>>> + girq->parents = NULL;
>>> + girq->chip->name = devm_kasprintf(&rpdev->dev, GFP_KERNEL, "%s-
>> gpio%d",
>>> + drvdata->rproc_name,
>>> + port->idx);
>>> +
>>> + ret = devm_add_action_or_reset(&rpdev->dev,
>> rpmsg_gpio_remove_action, port);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return devm_gpiochip_add_data(&rpdev->dev, gc, port); }
>>> +
>>> +static const char *rpmsg_get_rproc_node_name(struct rpmsg_device
>>> +*rpdev) {
>>> + const char *name = NULL;
>>> + struct device_node *np;
>>> + struct rproc *rproc;
>>> +
>>> + rproc = rproc_get_by_child(&rpdev->dev);
>>> +
>>> + if (!rproc)
>>> + return NULL;
>>> +
>>> + np = of_node_get(rproc->dev.of_node);
>>> + if (!np && rproc->dev.parent)
>>> + np = of_node_get(rproc->dev.parent->of_node);
>>> +
>>> + if (np) {
>>> + name = devm_kstrdup(&rpdev->dev, np->name, GFP_KERNEL);
>>> + of_node_put(np);
>>> + }
>>
>> What about simply returning rproc->name?
>>
>
> rproc->name doesn’t serve the purpose here. It only reflects the remoteproc driver’s name,
> not the identity of a specific remoteproc instance. What we need is a unique and meaningful
> identifier for this particular instance, and using the DT node name provides exactly that.
It should, rproc->name is used for find the
/dev/remoteproc/remoteproc<X> instance
Regards,
Arnaud
>
> Thanks,
> Shenwei
>
>>> +
>>> + return name;
>>> +}
>>> +
>>> +static struct device_node *
>>> +rpmsg_get_channel_ofnode(struct rpmsg_device *rpdev, char *chan_name)
>>> +{
>>> + struct device_node *np_chan = NULL, *np;
>>> + struct rproc *rproc;
>>> +
>>> + rproc = rproc_get_by_child(&rpdev->dev);
>>> + if (!rproc)
>>> + return NULL;
>>> +
>>> + np = of_node_get(rproc->dev.of_node);
>>> + if (!np && rproc->dev.parent)
>>> + np = of_node_get(rproc->dev.parent->of_node);
>>
>> Is a topology where they is no rproc->dev node but a parent node exist?
>>
>>> +
>>> + if (np) {
>>> + /* Balance the of_node_put() performed by of_find_node_by_name().
>> */
>>> + of_node_get(np);
>>> + np_chan = of_find_node_by_name(np, chan_name);
>>> + of_node_put(np);
>>> + }
>>> +
>>> + return np_chan;
>>> +}
>>> +
>>> +static int
>>> +rpmsg_gpio_channel_callback(struct rpmsg_device *rpdev, void *data,
>>> + int len, void *priv, u32 src) {
>>> + struct gpio_rpmsg_packet *msg = data;
>>> + struct rpmsg_gpio_port *port = NULL;
>>> + struct rpdev_drvdata *drvdata;
>>> +
>>> + drvdata = dev_get_drvdata(&rpdev->dev);
>>> + if (drvdata && msg && msg->port_idx < MAX_PORT_PER_CHANNEL)
>>> + 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);
>>
>> What happen if the remoteprocessor answer after the completion timeout?
>> Could it result in desynchronization between the request and the answer?
>>
>> Having a cmd_counter in gpio_rpmsg_head could help to identify current request
>> and answer
>>
>> the use of reinit_completion could be also needed
>>
>>> + } else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
>>> + generic_handle_domain_irq_safe(port->gc.irq.domain, msg->pin_idx);
>>> + } else
>>> + dev_err(&rpdev->dev, "wrong command type!\n");
>>
>> Could you print the msg->header.type value to help for debug?
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rpmsg_gpio_channel_probe(struct rpmsg_device *rpdev) {
>>> + struct device *dev = &rpdev->dev;
>>> + struct rpdev_drvdata *drvdata;
>>> + struct device_node *np;
>>> + int ret;
>>> +
>>> + if (!dev->of_node) {
>>> + np = rpmsg_get_channel_ofnode(rpdev, rpdev->id.name);
>>> + if (np) {
>>> + dev->of_node = np;
>>> + set_primary_fwnode(dev, of_fwnode_handle(np));
>>> + }
>>> + return -EPROBE_DEFER;
>>> + }
>>> +
>>> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>>> + if (!drvdata)
>>> + return -ENOMEM;
>>> +
>>> + drvdata->rproc_name = rpmsg_get_rproc_node_name(rpdev);
>>> + dev_set_drvdata(dev, drvdata);
>>> +
>>> + for_each_child_of_node_scoped(dev->of_node, child) {
>>> + if (!of_device_is_available(child))
>>> + continue;
>>> +
>>> + if (!of_match_node(dev->driver->of_match_table, child))
>>> + continue;
>>> +
>>> + ret = rpmsg_gpiochip_register(rpdev, child);
>>> + if (ret < 0)
>>> + dev_err(dev, "Failed to register: %pOF\n", child);
>>> + }
>>> +
>>> + return 0;
>>
>> return ret
>> or indicate why the return of rpmsg_gpiochip_register is not taken into account
>>
>>
>>> +}
>>> +
>>> +static void rpmsg_gpio_channel_remove(struct rpmsg_device *rpdev) {
>>> + dev_info(&rpdev->dev, "rpmsg gpio channel driver is removed\n");
>>> +}
>>> +
>>> +static const struct of_device_id rpmsg_gpio_dt_ids[] = {
>>> + { .compatible = "rpmsg-gpio" },
>>> + { /* sentinel */ }
>>> +};
>>> +
>>> +static struct rpmsg_device_id rpmsg_gpio_channel_id_table[] = {
>>> + { .name = "rpmsg-io-channel" },
>>
>> I would remove the "-channel" suffix to have similar naming than "rpmsg-tty" and
>> "rpmsg-raw"
>>
>> Regards,
>> Arnaud
>>
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(rpmsg, rpmsg_gpio_channel_id_table);
>>> +
>>> +static struct rpmsg_driver rpmsg_gpio_channel_client = {
>>> + .drv.name = KBUILD_MODNAME,
>>> + .drv.of_match_table = rpmsg_gpio_dt_ids,
>>> + .id_table = rpmsg_gpio_channel_id_table,
>>> + .probe = rpmsg_gpio_channel_probe,
>>> + .callback = rpmsg_gpio_channel_callback,
>>> + .remove = rpmsg_gpio_channel_remove,
>>> +};
>>> +module_rpmsg_driver(rpmsg_gpio_channel_client);
>>> +
>>> +MODULE_AUTHOR("Shenwei Wang <shenwei.wang at nxp.com>");
>>> +MODULE_DESCRIPTION("generic rpmsg gpio driver");
>>> +MODULE_LICENSE("GPL");
>
More information about the linux-arm-kernel
mailing list