Re: [PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO driver

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 2/19/26 21:04, Shenwei Wang wrote:
> 
> 
>> -----Original Message-----
>> From: Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
>> Sent: Thursday, February 19, 2026 3:21 AM
>> To: Shenwei Wang <shenwei.wang@nxp.com>; Linus Walleij
>> <linusw@kernel.org>; Bartosz Golaszewski <brgl@kernel.org>; Jonathan Corbet
>> <corbet@lwn.net>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
>> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Bjorn Andersson
>> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Frank Li
>> <frank.li@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>; linux-gpio@vger.kernel.org; linux-
>> doc@vger.kernel.org; linux-kernel@vger.kernel.org; Pengutronix Kernel Team
>> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
>> <peng.fan@nxp.com>; devicetree@vger.kernel.org; linux-
>> remoteproc@vger.kernel.org; imx@lists.linux.dev; linux-arm-
>> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; Bartosz
>> Golaszewski <brgl@bgdev.pl>; Andrew Lunn <andrew@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@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-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@nxp.com>");
>>> +MODULE_DESCRIPTION("generic rpmsg gpio driver");
>>> +MODULE_LICENSE("GPL");
>