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.neuschae...@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
