[PATCH v7 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
Shenwei Wang
shenwei.wang at nxp.com
Tue Feb 10 12:17:48 PST 2026
> -----Original Message-----
> From: Andrew Lunn <andrew at lunn.ch>
> Sent: Tuesday, February 10, 2026 11:48 AM
> To: Shenwei Wang <shenwei.wang at nxp.com>
> Cc: Linus Walleij <linusw at kernel.org>; Bartosz Golaszewski <brgl at kernel.org>;
> 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>; Shawn
> Guo <shawnguo at kernel.org>; Sascha Hauer <s.hauer at pengutronix.de>;
> Jonathan Corbet <corbet at lwn.net>; Pengutronix Kernel Team
> <kernel at pengutronix.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan
> <peng.fan at nxp.com>; linux-gpio at vger.kernel.org; devicetree at vger.kernel.org;
> linux-kernel at vger.kernel.org; linux-remoteproc at vger.kernel.org;
> imx at lists.linux.dev; linux-arm-kernel at lists.infradead.org; linux-
> doc at vger.kernel.org; dl-linux-imx <linux-imx at nxp.com>;
> arnaud.pouliquen at foss.st.com; Bartosz Golaszewski <brgl at bgdev.pl>
> Subject: [EXT] Re: [PATCH v7 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> > +#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.
> > +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.
>
> > +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.
> > +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;
>
> > +static struct gpio_rpmsg_packet *gpio_setup_msg_header(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;
>
> 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
> > +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
More information about the linux-arm-kernel
mailing list