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
