Hi,
Not being very familiar with GPIO, I just have a few general comments and one on the config space layout
On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote:
+static int virtio_gpio_req(struct virtio_gpio *vgpio, u16 type, u16 gpio,
u8 txdata, u8 *rxdata)+{
- struct virtio_gpio_response *res = &vgpio->cres;
- struct virtio_gpio_request *req = &vgpio->creq;
- struct scatterlist *sgs[2], req_sg, res_sg;
- struct device *dev = &vgpio->vdev->dev;
- unsigned long time_left;
- unsigned int len;
- int ret;
- req->type = cpu_to_le16(type);
- req->gpio = cpu_to_le16(gpio);
- req->data = txdata;
- sg_init_one(&req_sg, req, sizeof(*req));
- sg_init_one(&res_sg, res, sizeof(*res));
- sgs[0] = &req_sg;
- sgs[1] = &res_sg;
- mutex_lock(&vgpio->lock);
- ret = virtqueue_add_sgs(vgpio->command_vq, sgs, 1, 1, res, GFP_KERNEL);
- if (ret) {
dev_err(dev, "failed to add request to vq\n");goto out;- }
- reinit_completion(&vgpio->completion);
- virtqueue_kick(vgpio->command_vq);
- time_left = wait_for_completion_timeout(&vgpio->completion, HZ / 10);
- if (!time_left) {
dev_err(dev, "virtio GPIO backend timeout\n");return -ETIMEDOUT;
mutex is still held
- }
- WARN_ON(res != virtqueue_get_buf(vgpio->command_vq, &len));
- if (unlikely(res->status != VIRTIO_GPIO_STATUS_OK)) {
dev_err(dev, "virtio GPIO request failed: %d\n", gpio);return -EINVAL;
and here
- }
- if (rxdata)
*rxdata = res->data;+out:
- mutex_unlock(&vgpio->lock);
- return ret;
+}
+static int virtio_gpio_request(struct gpio_chip *gc, unsigned int gpio) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_ACTIVATE, gpio, 0, NULL);
+}
+static void virtio_gpio_free(struct gpio_chip *gc, unsigned int gpio) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DEACTIVATE, gpio, 0, NULL);
+}
+static int virtio_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- u8 direction;
- int ret;
- ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_DIRECTION, gpio, 0,
&direction);- if (ret)
return ret;- return direction;
+}
+static int virtio_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_IN, gpio, 0,
NULL);+}
+static int virtio_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
int value)+{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- return virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_DIRECTION_OUT, gpio, (u8)
(that dangling cast looks a bit odd to me)
value, NULL);+}
+static int virtio_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- u8 value;
- int ret;
- ret = virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_GET_VALUE, gpio, 0, &value);
- if (ret)
return ret;- return value;
+}
+static void virtio_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value) +{
- struct virtio_gpio *vgpio = gpio_chip_to_vgpio(gc);
- virtio_gpio_req(vgpio, VIRTIO_GPIO_REQ_SET_VALUE, gpio, value, NULL);
+}
+static void virtio_gpio_command(struct virtqueue *vq) +{
- struct virtio_gpio *vgpio = vq->vdev->priv;
- complete(&vgpio->completion);
+}
+static int virtio_gpio_alloc_vqs(struct virtio_device *vdev) +{
- struct virtio_gpio *vgpio = vdev->priv;
- const char * const names[] = { "command" };
- vq_callback_t *cbs[] = {
virtio_gpio_command,- };
- struct virtqueue *vqs[1] = {NULL};
- int ret;
- ret = virtio_find_vqs(vdev, 1, vqs, cbs, names, NULL);
- if (ret) {
dev_err(&vdev->dev, "failed to allocate vqs: %d\n", ret);return ret;- }
- vgpio->command_vq = vqs[0];
- /* Mark the device ready to perform operations from within probe() */
- virtio_device_ready(vgpio->vdev);
May fit better in the parent function
- return 0;
+}
+static void virtio_gpio_free_vqs(struct virtio_device *vdev) +{
- vdev->config->reset(vdev);
- vdev->config->del_vqs(vdev);
+}
+static const char **parse_gpio_names(struct virtio_device *vdev,
struct virtio_gpio_config *config)+{
- struct device *dev = &vdev->dev;
- char *str = config->gpio_names;
- const char **names;
- int i, len;
- if (!config->gpio_names_size)
return NULL;- names = devm_kcalloc(dev, config->ngpio, sizeof(names), GFP_KERNEL);
- if (!names)
return ERR_PTR(-ENOMEM);- /* NULL terminate the string instead of checking it */
- config->gpio_names[config->gpio_names_size - 1] = '\0';
- for (i = 0; i < config->ngpio; i++) {
/** We have already marked the last byte with '\0'* earlier, this shall be enough here.*/if (str == config->gpio_names + config->gpio_names_size) {dev_err(dev, "Invalid gpio_names\n");return ERR_PTR(-EINVAL);}len = strlen(str);if (!len) {dev_err(dev, "Missing GPIO name: %d\n", i);return ERR_PTR(-EINVAL);}names[i] = str;str += len + 1;- }
- return names;
+}
+static int virtio_gpio_probe(struct virtio_device *vdev) +{
- struct virtio_gpio_config *config;
- struct device *dev = &vdev->dev;
- struct virtio_gpio *vgpio;
- u32 size;
- int ret;
- virtio_cread(vdev, struct virtio_gpio_config, gpio_names_size, &size);
- size = cpu_to_le32(size);
le32_to_cpu()?
- vgpio = devm_kzalloc(dev, sizeof(*vgpio) + size, GFP_KERNEL);
- if (!vgpio)
return -ENOMEM;- config = &vgpio->config;
- /* Read configuration */
- virtio_cread_bytes(vdev, 0, config, sizeof(*config) + size);
- /* NULL terminate the string instead of checking it */
- config->name[sizeof(config->name) - 1] = '\0';
- config->ngpio = cpu_to_le16(config->ngpio);
- config->gpio_names_size = cpu_to_le32(config->gpio_names_size);
leXX_to_cpu()?
- if (!config->ngpio) {
dev_err(dev, "Number of GPIOs can't be zero\n");return -EINVAL;- }
- vgpio->vdev = vdev;
- vgpio->gc.request = virtio_gpio_request;
- vgpio->gc.free = virtio_gpio_free;
- vgpio->gc.get_direction = virtio_gpio_get_direction;
- vgpio->gc.direction_input = virtio_gpio_direction_input;
- vgpio->gc.direction_output = virtio_gpio_direction_output;
- vgpio->gc.get = virtio_gpio_get;
- vgpio->gc.set = virtio_gpio_set;
- vgpio->gc.ngpio = config->ngpio;
- vgpio->gc.base = -1; /* Allocate base dynamically */
- vgpio->gc.label = config->name[0] ?
config->name : dev_name(dev);- vgpio->gc.parent = dev;
- vgpio->gc.owner = THIS_MODULE;
- vgpio->gc.can_sleep = true;
- vgpio->gc.names = parse_gpio_names(vdev, config);
- if (IS_ERR(vgpio->gc.names))
return PTR_ERR(vgpio->gc.names);- vdev->priv = vgpio;
- mutex_init(&vgpio->lock);
- init_completion(&vgpio->completion);
- ret = virtio_gpio_alloc_vqs(vdev);
- if (ret)
return ret;- ret = gpiochip_add(&vgpio->gc);
- if (ret) {
virtio_gpio_free_vqs(vdev);dev_err(dev, "Failed to add virtio-gpio controller\n");- }
- return ret;
+}
+static void virtio_gpio_remove(struct virtio_device *vdev) +{
- struct virtio_gpio *vgpio = vdev->priv;
- gpiochip_remove(&vgpio->gc);
- virtio_gpio_free_vqs(vdev);
+}
+static const struct virtio_device_id id_table[] = {
- { VIRTIO_ID_GPIO, VIRTIO_DEV_ANY_ID },
- {},
+}; +MODULE_DEVICE_TABLE(virtio, id_table);
+static struct virtio_driver virtio_gpio_driver = {
- .id_table = id_table,
- .probe = virtio_gpio_probe,
- .remove = virtio_gpio_remove,
- .driver = {
.name = KBUILD_MODNAME,.owner = THIS_MODULE,- },
+}; +module_virtio_driver(virtio_gpio_driver);
+MODULE_AUTHOR("Enrico Weigelt, metux IT consult info@metux.net"); +MODULE_AUTHOR("Viresh Kumar viresh.kumar@linaro.org"); +MODULE_DESCRIPTION("VirtIO GPIO driver"); +MODULE_LICENSE("GPL"); diff --git a/include/uapi/linux/virtio_gpio.h b/include/uapi/linux/virtio_gpio.h new file mode 100644 index 000000000000..e805e33a79cb --- /dev/null +++ b/include/uapi/linux/virtio_gpio.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _LINUX_VIRTIO_GPIO_H +#define _LINUX_VIRTIO_GPIO_H
+#include <linux/types.h>
+/* Virtio GPIO request types */ +#define VIRTIO_GPIO_REQ_ACTIVATE 0x01 +#define VIRTIO_GPIO_REQ_DEACTIVATE 0x02 +#define VIRTIO_GPIO_REQ_GET_DIRECTION 0x03 +#define VIRTIO_GPIO_REQ_DIRECTION_IN 0x04 +#define VIRTIO_GPIO_REQ_DIRECTION_OUT 0x05 +#define VIRTIO_GPIO_REQ_GET_VALUE 0x06 +#define VIRTIO_GPIO_REQ_SET_VALUE 0x07
+/* Possible values of the status field */ +#define VIRTIO_GPIO_STATUS_OK 0x0 +#define VIRTIO_GPIO_STATUS_ERR 0x1
+struct virtio_gpio_config {
- char name[32];
- __u16 ngpio;
- __u16 padding;
- __u32 gpio_names_size;
- char gpio_names[0];
A variable-size array here will make it very difficult to append new fields to virtio_gpio_config, when adding new features to the device. (New fields cannot be inserted in between, since older drivers will expect existing fields at a specific offset.)
You could replace it with a reference to the string array, for example "__u16 gpio_names_offset" declaring the offset between the beginning of device-specific config and the string array. The 'name' field could also be indirect to avoid setting a fixed 32-char size, but that's not as important.
+} __packed;
No need for __packed, because the fields are naturally aligned (as required by the virtio spec)
Thanks, Jean
+/* Virtio GPIO Request / Response */ +struct virtio_gpio_request {
- __u16 type;
- __u16 gpio;
- __u8 data;
+};
+struct virtio_gpio_response {
- __u8 status;
- __u8 data;
+};
+#endif /* _LINUX_VIRTIO_GPIO_H */ diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h index b89391dff6c9..0c24e8ae2499 100644 --- a/include/uapi/linux/virtio_ids.h +++ b/include/uapi/linux/virtio_ids.h @@ -55,5 +55,6 @@ #define VIRTIO_ID_PMEM 27 /* virtio pmem */ #define VIRTIO_ID_MAC80211_HWSIM 29 /* virtio mac80211-hwsim */ #define VIRTIO_ID_I2C_ADAPTER 34 /* virtio i2c adapter */ +#define VIRTIO_ID_GPIO 41 /* virtio GPIO */
#endif /* _LINUX_VIRTIO_IDS_H */
2.31.1.272.g89b43f80a514
-- Stratos-dev mailing list Stratos-dev@op-lists.linaro.org https://op-lists.linaro.org/mailman/listinfo/stratos-dev
On 10-06-21, 19:40, Jean-Philippe Brucker wrote:
On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote:
Fixed everything else you suggested.
+struct virtio_gpio_config {
- char name[32];
- __u16 ngpio;
- __u16 padding;
- __u32 gpio_names_size;
- char gpio_names[0];
A variable-size array here will make it very difficult to append new fields to virtio_gpio_config, when adding new features to the device. (New fields cannot be inserted in between, since older drivers will expect existing fields at a specific offset.)
Yes, I thought about that earlier and though maybe we will be able to play with that using the virtio-features, I mean a different layout of config structure if we really need to add a field in config, based on the feature flag.
You could replace it with a reference to the string array, for example "__u16 gpio_names_offset" declaring the offset between the beginning of device-specific config and the string array.
But, I like this idea more and it does make it very flexible. Will adapt to it.
The 'name' field could also be indirect to avoid setting a fixed 32-char size, but that's not as important.
Yeah, 32 bytes is really enough. One won't be able to make any sense out of a bigger name anyway :)
+} __packed;
No need for __packed, because the fields are naturally aligned (as required by the virtio spec)
Yeah, I know, but I tend to add that for structures which aren't very simple (like the request/response ones), just to avoid human errors and hours of debugging someone need to go through. __packed won't harm at least :)
stratos-dev@op-lists.linaro.org
-
Jean-Philippe Brucker -
Viresh Kumar