lkml.org 
[lkml]   [2019]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2019-01-14 11:21    [W:0.093 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site