Hi Ye, thanks for your patch! On Sat, Mar 25, 2023 at 4:48 PM Ye Xiang <xiang.ye@xxxxxxxxx> wrote: > This patch implements the GPIO function of Intel USB-I2C/GPIO/SPI adapter > device named "La Jolla Cove Adapter" (LJCA). It communicate with LJCA > GPIO module with specific protocol through interfaces exported by LJCA USB > driver. > > Signed-off-by: Ye Xiang <xiang.ye@xxxxxxxxx> Lots of improvements! here are some comments: > @@ -1253,6 +1253,18 @@ config GPIO_KEMPLD > This driver can also be built as a module. If so, the module will be > called gpio-kempld. > > +config GPIO_LJCA > + tristate "INTEL La Jolla Cove Adapter GPIO support" > + depends on USB_LJCA > + select GPIOLIB_IRQCHIP > + default USB_LJCA > + help > + Select this option to enable GPIO driver for the INTEL > + La Jolla Cove Adapter (LJCA) board. > + > + This driver can also be built as a module. If so, the module > + will be called gpio-ljca. The GPIO Kconfig has a separate submenu for USB expanders, so put this Kconfig in that submenu. This makes the choice come in a more logical spot and not appear on configs that don't even have USB. (...) > + DECLARE_BITMAP(unmasked_irqs, LJCA_MAX_GPIO_NUM); > + DECLARE_BITMAP(enabled_irqs, LJCA_MAX_GPIO_NUM); > + DECLARE_BITMAP(reenable_irqs, LJCA_MAX_GPIO_NUM); > + u8 *connect_mode; > + /* mutex to protect irq bus */ > + struct mutex irq_lock; (...) With IRQ code like this from a USB callback: > +static void ljca_gpio_event_cb(void *context, u8 cmd, const void *evt_data, int len) > +{ > + const struct gpio_packet *packet = evt_data; > + struct ljca_gpio_dev *ljca_gpio = context; > + int i; > + int irq; > + > + if (cmd != LJCA_GPIO_INT_EVENT) > + return; > + > + for (i = 0; i < packet->num; i++) { > + irq = irq_find_mapping(ljca_gpio->gc.irq.domain, packet->item[i].index); > + if (!irq) { > + dev_err(ljca_gpio->gc.parent, "gpio_id %u does not mapped to IRQ yet\n", > + packet->item[i].index); > + return; > + } > + > + generic_handle_domain_irq(ljca_gpio->gc.irq.domain, irq); > + set_bit(packet->item[i].index, ljca_gpio->reenable_irqs); > + } > + > + schedule_work(&ljca_gpio->work); > +} I don't feel comfortable merging this unless Marc Zyngier has looked at the code first, so please CC him on this patch next time. > +static const struct irq_chip ljca_gpio_irqchip = { > + .name = "ljca-irq", > + .irq_mask = ljca_irq_mask, > + .irq_unmask = ljca_irq_unmask, > + .irq_set_type = ljca_irq_set_type, > + .irq_bus_lock = ljca_irq_bus_lock, > + .irq_bus_sync_unlock = ljca_irq_bus_unlock, > + .flags = IRQCHIP_IMMUTABLE, > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > +}; Thanks for fixing the immutable irq chip! > + ljca_gpio->auxdev = auxdev; > + ljca_gpio->gc.direction_input = ljca_gpio_direction_input; > + ljca_gpio->gc.direction_output = ljca_gpio_direction_output; Can you implement .get_direction()? It's scanned on probe to determine the initial state of each line so it is very nice to have. Yours, Linus Walleij