On Thu, Mar 31, 2022 at 09:33:05PM -0500, frank zago wrote:
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.
>
> Signed-off-by: frank zago <frank@xxxxxxxx>
> ---
> +struct ch341_gpio {
> + struct gpio_chip gpio;
> + struct mutex gpio_lock;
> + u16 gpio_dir; /* 1 bit per pin, 0=IN, 1=OUT. */
> + u16 gpio_last_read; /* last GPIO values read */
> + u16 gpio_last_written; /* last GPIO values written */
> + union {
> + u8 gpio_buf[SEG_SIZE];
> + __le16 gpio_buf_status;
> + };
> +
> + struct urb *irq_urb;
> + struct usb_anchor irq_urb_out;
> + u8 irq_buf[CH341_USB_MAX_INTR_SIZE];
> + struct irq_chip irq_chip;
> +
> + struct ch341_device *ch341;
> +};
> +static void ch341_complete_intr_urb(struct urb *urb)
> +{
> + struct ch341_gpio *dev = urb->context;
> + int rc;
> +
> + if (urb->status) {
> + usb_unanchor_urb(dev->irq_urb);
URBs are unanchored by USB core on completion.
> + } else {
> + /*
> + * Data is 8 bytes. Byte 0 might be the length of
> + * significant data, which is 3 more bytes. Bytes 1
> + * and 2, and possibly 3, are the pin status. The byte
> + * order is different than for the GET_STATUS
> + * command. Byte 1 is GPIOs 8 to 15, and byte 2 is
> + * GPIOs 0 to 7.
> + */
> +
> + handle_nested_irq(irq_find_mapping(dev->gpio.irq.domain,
> + CH341_GPIO_INT_LINE));
> +
> + rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> + if (rc)
> + usb_unanchor_urb(dev->irq_urb);
> + }
> +}
> +static void ch341_gpio_irq_enable(struct irq_data *data)
> +{
> + struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> + int rc;
> +
> + /*
> + * The URB might have just been unlinked in
> + * ch341_gpio_irq_disable, but the completion handler hasn't
> + * been called yet.
> + */
> + if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
> + usb_kill_anchored_urbs(&dev->irq_urb_out);
> +
> + usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
> + rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> + if (rc)
> + usb_unanchor_urb(dev->irq_urb);
This looks confused and broken.
usb_kill_anchored_urbs() can sleep so either calling it is broken or
using GFP_ATOMIC is unnecessary.
And isn't this function called multiple times when enabling more than
one irq?!
> +}
> +
> +static void ch341_gpio_irq_disable(struct irq_data *data)
> +{
> + struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> +
> + usb_unlink_urb(dev->irq_urb);
Same here...
> +}
> +
> +static int ch341_gpio_remove(struct platform_device *pdev)
> +{
> + struct ch341_gpio *dev = platform_get_drvdata(pdev);
> +
> + usb_kill_anchored_urbs(&dev->irq_urb_out);
You only have one URB...
And what prevents it from being resubmitted here?
> + gpiochip_remove(&dev->gpio);
> + usb_free_urb(dev->irq_urb);
> +
> + return 0;
> +}
> +
> +static int ch341_gpio_probe(struct platform_device *pdev)
> +{
> + struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
> + struct gpio_irq_chip *girq;
> + struct ch341_gpio *dev;
> + struct gpio_chip *gpio;
> + int rc;
> +
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> + if (dev == NULL)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, dev);
> + dev->ch341 = ch341;
> + mutex_init(&dev->gpio_lock);
> +
> + gpio = &dev->gpio;
> + gpio->label = dev_name(&pdev->dev);
> + gpio->parent = &pdev->dev;
> + gpio->owner = THIS_MODULE;
> + gpio->get_direction = ch341_gpio_get_direction;
> + gpio->direction_input = ch341_gpio_direction_input;
> + gpio->direction_output = ch341_gpio_direction_output;
> + gpio->get = ch341_gpio_get;
> + gpio->get_multiple = ch341_gpio_get_multiple;
> + gpio->set = ch341_gpio_set;
> + gpio->set_multiple = ch341_gpio_set_multiple;
> + gpio->base = -1;
> + gpio->ngpio = CH341_GPIO_NUM_PINS;
> + gpio->can_sleep = true;
> +
> + dev->irq_chip.name = dev_name(&pdev->dev);
> + dev->irq_chip.irq_set_type = ch341_gpio_irq_set_type;
> + dev->irq_chip.irq_enable = ch341_gpio_irq_enable;
> + dev->irq_chip.irq_disable = ch341_gpio_irq_disable;
> +
> + girq = &gpio->irq;
> + girq->chip = &dev->irq_chip;
> + girq->handler = handle_simple_irq;
> + girq->default_type = IRQ_TYPE_NONE;
> +
> + /* Create an URB for handling interrupt */
> + dev->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
> + if (!dev->irq_urb)
> + return dev_err_probe(&pdev->dev, -ENOMEM, "Cannot allocate the int URB\n");
> +
> + usb_fill_int_urb(dev->irq_urb, ch341->usb_dev,
> + usb_rcvintpipe(ch341->usb_dev, ch341->ep_intr),
> + dev->irq_buf, CH341_USB_MAX_INTR_SIZE,
> + ch341_complete_intr_urb, dev, ch341->ep_intr_interval);
> +
> + init_usb_anchor(&dev->irq_urb_out);
> +
> + rc = gpiochip_add_data(gpio, dev);
> + if (rc) {
> + rc = dev_err_probe(&pdev->dev, rc, "Could not add GPIO\n");
> + goto release_urb;
> + }
> +
> + return 0;
> +
> +release_urb:
> + usb_free_urb(dev->irq_urb);
> +
> + return rc;
> +}
Johan
