Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
From: Andy Shevchenko
Date: Fri Apr 23 2021 - 11:38:01 EST
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