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