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

[Mary Strodl <mstrodl@xxxxxxxxxxxxxxxxxxx>]

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