lkml.org 
[lkml]   [2019]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 1/2] gpio: sch: Add edge event support
    Hi Jan,

    thanks for your patch!

    On Wed, Nov 20, 2019 at 8:20 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:

    > From: Jan Kiszka <jan.kiszka@siemens.com>
    >
    > Add the required infrastructure consisting of an irq_chip_generic with
    > its irq_chip_type callbacks to enable and report edge events of the pins
    > to the gpio core. The actual hook-up of the event interrupt will happen
    > separately.
    >
    > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

    Please resend after the merge window, some comments:

    First I'm pretty sure this driver can select GPIOLIB_IRQCHIP
    and use infrastructure from the core to handle interrupts.

    The fact that you register your own irq handler does not
    stop that. See for example the solution in
    drivers/gpio/gpio-mt7621.c
    where we set the ->parent_handler to NULL to let
    the driver handle the IRQs itself.

    I will try to make this more explicit in the API as we work
    with this.

    > struct sch_gpio {
    > struct gpio_chip chip;
    > spinlock_t lock;
    > unsigned short iobase;
    > unsigned short resume_base;
    > + int irq_base;

    Why are you keeping this around in the state?
    Why not just a local variable?

    > +static int sch_gpio_to_irq(struct gpio_chip *gpio, unsigned int offset)
    > +{
    > + struct sch_gpio *sch = gpiochip_get_data(gpio);
    > + return sch->irq_base + offset;
    > +}
    (...)
    > + .to_irq = sch_gpio_to_irq,
    (...)
    > + irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, sch->chip.ngpio,
    > + NUMA_NO_NODE);
    > + if (irq_base < 0)
    > + return irq_base;
    > + sch->irq_base = irq_base;
    > +
    > + gc = devm_irq_alloc_generic_chip(&pdev->dev, "sch_gpio", 1, irq_base,
    > + NULL, handle_simple_irq);
    > + if (!gc)
    > + return -ENOMEM;
    (...)
    > + ret = devm_irq_setup_generic_chip(&pdev->dev, gc,
    > + IRQ_MSK(sch->chip.ngpio),
    > + 0, IRQ_NOREQUEST | IRQ_NOPROBE, 0);
    > + if (ret)
    > + return ret;

    So I think you can avoid this complexity by jus doing what
    gpio-mt7621.c is doing,
    use devm_request_irq(), populate girq = &gc->irq; before
    registering the gpio_chip pass a handle_simple_irq
    and reuse core gpio irqchip infrastructure.

    But I don't know everything so let's test and see!

    Yours,
    Linus Walleij

    \
     
     \ /
      Last update: 2019-11-28 10:35    [W:2.230 / U:0.324 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site