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

[Shenwei Wang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Monday, April 27, 2026 3:28 PM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: Padhi, Beleswar <b-padhi@xxxxxx>; Linus Walleij <linusw@xxxxxxxxxx>;
> Bartosz Golaszewski <brgl@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>;
> Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>;
> Conor Dooley <conor+dt@xxxxxxxxxx>; Bjorn Andersson
> <andersson@xxxxxxxxxx>; Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; Frank Li
> <frank.li@xxxxxxx>; Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; Shuah Khan
> <skhan@xxxxxxxxxxxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; Peng Fan
> <peng.fan@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
> remoteproc@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; Bartosz
> Golaszewski <brgl@xxxxxxxx>
> 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