linux-kernel - Re: [PATCH 1/2] gpio: hlwd: Add basic IRQ support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdYyMwu+hFQF4PUSb8N8zo3QEFt5=curhsdTFP1FCtGpNw@mail.gmail.com>
Date:   Mon, 14 Jan 2019 11:20:49 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Jonathan Neuschäfer <j.neuschaefer@....net>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
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@....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