Messages in this thread | | | From | Linus Walleij <> | Date | Mon, 14 Jan 2019 11:20:49 +0100 | Subject | Re: [PATCH 1/2] gpio: hlwd: Add basic IRQ support |
| |
Hi Jonathan,
thanks for the patch! It is looking very good. Some minor comments:
On Sun, Jan 13, 2019 at 3:00 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> select GPIO_GENERIC > + select GPIOLIB_IRQCHIP
Nice!
> +static void hlwd_gpio_irqhandler(struct irq_desc *desc) > +{ > + struct hlwd_gpio *hlwd = > + gpiochip_get_data(irq_desc_get_handler_data(desc)); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned long flags; > + unsigned long pending; > + int hwirq; > + > + spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags); > + pending = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTFLAG); > + pending &= hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK);
Please just access IO directly instead through the helper. ioread32be()/iowrite32be() I suppose?
> +static void hlwd_gpio_irq_ack(struct irq_data *data) > +{ > + struct hlwd_gpio *hlwd = > + gpiochip_get_data(irq_data_get_irq_chip_data(data)); > + > + hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTFLAG, BIT(data->hwirq));
Dito.
> + spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags); > + mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK); > + mask &= ~BIT(data->hwirq); > + hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);
Dito.
> + spin_lock_irqsave(&hlwd->gpioc.bgpio_lock, flags); > + mask = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTMASK); > + mask |= BIT(data->hwirq); > + hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTMASK, mask);
Dito.
> + switch (flow_type) { > + case IRQ_TYPE_LEVEL_HIGH: > + level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL); > + level |= BIT(data->hwirq); > + hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + level = hlwd->gpioc.read_reg(hlwd->regs + HW_GPIOB_INTLVL); > + level &= ~BIT(data->hwirq); > + hlwd->gpioc.write_reg(hlwd->regs + HW_GPIOB_INTLVL, level); > + break;
Dito.
> + hlwd->irqc.name = "GPIO";
What about using something device-unique?
hlwd->irqc.name = dev_name(&pdev->dev);
for example?
otherwise it looks fine!
Yours, Linus Walleij
| |