lkml.org 
[lkml]   [2023]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v7 4/6] gpio: Add support for Intel LJCA USB GPIO driver
    Hi Ye,

    thanks for your patch!

    On Sat, Mar 25, 2023 at 4:48 PM Ye Xiang <xiang.ye@intel.com> 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@intel.com>

    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

    \
     
     \ /
      Last update: 2023-03-29 10:37    [W:8.083 / U:0.628 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site