Re: [Stratos-dev] [PATCH V3 1/3] gpio: Add virtio-gpio driver

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