[PATCH V8 0/3] irqchip: qcom: Add IRQ combiner driver

[PATCH V8 0/3] irqchip: qcom: Add IRQ combiner driver

[Agustin Vega-Frias]

Add support for IRQ combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

The first patch adds support for ResourceSource/IRQ domain mapping
when using Extended IRQ Resources with a specific ResourceSource.
The patch prevents the ACPI core from attempting to map IRQ resources
with a valid ResourceSource as GSIs.

The second patch fixes IRQ probe deferral by allowing platform_device
IRQ resources to be re-initialized if the ACPI core failed to find
the IRQ domain during ACPI bus scan.

Both changes described above are conditional on the ACPI_GENERIC_GSI config.

The third patch takes advantage of the new capabilities to implement
the driver for the IRQ combiners.

Tested on top of v4.9-rc7.

Changes V7 -> V8:
* Reorder patches to allow all new code to be under drivers/acpi/irq.c.
* Change acpi_irq_get implementation to be more similar to of_irq_get
  to improve maintainability.

Changes V6 -> V7:
* Consolidate changes for ResourceSource/IRQ domain mapping to the same
  source file implementing the generic GSI support, making it conditional
  on CONFIG_ACPI_GENERIC_GSI.
* Eliminate some code duplication by implementing acpi_register_gsi in
  terms of the new acpi_register_irq API.

Changes V5 -> V6:
* Drop probe table and on-demand probing based on resource_source
* Add patch to fix probe deferral
* Change back combiner driver to use the platform_device/platform_driver
  APIs

Agustin Vega-Frias (3):
  ACPI: Add support for ResourceSource/IRQ domain mapping
  ACPI: Retry IRQ conversion if it failed previously
  irqchip: qcom: Add IRQ combiner driver

 drivers/acpi/Makefile               |   2 +-
 drivers/acpi/gsi.c                  |  98 -----------
 drivers/acpi/irq.c                  | 315 +++++++++++++++++++++++++++++++++
 drivers/acpi/resource.c             |  29 ++--
 drivers/base/platform.c             |   9 +-
 drivers/irqchip/Kconfig             |   8 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 337 ++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h                |  28 +++
 9 files changed, 714 insertions(+), 113 deletions(-)
 delete mode 100644 drivers/acpi/gsi.c
 create mode 100644 drivers/acpi/irq.c
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V8 1/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Agustin Vega-Frias]

When an Extended IRQ Resource contains a valid ResourceSource
use it to map the IRQ on the domain associated with the ACPI
device referenced.

With this in place an irqchip driver can create its domain using
irq_domain_create_linear and pass the device fwnode to create
the domain mapping. When dependent devices are probed these
changes allow the ACPI core find the domain and map the IRQ.

Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
---
 drivers/acpi/Makefile         |  2 +-
 drivers/acpi/{gsi.c => irq.c} | 99 +++++++++++++++++++++++++++++++++++++------
 drivers/acpi/resource.c       | 29 +++++++------
 include/linux/acpi.h          | 19 +++++++++
 4 files changed, 122 insertions(+), 27 deletions(-)
 rename drivers/acpi/{gsi.c => irq.c} (52%)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 9ed0878..a391bbc 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -55,7 +55,7 @@ acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
 acpi-$(CONFIG_ACPI_PROCFS_POWER) += cm_sbs.o
 acpi-y				+= acpi_lpat.o
-acpi-$(CONFIG_ACPI_GENERIC_GSI) += gsi.o
+acpi-$(CONFIG_ACPI_GENERIC_GSI) += irq.o
 acpi-$(CONFIG_ACPI_WATCHDOG)	+= acpi_watchdog.o
 
 # These are (potentially) separate modules
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/irq.c
similarity index 52%
rename from drivers/acpi/gsi.c
rename to drivers/acpi/irq.c
index ee9e0f2..e82eb6e 100644
--- a/drivers/acpi/gsi.c
+++ b/drivers/acpi/irq.c
@@ -18,6 +18,45 @@
 static struct fwnode_handle *acpi_gsi_domain_id;
 
 /**
+ * acpi_get_irq_source_fwhandle() - Retrieve the fwhandle of the given
+ *                                  acpi_resource_source which is used
+ *                                  to be used as an IRQ domain id
+ * @source: acpi_resource_source to use for the lookup
+ *
+ * Returns: The appropriate IRQ fwhandle domain id
+ *          NULL on failure
+ */
+struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source)
+{
+	struct fwnode_handle *result;
+	struct acpi_device *device;
+	acpi_handle handle;
+	acpi_status status;
+
+	if (!source->string_length)
+		return acpi_gsi_domain_id;
+
+	status = acpi_get_handle(NULL, source->string_ptr, &handle);
+	if (ACPI_FAILURE(status)) {
+		pr_warn("Could not find handle for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	device = acpi_bus_get_acpi_device(handle);
+	if (!device) {
+		pr_warn("Could not get device for %s\n", source->string_ptr);
+		return NULL;
+	}
+
+	result = &device->fwnode;
+	acpi_bus_put_acpi_device(device);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(acpi_get_irq_source_fwhandle);
+
+/**
  * acpi_gsi_to_irq() - Retrieve the linux irq number for a given GSI
  * @gsi: GSI IRQ number to map
  * @irq: pointer where linux IRQ number is stored
@@ -42,6 +81,51 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
 EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
 
 /**
+ * acpi_register_irq() - Map a hardware to a linux IRQ number
+ * @source: IRQ source
+ * @hwirq: Hardware IRQ number
+ * @trigger: trigger type of the IRQ number to be mapped
+ * @polarity: polarity of the IRQ to be mapped
+ *
+ * Returns: a valid linux IRQ number on success
+ *          -EINVAL on failure
+ *          -EPROBE_DEFER if the IRQ domain lookup failed
+ */
+int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
+		      int polarity)
+{
+	struct irq_fwspec fwspec;
+
+	if (!source)
+		source = acpi_gsi_domain_id;
+
+	if (irq_find_matching_fwnode(source, DOMAIN_BUS_ANY) == NULL)
+		return -EPROBE_DEFER;
+
+	fwspec.fwnode = source;
+	fwspec.param[0] = hwirq;
+	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
+	fwspec.param_count = 2;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+EXPORT_SYMBOL_GPL(acpi_register_irq);
+
+/**
+ * acpi_unregister_irq() - Free a Hardware IRQ<->linux IRQ number mapping
+ * @hwirq: Hardware IRQ number
+ */
+void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
+{
+	struct irq_domain *d = irq_find_matching_fwnode(source,
+							DOMAIN_BUS_ANY);
+	int irq = irq_find_mapping(d, hwirq);
+
+	irq_dispose_mapping(irq);
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_irq);
+
+/**
  * acpi_register_gsi() - Map a GSI to a linux IRQ number
  * @dev: device for which IRQ has to be mapped
  * @gsi: GSI IRQ number
@@ -54,19 +138,12 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
 int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
 		      int polarity)
 {
-	struct irq_fwspec fwspec;
-
 	if (WARN_ON(!acpi_gsi_domain_id)) {
 		pr_warn("GSI: No registered irqchip, giving up\n");
 		return -EINVAL;
 	}
 
-	fwspec.fwnode = acpi_gsi_domain_id;
-	fwspec.param[0] = gsi;
-	fwspec.param[1] = acpi_dev_get_irq_type(trigger, polarity);
-	fwspec.param_count = 2;
-
-	return irq_create_fwspec_mapping(&fwspec);
+	return acpi_register_irq(acpi_gsi_domain_id, gsi, trigger, polarity);
 }
 EXPORT_SYMBOL_GPL(acpi_register_gsi);
 
@@ -76,11 +153,7 @@ int acpi_register_gsi(struct device *dev, u32 gsi, int trigger,
  */
 void acpi_unregister_gsi(u32 gsi)
 {
-	struct irq_domain *d = irq_find_matching_fwnode(acpi_gsi_domain_id,
-							DOMAIN_BUS_ANY);
-	int irq = irq_find_mapping(d, gsi);
-
-	irq_dispose_mapping(irq);
+	acpi_unregister_irq(acpi_gsi_domain_id, gsi);
 }
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
 
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c
index 56241eb..98b94eb 100644
--- a/drivers/acpi/resource.c
+++ b/drivers/acpi/resource.c
@@ -374,21 +374,22 @@ unsigned int acpi_dev_get_irq_type(int triggering, int polarity)
 }
 EXPORT_SYMBOL_GPL(acpi_dev_get_irq_type);
 
-static void acpi_dev_irqresource_disabled(struct resource *res, u32 gsi)
+static void acpi_dev_irqresource_disabled(struct resource *res, u32 hwirq)
 {
-	res->start = gsi;
-	res->end = gsi;
+	res->start = hwirq;
+	res->end = hwirq;
 	res->flags = IORESOURCE_IRQ | IORESOURCE_DISABLED | IORESOURCE_UNSET;
 }
 
-static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
+static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
+				     struct fwnode_handle *source,
 				     u8 triggering, u8 polarity, u8 shareable,
 				     bool legacy)
 {
 	int irq, p, t;
 
-	if (!valid_IRQ(gsi)) {
-		acpi_dev_irqresource_disabled(res, gsi);
+	if (!source && !valid_IRQ(hwirq)) {
+		acpi_dev_irqresource_disabled(res, hwirq);
 		return;
 	}
 
@@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
 	 * using extended IRQ descriptors we take the IRQ configuration
 	 * from _CRS directly.
 	 */
-	if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
+	if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
 		u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
 		u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
 
 		if (triggering != trig || polarity != pol) {
-			pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
-				   t ? "level" : "edge", p ? "low" : "high");
+			pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
+				t ? "level" : "edge", p ? "low" : "high");
 			triggering = trig;
 			polarity = pol;
 		}
 	}
 
 	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
-	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
+	irq = acpi_register_irq(source, hwirq, triggering, polarity);
 	if (irq >= 0) {
 		res->start = irq;
 		res->end = irq;
 	} else {
-		acpi_dev_irqresource_disabled(res, gsi);
+		acpi_dev_irqresource_disabled(res, hwirq);
 	}
 }
 
@@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 {
 	struct acpi_resource_irq *irq;
 	struct acpi_resource_extended_irq *ext_irq;
+	struct fwnode_handle *src;
 
 	switch (ares->type) {
 	case ACPI_RESOURCE_TYPE_IRQ:
@@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, irq->interrupts[index],
+		acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
 					 irq->triggering, irq->polarity,
 					 irq->sharable, true);
 		break;
@@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
 			acpi_dev_irqresource_disabled(res, 0);
 			return false;
 		}
-		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
+		src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
+		acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src,
 					 ext_irq->triggering, ext_irq->polarity,
 					 ext_irq->sharable, false);
 		break;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 61a3d90..154e576 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model,
  */
 void acpi_unregister_gsi (u32 gsi);
 
+#ifdef CONFIG_ACPI_GENERIC_GSI
+struct fwnode_handle *
+acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source);
+int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
+		      int polarity);
+void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
+#else
+#define acpi_get_irq_source_fwhandle(source) (NULL)
+static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq,
+				    int trigger, int polarity)
+{
+	return acpi_register_gsi(NULL, hwirq, trigger, polarity);
+}
+static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
+{
+	acpi_unregister_gsi(hwirq);
+}
+#endif
+
 struct pci_dev;
 
 int acpi_pci_irq_enable (struct pci_dev *dev);
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V8 2/3] ACPI: Retry IRQ conversion if it failed previously

[Agustin Vega-Frias]

This allows probe deferral to work properly when a dependent device
fails to get a valid IRQ because the IRQ domain was not registered
at the time the resources were added to the platform_device.

Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
---
 drivers/acpi/irq.c      | 144 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/base/platform.c |   9 ++-
 include/linux/acpi.h    |   9 +++
 3 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index e82eb6e..9279168 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c
@@ -158,6 +158,150 @@ void acpi_unregister_gsi(u32 gsi)
 EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
 
 /**
+ * Context for the resource walk used to lookup IRQ resources.
+ */
+struct acpi_irq_parse_one_ctx {
+	int rc;
+	unsigned int index;
+	unsigned long *res_flags;
+	struct irq_fwspec *fwspec;
+};
+
+/**
+ * acpi_irq_parse_one_match - Handle a matching IRQ resource
+ */
+static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
+					    u32 hwirq, u8 triggering,
+					    u8 polarity, u8 shareable,
+					    struct acpi_irq_parse_one_ctx *ctx)
+{
+	ctx->rc = 0;
+	*ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable);
+	ctx->fwspec->fwnode = fwnode;
+	ctx->fwspec->param[0] = hwirq;
+	ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+	ctx->fwspec->param_count = 2;
+}
+
+/**
+ * acpi_irq_parse_one_cb - Handle the given resource
+ * @ares: resource to handle
+ * @context: context for the walk, contains the lookup index and references
+ *           to the flags and fwspec where the result is returned
+ *
+ * This is called by acpi_walk_resources passing each resource returned by
+ * the _CRS method. We only inspect IRQ resources. Since IRQ resources
+ * might contain multiple interrupts we check if the index is within this
+ * one's interrupt array, otherwise we subtract the current resource IRQ
+ * count from the lookup index to prepare for the next resource.
+ * Once a match is found we call acpi_irq_parse_one_match to populate
+ * the result and end the walk by returning AE_CTRL_TERMINATE.
+ *
+ * Return AE_OK if the walk should continue, AE_CTRL_TERMINATE if a matching
+ * IRQ resource was found.
+ */
+static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
+					 void *context)
+{
+	struct acpi_irq_parse_one_ctx *ctx = context;
+	struct acpi_resource_irq *irq;
+	struct acpi_resource_extended_irq *eirq;
+	struct fwnode_handle *fwnode;
+
+	switch (ares->type) {
+	case ACPI_RESOURCE_TYPE_IRQ:
+		irq = &ares->data.irq;
+		if (ctx->index >= irq->interrupt_count) {
+			ctx->index -= irq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_gsi_domain_id;
+		acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index],
+					 irq->triggering, irq->polarity,
+					 irq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+		eirq = &ares->data.extended_irq;
+		if (ctx->index >= eirq->interrupt_count) {
+			ctx->index -= eirq->interrupt_count;
+			return AE_OK;
+		}
+		fwnode = acpi_get_irq_source_fwhandle(&eirq->resource_source);
+		acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index],
+					 eirq->triggering, eirq->polarity,
+					 eirq->sharable, ctx);
+		return AE_CTRL_TERMINATE;
+	}
+
+	return AE_OK;
+}
+
+/**
+ * acpi_irq_parse_one - Resolve an interrupt for a device
+ * @handle: the device whose interrupt is to be resolved
+ * @index: index of the interrupt to resolve
+ * @fwspec: structure irq_fwspec filled by this function
+ * @flags: resource flags filled by this function
+ *
+ * This function resolves an interrupt for a device by walking its CRS resources
+ * to find the appropriate ACPI IRQ resource and populating the given structure
+ * which can be used to retrieve a Linux IRQ number.
+ *
+ * Returns the result stored in ctx.rc by the callback, or -EINVAL if the given
+ * index is out of range.
+ */
+static int acpi_irq_parse_one(acpi_handle handle, unsigned int index,
+			      struct irq_fwspec *fwspec, unsigned long *flags)
+{
+	struct acpi_irq_parse_one_ctx ctx = { -EINVAL, index, flags, fwspec };
+	acpi_status status;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     acpi_irq_parse_one_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return ctx.rc;
+}
+
+/**
+ * acpi_irq_get - Look for the ACPI IRQ resource with the given index and
+ *                use it to initialize the given Linux IRQ resource.
+ * @handle ACPI device handle
+ * @index  ACPI IRQ resource index to lookup
+ * @res    Linux IRQ resource to initialize
+ *
+ * Return: 0 on success
+ *         -EINVAL if an error occurs
+ *         -EPROBE_DEFER if the IRQ lookup/conversion failed
+ */
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res)
+{
+	int rc;
+	struct irq_fwspec fwspec;
+	struct irq_domain *domain;
+	unsigned long flags;
+
+	rc = acpi_irq_parse_one(handle, index, &fwspec, &flags);
+	if (rc)
+		return rc;
+
+	domain = irq_find_matching_fwnode(fwspec.fwnode, DOMAIN_BUS_ANY);
+	if (!domain)
+		return -EPROBE_DEFER;
+
+	rc = irq_create_fwspec_mapping(&fwspec);
+	if (rc <= 0)
+		return -EINVAL;
+
+	res->start = rc;
+	res->end = rc;
+	res->flags = flags;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_irq_get);
+
+/**
  * acpi_set_irq_model - Setup the GSI irqdomain information
  * @model: the value assigned to acpi_irq_model
  * @fwnode: the irq_domain identifier for mapping and looking up
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index c4af003..61423d2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -102,6 +102,14 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 	}
 
 	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
+	if (r && r->flags & IORESOURCE_DISABLED && ACPI_COMPANION(&dev->dev)) {
+		int ret;
+
+		ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * The resources may pass trigger flags to the irqs that need
 	 * to be set up. It so happens that the trigger flags for
@@ -1450,4 +1458,3 @@ void __init early_platform_cleanup(void)
 		memset(&pd->dev.devres_head, 0, sizeof(pd->dev.devres_head));
 	}
 }
-
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 154e576..6919bfd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -327,6 +327,7 @@ struct fwnode_handle *
 int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
 		      int polarity);
 void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
+int acpi_irq_get(acpi_handle handle, unsigned int index, struct resource *res);
 #else
 #define acpi_get_irq_source_fwhandle(source) (NULL)
 static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq,
@@ -785,6 +786,14 @@ static inline int acpi_reconfig_notifier_unregister(struct notifier_block *nb)
 
 #endif	/* !CONFIG_ACPI */
 
+#ifndef CONFIG_ACPI_GENERIC_GSI
+static inline int acpi_irq_get(acpi_handle handle, unsigned int index,
+			       struct resource *res)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_ACPI_GENERIC_GSI */
+
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
 int acpi_ioapic_add(acpi_handle root);
 #else
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Agustin Vega-Frias]

Driver for interrupt combiners in the Top-level Control and Status
Registers (TCSR) hardware block in Qualcomm Technologies chips.

An interrupt combiner in this block combines a set of interrupts by
OR'ing the individual interrupt signals into a summary interrupt
signal routed to a parent interrupt controller, and provides read-
only, 32-bit registers to query the status of individual interrupts.
The status bit for IRQ n is bit (n % 32) within register (n / 32)
of the given combiner. Thus, each combiner can be described as a set
of register offsets and the number of IRQs managed.

Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
---
 drivers/irqchip/Kconfig             |   8 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/qcom-irq-combiner.c | 337 ++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 drivers/irqchip/qcom-irq-combiner.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bc0af33..610f902 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -279,3 +279,11 @@ config EZNPS_GIC
 config STM32_EXTI
 	bool
 	select IRQ_DOMAIN
+
+config QCOM_IRQ_COMBINER
+	bool "QCOM IRQ combiner support"
+	depends on ARCH_QCOM
+	select IRQ_DOMAIN
+	help
+	  Say yes here to add support for the IRQ combiner devices embedded
+	  in Qualcomm Technologies chips.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc8..1818a0b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
+obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/qcom-irq-combiner.c b/drivers/irqchip/qcom-irq-combiner.c
new file mode 100644
index 0000000..fc25251
--- /dev/null
+++ b/drivers/irqchip/qcom-irq-combiner.c
@@ -0,0 +1,337 @@
+/* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * Driver for interrupt combiners in the Top-level Control and Status
+ * Registers (TCSR) hardware block in Qualcomm Technologies chips.
+ * An interrupt combiner in this block combines a set of interrupts by
+ * OR'ing the individual interrupt signals into a summary interrupt
+ * signal routed to a parent interrupt controller, and provides read-
+ * only, 32-bit registers to query the status of individual interrupts.
+ * The status bit for IRQ n is bit (n % 32) within register (n / 32)
+ * of the given combiner. Thus, each combiner can be described as a set
+ * of register offsets and the number of IRQs managed.
+ */
+
+#include <linux/acpi.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+
+#define REG_SIZE 32
+
+struct combiner_reg {
+	void __iomem *addr;
+	unsigned long mask;
+};
+
+struct combiner {
+	struct irq_chip     irq_chip;
+	struct irq_domain   *domain;
+	int                 parent_irq;
+	u32                 nirqs;
+	u32                 nregs;
+	struct combiner_reg regs[0];
+};
+
+static inline u32 irq_register(int irq)
+{
+	return irq / REG_SIZE;
+}
+
+static inline u32 irq_bit(int irq)
+{
+	return irq % REG_SIZE;
+
+}
+
+static inline int irq_nr(u32 reg, u32 bit)
+{
+	return reg * REG_SIZE + bit;
+}
+
+/*
+ * Handler for the cascaded IRQ.
+ */
+static void combiner_handle_irq(struct irq_desc *desc)
+{
+	struct combiner *combiner = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+
+	for (reg = 0; reg < combiner->nregs; reg++) {
+		int virq;
+		int hwirq;
+		u32 bit;
+		u32 status;
+
+		if (combiner->regs[reg].mask == 0)
+			continue;
+
+		status = readl_relaxed(combiner->regs[reg].addr);
+		status &= combiner->regs[reg].mask;
+
+		while (status) {
+			bit = __ffs(status);
+			status &= ~(1 << bit);
+			hwirq = irq_nr(reg, bit);
+			virq = irq_find_mapping(combiner->domain, hwirq);
+			if (virq >= 0)
+				generic_handle_irq(virq);
+
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * irqchip callbacks
+ */
+
+static void combiner_irq_chip_mask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
+
+	clear_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+static void combiner_irq_chip_unmask_irq(struct irq_data *data)
+{
+	struct combiner *combiner = irq_data_get_irq_chip_data(data);
+	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
+
+	set_bit(irq_bit(data->hwirq), &reg->mask);
+}
+
+/*
+ * irq_domain_ops callbacks
+ */
+
+static int combiner_irq_map(struct irq_domain *domain, unsigned int irq,
+				   irq_hw_number_t hwirq)
+{
+	struct combiner *combiner = domain->host_data;
+
+	if (hwirq >= combiner->nirqs)
+		return -EINVAL;
+
+	irq_set_chip_and_handler(irq, &combiner->irq_chip, handle_level_irq);
+	irq_set_chip_data(irq, combiner);
+	irq_set_parent(irq, combiner->parent_irq);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static void combiner_irq_unmap(struct irq_domain *domain, unsigned int irq)
+{
+	irq_set_chip_and_handler(irq, NULL, NULL);
+	irq_set_chip_data(irq, NULL);
+	irq_set_parent(irq, -1);
+}
+
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+static int combiner_irq_translate(struct irq_domain *d, struct irq_fwspec *fws,
+				  unsigned long *hwirq, unsigned int *type)
+{
+	if (is_acpi_node(fws->fwnode)) {
+		if (fws->param_count != 2)
+			return -EINVAL;
+
+		*hwirq = fws->param[0];
+		*type = fws->param[1];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+#endif
+
+static const struct irq_domain_ops domain_ops = {
+	.map = combiner_irq_map,
+	.unmap = combiner_irq_unmap,
+#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
+	.translate = combiner_irq_translate
+#endif
+};
+
+/*
+ * Device probing
+ */
+
+#ifdef CONFIG_ACPI
+
+static acpi_status count_registers_cb(struct acpi_resource *ares, void *context)
+{
+	int *count = context;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		++(*count);
+	return AE_OK;
+}
+
+static int count_registers(struct platform_device *pdev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	int count = 0;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     count_registers_cb, &count);
+	if (ACPI_FAILURE(status))
+		return -EINVAL;
+	return count;
+}
+
+struct get_registers_context {
+	struct device *dev;
+	struct combiner *combiner;
+	int err;
+};
+
+static acpi_status get_registers_cb(struct acpi_resource *ares, void *context)
+{
+	struct get_registers_context *ctx = context;
+	struct acpi_resource_generic_register *reg;
+	phys_addr_t paddr;
+	void __iomem *vaddr;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
+		return AE_OK;
+
+	reg = &ares->data.generic_reg;
+	paddr = reg->address;
+	if ((reg->space_id != ACPI_SPACE_MEM) ||
+	    (reg->bit_offset != 0) ||
+	    (reg->bit_width > REG_SIZE)) {
+		dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
+		ctx->err = -EINVAL;
+		return AE_ERROR;
+	}
+
+	vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
+	if (IS_ERR(vaddr)) {
+		dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
+		ctx->err = PTR_ERR(vaddr);
+		return AE_ERROR;
+	}
+
+	ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
+	ctx->combiner->nirqs += reg->bit_width;
+	ctx->combiner->nregs++;
+	return AE_OK;
+}
+
+static int get_registers(struct platform_device *pdev, struct combiner *comb)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+	acpi_status status;
+	struct get_registers_context ctx;
+
+	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
+		return -EINVAL;
+
+	ctx.dev = &pdev->dev;
+	ctx.combiner = comb;
+	ctx.err = 0;
+
+	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+				     get_registers_cb, &ctx);
+	if (ACPI_FAILURE(status))
+		return ctx.err;
+	return 0;
+}
+
+#else /* !CONFIG_ACPI */
+
+static int count_registers(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
+
+static int get_registers(struct platform_device *pdev, struct combiner *comb)
+{
+	return -EINVAL;
+}
+
+#endif
+
+static int __init combiner_probe(struct platform_device *pdev)
+{
+	struct combiner *combiner;
+	size_t alloc_sz;
+	u32 nregs;
+	int err;
+
+	nregs = count_registers(pdev);
+	if (nregs <= 0) {
+		dev_err(&pdev->dev, "Error reading register resources\n");
+		return -EINVAL;
+	}
+
+	alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
+	combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
+	if (!combiner)
+		return -ENOMEM;
+
+	err = get_registers(pdev, combiner);
+	if (err < 0)
+		return err;
+
+	combiner->parent_irq = platform_get_irq(pdev, 0);
+	if (combiner->parent_irq <= 0) {
+		dev_err(&pdev->dev, "Error getting IRQ resource\n");
+		return -EINVAL;
+	}
+
+	combiner->domain = irq_domain_create_linear(
+		pdev->dev.fwnode, combiner->nirqs, &domain_ops, combiner);
+	if (!combiner->domain)
+		/* Errors printed by irq_domain_create_linear */
+		return -ENODEV;
+
+	irq_set_chained_handler_and_data(combiner->parent_irq,
+					 combiner_handle_irq, combiner);
+	combiner->irq_chip.irq_mask = combiner_irq_chip_mask_irq;
+	combiner->irq_chip.irq_unmask = combiner_irq_chip_unmask_irq;
+	combiner->irq_chip.name = pdev->name;
+
+	dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
+		 combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
+	return 0;
+}
+
+static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
+	{ "QCOM80B1", },
+	{ }
+};
+
+static struct platform_driver qcom_irq_combiner_probe = {
+	.driver = {
+		.name = "qcom-irq-combiner",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
+	},
+	.probe = combiner_probe,
+};
+
+static int __init register_qcom_irq_combiner(void)
+{
+	return platform_driver_register(&qcom_irq_combiner_probe);
+}
+device_initcall(register_qcom_irq_combiner);
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Marc Zyngier]

Hi Agustin,

On 29/11/16 22:57, Agustin Vega-Frias wrote:
> Driver for interrupt combiners in the Top-level Control and Status
> Registers (TCSR) hardware block in Qualcomm Technologies chips.
> 
> An interrupt combiner in this block combines a set of interrupts by
> OR'ing the individual interrupt signals into a summary interrupt
> signal routed to a parent interrupt controller, and provides read-
> only, 32-bit registers to query the status of individual interrupts.
> The status bit for IRQ n is bit (n % 32) within register (n / 32)
> of the given combiner. Thus, each combiner can be described as a set
> of register offsets and the number of IRQs managed.
> 
> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> ---
>  drivers/irqchip/Kconfig             |   8 +
>  drivers/irqchip/Makefile            |   1 +
>  drivers/irqchip/qcom-irq-combiner.c | 337 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 346 insertions(+)
>  create mode 100644 drivers/irqchip/qcom-irq-combiner.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bc0af33..610f902 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -279,3 +279,11 @@ config EZNPS_GIC
>  config STM32_EXTI
>  	bool
>  	select IRQ_DOMAIN
> +
> +config QCOM_IRQ_COMBINER
> +	bool "QCOM IRQ combiner support"
> +	depends on ARCH_QCOM
> +	select IRQ_DOMAIN
> +	help
> +	  Say yes here to add support for the IRQ combiner devices embedded
> +	  in Qualcomm Technologies chips.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index e4dbfc8..1818a0b 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
> +obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> diff --git a/drivers/irqchip/qcom-irq-combiner.c b/drivers/irqchip/qcom-irq-combiner.c
> new file mode 100644
> index 0000000..fc25251
> --- /dev/null
> +++ b/drivers/irqchip/qcom-irq-combiner.c
> @@ -0,0 +1,337 @@
> +/* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +/*
> + * Driver for interrupt combiners in the Top-level Control and Status
> + * Registers (TCSR) hardware block in Qualcomm Technologies chips.
> + * An interrupt combiner in this block combines a set of interrupts by
> + * OR'ing the individual interrupt signals into a summary interrupt
> + * signal routed to a parent interrupt controller, and provides read-
> + * only, 32-bit registers to query the status of individual interrupts.
> + * The status bit for IRQ n is bit (n % 32) within register (n / 32)
> + * of the given combiner. Thus, each combiner can be described as a set
> + * of register offsets and the number of IRQs managed.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/platform_device.h>
> +
> +#define REG_SIZE 32
> +
> +struct combiner_reg {
> +	void __iomem *addr;
> +	unsigned long mask;
> +};
> +
> +struct combiner {
> +	struct irq_chip     irq_chip;
> +	struct irq_domain   *domain;
> +	int                 parent_irq;
> +	u32                 nirqs;
> +	u32                 nregs;
> +	struct combiner_reg regs[0];
> +};
> +
> +static inline u32 irq_register(int irq)
> +{
> +	return irq / REG_SIZE;
> +}
> +
> +static inline u32 irq_bit(int irq)
> +{
> +	return irq % REG_SIZE;
> +
> +}
> +
> +static inline int irq_nr(u32 reg, u32 bit)
> +{
> +	return reg * REG_SIZE + bit;
> +}
> +
> +/*
> + * Handler for the cascaded IRQ.
> + */
> +static void combiner_handle_irq(struct irq_desc *desc)
> +{
> +	struct combiner *combiner = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	u32 reg;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	for (reg = 0; reg < combiner->nregs; reg++) {
> +		int virq;
> +		int hwirq;
> +		u32 bit;
> +		u32 status;
> +
> +		if (combiner->regs[reg].mask == 0)
> +			continue;
> +
> +		status = readl_relaxed(combiner->regs[reg].addr);
> +		status &= combiner->regs[reg].mask;
> +
> +		while (status) {
> +			bit = __ffs(status);
> +			status &= ~(1 << bit);
> +			hwirq = irq_nr(reg, bit);
> +			virq = irq_find_mapping(combiner->domain, hwirq);
> +			if (virq >= 0)
> +				generic_handle_irq(virq);
> +
> +		}
> +	}
> +
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/*
> + * irqchip callbacks
> + */
> +
> +static void combiner_irq_chip_mask_irq(struct irq_data *data)
> +{
> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
> +	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
> +
> +	clear_bit(irq_bit(data->hwirq), &reg->mask);
> +}
> +
> +static void combiner_irq_chip_unmask_irq(struct irq_data *data)
> +{
> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
> +	struct combiner_reg *reg = combiner->regs + irq_register(data->hwirq);
> +
> +	set_bit(irq_bit(data->hwirq), &reg->mask);
> +}
> +
> +/*
> + * irq_domain_ops callbacks
> + */
> +
> +static int combiner_irq_map(struct irq_domain *domain, unsigned int irq,
> +				   irq_hw_number_t hwirq)
> +{
> +	struct combiner *combiner = domain->host_data;
> +
> +	if (hwirq >= combiner->nirqs)
> +		return -EINVAL;
> +
> +	irq_set_chip_and_handler(irq, &combiner->irq_chip, handle_level_irq);
> +	irq_set_chip_data(irq, combiner);
> +	irq_set_parent(irq, combiner->parent_irq);

Do you really need this irq_set_parent? This only makes sense if you're
resending edge interrupts, and you seem to be level only.

> +	irq_set_noprobe(irq);
> +	return 0;
> +}
> +
> +static void combiner_irq_unmap(struct irq_domain *domain, unsigned int irq)
> +{
> +	irq_set_chip_and_handler(irq, NULL, NULL);
> +	irq_set_chip_data(irq, NULL);
> +	irq_set_parent(irq, -1);

All of this should probably be replaced with a call to
irq_domain_reset_irq_data().

> +}
> +
> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY

Is there any case where this is not valid?

> +static int combiner_irq_translate(struct irq_domain *d, struct irq_fwspec *fws,
> +				  unsigned long *hwirq, unsigned int *type)
> +{
> +	if (is_acpi_node(fws->fwnode)) {
> +		if (fws->param_count != 2)
> +			return -EINVAL;
> +
> +		*hwirq = fws->param[0];
> +		*type = fws->param[1];

Given that you're only handling level interrupts, shouldn't you abort if
detecting an edge interrupt?

> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +#endif
> +
> +static const struct irq_domain_ops domain_ops = {
> +	.map = combiner_irq_map,
> +	.unmap = combiner_irq_unmap,
> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> +	.translate = combiner_irq_translate
> +#endif
> +};
> +
> +/*
> + * Device probing
> + */
> +
> +#ifdef CONFIG_ACPI
> +
> +static acpi_status count_registers_cb(struct acpi_resource *ares, void *context)
> +{
> +	int *count = context;
> +
> +	if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
> +		++(*count);
> +	return AE_OK;
> +}
> +
> +static int count_registers(struct platform_device *pdev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	acpi_status status;
> +	int count = 0;
> +
> +	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
> +		return -EINVAL;
> +
> +	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
> +				     count_registers_cb, &count);
> +	if (ACPI_FAILURE(status))
> +		return -EINVAL;
> +	return count;
> +}
> +
> +struct get_registers_context {
> +	struct device *dev;
> +	struct combiner *combiner;
> +	int err;
> +};
> +
> +static acpi_status get_registers_cb(struct acpi_resource *ares, void *context)
> +{
> +	struct get_registers_context *ctx = context;
> +	struct acpi_resource_generic_register *reg;
> +	phys_addr_t paddr;
> +	void __iomem *vaddr;
> +
> +	if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
> +		return AE_OK;
> +
> +	reg = &ares->data.generic_reg;
> +	paddr = reg->address;
> +	if ((reg->space_id != ACPI_SPACE_MEM) ||
> +	    (reg->bit_offset != 0) ||
> +	    (reg->bit_width > REG_SIZE)) {
> +		dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
> +		ctx->err = -EINVAL;
> +		return AE_ERROR;
> +	}
> +
> +	vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
> +	if (IS_ERR(vaddr)) {
> +		dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
> +		ctx->err = PTR_ERR(vaddr);
> +		return AE_ERROR;
> +	}
> +
> +	ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
> +	ctx->combiner->nirqs += reg->bit_width;
> +	ctx->combiner->nregs++;
> +	return AE_OK;
> +}
> +
> +static int get_registers(struct platform_device *pdev, struct combiner *comb)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	acpi_status status;
> +	struct get_registers_context ctx;
> +
> +	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
> +		return -EINVAL;
> +
> +	ctx.dev = &pdev->dev;
> +	ctx.combiner = comb;
> +	ctx.err = 0;
> +
> +	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
> +				     get_registers_cb, &ctx);
> +	if (ACPI_FAILURE(status))
> +		return ctx.err;
> +	return 0;
> +}
> +
> +#else /* !CONFIG_ACPI */
> +
> +static int count_registers(struct platform_device *pdev)
> +{
> +	return -EINVAL;
> +}
> +
> +static int get_registers(struct platform_device *pdev, struct combiner *comb)
> +{
> +	return -EINVAL;
> +}

So this driver is obviously ACPI only. Can't you just get rid of these,
of the #ifdef CONFIG_ACPI, and simply make it depend on ACPI?

> +
> +#endif
> +
> +static int __init combiner_probe(struct platform_device *pdev)
> +{
> +	struct combiner *combiner;
> +	size_t alloc_sz;
> +	u32 nregs;
> +	int err;
> +
> +	nregs = count_registers(pdev);
> +	if (nregs <= 0) {
> +		dev_err(&pdev->dev, "Error reading register resources\n");
> +		return -EINVAL;
> +	}
> +
> +	alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
> +	combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
> +	if (!combiner)
> +		return -ENOMEM;
> +
> +	err = get_registers(pdev, combiner);
> +	if (err < 0)
> +		return err;
> +
> +	combiner->parent_irq = platform_get_irq(pdev, 0);
> +	if (combiner->parent_irq <= 0) {
> +		dev_err(&pdev->dev, "Error getting IRQ resource\n");
> +		return -EINVAL;

Can you ever get in a situation where it'd be more sensible to return
-EPROBE_DEFER? I'm thinking of a case where you'd have this irq chip
cascaded into another one, which is not present yet?

> +	}
> +
> +	combiner->domain = irq_domain_create_linear(
> +		pdev->dev.fwnode, combiner->nirqs, &domain_ops, combiner);

On a single line, please. Do no listen to the checkpatch police that
will tell you otherwise. It really hurt my eyes to see this opening
bracket and *nothing* after that.

> +	if (!combiner->domain)
> +		/* Errors printed by irq_domain_create_linear */
> +		return -ENODEV;
> +
> +	irq_set_chained_handler_and_data(combiner->parent_irq,
> +					 combiner_handle_irq, combiner);
> +	combiner->irq_chip.irq_mask = combiner_irq_chip_mask_irq;
> +	combiner->irq_chip.irq_unmask = combiner_irq_chip_unmask_irq;
> +	combiner->irq_chip.name = pdev->name;

Arghh. Please don't do that. Once you've called irq_set_chained_*, the
interrupt is live, and can be requested. Unlikely, but still. In
general, feeding uninitialized data to a function is pretty bad.

Also, do you really need to show pdev->name in /proc/cpuinfo? Just make
the irq_chip structure static, outside of combiner, and have "QCOM80B1"
(or something of your liking) as the name once and for all.

> +
> +	dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
> +		 combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
> +	return 0;
> +}
> +
> +static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
> +	{ "QCOM80B1", },
> +	{ }
> +};
> +
> +static struct platform_driver qcom_irq_combiner_probe = {
> +	.driver = {
> +		.name = "qcom-irq-combiner",
> +		.owner = THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
> +	},
> +	.probe = combiner_probe,
> +};
> +
> +static int __init register_qcom_irq_combiner(void)
> +{
> +	return platform_driver_register(&qcom_irq_combiner_probe);
> +}
> +device_initcall(register_qcom_irq_combiner);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V8 1/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Lorenzo Pieralisi]

Hi Agustin,

please CC me for next version.

On Tue, Nov 29, 2016 at 05:57:37PM -0500, Agustin Vega-Frias wrote:
> When an Extended IRQ Resource contains a valid ResourceSource
> use it to map the IRQ on the domain associated with the ACPI
> device referenced.
> 
> With this in place an irqchip driver can create its domain using
> irq_domain_create_linear and pass the device fwnode to create
> the domain mapping. When dependent devices are probed these
> changes allow the ACPI core find the domain and map the IRQ.
> 
> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
> ---
>  drivers/acpi/Makefile         |  2 +-
>  drivers/acpi/{gsi.c => irq.c} | 99 +++++++++++++++++++++++++++++++++++++------

[...]

> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
> +static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
> +				     struct fwnode_handle *source,
>  				     u8 triggering, u8 polarity, u8 shareable,
>  				     bool legacy)
>  {
>  	int irq, p, t;
>  
> -	if (!valid_IRQ(gsi)) {
> -		acpi_dev_irqresource_disabled(res, gsi);
> +	if (!source && !valid_IRQ(hwirq)) {

If you make source a struct acpi_resource_source pointer it could be:

if (source || !valid_IRQ(hwirq))

Actually we would not even need to pass the pointer, if we detect
an acpi_resource_source dependency we can just disable the resource
(without even looking-up the fwnode_handle, see below), it is a design
choice we have to make.

> +		acpi_dev_irqresource_disabled(res, hwirq);
>  		return;
>  	}
>  
> @@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>  	 * using extended IRQ descriptors we take the IRQ configuration
>  	 * from _CRS directly.
>  	 */
> -	if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
> +	if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
>  		u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
>  		u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>  
>  		if (triggering != trig || polarity != pol) {
> -			pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
> -				   t ? "level" : "edge", p ? "low" : "high");
> +			pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
> +				t ? "level" : "edge", p ? "low" : "high");
>  			triggering = trig;
>  			polarity = pol;
>  		}
>  	}
>  
>  	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
> -	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
> +	irq = acpi_register_irq(source, hwirq, triggering, polarity);
>  	if (irq >= 0) {
>  		res->start = irq;
>  		res->end = irq;
>  	} else {
> -		acpi_dev_irqresource_disabled(res, gsi);
> +		acpi_dev_irqresource_disabled(res, hwirq);
>  	}
>  }
>  
> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  {
>  	struct acpi_resource_irq *irq;
>  	struct acpi_resource_extended_irq *ext_irq;
> +	struct fwnode_handle *src;
>  
>  	switch (ares->type) {
>  	case ACPI_RESOURCE_TYPE_IRQ:
> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, irq->interrupts[index],
> +		acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>  					 irq->triggering, irq->polarity,
>  					 irq->sharable, true);
>  		break;
> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
>  			acpi_dev_irqresource_disabled(res, 0);
>  			return false;
>  		}
> -		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
> +		src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);

I think the only pending question on my side for this series is whether
we still carry out the domain look-up here (as you are doing now), or,
if we detect a resource_source dependency, we just disable the resource
and leave the deferred probing mechanism to deal with it, this will
completely decouple the current resource parsing from the deferred probe
mechanism that you are introducing; basically this is equivalent to
saying "if the IRQ resource has a dependency let's resolve it at
acpi_irq_get() time, not now".

I am fine either way, I just think that leaving the domain look-up
in the middle of the IRQ resource parsing is not really clean-cut.

Thanks,
Lorenzo

> +		acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src,
>  					 ext_irq->triggering, ext_irq->polarity,
>  					 ext_irq->sharable, false);
>  		break;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 61a3d90..154e576 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id model,
>   */
>  void acpi_unregister_gsi (u32 gsi);
>  
> +#ifdef CONFIG_ACPI_GENERIC_GSI
> +struct fwnode_handle *
> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source *source);
> +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int trigger,
> +		      int polarity);
> +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
> +#else
> +#define acpi_get_irq_source_fwhandle(source) (NULL)
> +static inline int acpi_register_irq(struct fwnode_handle *source, u32 hwirq,
> +				    int trigger, int polarity)
> +{
> +	return acpi_register_gsi(NULL, hwirq, trigger, polarity);
> +}
> +static inline void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq)
> +{
> +	acpi_unregister_gsi(hwirq);
> +}
> +#endif
> +
>  struct pci_dev;
>  
>  int acpi_pci_irq_enable (struct pci_dev *dev);
> -- 
> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V8 1/3] ACPI: Add support for ResourceSource/IRQ domain mapping

[Agustin Vega-Frias]

Hi Lorenzo,

On 2016-12-08 06:05, Lorenzo Pieralisi wrote:
> Hi Agustin,
> 
> please CC me for next version.
> 
> On Tue, Nov 29, 2016 at 05:57:37PM -0500, Agustin Vega-Frias wrote:
>> When an Extended IRQ Resource contains a valid ResourceSource
>> use it to map the IRQ on the domain associated with the ACPI
>> device referenced.
>> 
>> With this in place an irqchip driver can create its domain using
>> irq_domain_create_linear and pass the device fwnode to create
>> the domain mapping. When dependent devices are probed these
>> changes allow the ACPI core find the domain and map the IRQ.
>> 
>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>> ---
>>  drivers/acpi/Makefile         |  2 +-
>>  drivers/acpi/{gsi.c => irq.c} | 99 
>> +++++++++++++++++++++++++++++++++++++------
> 
> [...]
> 
>> -static void acpi_dev_get_irqresource(struct resource *res, u32 gsi,
>> +static void acpi_dev_get_irqresource(struct resource *res, u32 hwirq,
>> +				     struct fwnode_handle *source,
>>  				     u8 triggering, u8 polarity, u8 shareable,
>>  				     bool legacy)
>>  {
>>  	int irq, p, t;
>> 
>> -	if (!valid_IRQ(gsi)) {
>> -		acpi_dev_irqresource_disabled(res, gsi);
>> +	if (!source && !valid_IRQ(hwirq)) {
> 
> If you make source a struct acpi_resource_source pointer it could be:
> 
> if (source || !valid_IRQ(hwirq))
> 
> Actually we would not even need to pass the pointer, if we detect
> an acpi_resource_source dependency we can just disable the resource
> (without even looking-up the fwnode_handle, see below), it is a design
> choice we have to make.
> 
>> +		acpi_dev_irqresource_disabled(res, hwirq);
>>  		return;
>>  	}
>> 
>> @@ -402,25 +403,25 @@ static void acpi_dev_get_irqresource(struct 
>> resource *res, u32 gsi,
>>  	 * using extended IRQ descriptors we take the IRQ configuration
>>  	 * from _CRS directly.
>>  	 */
>> -	if (legacy && !acpi_get_override_irq(gsi, &t, &p)) {
>> +	if (legacy && !acpi_get_override_irq(hwirq, &t, &p)) {
>>  		u8 trig = t ? ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
>>  		u8 pol = p ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>> 
>>  		if (triggering != trig || polarity != pol) {
>> -			pr_warning("ACPI: IRQ %d override to %s, %s\n", gsi,
>> -				   t ? "level" : "edge", p ? "low" : "high");
>> +			pr_warn("ACPI: IRQ %d override to %s, %s\n", hwirq,
>> +				t ? "level" : "edge", p ? "low" : "high");
>>  			triggering = trig;
>>  			polarity = pol;
>>  		}
>>  	}
>> 
>>  	res->flags = acpi_dev_irq_flags(triggering, polarity, shareable);
>> -	irq = acpi_register_gsi(NULL, gsi, triggering, polarity);
>> +	irq = acpi_register_irq(source, hwirq, triggering, polarity);
>>  	if (irq >= 0) {
>>  		res->start = irq;
>>  		res->end = irq;
>>  	} else {
>> -		acpi_dev_irqresource_disabled(res, gsi);
>> +		acpi_dev_irqresource_disabled(res, hwirq);
>>  	}
>>  }
>> 
>> @@ -448,6 +449,7 @@ bool acpi_dev_resource_interrupt(struct 
>> acpi_resource *ares, int index,
>>  {
>>  	struct acpi_resource_irq *irq;
>>  	struct acpi_resource_extended_irq *ext_irq;
>> +	struct fwnode_handle *src;
>> 
>>  	switch (ares->type) {
>>  	case ACPI_RESOURCE_TYPE_IRQ:
>> @@ -460,7 +462,7 @@ bool acpi_dev_resource_interrupt(struct 
>> acpi_resource *ares, int index,
>>  			acpi_dev_irqresource_disabled(res, 0);
>>  			return false;
>>  		}
>> -		acpi_dev_get_irqresource(res, irq->interrupts[index],
>> +		acpi_dev_get_irqresource(res, irq->interrupts[index], NULL,
>>  					 irq->triggering, irq->polarity,
>>  					 irq->sharable, true);
>>  		break;
>> @@ -470,7 +472,8 @@ bool acpi_dev_resource_interrupt(struct 
>> acpi_resource *ares, int index,
>>  			acpi_dev_irqresource_disabled(res, 0);
>>  			return false;
>>  		}
>> -		acpi_dev_get_irqresource(res, ext_irq->interrupts[index],
>> +		src = acpi_get_irq_source_fwhandle(&ext_irq->resource_source);
> 
> I think the only pending question on my side for this series is whether
> we still carry out the domain look-up here (as you are doing now), or,
> if we detect a resource_source dependency, we just disable the resource
> and leave the deferred probing mechanism to deal with it, this will
> completely decouple the current resource parsing from the deferred 
> probe
> mechanism that you are introducing; basically this is equivalent to
> saying "if the IRQ resource has a dependency let's resolve it at
> acpi_irq_get() time, not now".
> 

I'm also torn about this so I am going to leave most of this code as it
was originally and just ensure we don't attempt the mapping when we have
a resource source.

I other words bus scan only maps GSIs and other IRQs are mapped via
acpi_irq_get().

Thanks,
Agustin

> I am fine either way, I just think that leaving the domain look-up
> in the middle of the IRQ resource parsing is not really clean-cut.
> 
> Thanks,
> Lorenzo
> 
>> +		acpi_dev_get_irqresource(res, ext_irq->interrupts[index], src,
>>  					 ext_irq->triggering, ext_irq->polarity,
>>  					 ext_irq->sharable, false);
>>  		break;
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 61a3d90..154e576 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -321,6 +321,25 @@ void acpi_set_irq_model(enum acpi_irq_model_id 
>> model,
>>   */
>>  void acpi_unregister_gsi (u32 gsi);
>> 
>> +#ifdef CONFIG_ACPI_GENERIC_GSI
>> +struct fwnode_handle *
>> +acpi_get_irq_source_fwhandle(const struct acpi_resource_source 
>> *source);
>> +int acpi_register_irq(struct fwnode_handle *source, u32 hwirq, int 
>> trigger,
>> +		      int polarity);
>> +void acpi_unregister_irq(struct fwnode_handle *source, u32 hwirq);
>> +#else
>> +#define acpi_get_irq_source_fwhandle(source) (NULL)
>> +static inline int acpi_register_irq(struct fwnode_handle *source, u32 
>> hwirq,
>> +				    int trigger, int polarity)
>> +{
>> +	return acpi_register_gsi(NULL, hwirq, trigger, polarity);
>> +}
>> +static inline void acpi_unregister_irq(struct fwnode_handle *source, 
>> u32 hwirq)
>> +{
>> +	acpi_unregister_gsi(hwirq);
>> +}
>> +#endif
>> +
>>  struct pci_dev;
>> 
>>  int acpi_pci_irq_enable (struct pci_dev *dev);
>> --
>> Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
>> Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
>> Linux Foundation Collaborative Project.
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Agustin Vega-Frias]

On 2016-12-07 13:16, Marc Zyngier wrote:
> Hi Agustin,
> 
> On 29/11/16 22:57, Agustin Vega-Frias wrote:
>> Driver for interrupt combiners in the Top-level Control and Status
>> Registers (TCSR) hardware block in Qualcomm Technologies chips.
>> 
>> An interrupt combiner in this block combines a set of interrupts by
>> OR'ing the individual interrupt signals into a summary interrupt
>> signal routed to a parent interrupt controller, and provides read-
>> only, 32-bit registers to query the status of individual interrupts.
>> The status bit for IRQ n is bit (n % 32) within register (n / 32)
>> of the given combiner. Thus, each combiner can be described as a set
>> of register offsets and the number of IRQs managed.
>> 
>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>> ---
>>  drivers/irqchip/Kconfig             |   8 +
>>  drivers/irqchip/Makefile            |   1 +
>>  drivers/irqchip/qcom-irq-combiner.c | 337 
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 346 insertions(+)
>>  create mode 100644 drivers/irqchip/qcom-irq-combiner.c
>> 
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index bc0af33..610f902 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -279,3 +279,11 @@ config EZNPS_GIC
>>  config STM32_EXTI
>>  	bool
>>  	select IRQ_DOMAIN
>> +
>> +config QCOM_IRQ_COMBINER
>> +	bool "QCOM IRQ combiner support"
>> +	depends on ARCH_QCOM
>> +	select IRQ_DOMAIN
>> +	help
>> +	  Say yes here to add support for the IRQ combiner devices embedded
>> +	  in Qualcomm Technologies chips.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index e4dbfc8..1818a0b 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>> +obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> diff --git a/drivers/irqchip/qcom-irq-combiner.c 
>> b/drivers/irqchip/qcom-irq-combiner.c
>> new file mode 100644
>> index 0000000..fc25251
>> --- /dev/null
>> +++ b/drivers/irqchip/qcom-irq-combiner.c
>> @@ -0,0 +1,337 @@
>> +/* Copyright (c) 2015-2016, The Linux Foundation. All rights 
>> reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +/*
>> + * Driver for interrupt combiners in the Top-level Control and Status
>> + * Registers (TCSR) hardware block in Qualcomm Technologies chips.
>> + * An interrupt combiner in this block combines a set of interrupts 
>> by
>> + * OR'ing the individual interrupt signals into a summary interrupt
>> + * signal routed to a parent interrupt controller, and provides read-
>> + * only, 32-bit registers to query the status of individual 
>> interrupts.
>> + * The status bit for IRQ n is bit (n % 32) within register (n / 32)
>> + * of the given combiner. Thus, each combiner can be described as a 
>> set
>> + * of register offsets and the number of IRQs managed.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define REG_SIZE 32
>> +
>> +struct combiner_reg {
>> +	void __iomem *addr;
>> +	unsigned long mask;
>> +};
>> +
>> +struct combiner {
>> +	struct irq_chip     irq_chip;
>> +	struct irq_domain   *domain;
>> +	int                 parent_irq;
>> +	u32                 nirqs;
>> +	u32                 nregs;
>> +	struct combiner_reg regs[0];
>> +};
>> +
>> +static inline u32 irq_register(int irq)
>> +{
>> +	return irq / REG_SIZE;
>> +}
>> +
>> +static inline u32 irq_bit(int irq)
>> +{
>> +	return irq % REG_SIZE;
>> +
>> +}
>> +
>> +static inline int irq_nr(u32 reg, u32 bit)
>> +{
>> +	return reg * REG_SIZE + bit;
>> +}
>> +
>> +/*
>> + * Handler for the cascaded IRQ.
>> + */
>> +static void combiner_handle_irq(struct irq_desc *desc)
>> +{
>> +	struct combiner *combiner = irq_desc_get_handler_data(desc);
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	u32 reg;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	for (reg = 0; reg < combiner->nregs; reg++) {
>> +		int virq;
>> +		int hwirq;
>> +		u32 bit;
>> +		u32 status;
>> +
>> +		if (combiner->regs[reg].mask == 0)
>> +			continue;
>> +
>> +		status = readl_relaxed(combiner->regs[reg].addr);
>> +		status &= combiner->regs[reg].mask;
>> +
>> +		while (status) {
>> +			bit = __ffs(status);
>> +			status &= ~(1 << bit);
>> +			hwirq = irq_nr(reg, bit);
>> +			virq = irq_find_mapping(combiner->domain, hwirq);
>> +			if (virq >= 0)
>> +				generic_handle_irq(virq);
>> +
>> +		}
>> +	}
>> +
>> +	chained_irq_exit(chip, desc);
>> +}
>> +
>> +/*
>> + * irqchip callbacks
>> + */
>> +
>> +static void combiner_irq_chip_mask_irq(struct irq_data *data)
>> +{
>> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
>> +	struct combiner_reg *reg = combiner->regs + 
>> irq_register(data->hwirq);
>> +
>> +	clear_bit(irq_bit(data->hwirq), &reg->mask);
>> +}
>> +
>> +static void combiner_irq_chip_unmask_irq(struct irq_data *data)
>> +{
>> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
>> +	struct combiner_reg *reg = combiner->regs + 
>> irq_register(data->hwirq);
>> +
>> +	set_bit(irq_bit(data->hwirq), &reg->mask);
>> +}
>> +
>> +/*
>> + * irq_domain_ops callbacks
>> + */
>> +
>> +static int combiner_irq_map(struct irq_domain *domain, unsigned int 
>> irq,
>> +				   irq_hw_number_t hwirq)
>> +{
>> +	struct combiner *combiner = domain->host_data;
>> +
>> +	if (hwirq >= combiner->nirqs)
>> +		return -EINVAL;
>> +
>> +	irq_set_chip_and_handler(irq, &combiner->irq_chip, 
>> handle_level_irq);
>> +	irq_set_chip_data(irq, combiner);
>> +	irq_set_parent(irq, combiner->parent_irq);
> 
> Do you really need this irq_set_parent? This only makes sense if you're
> resending edge interrupts, and you seem to be level only.

OK, I'll take a look.

> 
>> +	irq_set_noprobe(irq);
>> +	return 0;
>> +}
>> +
>> +static void combiner_irq_unmap(struct irq_domain *domain, unsigned 
>> int irq)
>> +{
>> +	irq_set_chip_and_handler(irq, NULL, NULL);
>> +	irq_set_chip_data(irq, NULL);
>> +	irq_set_parent(irq, -1);
> 
> All of this should probably be replaced with a call to
> irq_domain_reset_irq_data().

Will do.

> 
>> +}
>> +
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
> 
> Is there any case where this is not valid?

What do you mean? Are you asking why is this under a preprocessor
conditional? If so I did it to be on the safe side since translate
in struct irq_domain_ops under that conditional.

> 
>> +static int combiner_irq_translate(struct irq_domain *d, struct 
>> irq_fwspec *fws,
>> +				  unsigned long *hwirq, unsigned int *type)
>> +{
>> +	if (is_acpi_node(fws->fwnode)) {
>> +		if (fws->param_count != 2)
>> +			return -EINVAL;
>> +
>> +		*hwirq = fws->param[0];
>> +		*type = fws->param[1];
> 
> Given that you're only handling level interrupts, shouldn't you abort 
> if
> detecting an edge interrupt?

I'll add the check.

> 
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>> +static const struct irq_domain_ops domain_ops = {
>> +	.map = combiner_irq_map,
>> +	.unmap = combiner_irq_unmap,
>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	.translate = combiner_irq_translate
>> +#endif
>> +};
>> +
>> +/*
>> + * Device probing
>> + */
>> +
>> +#ifdef CONFIG_ACPI
>> +
>> +static acpi_status count_registers_cb(struct acpi_resource *ares, 
>> void *context)
>> +{
>> +	int *count = context;
>> +
>> +	if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
>> +		++(*count);
>> +	return AE_OK;
>> +}
>> +
>> +static int count_registers(struct platform_device *pdev)
>> +{
>> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +	acpi_status status;
>> +	int count = 0;
>> +
>> +	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
>> +		return -EINVAL;
>> +
>> +	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>> +				     count_registers_cb, &count);
>> +	if (ACPI_FAILURE(status))
>> +		return -EINVAL;
>> +	return count;
>> +}
>> +
>> +struct get_registers_context {
>> +	struct device *dev;
>> +	struct combiner *combiner;
>> +	int err;
>> +};
>> +
>> +static acpi_status get_registers_cb(struct acpi_resource *ares, void 
>> *context)
>> +{
>> +	struct get_registers_context *ctx = context;
>> +	struct acpi_resource_generic_register *reg;
>> +	phys_addr_t paddr;
>> +	void __iomem *vaddr;
>> +
>> +	if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
>> +		return AE_OK;
>> +
>> +	reg = &ares->data.generic_reg;
>> +	paddr = reg->address;
>> +	if ((reg->space_id != ACPI_SPACE_MEM) ||
>> +	    (reg->bit_offset != 0) ||
>> +	    (reg->bit_width > REG_SIZE)) {
>> +		dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
>> +		ctx->err = -EINVAL;
>> +		return AE_ERROR;
>> +	}
>> +
>> +	vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
>> +	if (IS_ERR(vaddr)) {
>> +		dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
>> +		ctx->err = PTR_ERR(vaddr);
>> +		return AE_ERROR;
>> +	}
>> +
>> +	ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
>> +	ctx->combiner->nirqs += reg->bit_width;
>> +	ctx->combiner->nregs++;
>> +	return AE_OK;
>> +}
>> +
>> +static int get_registers(struct platform_device *pdev, struct 
>> combiner *comb)
>> +{
>> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +	acpi_status status;
>> +	struct get_registers_context ctx;
>> +
>> +	if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
>> +		return -EINVAL;
>> +
>> +	ctx.dev = &pdev->dev;
>> +	ctx.combiner = comb;
>> +	ctx.err = 0;
>> +
>> +	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>> +				     get_registers_cb, &ctx);
>> +	if (ACPI_FAILURE(status))
>> +		return ctx.err;
>> +	return 0;
>> +}
>> +
>> +#else /* !CONFIG_ACPI */
>> +
>> +static int count_registers(struct platform_device *pdev)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static int get_registers(struct platform_device *pdev, struct 
>> combiner *comb)
>> +{
>> +	return -EINVAL;
>> +}
> 
> So this driver is obviously ACPI only. Can't you just get rid of these,
> of the #ifdef CONFIG_ACPI, and simply make it depend on ACPI?

Good point, will do.

> 
>> +
>> +#endif
>> +
>> +static int __init combiner_probe(struct platform_device *pdev)
>> +{
>> +	struct combiner *combiner;
>> +	size_t alloc_sz;
>> +	u32 nregs;
>> +	int err;
>> +
>> +	nregs = count_registers(pdev);
>> +	if (nregs <= 0) {
>> +		dev_err(&pdev->dev, "Error reading register resources\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
>> +	combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
>> +	if (!combiner)
>> +		return -ENOMEM;
>> +
>> +	err = get_registers(pdev, combiner);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	combiner->parent_irq = platform_get_irq(pdev, 0);
>> +	if (combiner->parent_irq <= 0) {
>> +		dev_err(&pdev->dev, "Error getting IRQ resource\n");
>> +		return -EINVAL;
> 
> Can you ever get in a situation where it'd be more sensible to return
> -EPROBE_DEFER? I'm thinking of a case where you'd have this irq chip
> cascaded into another one, which is not present yet?

Not in the current architecture, but I agree it would accommodate other
use cases. I will change it to return -EPROBE_DEFER.

> 
>> +	}
>> +
>> +	combiner->domain = irq_domain_create_linear(
>> +		pdev->dev.fwnode, combiner->nirqs, &domain_ops, combiner);
> 
> On a single line, please. Do no listen to the checkpatch police that
> will tell you otherwise. It really hurt my eyes to see this opening
> bracket and *nothing* after that.

Will do.

> 
>> +	if (!combiner->domain)
>> +		/* Errors printed by irq_domain_create_linear */
>> +		return -ENODEV;
>> +
>> +	irq_set_chained_handler_and_data(combiner->parent_irq,
>> +					 combiner_handle_irq, combiner);
>> +	combiner->irq_chip.irq_mask = combiner_irq_chip_mask_irq;
>> +	combiner->irq_chip.irq_unmask = combiner_irq_chip_unmask_irq;
>> +	combiner->irq_chip.name = pdev->name;
> 
> Arghh. Please don't do that. Once you've called irq_set_chained_*, the
> interrupt is live, and can be requested. Unlikely, but still. In
> general, feeding uninitialized data to a function is pretty bad.
> 
> Also, do you really need to show pdev->name in /proc/cpuinfo? Just make
> the irq_chip structure static, outside of combiner, and have "QCOM80B1"
> (or something of your liking) as the name once and for all.

I'll look at this.

Thanks for the detailed feedback.

Agustin.

> 
>> +
>> +	dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
>> +		 combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
>> +	{ "QCOM80B1", },
>> +	{ }
>> +};
>> +
>> +static struct platform_driver qcom_irq_combiner_probe = {
>> +	.driver = {
>> +		.name = "qcom-irq-combiner",
>> +		.owner = THIS_MODULE,
>> +		.acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
>> +	},
>> +	.probe = combiner_probe,
>> +};
>> +
>> +static int __init register_qcom_irq_combiner(void)
>> +{
>> +	return platform_driver_register(&qcom_irq_combiner_probe);
>> +}
>> +device_initcall(register_qcom_irq_combiner);
>> 
> 
> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...

-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a 
Linux Foundation Collaborative Project.

Re: [PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Marc Zyngier]

On 13/12/16 15:23, Agustin Vega-Frias wrote:
> On 2016-12-07 13:16, Marc Zyngier wrote:
>> Hi Agustin,
>>
>> On 29/11/16 22:57, Agustin Vega-Frias wrote:
>>> Driver for interrupt combiners in the Top-level Control and Status
>>> Registers (TCSR) hardware block in Qualcomm Technologies chips.
>>>
>>> An interrupt combiner in this block combines a set of interrupts by
>>> OR'ing the individual interrupt signals into a summary interrupt
>>> signal routed to a parent interrupt controller, and provides read-
>>> only, 32-bit registers to query the status of individual interrupts.
>>> The status bit for IRQ n is bit (n % 32) within register (n / 32)
>>> of the given combiner. Thus, each combiner can be described as a set
>>> of register offsets and the number of IRQs managed.
>>>
>>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>>> ---
>>>  drivers/irqchip/Kconfig             |   8 +
>>>  drivers/irqchip/Makefile            |   1 +
>>>  drivers/irqchip/qcom-irq-combiner.c | 337 
>>> ++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 346 insertions(+)
>>>  create mode 100644 drivers/irqchip/qcom-irq-combiner.c
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index bc0af33..610f902 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -279,3 +279,11 @@ config EZNPS_GIC
>>>  config STM32_EXTI
>>>  	bool
>>>  	select IRQ_DOMAIN
>>> +
>>> +config QCOM_IRQ_COMBINER
>>> +	bool "QCOM IRQ combiner support"
>>> +	depends on ARCH_QCOM
>>> +	select IRQ_DOMAIN
>>> +	help
>>> +	  Say yes here to add support for the IRQ combiner devices embedded
>>> +	  in Qualcomm Technologies chips.
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index e4dbfc8..1818a0b 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>>>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>>  obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
>>>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>>> +obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>> diff --git a/drivers/irqchip/qcom-irq-combiner.c 
>>> b/drivers/irqchip/qcom-irq-combiner.c
>>> new file mode 100644
>>> index 0000000..fc25251
>>> --- /dev/null
>>> +++ b/drivers/irqchip/qcom-irq-combiner.c
>>> @@ -0,0 +1,337 @@
>>> +/* Copyright (c) 2015-2016, The Linux Foundation. All rights 
>>> reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or 
>>> modify
>>> + * it under the terms of the GNU General Public License version 2 and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +/*
>>> + * Driver for interrupt combiners in the Top-level Control and Status
>>> + * Registers (TCSR) hardware block in Qualcomm Technologies chips.
>>> + * An interrupt combiner in this block combines a set of interrupts 
>>> by
>>> + * OR'ing the individual interrupt signals into a summary interrupt
>>> + * signal routed to a parent interrupt controller, and provides read-
>>> + * only, 32-bit registers to query the status of individual 
>>> interrupts.
>>> + * The status bit for IRQ n is bit (n % 32) within register (n / 32)
>>> + * of the given combiner. Thus, each combiner can be described as a 
>>> set
>>> + * of register offsets and the number of IRQs managed.
>>> + */
>>> +
>>> +#include <linux/acpi.h>
>>> +#include <linux/irqchip/chained_irq.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#define REG_SIZE 32
>>> +
>>> +struct combiner_reg {
>>> +	void __iomem *addr;
>>> +	unsigned long mask;
>>> +};
>>> +
>>> +struct combiner {
>>> +	struct irq_chip     irq_chip;
>>> +	struct irq_domain   *domain;
>>> +	int                 parent_irq;
>>> +	u32                 nirqs;
>>> +	u32                 nregs;
>>> +	struct combiner_reg regs[0];
>>> +};
>>> +
>>> +static inline u32 irq_register(int irq)
>>> +{
>>> +	return irq / REG_SIZE;
>>> +}
>>> +
>>> +static inline u32 irq_bit(int irq)
>>> +{
>>> +	return irq % REG_SIZE;
>>> +
>>> +}
>>> +
>>> +static inline int irq_nr(u32 reg, u32 bit)
>>> +{
>>> +	return reg * REG_SIZE + bit;
>>> +}
>>> +
>>> +/*
>>> + * Handler for the cascaded IRQ.
>>> + */
>>> +static void combiner_handle_irq(struct irq_desc *desc)
>>> +{
>>> +	struct combiner *combiner = irq_desc_get_handler_data(desc);
>>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>>> +	u32 reg;
>>> +
>>> +	chained_irq_enter(chip, desc);
>>> +
>>> +	for (reg = 0; reg < combiner->nregs; reg++) {
>>> +		int virq;
>>> +		int hwirq;
>>> +		u32 bit;
>>> +		u32 status;
>>> +
>>> +		if (combiner->regs[reg].mask == 0)
>>> +			continue;
>>> +
>>> +		status = readl_relaxed(combiner->regs[reg].addr);
>>> +		status &= combiner->regs[reg].mask;
>>> +
>>> +		while (status) {
>>> +			bit = __ffs(status);
>>> +			status &= ~(1 << bit);
>>> +			hwirq = irq_nr(reg, bit);
>>> +			virq = irq_find_mapping(combiner->domain, hwirq);
>>> +			if (virq >= 0)
>>> +				generic_handle_irq(virq);
>>> +
>>> +		}
>>> +	}
>>> +
>>> +	chained_irq_exit(chip, desc);
>>> +}
>>> +
>>> +/*
>>> + * irqchip callbacks
>>> + */
>>> +
>>> +static void combiner_irq_chip_mask_irq(struct irq_data *data)
>>> +{
>>> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
>>> +	struct combiner_reg *reg = combiner->regs + 
>>> irq_register(data->hwirq);
>>> +
>>> +	clear_bit(irq_bit(data->hwirq), &reg->mask);
>>> +}
>>> +
>>> +static void combiner_irq_chip_unmask_irq(struct irq_data *data)
>>> +{
>>> +	struct combiner *combiner = irq_data_get_irq_chip_data(data);
>>> +	struct combiner_reg *reg = combiner->regs + 
>>> irq_register(data->hwirq);
>>> +
>>> +	set_bit(irq_bit(data->hwirq), &reg->mask);
>>> +}
>>> +
>>> +/*
>>> + * irq_domain_ops callbacks
>>> + */
>>> +
>>> +static int combiner_irq_map(struct irq_domain *domain, unsigned int 
>>> irq,
>>> +				   irq_hw_number_t hwirq)
>>> +{
>>> +	struct combiner *combiner = domain->host_data;
>>> +
>>> +	if (hwirq >= combiner->nirqs)
>>> +		return -EINVAL;
>>> +
>>> +	irq_set_chip_and_handler(irq, &combiner->irq_chip, 
>>> handle_level_irq);
>>> +	irq_set_chip_data(irq, combiner);
>>> +	irq_set_parent(irq, combiner->parent_irq);
>>
>> Do you really need this irq_set_parent? This only makes sense if you're
>> resending edge interrupts, and you seem to be level only.
> 
> OK, I'll take a look.
> 
>>
>>> +	irq_set_noprobe(irq);
>>> +	return 0;
>>> +}
>>> +
>>> +static void combiner_irq_unmap(struct irq_domain *domain, unsigned 
>>> int irq)
>>> +{
>>> +	irq_set_chip_and_handler(irq, NULL, NULL);
>>> +	irq_set_chip_data(irq, NULL);
>>> +	irq_set_parent(irq, -1);
>>
>> All of this should probably be replaced with a call to
>> irq_domain_reset_irq_data().
> 
> Will do.
> 
>>
>>> +}
>>> +
>>> +#ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
>>
>> Is there any case where this is not valid?
> 
> What do you mean? Are you asking why is this under a preprocessor
> conditional? If so I did it to be on the safe side since translate
> in struct irq_domain_ops under that conditional.

Since this code can only work when CONFIG_IRQ_DOMAIN_HIERARCHY is
selected, you might as well make the KConfig entry select (or depend on
- I'm always confused about which one should be used when) this
configuration symbol, and make the code more readable.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Joe Perches]

On Tue, 2016-12-13 at 10:23 -0500, Agustin Vega-Frias wrote:
> On 2016-12-07 13:16, Marc Zyngier wrote:
> > > +	}
> > > +
> > > +	combiner->domain = irq_domain_create_linear(
> > > +		pdev->dev.fwnode, combiner->nirqs, &domain_ops, combiner);
> > 
> > On a single line, please. Do no listen to the checkpatch police that
> > will tell you otherwise. It really hurt my eyes to see this opening
> > bracket and *nothing* after that.
> 
> Will do.

It seems generally preferred to have at least one argument on the
same line as the function being called.

So, here are some options:

Maximally fill the lines to 80 columns with the value being set
and function call while aligning to open parenthesis

	combiner->domain = irq_domain_create_linear(pdev->dev.fwnode,
						    combiner->nirqs,
						    &domain_ops, combiner);

Use a separate line for the function call:

	combiner->domain =
		irq_domain_create_linear(pdev->dev.fwnode, combiner->nirqs,
					 &domain_ops, combiner);

Or just ignore the 80 column limit wherever you deem appropriate.

No single style is universal, use what you think best.

Anyway, long identifiers (24 chars here) make staying within the
"strongly preferred" 80 column limit produce quite silly looking
code.

Re: [PATCH V8 3/3] irqchip: qcom: Add IRQ combiner driver

[Marc Zyngier]

On 13/12/16 18:21, Joe Perches wrote:
> On Tue, 2016-12-13 at 10:23 -0500, Agustin Vega-Frias wrote:
>> On 2016-12-07 13:16, Marc Zyngier wrote:
>>>> +	}
>>>> +
>>>> +	combiner->domain = irq_domain_create_linear(
>>>> +		pdev->dev.fwnode, combiner->nirqs, &domain_ops, combiner);
>>>
>>> On a single line, please. Do no listen to the checkpatch police that
>>> will tell you otherwise. It really hurt my eyes to see this opening
>>> bracket and *nothing* after that.
>>
>> Will do.
> 
> It seems generally preferred to have at least one argument on the
> same line as the function being called.
> 
> So, here are some options:
> 
> Maximally fill the lines to 80 columns with the value being set
> and function call while aligning to open parenthesis
> 
> 	combiner->domain = irq_domain_create_linear(pdev->dev.fwnode,
> 						    combiner->nirqs,
> 						    &domain_ops, combiner);

I can live with something like this.

> Use a separate line for the function call:
> 
> 	combiner->domain =
> 		irq_domain_create_linear(pdev->dev.fwnode, combiner->nirqs,
> 					 &domain_ops, combiner);

But I find this one pretty horrid.

> Or just ignore the 80 column limit wherever you deem appropriate.

Which is my usual advise. I consider the 80 column rule a good way of
limiting the complexity of code (if the nesting pushes you too far on
the right of the screen, you're doing something wrong), but not for
simple statements such as this one.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...