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

Andrew Lunn
2026-04-27T20:28:25Z

> > > +struct rpmsg_gpio_packet {
> > > + u8 type; /* Message type */
> > > + u8 cmd; /* Command code */
> > > + u8 port_idx;
> > > + u8 line;
> > > + u8 val1;
> > > + u8 val2;
> > > +};
> >
> >
> > Could you please document the fields in these structs (and the below ones too)?
> > From the code, it looks like while sending a message from Linux to Firmware; val1
> > and val2 are used to describe the values to set. Whereas while receiving a
> > response, val1 represents a possible error code, and val2 represents the actual
> > message of get type queries. If that is so, you might want to change the variable
> > names to be more descriptive and also use a union.
> >
>
> The fields in the two structs are fairly self-explanatory. Do we really need the additional comments?
> The previous version of the patch used a union, which was updated to support the fixed_up hooks.
> Now that the fixed_up hooks have been removed, I can revert this back to the union-based implementation.

I thought you had already adopted the virtio message format?

/* Possible values of the status field */
#define VIRTIO_GPIO_STATUS_OK 0x0
#define VIRTIO_GPIO_STATUS_ERR 0x1

struct virtio_gpio_response {
__u8 status;
__u8 value;
};

Seems pretty obvious what status means. value depends on the request,
get_direction actually uses it, and it can be one of

#define VIRTIO_GPIO_DIRECTION_NONE 0x00
#define VIRTIO_GPIO_DIRECTION_OUT 0x01
#define VIRTIO_GPIO_DIRECTION_IN 0x02

and gpio_get uses it as a bool for the state of the GPIO.

Why do we need all the complexity for val1, val2, etc?

Andrew