On Sun, May 7, 2017 at 6:34 PM, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
> Add support for GPIO interrupts on Amlogic Meson SoC's.
>
> There's a limit of 8 parent interupts which can be used in total.
> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
> one for each edge.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
(..)
> select GPIOLIB
> + select GPIOLIB_IRQCHIP
This is nice.
> +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned int offset)
> +{
> + int hwirq;
> +
> + if (bank->irq_first < 0)
> + /* this bank cannot generate irqs */
> + return -EINVAL;
> +
> + hwirq = offset - bank->first + bank->irq_first;
> +
> + if (hwirq > bank->irq_last)
> + /* this pin cannot generate irqs */
> + return -EINVAL;
> +
> + return hwirq;
> +}
So this is the kind of stuff I want the gpiolib core to handle. We
cant just proliferate
hundreds of drivers to bank-to-irq mappings of cascaded IRQs.
> +static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
> + int *slots)
> +{
> + int i, cnt = 0;
> +
> + mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> + for (i = 0; i < meson_gpio_num_irq_slots; i++)
> + if (!meson_gpio_irq_slots[i].owner) {
> + meson_gpio_irq_slots[i].owner = data->irq;
> + slots[cnt++] = i;
> + if (cnt == num_slots)
> + break;
> + }
> +
> + if (cnt < num_slots)
> + for (i = 0; i < cnt; i++)
> + meson_gpio_irq_slots[i].owner = 0;
> +
> + mutex_unlock(&meson_gpio_irq_slot_mutex);
> +
> + return cnt == num_slots ? 0 : -ENOSPC;
> +}
I don't understand what is happening here but it looks like a hierarchical
IRQ domain.
> +static int meson_gpio_irq_request_resources(struct irq_data *data)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> + unsigned gpio = irqd_to_hwirq(data);
> +
> + return gpiochip_lock_as_irq(chip, gpio);
> +}
> +
> +static void meson_gpio_irq_release_resources(struct irq_data *data)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> + unsigned gpio = irqd_to_hwirq(data);
> +
> + meson_gpio_free_irq_slot(data);
> + gpiochip_unlock_as_irq(chip, gpio);
> +}
Nice that you implemented this though.
> +static void meson_gpio_set_hwirq(int idx, int hwirq)
> +{
> + int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> + int shift = 8 * (idx % 4);
> +
> + regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
> + hwirq << shift);
> +}
So you program the hardware to route the IRQ, OK.
> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +{
(...)
> + for (i = 0; i < num_slots; i++) {
> + irq = meson_gpio_irq_slots[slots[i]].irq;
> + ret = irq_set_irq_type(irq, val);
> + if (ret)
> + break;
> + ret = request_irq(irq, meson_gpio_irq_handler, 0,
> + "GPIO parent", data);
> + if (ret)
> + break;
Issueing request_irq() inside .set_type() is definately wrong.
You want to do chained or nested IRQs for this, possible hierarchical
as well.
> + return gpiochip_irqchip_add(&pc->chip, &pc->irq_chip, 0,
> + handle_simple_irq, IRQ_TYPE_NONE);
gpiochip_irqchip_add() should be followed by
gpiochip_set_chained_irqchip() or gpiochip_set_nested_irqchip().
Else we are not providing the helpers you need.
Which I guess is the case.
I'm a bit confused, I think we need a better understanding of how the
hardware works.
Is it a chained interrupt handler where you can select a number of
GPIOs in each bank to cascade IRQs off?
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
