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