> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Monday, April 27, 2026 3:28 PM
> To: Shenwei Wang <[email protected]>
> Cc: Padhi, Beleswar <[email protected]>; Linus Walleij <[email protected]>;
> Bartosz Golaszewski <[email protected]>; Jonathan Corbet <[email protected]>;
> Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>;
> Conor Dooley <[email protected]>; Bjorn Andersson
> <[email protected]>; Mathieu Poirier <[email protected]>; Frank Li
> <[email protected]>; Sascha Hauer <[email protected]>; Shuah Khan
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; Pengutronix Kernel Team
> <[email protected]>; Fabio Estevam <[email protected]>; Peng Fan
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; Bartosz
> Golaszewski <[email protected]>
> Subject: [EXT] Re: [PATCH v13 3/4] gpio: rpmsg: add generic rpmsg GPIO driver
> > > > +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?
>
It is the same message format. Please see the message definition
(GET_DIRECTION) below:
+GET_DIRECTION (Cmd=2)
+~~~~~~~~~~~~~~~~~~~~~
+
+**Request:**
+
+.. code-block:: none
+
+ +-----+-----+-----+-----+-----+----+
+ |0x00 |0x01 |0x02 |0x03 |0x04 |0x05|
+ | 0 | 2 |port |line | 0 | 0 |
+ +-----+-----+-----+-----+-----+----+
+
+**Reply:**
+
+.. code-block:: none
+
+ +-----+-----+-----+-----+-----+----+
+ |0x00 |0x01 |0x02 |0x03 |0x04 |0x05|
+ | 1 | 2 |port |line | err | dir|
+ +-----+-----+-----+-----+-----+----+
+
+- **err**: See above for definitions.
+
+- **dir**: Direction.
+
+ - 0: None
+ - 1: Output
+ - 2: Input
+
Shenwei
> Andrew
