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

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Tuesday, February 10, 2026 11:48 AM
To: Shenwei Wang <shenwei.wang@nxp.com>
Cc: Linus Walleij <linusw@kernel.org>; Bartosz Golaszewski <brgl@kernel.org>;
Rob Herring [off-list ref]; Krzysztof Kozlowski [off-list ref];
Conor Dooley [off-list ref]; Bjorn Andersson
[off-list ref]; Mathieu Poirier [off-list ref]; Shawn
Guo [off-list ref]; Sascha Hauer [off-list ref];
Jonathan Corbet [off-list ref]; Pengutronix Kernel Team
[off-list ref]; Fabio Estevam [off-list ref]; Peng Fan
[off-list ref]; linux-gpio@vger.kernel.org; devicetree@vger.kernel.org;
linux-kernel@vger.kernel.org; linux-remoteproc@vger.kernel.org;
imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org; linux-
doc@vger.kernel.org; dl-linux-imx [off-list ref];
arnaud.pouliquen@foss.st.com; Bartosz Golaszewski [off-list ref]
Subject: [EXT] Re: [PATCH v7 3/4] gpio: rpmsg: add generic rpmsg GPIO driver

quoted

+#define GPIOS_PER_PORT               32

Maybe this should be from DT, using "ngpios". The Documentation says:

  Optionally, a GPIO controller may have a "ngpios" property. This
  property indicates the number of in-use slots of available slots for
  GPIOs. The typical example is something like this: the hardware
  register is 32 bits wide, but only 18 of the bits have a physical
  counterpart. The driver is generally written so that all 32 bits can
  be used, but the IP block is reused in a lot of designs, some using
  all 32 bits, some using 18 and some using 12. In this case, setting
  "ngpios = <18>;" informs the driver that only the first 18 GPIOs, at
  local offset 0 .. 17, are in use.

Just because your hardware has 32 does not mean every vendor does.

32 just represents the maximum number of GPIO lines that the driver can 
support, so there's no need to declare all of them as in use. 
Adding support for the ngpios property is a good suggestion, and I'll include 
this property in the next version.

quoted

+struct gpio_rpmsg_head {
+     u8 id;          /* Message ID Code */
+     u8 vendor;      /* Vendor ID number */
+     u8 version;     /* Vendor-specific version number */
+     u8 type;        /* Message type */
+     u8 cmd;         /* Command code */
+     u8 reserved[5];
+} __packed;

I still think this should be a clean design from scratch, and you modify your
firmware.

I do need to take the existing constraints into account. It's always ideal to start with 
a clean design, but I also have to maintain compatibility with the current products in 
the field. The approach should break what already exists.

However, as the companion firmware is updated over time, the driver can be refined 
accordingly.

This data structure is 10 bytes. Are these all needed for a generic GPIO
controller? version, type, command and one reserved byte seems like enough,
and it is then 4 bytes, so there is no need for __packed.

quoted

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

This then becomes 8 bytes, so there is no need for __packed or __aligned(8).

Even though the struct currently evaluates to 8 bytes, that doesn't make __packed or __aligned(8) 
unnecessary. Because struct layout and default alignment vary between compilers and architectures, 
these attributes still serve specific purposes-__packed ensures there's no internal padding, 
and __aligned(8) enforces the external alignment. Without them, the layout may not be consistent 
across toolchains or build configurations.

I don't want to force this, it is something i think which should be discussed. Do we
adopt your design, which is not so nice, but at least has one working
implementation, or do we do a clean design?

I'd lean toward getting a working solution in place first and then improving the design 
over time. This approach lets us ensure functionality for current users while still giving 
us room to evolve toward a cleaner, more consistent design as the code and firmware mature.

quoted

+static int gpio_send_message(struct rpmsg_gpio_port *port,
+                          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) {
+                     dev_err(&info->rpdev->dev, "rpmsg_send timeout!\n");
+                     return -ETIMEDOUT;
+             }

I _think_ you need to handle negative values of err. It looks like
do_wait_for_common() can return -ERESTARTSYS;

quoted

+static struct gpio_rpmsg_packet *gpio_setup_msg_header(struct

rpmsg_gpio_port *port,

quoted

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

Why is a function called gpio_setup_msg_header() setting things outside of the
header?

How about change to gpio_setup_msg_common()?

Thanks,
Shenwei

quoted

+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_header(port, gpio, GPIO_RPMSG_INPUT_GET);
+
+     ret = gpio_send_message(port, msg, true);

If gpio_setup_msg_header() does what it sounds like it should do, what is setting
up the message body before you send the message?

        Andrew