> -----Original Message-----
> From: Arnaud POULIQUEN <[email protected]>
> Sent: Friday, February 20, 2026 3:13 AM
> To: Shenwei Wang <[email protected]>; Linus Walleij
> <[email protected]>; Bartosz Golaszewski <[email protected]>; Jonathan Corbet
> <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>; Bjorn Andersson
> <[email protected]>; Mathieu Poirier <[email protected]>; Frank Li
> <[email protected]>; Sascha Hauer <[email protected]>
> Cc: Shuah Khan <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; Pengutronix Kernel Team
> <[email protected]>; Fabio Estevam <[email protected]>; Peng Fan
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; Bartosz
> Golaszewski <[email protected]>; Andrew Lunn <[email protected]>
> Subject: [EXT] Re: [PATCH v8 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> >>> @@ -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?
>
I asked the AI to provide the following examples.
Example: 32‑bit ARM vs 64‑bit ARM (AArch32 vs AArch64)
Consider this message header:
struct msg_hdr {
u8 id;
u32 size;
u8 flags;
};
What happens without __packed:
On AArch32 (ARMv7), u32 is 4‑byte aligned, but the compiler may insert 1 byte
of padding between id and size,
and then 3 bytes of padding between flags and the end of the struct, making the
layout:
Byte offsets (AArch32):
0: id
1-3: padding
4-7: size
8: flags
9-11: padding <-- trailing padding may be added
struct size = 12 bytes
On AArch64, compilers may choose a different padding strategy (e.g., using
8‑byte alignment rules), resulting in:
Byte offsets (AArch64):
0: id
1-7: padding <-- larger padding may appear
8-11: size
12: flags
13-15: padding
struct size = 16 bytes
Compiler‑specific example (GCC vs Clang)
GCC and Clang historically differed in how they align mixed‑type fields on ARM:
struct example {
u8 a;
u16 b;
u32 c;
};
Without __packed:
GCC on ARM might place:
1 byte padding after a
align b at offset 2
align c at offset 4
Clang on ARM for the same structure may use:
1 byte padding after a
2 bytes padding after b
align c at offset 4 (same), but produce different total size.
This difference alone is enough to corrupt RPMsg payload interpretation if one
side expects offsets computed by GCC and the other compiled with Clang.
Thanks,
Shenwei
>
> >>> +
> >>> +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
> > rproc->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 <[email protected]>");
> >>> +MODULE_DESCRIPTION("generic rpmsg gpio driver");
> >>> +MODULE_LICENSE("GPL");
> >
