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

From: Mary Strodl
Date: Thu Aug 29 2024 - 10:23:54 EST


On Sat, Aug 24, 2024 at 04:25:59PM +0200, Linus Walleij wrote:
> thanks for your patch!

Thank you for reviewing!

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

Will-do!

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

Oops. Forgot to remove this, thanks.

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

Do you mean gpio_regmap or using regmap directly? I'm not sure that gpio_regmap
will do what I want because I need to provide an irq_chip (and I don't see a way
to "break the glass" and access the gpio_chip directly)

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

Oops, thanks.

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

I've ripped out all the extra stuff, I didn't realise how much was
already being done for me!

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

Are you asking about intf_id? Yes, the hardware I'm supporting here populates
as a composite USB device with 2 MPSSEs.

The terminology is kind of confusing by the way. MPSSE is a functional unit inside one
chip. The device I have here has one chip, and shows up as one usb device with two interfaces:

$ lsusb -t # trimmed down to just the relevant bits
/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/10p, 480M
|__ Port 5: Dev 4, If 0, Class=Vendor Specific Class, Driver=gpio-mpsse, 480M
|__ Port 5: Dev 4, If 1, Class=Vendor Specific Class, Driver=gpio-mpsse, 480M
$ lsusb
Bus 001 Device 004: ID 0c52:a064 Sealevel Systems, Inc. USB <-> Serial Converter

Other models of this chip (FT232) only have 1 MPSSE. I don't have any to test with,
but my assumption is that the 2nd interface won't populate.

As for `priv->id`, I do that because these are USB peripherals, it's conceivable
that more than one of these chips could be attached at once.

> Yours,
> Linus Walleij

Thanks for taking the time to review!

Mary Strodl