Re: [PATCH] gpio: add support for FTDI's MPSSE as GPIO

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Mary,

thanks for your patch!

On Wed, Aug 14, 2024 at 9:15 PM Mary Strodl <mstrodl@xxxxxxxxxxx> wrote:

> +config GPIO_MPSSE
> +       tristate "FTDI MPSSE GPIO support"
> +       help
> +         GPIO driver for FTDI's MPSSE interface. These can do input and
> +         output. Each MPSSE provides 16 IO pins.

select GPIOLIB_IRQCHIP

you are already halfway using it.

(...)
> +struct mpsse_priv {
> +       struct gpio_chip gpio;
> +       struct usb_device *udev;     /* USB device encompassing all MPSSEs */
> +       struct usb_interface *intf;  /* USB interface for this MPSSE */
> +       u8 intf_id;                  /* USB interface number for this MPSSE */
> +       struct irq_chip irq;

What is this irq_chip? You already have an immutable one lower in the code.

> +       struct work_struct irq_work; /* polling work thread */
> +       struct mutex irq_mutex;      /* lock over irq_data */
> +       atomic_t irq_type[16];       /* pin -> edge detection type */
> +       atomic_t irq_enabled;
> +       int id;
> +
> +       u8 gpio_outputs[2];          /* Output states for GPIOs [L, H] */
> +       u8 gpio_dir[2];              /* Directions for GPIOs [L, H] */

Caching states of lines is a bit regmap territory. Have you looked into
just using regmap?

> +static DEFINE_IDA(gpio_mpsse_ida);

Hm what is this for...

> +static int gpio_mpsse_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int err;
> +       unsigned long mask = 0, bits = 0;
> +
> +       set_bit(offset, &mask);

If this doesn't need to be atomic you should use
__set_bit() and __clear_bit().

Yeah I know it's confusing... I think you should use the __variants
everywhere.

> +static const struct irq_chip gpio_mpsse_irq_chip = {
> +       .name = "gpio-mpsse-irq",
> +       .irq_enable = gpio_mpsse_irq_enable,
> +       .irq_disable = gpio_mpsse_irq_disable,
> +       .irq_set_type = gpio_mpsse_set_irq_type,
> +       .flags = IRQCHIP_IMMUTABLE,
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

Why was there also an irq_chip in the struct above?

I'm confused.

This is how it should look though.

> +static int gpio_mpsse_irq_map(struct irq_domain *d, unsigned int irq,
> +                             irq_hw_number_t hwirq)
> +{
> +       int ret;
> +
> +       ret = irq_set_chip_data(irq, d->host_data);
> +       if (ret < 0)
> +               return ret;
> +       irq_set_chip_and_handler(irq, &gpio_mpsse_irq_chip, handle_simple_irq);
> +       irq_set_noprobe(irq);
> +
> +       return 0;
> +}
> +
> +static void gpio_mpsse_irq_unmap(struct irq_domain *d, unsigned int irq)
> +{
> +       irq_set_chip_and_handler(irq, NULL, NULL);
> +       irq_set_chip_data(irq, NULL);
> +}
> +
> +static const struct irq_domain_ops gpio_mpsse_irq_ops = {
> +       .map = gpio_mpsse_irq_map,
> +       .unmap = gpio_mpsse_irq_unmap,
> +       .xlate = irq_domain_xlate_twocell,
> +};

Is there something wrong with just using the gpiolib irqchip library

select GPIOLIB_IRQCHIP

there are several examples in other drivers of how to use this.

> +static int gpio_mpsse_probe(struct usb_interface *interface,
> +                           const struct usb_device_id *id)
> +{
> +       struct mpsse_priv *priv;
> +       struct device *dev;
> +       int err, irq, offset;
> +
> +       dev = &interface->dev;
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->udev = usb_get_dev(interface_to_usbdev(interface));
> +       priv->intf = interface;
> +       priv->intf_id = interface->cur_altsetting->desc.bInterfaceNumber;
> +
> +       priv->id = ida_simple_get(&gpio_mpsse_ida, 0, 0, GFP_KERNEL);
> +       if (priv->id < 0)
> +               return priv->id;
> +
> +       devm_mutex_init(dev, &priv->io_mutex);
> +       devm_mutex_init(dev, &priv->irq_mutex);
> +
> +       priv->gpio.label = devm_kasprintf(dev, GFP_KERNEL,
> +                                         "gpio-mpsse.%d.%d",
> +                                         priv->id, priv->intf_id);
> +       if (!priv->gpio.label) {
> +               err = -ENOMEM;
> +               goto err;
> +       }

So you are accomodating for several irqchips in the same device,
and handling it like we don't really know how many they will be?
Does it happen in practice that this is anything else than 0?

> +       gpio_irq_chip_set_chip(&priv->gpio.irq, &gpio_mpsse_irq_chip);
> +
> +       priv->gpio.irq.domain = irq_domain_create_linear(dev_fwnode(dev),
> +                                                        priv->gpio.ngpio,
> +                                                        &gpio_mpsse_irq_ops,
> +                                                        priv);
> +
> +       for (offset = 0; offset < priv->gpio.ngpio; ++offset) {
> +               irq = irq_create_mapping(priv->gpio.irq.domain, offset);
> +               if (irq < 0) {
> +                       err = irq;
> +                       goto err;
> +               }
> +       }

This is where you are not using GPIOLIB_IRQCHIP

> +
> +       priv->gpio.irq.parent_handler = NULL;
> +       priv->gpio.irq.num_parents = 0;
> +       priv->gpio.irq.parents = NULL;
> +       priv->gpio.irq.default_type = IRQ_TYPE_NONE;
> +       priv->gpio.irq.handler = handle_simple_irq;

But here you rely on GPIOLIB_IRQCHIP being selected!

Yours,
Linus Walleij