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

[Andrew Lunn]

> > > +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