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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZtCBgWrdFI6h3zbo@freedom.csh.rit.edu>
Date: Thu, 29 Aug 2024 10:11:13 -0400
From: Mary Strodl <mstrodl@...edom.csh.rit.edu>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Mary Strodl <mstrodl@....rit.edu>, linux-kernel@...r.kernel.org,
	brgl@...ev.pl, linux-gpio@...r.kernel.org
Subject: Re: [PATCH] gpio: add support for FTDI's MPSSE as GPIO

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