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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Apr 22, 2021 at 6:21 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 interrupt source.

as an interrupt

Thanks for an update, my comments below (minus that you already figured out).

...

> +config GPIO_IDT3243X
> +       tristate "IDT 79RC3243X GPIO support"
> +       depends on MIKROTIK_RB532 || COMPILE_TEST

Right.

But if MikroTik is dependent on this you may return default in a form of

  default MIKROTIK_RB532

Up to you. (What I meant previously is the unnecessary ' y if' part).

> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Select this option to enable GPIO driver for
> +         IDT 79RC3243X SoC devices.

Seems like you may elaborate a bit more here, what kind of the
devices, list one or couple of examples, etc.

> +         To compile this driver as a module, choose M here: the module will
> +         be called gpio-idt3243x.

...

> +/*
> + * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
> + */

One line?

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Why is this?

...

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>

> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>

Aren't those guaranteed to be included by irq.h?

> +#include <linux/irqdomain.h>

Missed mod_devicetable.h
module.h

> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>

Do you use them anyhow? (See below as well)

Missed types.h

...

> +static void idt_gpio_dispatch(struct irq_desc *desc)
> +{
> +       struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +       struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +       struct irq_chip *host_chip = irq_desc_get_chip(desc);
> +       unsigned int bit, virq;
> +       unsigned long pending;
> +
> +       chained_irq_enter(host_chip, desc);
> +
> +       pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
> +       pending &= ~ctrl->mask_cache;
> +       for_each_set_bit(bit, &pending, gc->ngpio) {

> +               virq = irq_linear_revmap(gc->irq.domain, bit);

Is it guaranteed to be linear always?

> +               if (virq)
> +                       generic_handle_irq(virq);
> +       }
> +
> +       chained_irq_exit(host_chip, desc);
> +}

...

> +       if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))

There is a _BOTH variant.

> +               return -EINVAL;

> +       ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
> +       if (sense & IRQ_TYPE_LEVEL_HIGH)
> +               ilevel |= BIT(d->hwirq);
> +       else if (sense & IRQ_TYPE_LEVEL_LOW)
> +               ilevel &= ~BIT(d->hwirq);

> +       else
> +               return -EINVAL;

Is it a double check of the above?

...

> +       ctrl->gc.parent = dev;

Wondering if it's already done by GPIO library.

...

> +       ctrl->gc.ngpio = ngpios;

Shouldn't you do this before calling for bgpio_init()?

...

> +       parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

platform_get_irq() ?..

> +       if (!parent_irq) {

> +               dev_err(&pdev->dev, "Failed to map parent IRQ!\n");

...and drop this, since it will be taken care of.

> +               return -EINVAL;
> +       }

...

> +       /* Mask interrupts. */
> +       ctrl->mask_cache = 0xffffffff;
> +       writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);

What about using ->init_hw() call back?

...

> +       girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),

1 -> girq->num_parents

> +                                    GFP_KERNEL);
> +       if (!girq->parents) {
> +               ret = -ENOMEM;
> +               goto out_unmap_irq;
> +       }

...

> +       girq->handler = handle_level_irq;

handle_bad_irq()

...

> +       ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
> +       if (ret)
> +               goto out_unmap_irq;
> +
> +       return 0;

return devm_...;

...

> +out_unmap_irq:
> +       irq_dispose_mapping(parent_irq);
> +       return ret;
> +}

No need.

-- 
With Best Regards,
Andy Shevchenko