> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Tuesday, February 10, 2026 11:48 AM
> To: Shenwei Wang <[email protected]>
> Cc: Linus Walleij <[email protected]>; Bartosz Golaszewski <[email protected]>;
> Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>;
> Conor Dooley <[email protected]>; Bjorn Andersson
> <[email protected]>; Mathieu Poirier <[email protected]>; Shawn
> Guo <[email protected]>; Sascha Hauer <[email protected]>;
> Jonathan Corbet <[email protected]>; Pengutronix Kernel Team
> <[email protected]>; Fabio Estevam <[email protected]>; Peng Fan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; dl-linux-imx <[email protected]>;
> [email protected]; Bartosz Golaszewski <[email protected]>
> 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
