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

[Marc Zyngier]

Nikita,

This patch is obviously part of a series, and yet you post them as
unrelated patches. Please don't do that. Keep the patches together and
treat them as a proper series.

On Mon, 24 Jan 2022 09:56:52 +0000,
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>;
> 	};
> };

You need a proper DT binding.

I also don't see why you model this as the actual device that triggers
the interrupt. This goes against the way we have modelled similar
devices in the tree. I'm also pretty sure that with the current code,
you end-up with *two* interrupts (one for the inverter and one for the
end-point). More on that later.

> 
> &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>
> ---
> v1: https://lore.kernel.org/lkml/20220119201741.717770-1-nikita.yoush@xxxxxxxxxxxxxxxxxx/
> Changes from v1:
> - fixed order of kzalloc() args, caught by kbuild robot
> 
>  drivers/irqchip/Kconfig            |  18 ++++
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-type-changer.c | 162 +++++++++++++++++++++++++++++
>  3 files changed, 181 insertions(+)
>  create mode 100644 drivers/irqchip/irq-type-changer.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 7038957f4a77..7f348016e82c 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -617,4 +617,22 @@ config MCHP_EIC
>  	help
>  	  Support for Microchip External Interrupt Controller.
>  
> +config IRQ_TYPE_CHANGER
> +	bool "Interrupt trigger type changer"
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Interrupt trigger type changer is designed to support boards that
> +	  modify (e.g. invert) signal between interrupt source and interrupt
> +	  controller input. So trigger type configured by a driver for some
> +	  interrupt output pin does not match trigger type that shall be used
> +	  to configure interrupt controller's input where that pin is connected.
> +
> +	  In this case, board device tree shall add an interrupt trigger
> +	  type changer node and use it as the interrupt parent for the node
> +	  representing interrupt source. Then, interrupt trigger type defined
> +	  in the interrupt source node will be visible for the interrupt source
> +	  driver, and (different) trigger type defined inside the changer node
> +	  will be used to configure the interrupt controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c1f611cbfbf8..57a664837857 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -117,3 +117,4 @@ obj-$(CONFIG_WPCM450_AIC)		+= irq-wpcm450-aic.o
>  obj-$(CONFIG_IRQ_IDT3243X)		+= irq-idt3243x.o
>  obj-$(CONFIG_APPLE_AIC)			+= irq-apple-aic.o
>  obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
> +obj-$(CONFIG_IRQ_TYPE_CHANGER)		+= irq-type-changer.o
> diff --git a/drivers/irqchip/irq-type-changer.c b/drivers/irqchip/irq-type-changer.c
> new file mode 100644
> index 000000000000..0f461fda6610
> --- /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 {

Changer doesn't quite describe it. This is an inverter, please name it
as such.

> +	unsigned long count;
> +	struct {
> +		struct irq_fwspec fwspec;
> +		unsigned int type;
> +	} out[0];

Geert commented on why this is wrong.

> +};
> +
> +static int changer_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct changer *ch = data->domain->host_data;
> +	struct irq_data *parent_data = data->parent_data;
> +
> +	return parent_data->chip->irq_set_type(parent_data,
> +					       ch->out[data->hwirq].type);

Can you actually take this at face-value? Some transformations don't
make any sense (you can't turn an edge into a level, for example).
Which is also why calling this a 'type changer' is wrong.

> +}
> +
> +static struct irq_chip changer_chip = {
> +	.name			= "type-changer",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_type		= changer_set_type,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.irq_set_wake		= irq_chip_set_wake_parent,
> +};
> +
> +static int changer_domain_translate(struct irq_domain *domain,
> +				    struct irq_fwspec *fwspec,
> +				    unsigned long *hwirq,
> +				    unsigned int *type)
> +{
> +	struct changer *ch = domain->host_data;
> +
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +	if (fwspec->param[0] >= ch->count)
> +		return -ENXIO;
> +
> +	*hwirq = fwspec->param[0];
> +	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +
> +static int changer_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	struct changer *ch = domain->host_data;
> +	struct irq_fwspec *fwspec = arg;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int ret;
> +
> +	if (WARN_ON(nr_irqs != 1))
> +		return -EINVAL;
> +
> +	ret = changer_domain_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &changer_chip, ch);
> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, 1,
> +					    &ch->out[hwirq].fwspec);

So here, you are forcing the reallocation of an interrupt that was
already created, and overriding the existing mapping. That's not
right.

> +}
> +
> +static const struct irq_domain_ops changer_domain_ops = {
> +	.translate	= changer_domain_translate,
> +	.alloc		= changer_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +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(sizeof(*ch) + count * sizeof(ch->out[0]), GFP_KERNEL);

Use struct_size().

> +	if (!ch)
> +		return -ENOMEM;
> +	ch->count = count;
> +
> +	for (i = 0; i < count; i++) {
> +		ret = of_irq_parse_one(node, i, &pargs);
> +		if (ret) {
> +			pr_err("%pOF: interrupt %d: error %d parsing\n",
> +			       node, i, ret);
> +			goto out_free;
> +		}
> +		of_phandle_args_to_fwspec(pargs.np, pargs.args,
> +					  pargs.args_count,
> +					  &ch->out[i].fwspec);
> +		ret = parent_domain->ops->translate(parent_domain,
> +						    &ch->out[i].fwspec,
> +						    &unused,
> +						    &ch->out[i].type);
> +		if (ret) {
> +			pr_err("%pOF: interrupt %d: error %d extracting type\n",
> +			       node, i, ret);
> +			goto out_free;
> +		}
> +		if (ch->out[i].type == IRQ_TYPE_NONE) {
> +			pr_err("%pOF: interrupt %d: no type\n", node, i);
> +			ret = -ENXIO;
> +			goto out_free;
> +		}
> +	}

This would be all pretty unnecessary if you adopted the scheme I gave
you last time [1]. The interrupt duality definitely is a no-go.

> +
> +	domain = irq_domain_create_hierarchy(parent_domain, 0, count,
> +					     of_node_to_fwnode(node),
> +					     &changer_domain_ops, ch);
> +	if (!domain) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	return 0;
> +
> +out_free:
> +	kfree(ch);
> +	return ret;
> +}
> +
> +IRQCHIP_PLATFORM_DRIVER_BEGIN(changer)
> +IRQCHIP_MATCH("linux,irq-type-changer", changer_of_init)
> +IRQCHIP_PLATFORM_DRIVER_END(changer)
> +MODULE_AUTHOR("Nikita Yushchenko <nikita.yoush.cogentembedded.com>");
> +MODULE_DESCRIPTION("Virtual irqchip to support trigger type change in route");
> +MODULE_LICENSE("GPL v2");

Thanks,

	M.

[1] https://lore.kernel.org/lkml/87fsqbznc2.wl-maz@xxxxxxxxxx

-- 
Without deviation from the norm, progress is not possible.