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

Shenwei Wang
2026-04-27T20:43:54Z

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, April 27, 2026 3:28 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Padhi, Beleswar <b-padhi@ti.com>; Linus Walleij <linusw@kernel.org>;
> Bartosz Golaszewski <brgl@kernel.org>; Jonathan Corbet <corbet@lwn.net>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; Bjorn Andersson
> <andersson@kernel.org>; Mathieu Poirier <mathieu.poirier@linaro.org>; Frank Li
> <frank.li@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de>; Shuah Khan
> <skhan@linuxfoundation.org>; linux-gpio@vger.kernel.org; linux-
> doc@vger.kernel.org; linux-kernel@vger.kernel.org; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; Peng Fan
> <peng.fan@nxp.com>; devicetree@vger.kernel.org; linux-
> remoteproc@vger.kernel.org; imx@lists.linux.dev; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; Bartosz
> Golaszewski <brgl@bgdev.pl>
> 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