Re: [PATCH v5 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Linus Walleij]

Hi Thomas,

thanks for your patch!

I can see this is starting to look really good.

There is one thing that confuses me:

On Fri, May 14, 2021 at 2:33 PM Thomas Bogendoerfer
<tsbogend@xxxxxxxxxxxxxxxx> wrote:

> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.
>
> Signed-off-by: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> ---
> Changes in v5:
(...)

> +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
(...)
> +       /* hardware only supports level triggered */
> +       if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH))
> +               return -EINVAL;
(...)
> +       irq_set_handler_locked(d, handle_level_irq);

But:

> +static void idt_gpio_ack(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +
> +       writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
> +}
(...)
> +       .irq_ack = idt_gpio_ack,

Correct me if I'm wrong but I thing .irq_ack() is only called
from handle_edge_irq ... so never in this case.

Can this ACK just be deleted?

The code in the ACK callback also looks really weird:
write all bits except for the current IRQ into the status
register? It's usually the other way around with these
things. That really makes me suspect it is unused.

Yours,
Linus Walleij