Re: [PATCH] drivers: irqchip: add irq-type-changer

[Geert Uytterhoeven]

Hi Nikita,

On Wed, Jan 19, 2022 at 9:17 PM Nikita Yushchenko
<nikita.yoush@xxxxxxxxxxxxxxxxxx> wrote:
> Irq type changer is a virtual irqchip useful to support boards that
> change (e.g. invert) interrupt signal between producer and consumer.
>
> Usage example, for Kingfisher extension board for Renesas Gen-3 Soc,
> that has WiFi interrupt delivered over inverting level-shifter:
>
> / {
>         gpio1_25_inverted: inverter {
>                 compatible = "linux,irq-type-changer";
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>                 interrupt-parent = <&gpio1>;
>                 interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
>         };
> };
>
> &wlcore {
>         interrupt-parent = <&gpio1_25_inverted>;
>         interrupts = <0 IRQ_TYPE_EDGE_RISING>;
> };
>
> Then, wlcore driver observes IRQ_TYPE_EDGE_RISING trigger type and
> configures interrupt output as such. At the same time, gpio-rcar driver
> gets interrupt configured for IRQ_TYPE_EDGE_FALLING.
>
> This version uses hierarchical irq_domain API, and works only with
> parent interrupt domains compatible with that API.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/irqchip/irq-type-changer.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +
> +struct changer {
> +       unsigned long count;
> +       struct {
> +               struct irq_fwspec fwspec;
> +               unsigned int type;
> +       } out[0];

Please use [] instead of [0] for flexible arrays, else the compiler
doesn't realize it is a flexible array, and won't complain if you add
more members below.

> +};

> +static int __init changer_of_init(struct device_node *node,
> +                                 struct device_node *parent)
> +{
> +       struct irq_domain *domain, *parent_domain;
> +       int count, i, ret;
> +       struct changer *ch;
> +       struct of_phandle_args pargs;
> +       irq_hw_number_t unused;
> +
> +       if (!parent) {
> +               pr_err("%pOF: no parent node\n", node);
> +               return -EINVAL;
> +       }
> +
> +       parent_domain = irq_find_host(parent);
> +       if (!parent_domain) {
> +               pr_err("%pOF: no parent domain\n", node);
> +               return -EINVAL;
> +       }
> +
> +       if (WARN_ON(!parent_domain->ops->translate))
> +               return -EINVAL;
> +
> +       count = of_irq_count(node);
> +       if (count < 1) {
> +               pr_err("%pOF: no interrupts defined\n", node);
> +               return -EINVAL;
> +       }
> +
> +       ch = kzalloc(GFP_KERNEL, sizeof(*ch) + count * sizeof(ch->out[0]));

Oops, wrong parameter order, as detected by the  kernel test robot.
Please use struct_size() to simplify and harden the size calculation.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds