[PATCH 0/4] mfd: twl6030-irq: rework and add twl6032 support

[PATCH 0/4] mfd: twl6030-irq: rework and add twl6032 support

[Grygorii Strashko]

This patch series intorduces twl6030-irq module rework to use Threaded IRQ and
linear irq_domain, and adds support for PMIC TWL6032 IRQs.

After this patch series TWL6030/6032 IRQs will be supported only for DT boot mode.

Based on v3.11-rc1

Tested generation of RTC_ALARM(3) and PWRON(0) IRQs on OMAP4430/TWL6030 and
OMAP4470/TWL6032.

Grygorii Strashko (2):
  mfd: twl6030-irq: add error check when IRQs are masked initially
  mfd: twl6030-irq: convert to use linear irq_domain

Naga Venkata Srikanth V (1):
  mfd: twl6030-irq: migrate to IRQ threaded handler

Oleksandr Dmytryshyn (1):
  mfd: twl6030-irq: Add interrupt mapping table for the twl6032

 drivers/mfd/twl6030-irq.c |  296 +++++++++++++++++++++++----------------------
 1 file changed, 154 insertions(+), 142 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Grygorii Strashko]

From: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>

1) Removed request_irq() and replaced it with request_threaded_irq().

2) Removed generic_handle_irq() and replaced it with
handle_nested_irq().
  Handling of these interrupts is nested, as we are handling an
interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.

3) Removed I2C read-retry logic for the case when twl_i2c_read() is
failed inside IRQ handler - there is no sense to do that, so just report
an error and return.

Signed-off-by: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
Signed-off-by: Oleg_Kosheliev <oleg.kosheliev@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/mfd/twl6030-irq.c |  146 +++++++++++++++------------------------------
 1 file changed, 49 insertions(+), 97 deletions(-)

diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 277a8db..b6030d9 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
 static int twl_irq;
 static bool twl_irq_wake_enabled;
 
-static struct completion irq_event;
 static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
 
 static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
@@ -131,95 +130,57 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
 };
 
 /*
- * This thread processes interrupts reported by the Primary Interrupt Handler.
- */
-static int twl6030_irq_thread(void *data)
+* Threaded irq handler for the twl6030 interrupt.
+* We query the interrupt controller in the twl6030 to determine
+* which module is generating the interrupt request and call
+* handle_nested_irq for that module.
+*/
+static irqreturn_t twl6030_irq_thread(int irq, void *data)
 {
-	long irq = (long)data;
-	static unsigned i2c_errors;
-	static const unsigned max_i2c_errors = 100;
-	int ret;
-
-	while (!kthread_should_stop()) {
-		int i;
-		union {
+	int i, ret;
+	union {
 		u8 bytes[4];
 		u32 int_sts;
-		} sts;
-
-		/* Wait for IRQ, then read PIH irq status (also blocking) */
-		wait_for_completion_interruptible(&irq_event);
-
-		/* read INT_STS_A, B and C in one shot using a burst read */
-		ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
-				REG_INT_STS_A, 3);
-		if (ret) {
-			pr_warning("twl6030: I2C error %d reading PIH ISR\n",
-					ret);
-			if (++i2c_errors >= max_i2c_errors) {
-				printk(KERN_ERR "Maximum I2C error count"
-						" exceeded.  Terminating %s.\n",
-						__func__);
-				break;
-			}
-			complete(&irq_event);
-			continue;
-		}
-
+	} sts;
 
+	/* read INT_STS_A, B and C in one shot using a burst read */
+	ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes, REG_INT_STS_A, 3);
+	if (ret) {
+		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);
+		return IRQ_HANDLED;
+	}
 
-		sts.bytes[3] = 0; /* Only 24 bits are valid*/
+	sts.bytes[3] = 0; /* Only 24 bits are valid*/
 
-		/*
-		 * Since VBUS status bit is not reliable for VBUS disconnect
-		 * use CHARGER VBUS detection status bit instead.
-		 */
-		if (sts.bytes[2] & 0x10)
-			sts.bytes[2] |= 0x08;
+	/*
+	 * Since VBUS status bit is not reliable for VBUS disconnect
+	 * use CHARGER VBUS detection status bit instead.
+	 */
+	if (sts.bytes[2] & 0x10)
+		sts.bytes[2] |= 0x08;
 
-		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
-			local_irq_disable();
-			if (sts.int_sts & 0x1) {
-				int module_irq = twl6030_irq_base +
+	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
+		if (sts.int_sts & 0x1) {
+			int module_irq = twl6030_irq_base +
 					twl6030_interrupt_mapping[i];
-				generic_handle_irq(module_irq);
-
-			}
-		local_irq_enable();
+			handle_nested_irq(module_irq);
+			pr_debug("%s: PIH ISR %u, virq%u\n",
+				 __func__, i, module_irq);
 		}
 
-		/*
-		 * NOTE:
-		 * Simulation confirms that documentation is wrong w.r.t the
-		 * interrupt status clear operation. A single *byte* write to
-		 * any one of STS_A to STS_C register results in all three
-		 * STS registers being reset. Since it does not matter which
-		 * value is written, all three registers are cleared on a
-		 * single byte write, so we just use 0x0 to clear.
-		 */
-		ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
-		if (ret)
-			pr_warning("twl6030: I2C error in clearing PIH ISR\n");
-
-		enable_irq(irq);
-	}
-
-	return 0;
-}
+	/*
+	 * NOTE:
+	 * Simulation confirms that documentation is wrong w.r.t the
+	 * interrupt status clear operation. A single *byte* write to
+	 * any one of STS_A to STS_C register results in all three
+	 * STS registers being reset. Since it does not matter which
+	 * value is written, all three registers are cleared on a
+	 * single byte write, so we just use 0x0 to clear.
+	 */
+	ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
+	if (ret)
+		pr_warn("twl6030: I2C error in clearing PIH ISR\n");
 
-/*
- * handle_twl6030_int() is the desc->handle method for the twl6030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
- * Now we need to query the interrupt controller in the twl6030 to determine
- * which module is generating the interrupt request.  However, we can't do i2c
- * transactions in interrupt context, so we must defer that work to a kernel
- * thread.  All we do here is acknowledge and mask the interrupt and wakeup
- * the kernel thread.
- */
-static irqreturn_t handle_twl6030_pih(int irq, void *devid)
-{
-	disable_irq_nosync(irq);
-	complete(devid);
 	return IRQ_HANDLED;
 }
 
@@ -351,7 +312,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 {
 	struct			device_node *node = dev->of_node;
 	int			nr_irqs, irq_base, irq_end;
-	struct task_struct	*task;
 	static struct irq_chip  twl6030_irq_chip;
 	int			status = 0;
 	int			i;
@@ -396,36 +356,25 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 		irq_set_chip_and_handler(i, &twl6030_irq_chip,
 					 handle_simple_irq);
 		irq_set_chip_data(i, (void *)irq_num);
+		irq_set_nested_thread(i, true);
 		activate_irq(i);
 	}
 
-	dev_info(dev, "PIH (irq %d) chaining IRQs %d..%d\n",
-			irq_num, irq_base, irq_end);
+	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
+		 irq_num, irq_base, irq_end);
 
 	/* install an irq handler to demultiplex the TWL6030 interrupt */
-	init_completion(&irq_event);
-
-	status = request_irq(irq_num, handle_twl6030_pih, 0, "TWL6030-PIH",
-			     &irq_event);
+	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
+				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
 	if (status < 0) {
 		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
 		goto fail_irq;
 	}
 
-	task = kthread_run(twl6030_irq_thread, (void *)irq_num, "twl6030-irq");
-	if (IS_ERR(task)) {
-		dev_err(dev, "could not create irq %d thread!\n", irq_num);
-		status = PTR_ERR(task);
-		goto fail_kthread;
-	}
-
 	twl_irq = irq_num;
 	register_pm_notifier(&twl6030_irq_pm_notifier_block);
 	return irq_base;
 
-fail_kthread:
-	free_irq(irq_num, &irq_event);
-
 fail_irq:
 	for (i = irq_base; i < irq_end; i++)
 		irq_set_chip_and_handler(i, NULL, NULL);
@@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
 {
 	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
 
-	if (twl6030_irq_base) {
+	if (!twl6030_irq_base) {
 		pr_err("twl6030: can't yet clean up IRQs?\n");
 		return -ENOSYS;
 	}
+
+	free_irq(twl_irq, NULL);
+
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 2/4] mfd: twl6030-irq: add error check when IRQs are masked initially

[Grygorii Strashko]

Add a missed check for errors when TWL IRQs are masked
initially on probe and report an error in case of failure.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/mfd/twl6030-irq.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index b6030d9..790cc28 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -313,7 +313,7 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 	struct			device_node *node = dev->of_node;
 	int			nr_irqs, irq_base, irq_end;
 	static struct irq_chip  twl6030_irq_chip;
-	int			status = 0;
+	int			status;
 	int			i;
 	u8			mask[3];
 
@@ -335,11 +335,16 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 	mask[2] = 0xFF;
 
 	/* mask all int lines */
-	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
+	status = twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
 	/* mask all int sts */
-	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
+	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
 	/* clear INT_STS_A,B,C */
-	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
+	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
+
+	if (status < 0) {
+		dev_err(dev, "I2C err writing TWL_MODULE_PIH: %d\n", status);
+		return status;
+	}
 
 	twl6030_irq_base = irq_base;
 
-- 
1.7.9.5

[PATCH 3/4] mfd: twl6030-irq: convert to use linear irq_domain

[Grygorii Strashko]

Since the TWL6030 PMIC is used with OMAP4 SoCs only and OMAP4 legacy
boot is dropped there are no needs to allocate the range of IRQ
descriptors during system boot to support TWL6030 IRQs.

Hence, convert it to use linear irq_domain and move IRQ configuration in
.map()/.unmap() callbacks of irq_domain. So, IRQ mapping and descriptors
allocation will be performed dynamically basing on DT configuration.

The error message will be reported in case if unmapped IRQ is received by
TWL6030 (virq==0).

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/mfd/twl6030-irq.c |  114 ++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 790cc28..89f130b 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -138,6 +138,7 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
 static irqreturn_t twl6030_irq_thread(int irq, void *data)
 {
 	int i, ret;
+	struct irq_domain *irq_domain = (struct irq_domain *)data;
 	union {
 		u8 bytes[4];
 		u32 int_sts;
@@ -161,9 +162,14 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
 
 	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
 		if (sts.int_sts & 0x1) {
-			int module_irq = twl6030_irq_base +
-					twl6030_interrupt_mapping[i];
-			handle_nested_irq(module_irq);
+			int module_irq =
+				irq_find_mapping(irq_domain,
+						 twl6030_interrupt_mapping[i]);
+			if (module_irq)
+				handle_nested_irq(module_irq);
+			else
+				pr_err("%s: Unmapped PIH ISR %u detected\n",
+				       __func__, i);
 			pr_debug("%s: PIH ISR %u, virq%u\n",
 				 __func__, i, module_irq);
 		}
@@ -186,19 +192,6 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
 
 /*----------------------------------------------------------------------*/
 
-static inline void activate_irq(int irq)
-{
-#ifdef CONFIG_ARM
-	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
-	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
-	 */
-	set_irq_flags(irq, IRQF_VALID);
-#else
-	/* same effect on other architectures */
-	irq_set_noprobe(irq);
-#endif
-}
-
 static int twl6030_irq_set_wake(struct irq_data *d, unsigned int on)
 {
 	if (on)
@@ -308,28 +301,54 @@ int twl6030_mmc_card_detect(struct device *dev, int slot)
 }
 EXPORT_SYMBOL(twl6030_mmc_card_detect);
 
+static struct irq_chip twl6030_irq_chip;
+
+static int twl6030_irq_map(struct irq_domain *d, unsigned int virq,
+			      irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(virq, &twl6030_irq_chip);
+	irq_set_chip_and_handler(virq,  &twl6030_irq_chip, handle_simple_irq);
+	irq_set_nested_thread(virq, true);
+
+#ifdef CONFIG_ARM
+	/*
+	 * ARM requires an extra step to clear IRQ_NOREQUEST, which it
+	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
+	 */
+	set_irq_flags(virq, IRQF_VALID);
+#else
+	/* same effect on other architectures */
+	irq_set_noprobe(virq);
+#endif
+
+	return 0;
+}
+
+static void twl6030_irq_unmap(struct irq_domain *d, unsigned int virq)
+{
+#ifdef CONFIG_ARM
+	set_irq_flags(virq, 0);
+#endif
+	irq_set_chip_and_handler(virq, NULL, NULL);
+	irq_set_chip_data(virq, NULL);
+}
+
+static struct irq_domain_ops twl6030_irq_domain_ops = {
+	.map	= twl6030_irq_map,
+	.unmap	= twl6030_irq_unmap,
+	.xlate	= irq_domain_xlate_onetwocell,
+};
+
 int twl6030_init_irq(struct device *dev, int irq_num)
 {
 	struct			device_node *node = dev->of_node;
-	int			nr_irqs, irq_base, irq_end;
-	static struct irq_chip  twl6030_irq_chip;
+	int			nr_irqs;
 	int			status;
-	int			i;
 	u8			mask[3];
+	struct irq_domain	*irq_domain;
 
 	nr_irqs = TWL6030_NR_IRQS;
 
-	irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
-	if (IS_ERR_VALUE(irq_base)) {
-		dev_err(dev, "Fail to allocate IRQ descs\n");
-		return irq_base;
-	}
-
-	irq_domain_add_legacy(node, nr_irqs, irq_base, 0,
-			      &irq_domain_simple_ops, NULL);
-
-	irq_end = irq_base + nr_irqs;
-
 	mask[0] = 0xFF;
 	mask[1] = 0xFF;
 	mask[2] = 0xFF;
@@ -346,8 +365,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 		return status;
 	}
 
-	twl6030_irq_base = irq_base;
-
 	/*
 	 * install an irq handler for each of the modules;
 	 * clone dummy irq_chip since PIH can't *do* anything
@@ -357,20 +374,18 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 	twl6030_irq_chip.irq_set_type = NULL;
 	twl6030_irq_chip.irq_set_wake = twl6030_irq_set_wake;
 
-	for (i = irq_base; i < irq_end; i++) {
-		irq_set_chip_and_handler(i, &twl6030_irq_chip,
-					 handle_simple_irq);
-		irq_set_chip_data(i, (void *)irq_num);
-		irq_set_nested_thread(i, true);
-		activate_irq(i);
+	irq_domain = irq_domain_add_linear(node, nr_irqs,
+					   &twl6030_irq_domain_ops, NULL);
+	if (!irq_domain) {
+		dev_err(dev, "Can't add irq_domain\n");
+		return -ENOMEM;
 	}
 
-	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
-		 irq_num, irq_base, irq_end);
+	dev_info(dev, "PIH (irq %d) nested IRQs\n", irq_num);
 
 	/* install an irq handler to demultiplex the TWL6030 interrupt */
 	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
-				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
+				      IRQF_ONESHOT, "TWL6030-PIH", irq_domain);
 	if (status < 0) {
 		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
 		goto fail_irq;
@@ -378,26 +393,19 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 
 	twl_irq = irq_num;
 	register_pm_notifier(&twl6030_irq_pm_notifier_block);
-	return irq_base;
+	return irq_num;
 
 fail_irq:
-	for (i = irq_base; i < irq_end; i++)
-		irq_set_chip_and_handler(i, NULL, NULL);
-
+	irq_domain_remove(irq_domain);
 	return status;
 }
 
 int twl6030_exit_irq(void)
 {
-	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
-
-	if (!twl6030_irq_base) {
-		pr_err("twl6030: can't yet clean up IRQs?\n");
-		return -ENOSYS;
+	if (twl_irq) {
+		unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
+		free_irq(twl_irq, NULL);
 	}
-
-	free_irq(twl_irq, NULL);
-
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 4/4] mfd: twl6030-irq: Add interrupt mapping table for the twl6032

[Grygorii Strashko]

From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>

This patch adds interrupt mapping table for the twl6032.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/mfd/twl6030-irq.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index 89f130b..e4df87f 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -41,6 +41,7 @@
 #include <linux/suspend.h>
 #include <linux/of.h>
 #include <linux/irqdomain.h>
+#include <linux/of_device.h>
 
 #include "twl-core.h"
 
@@ -84,6 +85,36 @@ static int twl6030_interrupt_mapping[24] = {
 	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/
 	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
 };
+
+static int twl6032_interrupt_mapping[24] = {
+	PWR_INTR_OFFSET,	/* Bit 0	PWRON			*/
+	PWR_INTR_OFFSET,	/* Bit 1	RPWRON			*/
+	PWR_INTR_OFFSET,	/* Bit 2	SYS_VLOW		*/
+	RTC_INTR_OFFSET,	/* Bit 3	RTC_ALARM		*/
+	RTC_INTR_OFFSET,	/* Bit 4	RTC_PERIOD		*/
+	HOTDIE_INTR_OFFSET,	/* Bit 5	HOT_DIE			*/
+	SMPSLDO_INTR_OFFSET,	/* Bit 6	VXXX_SHORT		*/
+	PWR_INTR_OFFSET,	/* Bit 7	SPDURATION		*/
+
+	PWR_INTR_OFFSET,	/* Bit 8	WATCHDOG		*/
+	BATDETECT_INTR_OFFSET,	/* Bit 9	BAT			*/
+	SIMDETECT_INTR_OFFSET,	/* Bit 10	SIM			*/
+	MMCDETECT_INTR_OFFSET,	/* Bit 11	MMC			*/
+	MADC_INTR_OFFSET,	/* Bit 12	GPADC_RT_EOC		*/
+	MADC_INTR_OFFSET,	/* Bit 13	GPADC_SW_EOC		*/
+	GASGAUGE_INTR_OFFSET,	/* Bit 14	CC_EOC			*/
+	GASGAUGE_INTR_OFFSET,	/* Bit 15	CC_AUTOCAL		*/
+
+	USBOTG_INTR_OFFSET,	/* Bit 16	ID_WKUP			*/
+	USBOTG_INTR_OFFSET,	/* Bit 17	VBUS_WKUP		*/
+	USBOTG_INTR_OFFSET,	/* Bit 18	ID			*/
+	USB_PRES_INTR_OFFSET,	/* Bit 19	VBUS			*/
+	CHARGER_INTR_OFFSET,	/* Bit 20	CHRG_CTRL		*/
+	CHARGERFAULT_INTR_OFFSET,	/* Bit 21	EXT_CHRG	*/
+	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/
+	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
+};
+
 /*----------------------------------------------------------------------*/
 
 static unsigned twl6030_irq_base;
@@ -91,6 +122,7 @@ static int twl_irq;
 static bool twl_irq_wake_enabled;
 
 static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
+static const int *irq_mapping_tbl;
 
 static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
 				   unsigned long pm_event, void *unused)
@@ -164,7 +196,7 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
 		if (sts.int_sts & 0x1) {
 			int module_irq =
 				irq_find_mapping(irq_domain,
-						 twl6030_interrupt_mapping[i]);
+						 irq_mapping_tbl[i]);
 			if (module_irq)
 				handle_nested_irq(module_irq);
 			else
@@ -339,6 +371,12 @@ static struct irq_domain_ops twl6030_irq_domain_ops = {
 	.xlate	= irq_domain_xlate_onetwocell,
 };
 
+static const struct of_device_id twl6030_of_match[] = {
+	{.compatible = "ti,twl6030", &twl6030_interrupt_mapping},
+	{.compatible = "ti,twl6032", &twl6032_interrupt_mapping},
+	{ },
+};
+
 int twl6030_init_irq(struct device *dev, int irq_num)
 {
 	struct			device_node *node = dev->of_node;
@@ -346,6 +384,15 @@ int twl6030_init_irq(struct device *dev, int irq_num)
 	int			status;
 	u8			mask[3];
 	struct irq_domain	*irq_domain;
+	const struct of_device_id *of_id;
+
+	of_id = of_match_device(twl6030_of_match, dev);
+	if (!of_id || !of_id->data) {
+		dev_err(dev, "Unknown TWL device model\n");
+		return -EINVAL;
+	}
+
+	irq_mapping_tbl = of_id->data;
 
 	nr_irqs = TWL6030_NR_IRQS;
 
-- 
1.7.9.5

Re: [PATCH 2/4] mfd: twl6030-irq: add error check when IRQs are masked initially

[Graeme Gregory]

On 23/07/13 17:07, Grygorii Strashko wrote:
> Add a missed check for errors when TWL IRQs are masked
> initially on probe and report an error in case of failure.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/mfd/twl6030-irq.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index b6030d9..790cc28 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -313,7 +313,7 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  	struct			device_node *node = dev->of_node;
>  	int			nr_irqs, irq_base, irq_end;
>  	static struct irq_chip  twl6030_irq_chip;
> -	int			status = 0;
> +	int			status;
>  	int			i;
>  	u8			mask[3];
>  
> @@ -335,11 +335,16 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  	mask[2] = 0xFF;
>  
>  	/* mask all int lines */
> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
> +	status = twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
>  	/* mask all int sts */
> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
> +	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
>  	/* clear INT_STS_A,B,C */
> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
> +	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
> +
You can save two i2c writes here for slightly faster initialisation,
only one of the REG_INT_STS_A registers needs to be written to clear
them all. As per the irq handling routine comment.
> +	if (status < 0) {
> +		dev_err(dev, "I2C err writing TWL_MODULE_PIH: %d\n", status);
> +		return status;
> +	}
>  
>  	twl6030_irq_base = irq_base;
>  

Graeme

Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Lee Jones]

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> From: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
> 
> 1) Removed request_irq() and replaced it with request_threaded_irq().
> 
> 2) Removed generic_handle_irq() and replaced it with
> handle_nested_irq().
>   Handling of these interrupts is nested, as we are handling an
> interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
> 
> 3) Removed I2C read-retry logic for the case when twl_i2c_read() is
> failed inside IRQ handler - there is no sense to do that, so just report
> an error and return.
> 
> Signed-off-by: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
> Signed-off-by: Oleg_Kosheliev <oleg.kosheliev@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/mfd/twl6030-irq.c |  146 +++++++++++++++------------------------------
>  1 file changed, 49 insertions(+), 97 deletions(-)

Besides the points I mention below I like the way this patch is
going.

> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index 277a8db..b6030d9 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
>  static int twl_irq;
>  static bool twl_irq_wake_enabled;
>  
> -static struct completion irq_event;
>  static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
>  
>  static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
> @@ -131,95 +130,57 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>  };
>  
>  /*
> - * This thread processes interrupts reported by the Primary Interrupt Handler.
> - */
> -static int twl6030_irq_thread(void *data)
> +* Threaded irq handler for the twl6030 interrupt.
> +* We query the interrupt controller in the twl6030 to determine
> +* which module is generating the interrupt request and call
> +* handle_nested_irq for that module.
> +*/
> +static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  {
> -	long irq = (long)data;
> -	static unsigned i2c_errors;
> -	static const unsigned max_i2c_errors = 100;
> -	int ret;
> -
> -	while (!kthread_should_stop()) {
> -		int i;
> -		union {
> +	int i, ret;
> +	union {
>  		u8 bytes[4];
>  		u32 int_sts;
> -		} sts;
> -
> -		/* Wait for IRQ, then read PIH irq status (also blocking) */
> -		wait_for_completion_interruptible(&irq_event);
> -
> -		/* read INT_STS_A, B and C in one shot using a burst read */
> -		ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
> -				REG_INT_STS_A, 3);
> -		if (ret) {
> -			pr_warning("twl6030: I2C error %d reading PIH ISR\n",
> -					ret);
> -			if (++i2c_errors >= max_i2c_errors) {
> -				printk(KERN_ERR "Maximum I2C error count"
> -						" exceeded.  Terminating %s.\n",
> -						__func__);
> -				break;
> -			}
> -			complete(&irq_event);
> -			continue;
> -		}
> -
> +	} sts;
>  
> +	/* read INT_STS_A, B and C in one shot using a burst read */
> +	ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes, REG_INT_STS_A, 3);
> +	if (ret) {
> +		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);

Does the user really care which function we're returning from.

Would it be better if you replace '__func__' with the device name?

> +		return IRQ_HANDLED;
> +	}
>  
> -		sts.bytes[3] = 0; /* Only 24 bits are valid*/
> +	sts.bytes[3] = 0; /* Only 24 bits are valid*/
>  
> -		/*
> -		 * Since VBUS status bit is not reliable for VBUS disconnect
> -		 * use CHARGER VBUS detection status bit instead.
> -		 */
> -		if (sts.bytes[2] & 0x10)
> -			sts.bytes[2] |= 0x08;
> +	/*
> +	 * Since VBUS status bit is not reliable for VBUS disconnect
> +	 * use CHARGER VBUS detection status bit instead.
> +	 */
> +	if (sts.bytes[2] & 0x10)
> +		sts.bytes[2] |= 0x08;
>  
> -		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
> -			local_irq_disable();
> -			if (sts.int_sts & 0x1) {
> -				int module_irq = twl6030_irq_base +
> +	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
> +		if (sts.int_sts & 0x1) {

I'm a little confused by this. Where does sts.int_sts come from?

> +			int module_irq = twl6030_irq_base +
>  					twl6030_interrupt_mapping[i];
> -				generic_handle_irq(module_irq);
> -
> -			}
> -		local_irq_enable();
> +			handle_nested_irq(module_irq);
> +			pr_debug("%s: PIH ISR %u, virq%u\n",
> +				 __func__, i, module_irq);
>  		}
>  
> -		/*
> -		 * NOTE:
> -		 * Simulation confirms that documentation is wrong w.r.t the
> -		 * interrupt status clear operation. A single *byte* write to
> -		 * any one of STS_A to STS_C register results in all three
> -		 * STS registers being reset. Since it does not matter which
> -		 * value is written, all three registers are cleared on a
> -		 * single byte write, so we just use 0x0 to clear.
> -		 */
> -		ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
> -		if (ret)
> -			pr_warning("twl6030: I2C error in clearing PIH ISR\n");
> -
> -		enable_irq(irq);
> -	}
> -
> -	return 0;
> -}
> +	/*
> +	 * NOTE:
> +	 * Simulation confirms that documentation is wrong w.r.t the
> +	 * interrupt status clear operation. A single *byte* write to
> +	 * any one of STS_A to STS_C register results in all three
> +	 * STS registers being reset. Since it does not matter which
> +	 * value is written, all three registers are cleared on a
> +	 * single byte write, so we just use 0x0 to clear.
> +	 */
> +	ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
> +	if (ret)
> +		pr_warn("twl6030: I2C error in clearing PIH ISR\n");
>  
> -/*
> - * handle_twl6030_int() is the desc->handle method for the twl6030 interrupt.
> - * This is a chained interrupt, so there is no desc->action method for it.
> - * Now we need to query the interrupt controller in the twl6030 to determine
> - * which module is generating the interrupt request.  However, we can't do i2c
> - * transactions in interrupt context, so we must defer that work to a kernel
> - * thread.  All we do here is acknowledge and mask the interrupt and wakeup
> - * the kernel thread.
> - */
> -static irqreturn_t handle_twl6030_pih(int irq, void *devid)
> -{
> -	disable_irq_nosync(irq);
> -	complete(devid);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -351,7 +312,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  {
>  	struct			device_node *node = dev->of_node;
>  	int			nr_irqs, irq_base, irq_end;
> -	struct task_struct	*task;
>  	static struct irq_chip  twl6030_irq_chip;
>  	int			status = 0;
>  	int			i;
> @@ -396,36 +356,25 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  		irq_set_chip_and_handler(i, &twl6030_irq_chip,
>  					 handle_simple_irq);
>  		irq_set_chip_data(i, (void *)irq_num);
> +		irq_set_nested_thread(i, true);
>  		activate_irq(i);
>  	}
>  
> -	dev_info(dev, "PIH (irq %d) chaining IRQs %d..%d\n",
> -			irq_num, irq_base, irq_end);
> +	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
> +		 irq_num, irq_base, irq_end);
>  
>  	/* install an irq handler to demultiplex the TWL6030 interrupt */
> -	init_completion(&irq_event);
> -
> -	status = request_irq(irq_num, handle_twl6030_pih, 0, "TWL6030-PIH",
> -			     &irq_event);
> +	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
> +				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
>  	if (status < 0) {
>  		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>  		goto fail_irq;
>  	}
>  
> -	task = kthread_run(twl6030_irq_thread, (void *)irq_num, "twl6030-irq");
> -	if (IS_ERR(task)) {
> -		dev_err(dev, "could not create irq %d thread!\n", irq_num);
> -		status = PTR_ERR(task);
> -		goto fail_kthread;
> -	}
> -
>  	twl_irq = irq_num;
>  	register_pm_notifier(&twl6030_irq_pm_notifier_block);
>  	return irq_base;
>  
> -fail_kthread:
> -	free_irq(irq_num, &irq_event);
> -
>  fail_irq:
>  	for (i = irq_base; i < irq_end; i++)
>  		irq_set_chip_and_handler(i, NULL, NULL);
> @@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
>  {
>  	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>  
> -	if (twl6030_irq_base) {
> +	if (!twl6030_irq_base) {
>  		pr_err("twl6030: can't yet clean up IRQs?\n");
>  		return -ENOSYS;
>  	}
> +
> +	free_irq(twl_irq, NULL);
> +

If request_threaded_irq() fails, isn't there a chance that
twl6030_irq_base will be allocated, but twl_irq will still be
undefined?

>  	return 0;
>  }
>  

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4] mfd: twl6030-irq: convert to use linear irq_domain

[Lee Jones]

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> Since the TWL6030 PMIC is used with OMAP4 SoCs only and OMAP4 legacy
> boot is dropped there are no needs to allocate the range of IRQ
> descriptors during system boot to support TWL6030 IRQs.
> 
> Hence, convert it to use linear irq_domain and move IRQ configuration in
> .map()/.unmap() callbacks of irq_domain. So, IRQ mapping and descriptors
> allocation will be performed dynamically basing on DT configuration.
> 
> The error message will be reported in case if unmapped IRQ is received by
> TWL6030 (virq==0).
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/mfd/twl6030-irq.c |  114 ++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index 790cc28..89f130b 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -138,6 +138,7 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>  static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  {
>  	int i, ret;
> +	struct irq_domain *irq_domain = (struct irq_domain *)data;
>  	union {
>  		u8 bytes[4];
>  		u32 int_sts;
> @@ -161,9 +162,14 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  
>  	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>  		if (sts.int_sts & 0x1) {
> -			int module_irq = twl6030_irq_base +
> -					twl6030_interrupt_mapping[i];
> -			handle_nested_irq(module_irq);
> +			int module_irq =
> +				irq_find_mapping(irq_domain,
> +						 twl6030_interrupt_mapping[i]);
> +			if (module_irq)
> +				handle_nested_irq(module_irq);
> +			else
> +				pr_err("%s: Unmapped PIH ISR %u detected\n",
> +				       __func__, i);

Same here. Does the user really care which function failed?

Please consider replacing with the device name.

>  			pr_debug("%s: PIH ISR %u, virq%u\n",
>  				 __func__, i, module_irq);
>  		}
> @@ -186,19 +192,6 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  
>  /*----------------------------------------------------------------------*/
>  
> -static inline void activate_irq(int irq)
> -{
> -#ifdef CONFIG_ARM
> -	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
> -	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
> -	 */
> -	set_irq_flags(irq, IRQF_VALID);
> -#else
> -	/* same effect on other architectures */
> -	irq_set_noprobe(irq);
> -#endif
> -}
> -
>  static int twl6030_irq_set_wake(struct irq_data *d, unsigned int on)
>  {
>  	if (on)
> @@ -308,28 +301,54 @@ int twl6030_mmc_card_detect(struct device *dev, int slot)
>  }
>  EXPORT_SYMBOL(twl6030_mmc_card_detect);
>  
> +static struct irq_chip twl6030_irq_chip;
> +
> +static int twl6030_irq_map(struct irq_domain *d, unsigned int virq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(virq, &twl6030_irq_chip);
> +	irq_set_chip_and_handler(virq,  &twl6030_irq_chip, handle_simple_irq);
> +	irq_set_nested_thread(virq, true);
> +
> +#ifdef CONFIG_ARM
> +	/*
> +	 * ARM requires an extra step to clear IRQ_NOREQUEST, which it
> +	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
> +	 */
> +	set_irq_flags(virq, IRQF_VALID);
> +#else
> +	/* same effect on other architectures */
> +	irq_set_noprobe(virq);
> +#endif
> +
> +	return 0;
> +}
> +
> +static void twl6030_irq_unmap(struct irq_domain *d, unsigned int virq)
> +{
> +#ifdef CONFIG_ARM
> +	set_irq_flags(virq, 0);
> +#endif
> +	irq_set_chip_and_handler(virq, NULL, NULL);
> +	irq_set_chip_data(virq, NULL);
> +}
> +
> +static struct irq_domain_ops twl6030_irq_domain_ops = {
> +	.map	= twl6030_irq_map,
> +	.unmap	= twl6030_irq_unmap,
> +	.xlate	= irq_domain_xlate_onetwocell,
> +};
> +
>  int twl6030_init_irq(struct device *dev, int irq_num)
>  {
>  	struct			device_node *node = dev->of_node;
> -	int			nr_irqs, irq_base, irq_end;
> -	static struct irq_chip  twl6030_irq_chip;
> +	int			nr_irqs;
>  	int			status;
> -	int			i;
>  	u8			mask[3];
> +	struct irq_domain	*irq_domain;
>  
>  	nr_irqs = TWL6030_NR_IRQS;
>  
> -	irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
> -	if (IS_ERR_VALUE(irq_base)) {
> -		dev_err(dev, "Fail to allocate IRQ descs\n");
> -		return irq_base;
> -	}
> -
> -	irq_domain_add_legacy(node, nr_irqs, irq_base, 0,
> -			      &irq_domain_simple_ops, NULL);
> -
> -	irq_end = irq_base + nr_irqs;
> -
>  	mask[0] = 0xFF;
>  	mask[1] = 0xFF;
>  	mask[2] = 0xFF;
> @@ -346,8 +365,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  		return status;
>  	}
>  
> -	twl6030_irq_base = irq_base;
> -
>  	/*
>  	 * install an irq handler for each of the modules;
>  	 * clone dummy irq_chip since PIH can't *do* anything
> @@ -357,20 +374,18 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  	twl6030_irq_chip.irq_set_type = NULL;
>  	twl6030_irq_chip.irq_set_wake = twl6030_irq_set_wake;
>  
> -	for (i = irq_base; i < irq_end; i++) {
> -		irq_set_chip_and_handler(i, &twl6030_irq_chip,
> -					 handle_simple_irq);
> -		irq_set_chip_data(i, (void *)irq_num);
> -		irq_set_nested_thread(i, true);
> -		activate_irq(i);
> +	irq_domain = irq_domain_add_linear(node, nr_irqs,
> +					   &twl6030_irq_domain_ops, NULL);
> +	if (!irq_domain) {
> +		dev_err(dev, "Can't add irq_domain\n");
> +		return -ENOMEM;
>  	}
>  
> -	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
> -		 irq_num, irq_base, irq_end);
> +	dev_info(dev, "PIH (irq %d) nested IRQs\n", irq_num);
>  
>  	/* install an irq handler to demultiplex the TWL6030 interrupt */
>  	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
> -				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
> +				      IRQF_ONESHOT, "TWL6030-PIH", irq_domain);
>  	if (status < 0) {
>  		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>  		goto fail_irq;
> @@ -378,26 +393,19 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  
>  	twl_irq = irq_num;
>  	register_pm_notifier(&twl6030_irq_pm_notifier_block);
> -	return irq_base;
> +	return irq_num;

I think you need to change twl-core to now expect the total number of
IRQs rather than the base one now.

>  fail_irq:
> -	for (i = irq_base; i < irq_end; i++)
> -		irq_set_chip_and_handler(i, NULL, NULL);
> -
> +	irq_domain_remove(irq_domain);

Why do you kill the irqdomain here, but not in exit()?

>  	return status;
>  }
>  
>  int twl6030_exit_irq(void)
>  {
> -	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
> -
> -	if (!twl6030_irq_base) {
> -		pr_err("twl6030: can't yet clean up IRQs?\n");
> -		return -ENOSYS;
> +	if (twl_irq) {
> +		unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
> +		free_irq(twl_irq, NULL);
>  	}

Ah yes, that's better.

> -
> -	free_irq(twl_irq, NULL);
> -
>  	return 0;
>  }
>  

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] mfd: twl6030-irq: add error check when IRQs are masked initially

[Grygorii Strashko]

On 07/23/2013 09:08 PM, Graeme Gregory wrote:
> On 23/07/13 17:07, Grygorii Strashko wrote:
>> Add a missed check for errors when TWL IRQs are masked
>> initially on probe and report an error in case of failure.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/mfd/twl6030-irq.c |   13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
>> index b6030d9..790cc28 100644
>> --- a/drivers/mfd/twl6030-irq.c
>> +++ b/drivers/mfd/twl6030-irq.c
>> @@ -313,7 +313,7 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   	struct			device_node *node = dev->of_node;
>>   	int			nr_irqs, irq_base, irq_end;
>>   	static struct irq_chip  twl6030_irq_chip;
>> -	int			status = 0;
>> +	int			status;
>>   	int			i;
>>   	u8			mask[3];
>>
>> @@ -335,11 +335,16 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   	mask[2] = 0xFF;
>>
>>   	/* mask all int lines */
>> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
>> +	status = twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_LINE_A, 3);
>>   	/* mask all int sts */
>> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
>> +	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_MSK_STS_A, 3);
>>   	/* clear INT_STS_A,B,C */
>> -	twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
>> +	status |= twl_i2c_write(TWL_MODULE_PIH, &mask[0], REG_INT_STS_A, 3);
>> +
> You can save two i2c writes here for slightly faster initialisation,
> only one of the REG_INT_STS_A registers needs to be written to clear
> them all. As per the irq handling routine comment.

Good point. thanks

>> +	if (status < 0) {
>> +		dev_err(dev, "I2C err writing TWL_MODULE_PIH: %d\n", status);
>> +		return status;
>> +	}
>>
>>   	twl6030_irq_base = irq_base;
>>
>
> Graeme
>

Regards,
- grygorii

Re: [PATCH 4/4] mfd: twl6030-irq: Add interrupt mapping table for the twl6032

[Lee Jones]

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>
> 
> This patch adds interrupt mapping table for the twl6032.

Repeating the $SUBJECT line is never helpful.

> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/mfd/twl6030-irq.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index 89f130b..e4df87f 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -41,6 +41,7 @@
>  #include <linux/suspend.h>
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
> +#include <linux/of_device.h>
>  
>  #include "twl-core.h"
>  
> @@ -84,6 +85,36 @@ static int twl6030_interrupt_mapping[24] = {
>  	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/
>  	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
>  };
> +
> +static int twl6032_interrupt_mapping[24] = {
> +	PWR_INTR_OFFSET,	/* Bit 0	PWRON			*/
> +	PWR_INTR_OFFSET,	/* Bit 1	RPWRON			*/
> +	PWR_INTR_OFFSET,	/* Bit 2	SYS_VLOW		*/
> +	RTC_INTR_OFFSET,	/* Bit 3	RTC_ALARM		*/
> +	RTC_INTR_OFFSET,	/* Bit 4	RTC_PERIOD		*/
> +	HOTDIE_INTR_OFFSET,	/* Bit 5	HOT_DIE			*/
> +	SMPSLDO_INTR_OFFSET,	/* Bit 6	VXXX_SHORT		*/
> +	PWR_INTR_OFFSET,	/* Bit 7	SPDURATION		*/
> +
> +	PWR_INTR_OFFSET,	/* Bit 8	WATCHDOG		*/
> +	BATDETECT_INTR_OFFSET,	/* Bit 9	BAT			*/
> +	SIMDETECT_INTR_OFFSET,	/* Bit 10	SIM			*/
> +	MMCDETECT_INTR_OFFSET,	/* Bit 11	MMC			*/
> +	MADC_INTR_OFFSET,	/* Bit 12	GPADC_RT_EOC		*/
> +	MADC_INTR_OFFSET,	/* Bit 13	GPADC_SW_EOC		*/
> +	GASGAUGE_INTR_OFFSET,	/* Bit 14	CC_EOC			*/
> +	GASGAUGE_INTR_OFFSET,	/* Bit 15	CC_AUTOCAL		*/
> +
> +	USBOTG_INTR_OFFSET,	/* Bit 16	ID_WKUP			*/
> +	USBOTG_INTR_OFFSET,	/* Bit 17	VBUS_WKUP		*/
> +	USBOTG_INTR_OFFSET,	/* Bit 18	ID			*/
> +	USB_PRES_INTR_OFFSET,	/* Bit 19	VBUS			*/
> +	CHARGER_INTR_OFFSET,	/* Bit 20	CHRG_CTRL		*/
> +	CHARGERFAULT_INTR_OFFSET,	/* Bit 21	EXT_CHRG	*/
> +	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/

OCD failure. ;)

NB: Kidding, you don't have to do anything about this.

> +	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
> +};
> +
>  /*----------------------------------------------------------------------*/
>  
>  static unsigned twl6030_irq_base;
> @@ -91,6 +122,7 @@ static int twl_irq;
>  static bool twl_irq_wake_enabled;
>  
>  static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
> +static const int *irq_mapping_tbl;

What I'd actually like to see is the creation of 'struct twl6030' to
keep all your goodies in; irq_domain, irq_mapping_tbl etc and for you
to pass that around instead of creating more global variables e.g. via
request_threaded_irq(..., void *dev_id) to access the aforementioned
information.

>  static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
>  				   unsigned long pm_event, void *unused)
> @@ -164,7 +196,7 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  		if (sts.int_sts & 0x1) {
>  			int module_irq =
>  				irq_find_mapping(irq_domain,
> -						 twl6030_interrupt_mapping[i]);
> +						 irq_mapping_tbl[i]);
>  			if (module_irq)
>  				handle_nested_irq(module_irq);
>  			else
> @@ -339,6 +371,12 @@ static struct irq_domain_ops twl6030_irq_domain_ops = {
>  	.xlate	= irq_domain_xlate_onetwocell,
>  };
>  
> +static const struct of_device_id twl6030_of_match[] = {
> +	{.compatible = "ti,twl6030", &twl6030_interrupt_mapping},
> +	{.compatible = "ti,twl6032", &twl6032_interrupt_mapping},
> +	{ },
> +};
> +
>  int twl6030_init_irq(struct device *dev, int irq_num)
>  {
>  	struct			device_node *node = dev->of_node;
> @@ -346,6 +384,15 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  	int			status;
>  	u8			mask[3];
>  	struct irq_domain	*irq_domain;
> +	const struct of_device_id *of_id;
> +
> +	of_id = of_match_device(twl6030_of_match, dev);
> +	if (!of_id || !of_id->data) {
> +		dev_err(dev, "Unknown TWL device model\n");
> +		return -EINVAL;
> +	}
> +
> +	irq_mapping_tbl = of_id->data;
>  
>  	nr_irqs = TWL6030_NR_IRQS;
>  

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Lee Jones]

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> From: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
> 
> 1) Removed request_irq() and replaced it with request_threaded_irq().
> 
> 2) Removed generic_handle_irq() and replaced it with
> handle_nested_irq().
>   Handling of these interrupts is nested, as we are handling an
> interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
> 
> 3) Removed I2C read-retry logic for the case when twl_i2c_read() is
> failed inside IRQ handler - there is no sense to do that, so just report
> an error and return.
> 
> Signed-off-by: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
> Signed-off-by: Oleg_Kosheliev <oleg.kosheliev@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/mfd/twl6030-irq.c |  146 +++++++++++++++------------------------------
>  1 file changed, 49 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c

<snip>

> +	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
> +				      IRQF_ONESHOT, "TWL6030-PIH", NULL);

Oh, and please use managed resources for this: devm_*

>  	if (status < 0) {
>  		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>  		goto fail_irq;
>  	}
>  

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Grygorii Strashko]

On 07/24/2013 01:49 PM, Lee Jones wrote:
> On Tue, 23 Jul 2013, Grygorii Strashko wrote:
>
>> From: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
>>
>> 1) Removed request_irq() and replaced it with request_threaded_irq().
>>
>> 2) Removed generic_handle_irq() and replaced it with
>> handle_nested_irq().
>>    Handling of these interrupts is nested, as we are handling an
>> interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
>>
>> 3) Removed I2C read-retry logic for the case when twl_i2c_read() is
>> failed inside IRQ handler - there is no sense to do that, so just report
>> an error and return.
>>
>> Signed-off-by: Naga Venkata Srikanth V <vnv.srikanth@samsung.com>
>> Signed-off-by: Oleg_Kosheliev <oleg.kosheliev@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/mfd/twl6030-irq.c |  146 +++++++++++++++------------------------------
>>   1 file changed, 49 insertions(+), 97 deletions(-)
>
> Besides the points I mention below I like the way this patch is
> going.
>
>> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
>> index 277a8db..b6030d9 100644
>> --- a/drivers/mfd/twl6030-irq.c
>> +++ b/drivers/mfd/twl6030-irq.c
>> @@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
>>   static int twl_irq;
>>   static bool twl_irq_wake_enabled;
>>
>> -static struct completion irq_event;
>>   static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
>>
>>   static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
>> @@ -131,95 +130,57 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>>   };
>>
>>   /*
>> - * This thread processes interrupts reported by the Primary Interrupt Handler.
>> - */
>> -static int twl6030_irq_thread(void *data)
>> +* Threaded irq handler for the twl6030 interrupt.
>> +* We query the interrupt controller in the twl6030 to determine
>> +* which module is generating the interrupt request and call
>> +* handle_nested_irq for that module.
>> +*/
>> +static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>   {
>> -	long irq = (long)data;
>> -	static unsigned i2c_errors;
>> -	static const unsigned max_i2c_errors = 100;
>> -	int ret;
>> -
>> -	while (!kthread_should_stop()) {
>> -		int i;
>> -		union {
>> +	int i, ret;
>> +	union {
>>   		u8 bytes[4];
>>   		u32 int_sts;
>> -		} sts;
>> -
>> -		/* Wait for IRQ, then read PIH irq status (also blocking) */
>> -		wait_for_completion_interruptible(&irq_event);
>> -
>> -		/* read INT_STS_A, B and C in one shot using a burst read */
>> -		ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
>> -				REG_INT_STS_A, 3);
>> -		if (ret) {
>> -			pr_warning("twl6030: I2C error %d reading PIH ISR\n",
>> -					ret);
>> -			if (++i2c_errors >= max_i2c_errors) {
>> -				printk(KERN_ERR "Maximum I2C error count"
>> -						" exceeded.  Terminating %s.\n",
>> -						__func__);
>> -				break;
>> -			}
>> -			complete(&irq_event);
>> -			continue;
>> -		}
>> -
>> +	} sts;
>>
>> +	/* read INT_STS_A, B and C in one shot using a burst read */
>> +	ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes, REG_INT_STS_A, 3);

sts.int_sts - is filled here

>> +	if (ret) {
>> +		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);
>
> Does the user really care which function we're returning from.
>
> Would it be better if you replace '__func__' with the device name?

This module hasn't been converted to the device yet:(
(I mean "interrupt-controller").
But I'm thinking about it as the next step :) and then It will be 
absolutely reasonable change to replace pr_*() with dev_*() and remove 
__func__.

Now, the pointer on "dev" (in our case "twl-core" device) isn't passed
in IRQ handler, so It can't be used here.

Of course it can be done, but would it make code better?
My opinion - no.

>
>> +		return IRQ_HANDLED;
>> +	}
>>
>> -		sts.bytes[3] = 0; /* Only 24 bits are valid*/
>> +	sts.bytes[3] = 0; /* Only 24 bits are valid*/
>>
>> -		/*
>> -		 * Since VBUS status bit is not reliable for VBUS disconnect
>> -		 * use CHARGER VBUS detection status bit instead.
>> -		 */
>> -		if (sts.bytes[2] & 0x10)
>> -			sts.bytes[2] |= 0x08;
>> +	/*
>> +	 * Since VBUS status bit is not reliable for VBUS disconnect
>> +	 * use CHARGER VBUS detection status bit instead.
>> +	 */
>> +	if (sts.bytes[2] & 0x10)
>> +		sts.bytes[2] |= 0x08;
>>
>> -		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
>> -			local_irq_disable();
>> -			if (sts.int_sts & 0x1) {
>> -				int module_irq = twl6030_irq_base +
>> +	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>> +		if (sts.int_sts & 0x1) {
>
> I'm a little confused by this. Where does sts.int_sts come from?

See my comment above, pls

>
>> +			int module_irq = twl6030_irq_base +
>>   					twl6030_interrupt_mapping[i];
>> -				generic_handle_irq(module_irq);
>> -
>> -			}
>> -		local_irq_enable();
>> +			handle_nested_irq(module_irq);
>> +			pr_debug("%s: PIH ISR %u, virq%u\n",
>> +				 __func__, i, module_irq);
>>   		}
>>
>> -		/*
>> -		 * NOTE:
>> -		 * Simulation confirms that documentation is wrong w.r.t the
>> -		 * interrupt status clear operation. A single *byte* write to
>> -		 * any one of STS_A to STS_C register results in all three
>> -		 * STS registers being reset. Since it does not matter which
>> -		 * value is written, all three registers are cleared on a
>> -		 * single byte write, so we just use 0x0 to clear.
>> -		 */
>> -		ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
>> -		if (ret)
>> -			pr_warning("twl6030: I2C error in clearing PIH ISR\n");
>> -
>> -		enable_irq(irq);
>> -	}
>> -
>> -	return 0;
>> -}
>> +	/*
>> +	 * NOTE:
>> +	 * Simulation confirms that documentation is wrong w.r.t the
>> +	 * interrupt status clear operation. A single *byte* write to
>> +	 * any one of STS_A to STS_C register results in all three
>> +	 * STS registers being reset. Since it does not matter which
>> +	 * value is written, all three registers are cleared on a
>> +	 * single byte write, so we just use 0x0 to clear.
>> +	 */
>> +	ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
>> +	if (ret)
>> +		pr_warn("twl6030: I2C error in clearing PIH ISR\n");
>>
>> -/*
>> - * handle_twl6030_int() is the desc->handle method for the twl6030 interrupt.
>> - * This is a chained interrupt, so there is no desc->action method for it.
>> - * Now we need to query the interrupt controller in the twl6030 to determine
>> - * which module is generating the interrupt request.  However, we can't do i2c
>> - * transactions in interrupt context, so we must defer that work to a kernel
>> - * thread.  All we do here is acknowledge and mask the interrupt and wakeup
>> - * the kernel thread.
>> - */
>> -static irqreturn_t handle_twl6030_pih(int irq, void *devid)
>> -{
>> -	disable_irq_nosync(irq);
>> -	complete(devid);
>>   	return IRQ_HANDLED;
>>   }
>>
>> @@ -351,7 +312,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   {
>>   	struct			device_node *node = dev->of_node;
>>   	int			nr_irqs, irq_base, irq_end;
>> -	struct task_struct	*task;
>>   	static struct irq_chip  twl6030_irq_chip;
>>   	int			status = 0;
>>   	int			i;
>> @@ -396,36 +356,25 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   		irq_set_chip_and_handler(i, &twl6030_irq_chip,
>>   					 handle_simple_irq);
>>   		irq_set_chip_data(i, (void *)irq_num);
>> +		irq_set_nested_thread(i, true);
>>   		activate_irq(i);
>>   	}
>>
>> -	dev_info(dev, "PIH (irq %d) chaining IRQs %d..%d\n",
>> -			irq_num, irq_base, irq_end);
>> +	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
>> +		 irq_num, irq_base, irq_end);
>>
>>   	/* install an irq handler to demultiplex the TWL6030 interrupt */
>> -	init_completion(&irq_event);
>> -
>> -	status = request_irq(irq_num, handle_twl6030_pih, 0, "TWL6030-PIH",
>> -			     &irq_event);
>> +	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
>> +				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
>>   	if (status < 0) {
>>   		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>>   		goto fail_irq;
>>   	}
>>
>> -	task = kthread_run(twl6030_irq_thread, (void *)irq_num, "twl6030-irq");
>> -	if (IS_ERR(task)) {
>> -		dev_err(dev, "could not create irq %d thread!\n", irq_num);
>> -		status = PTR_ERR(task);
>> -		goto fail_kthread;
>> -	}
>> -
>>   	twl_irq = irq_num;
>>   	register_pm_notifier(&twl6030_irq_pm_notifier_block);
>>   	return irq_base;
>>
>> -fail_kthread:
>> -	free_irq(irq_num, &irq_event);
>> -
>>   fail_irq:
>>   	for (i = irq_base; i < irq_end; i++)
>>   		irq_set_chip_and_handler(i, NULL, NULL);
>> @@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
>>   {
>>   	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>>
>> -	if (twl6030_irq_base) {
>> +	if (!twl6030_irq_base) {
>>   		pr_err("twl6030: can't yet clean up IRQs?\n");
>>   		return -ENOSYS;
>>   	}
>> +
>> +	free_irq(twl_irq, NULL);
>> +
>
> If request_threaded_irq() fails, isn't there a chance that
> twl6030_irq_base will be allocated, but twl_irq will still be
> undefined?

Yes. A mess is here (historically:), thanks. Will use twl_irq instead of 
twl6030_irq_base (I did it, actually, in patch [3]:).

>
>>   	return 0;
>>   }
>>
>

Thanks for review.

Regards,
- grygorii

Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Lee Jones]

> >>+	if (ret) {
> >>+		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);
> >
> >Does the user really care which function we're returning from.
> >
> >Would it be better if you replace '__func__' with the device name?
> 
> This module hasn't been converted to the device yet:(
> (I mean "interrupt-controller").
> But I'm thinking about it as the next step :) and then It will be
> absolutely reasonable change to replace pr_*() with dev_*() and
> remove __func__.

I don't mean anything as compicated as that for 'this' patch. (NB: See my
comment in subsequent patches about creating a 'struct twl6030' where
you could store 'struct dev'.) In this patch I mean litterally
replacing "%s: ", with "tw16030_irq: ". Simples. :)

> Now, the pointer on "dev" (in our case "twl-core" device) isn't passed
> in IRQ handler, so It can't be used here.
> 
> Of course it can be done, but would it make code better?
> My opinion - no.

> >>+	if (sts.bytes[2] & 0x10)
> >>+		sts.bytes[2] |= 0x08;
> >>
> >>-		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
> >>-			local_irq_disable();
> >>-			if (sts.int_sts & 0x1) {
> >>-				int module_irq = twl6030_irq_base +
> >>+	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
> >>+		if (sts.int_sts & 0x1) {
> >
> >I'm a little confused by this. Where does sts.int_sts come from?
> 
> See my comment above, pls

Okay, that's my fault for not understanding unions properly as I've
never had to use one, but now I do, thanks.

> >>@@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
> >>  {
> >>  	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
> >>
> >>-	if (twl6030_irq_base) {
> >>+	if (!twl6030_irq_base) {
> >>  		pr_err("twl6030: can't yet clean up IRQs?\n");
> >>  		return -ENOSYS;
> >>  	}
> >>+
> >>+	free_irq(twl_irq, NULL);
> >>+
> >
> >If request_threaded_irq() fails, isn't there a chance that
> >twl6030_irq_base will be allocated, but twl_irq will still be
> >undefined?
> 
> Yes. A mess is here (historically:), thanks. Will use twl_irq
> instead of twl6030_irq_base (I did it, actually, in patch [3]:).

Yes, I saw it. It would be better if you still fixed up this patch to
be correct though. Even if you break it out and add it as [PATCH 1/x].

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

[Grygorii Strashko]

On 07/24/2013 03:50 PM, Lee Jones wrote:
>>>> +	if (ret) {
>>>> +		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);
>>>
>>> Does the user really care which function we're returning from.
>>>
>>> Would it be better if you replace '__func__' with the device name?
>>
>> This module hasn't been converted to the device yet:(
>> (I mean "interrupt-controller").
>> But I'm thinking about it as the next step :) and then It will be
>> absolutely reasonable change to replace pr_*() with dev_*() and
>> remove __func__.
>
> I don't mean anything as compicated as that for 'this' patch. (NB: See my
> comment in subsequent patches about creating a 'struct twl6030' where
> you could store 'struct dev'.) In this patch I mean litterally
> replacing "%s: ", with "tw16030_irq: ". Simples. :)

Ok. I understand it now - will redo.


>
>> Now, the pointer on "dev" (in our case "twl-core" device) isn't passed
>> in IRQ handler, so It can't be used here.
>>
>> Of course it can be done, but would it make code better?
>> My opinion - no.
>
>>>> +	if (sts.bytes[2] & 0x10)
>>>> +		sts.bytes[2] |= 0x08;
>>>>
>>>> -		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
>>>> -			local_irq_disable();
>>>> -			if (sts.int_sts & 0x1) {
>>>> -				int module_irq = twl6030_irq_base +
>>>> +	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>>>> +		if (sts.int_sts & 0x1) {
>>>
>>> I'm a little confused by this. Where does sts.int_sts come from?
>>
>> See my comment above, pls
>
> Okay, that's my fault for not understanding unions properly as I've
> never had to use one, but now I do, thanks.
>
>>>> @@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
>>>>   {
>>>>   	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>>>>
>>>> -	if (twl6030_irq_base) {
>>>> +	if (!twl6030_irq_base) {
>>>>   		pr_err("twl6030: can't yet clean up IRQs?\n");
>>>>   		return -ENOSYS;
>>>>   	}
>>>> +
>>>> +	free_irq(twl_irq, NULL);
>>>> +
>>>
>>> If request_threaded_irq() fails, isn't there a chance that
>>> twl6030_irq_base will be allocated, but twl_irq will still be
>>> undefined?
>>
>> Yes. A mess is here (historically:), thanks. Will use twl_irq
>> instead of twl6030_irq_base (I did it, actually, in patch [3]:).
>
> Yes, I saw it. It would be better if you still fixed up this patch to
> be correct though. Even if you break it out and add it as [PATCH 1/x].
>
ok

Re: [PATCH 3/4] mfd: twl6030-irq: convert to use linear irq_domain

[Grygorii Strashko]

On 07/24/2013 02:35 PM, Lee Jones wrote:
> On Tue, 23 Jul 2013, Grygorii Strashko wrote:
>
>> Since the TWL6030 PMIC is used with OMAP4 SoCs only and OMAP4 legacy
>> boot is dropped there are no needs to allocate the range of IRQ
>> descriptors during system boot to support TWL6030 IRQs.
>>
>> Hence, convert it to use linear irq_domain and move IRQ configuration in
>> .map()/.unmap() callbacks of irq_domain. So, IRQ mapping and descriptors
>> allocation will be performed dynamically basing on DT configuration.
>>
>> The error message will be reported in case if unmapped IRQ is received by
>> TWL6030 (virq==0).
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/mfd/twl6030-irq.c |  114 ++++++++++++++++++++++++---------------------
>>   1 file changed, 61 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
>> index 790cc28..89f130b 100644
>> --- a/drivers/mfd/twl6030-irq.c
>> +++ b/drivers/mfd/twl6030-irq.c
>> @@ -138,6 +138,7 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>>   static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>   {
>>   	int i, ret;
>> +	struct irq_domain *irq_domain = (struct irq_domain *)data;
>>   	union {
>>   		u8 bytes[4];
>>   		u32 int_sts;
>> @@ -161,9 +162,14 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>
>>   	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>>   		if (sts.int_sts & 0x1) {
>> -			int module_irq = twl6030_irq_base +
>> -					twl6030_interrupt_mapping[i];
>> -			handle_nested_irq(module_irq);
>> +			int module_irq =
>> +				irq_find_mapping(irq_domain,
>> +						 twl6030_interrupt_mapping[i]);
>> +			if (module_irq)
>> +				handle_nested_irq(module_irq);
>> +			else
>> +				pr_err("%s: Unmapped PIH ISR %u detected\n",
>> +				       __func__, i);
>
> Same here. Does the user really care which function failed?
>
> Please consider replacing with the device name.

ok.

>
>>   			pr_debug("%s: PIH ISR %u, virq%u\n",
>>   				 __func__, i, module_irq);
>>   		}
>> @@ -186,19 +192,6 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>
>>   /*----------------------------------------------------------------------*/
>>
>> -static inline void activate_irq(int irq)
>> -{
>> -#ifdef CONFIG_ARM
>> -	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
>> -	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
>> -	 */
>> -	set_irq_flags(irq, IRQF_VALID);
>> -#else
>> -	/* same effect on other architectures */
>> -	irq_set_noprobe(irq);
>> -#endif
>> -}
>> -
>>   static int twl6030_irq_set_wake(struct irq_data *d, unsigned int on)
>>   {
>>   	if (on)
>> @@ -308,28 +301,54 @@ int twl6030_mmc_card_detect(struct device *dev, int slot)
>>   }
>>   EXPORT_SYMBOL(twl6030_mmc_card_detect);
>>
>> +static struct irq_chip twl6030_irq_chip;
>> +
>> +static int twl6030_irq_map(struct irq_domain *d, unsigned int virq,
>> +			      irq_hw_number_t hwirq)
>> +{
>> +	irq_set_chip_data(virq, &twl6030_irq_chip);
>> +	irq_set_chip_and_handler(virq,  &twl6030_irq_chip, handle_simple_irq);
>> +	irq_set_nested_thread(virq, true);
>> +
>> +#ifdef CONFIG_ARM
>> +	/*
>> +	 * ARM requires an extra step to clear IRQ_NOREQUEST, which it
>> +	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
>> +	 */
>> +	set_irq_flags(virq, IRQF_VALID);
>> +#else
>> +	/* same effect on other architectures */
>> +	irq_set_noprobe(virq);
>> +#endif
>> +
>> +	return 0;
>> +}
>> +
>> +static void twl6030_irq_unmap(struct irq_domain *d, unsigned int virq)
>> +{
>> +#ifdef CONFIG_ARM
>> +	set_irq_flags(virq, 0);
>> +#endif
>> +	irq_set_chip_and_handler(virq, NULL, NULL);
>> +	irq_set_chip_data(virq, NULL);
>> +}
>> +
>> +static struct irq_domain_ops twl6030_irq_domain_ops = {
>> +	.map	= twl6030_irq_map,
>> +	.unmap	= twl6030_irq_unmap,
>> +	.xlate	= irq_domain_xlate_onetwocell,
>> +};
>> +
>>   int twl6030_init_irq(struct device *dev, int irq_num)
>>   {
>>   	struct			device_node *node = dev->of_node;
>> -	int			nr_irqs, irq_base, irq_end;
>> -	static struct irq_chip  twl6030_irq_chip;
>> +	int			nr_irqs;
>>   	int			status;
>> -	int			i;
>>   	u8			mask[3];
>> +	struct irq_domain	*irq_domain;
>>
>>   	nr_irqs = TWL6030_NR_IRQS;
>>
>> -	irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
>> -	if (IS_ERR_VALUE(irq_base)) {
>> -		dev_err(dev, "Fail to allocate IRQ descs\n");
>> -		return irq_base;
>> -	}
>> -
>> -	irq_domain_add_legacy(node, nr_irqs, irq_base, 0,
>> -			      &irq_domain_simple_ops, NULL);
>> -
>> -	irq_end = irq_base + nr_irqs;
>> -
>>   	mask[0] = 0xFF;
>>   	mask[1] = 0xFF;
>>   	mask[2] = 0xFF;
>> @@ -346,8 +365,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   		return status;
>>   	}
>>
>> -	twl6030_irq_base = irq_base;
>> -
>>   	/*
>>   	 * install an irq handler for each of the modules;
>>   	 * clone dummy irq_chip since PIH can't *do* anything
>> @@ -357,20 +374,18 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   	twl6030_irq_chip.irq_set_type = NULL;
>>   	twl6030_irq_chip.irq_set_wake = twl6030_irq_set_wake;
>>
>> -	for (i = irq_base; i < irq_end; i++) {
>> -		irq_set_chip_and_handler(i, &twl6030_irq_chip,
>> -					 handle_simple_irq);
>> -		irq_set_chip_data(i, (void *)irq_num);
>> -		irq_set_nested_thread(i, true);
>> -		activate_irq(i);
>> +	irq_domain = irq_domain_add_linear(node, nr_irqs,
>> +					   &twl6030_irq_domain_ops, NULL);
>> +	if (!irq_domain) {
>> +		dev_err(dev, "Can't add irq_domain\n");
>> +		return -ENOMEM;
>>   	}
>>
>> -	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
>> -		 irq_num, irq_base, irq_end);
>> +	dev_info(dev, "PIH (irq %d) nested IRQs\n", irq_num);
>>
>>   	/* install an irq handler to demultiplex the TWL6030 interrupt */
>>   	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
>> -				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
>> +				      IRQF_ONESHOT, "TWL6030-PIH", irq_domain);
>>   	if (status < 0) {
>>   		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>>   		goto fail_irq;
>> @@ -378,26 +393,19 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>
>>   	twl_irq = irq_num;
>>   	register_pm_notifier(&twl6030_irq_pm_notifier_block);
>> -	return irq_base;
>> +	return irq_num;
>
> I think you need to change twl-core to now expect the total number of
> IRQs rather than the base one now.

Would it be ok if twl6030_init_irq() will return 0 on success or error 
code on failure?

The change of twl6030_init_irq() will not affect twl-core in case of 
TWL6030/32 DT-boot --- not DT boot (legacy mode) seems need to be 
dropped form twl-core for TWL6030/32.

>
>>   fail_irq:
>> -	for (i = irq_base; i < irq_end; i++)
>> -		irq_set_chip_and_handler(i, NULL, NULL);
>> -
>> +	irq_domain_remove(irq_domain);
>
> Why do you kill the irqdomain here, but not in exit()?

this is a failure path and twl_irq == 0, so exit will do nothing,
and it safe to delete irq_domain here, because no child devices will be
created.

>
>>   	return status;
>>   }
>>
>>   int twl6030_exit_irq(void)
>>   {
>> -	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>> -
>> -	if (!twl6030_irq_base) {
>> -		pr_err("twl6030: can't yet clean up IRQs?\n");
>> -		return -ENOSYS;
>> +	if (twl_irq) {
>> +		unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>> +		free_irq(twl_irq, NULL);

In general, I need to add irq_domain_remove(irq_domain) here too
But, there is a big BUT!

>>   	}
>
> Ah yes, that's better.
>
>> -
>> -	free_irq(twl_irq, NULL);
>> -
>>   	return 0;
>>   }
>>
>

Unfortunately, I'm not fully understand how to dispose IRQ mapping and 
free IRQ domain/descriptors correctly.
- IRQ mapping is done when devices are being created from DT (including 
IRQ desc allocation)
- there is API irq_dispose_mapping(), but It's not been called from 
irq_domain_remove() and OF and Driver cores.
- there is no of_platform_unpopulate() yet

So, I can call irq_dispose_mapping() manually from twl6030_exit_irq, but 
child devices will still keep mapped IRQ numbers as their Resources.
:((

More over, in this case I can't use devm_*() API to request IRQ, because 
free_irq() will be called after irq_domian de-initialization.

That's why I've not added irq_domian de-initialization in 
twl6030_exit_irq() and not used devm_*() to request IRQ.


Regards,
- grygorii

Re: [PATCH 4/4] mfd: twl6030-irq: Add interrupt mapping table for the twl6032

[Grygorii Strashko]

On 07/24/2013 02:52 PM, Lee Jones wrote:
> On Tue, 23 Jul 2013, Grygorii Strashko wrote:
>
>> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>
>>
>> This patch adds interrupt mapping table for the twl6032.
>
> Repeating the $SUBJECT line is never helpful.
>
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   drivers/mfd/twl6030-irq.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
>> index 89f130b..e4df87f 100644
>> --- a/drivers/mfd/twl6030-irq.c
>> +++ b/drivers/mfd/twl6030-irq.c
>> @@ -41,6 +41,7 @@
>>   #include <linux/suspend.h>
>>   #include <linux/of.h>
>>   #include <linux/irqdomain.h>
>> +#include <linux/of_device.h>
>>
>>   #include "twl-core.h"
>>
>> @@ -84,6 +85,36 @@ static int twl6030_interrupt_mapping[24] = {
>>   	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/
>>   	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
>>   };
>> +
>> +static int twl6032_interrupt_mapping[24] = {
>> +	PWR_INTR_OFFSET,	/* Bit 0	PWRON			*/
>> +	PWR_INTR_OFFSET,	/* Bit 1	RPWRON			*/
>> +	PWR_INTR_OFFSET,	/* Bit 2	SYS_VLOW		*/
>> +	RTC_INTR_OFFSET,	/* Bit 3	RTC_ALARM		*/
>> +	RTC_INTR_OFFSET,	/* Bit 4	RTC_PERIOD		*/
>> +	HOTDIE_INTR_OFFSET,	/* Bit 5	HOT_DIE			*/
>> +	SMPSLDO_INTR_OFFSET,	/* Bit 6	VXXX_SHORT		*/
>> +	PWR_INTR_OFFSET,	/* Bit 7	SPDURATION		*/
>> +
>> +	PWR_INTR_OFFSET,	/* Bit 8	WATCHDOG		*/
>> +	BATDETECT_INTR_OFFSET,	/* Bit 9	BAT			*/
>> +	SIMDETECT_INTR_OFFSET,	/* Bit 10	SIM			*/
>> +	MMCDETECT_INTR_OFFSET,	/* Bit 11	MMC			*/
>> +	MADC_INTR_OFFSET,	/* Bit 12	GPADC_RT_EOC		*/
>> +	MADC_INTR_OFFSET,	/* Bit 13	GPADC_SW_EOC		*/
>> +	GASGAUGE_INTR_OFFSET,	/* Bit 14	CC_EOC			*/
>> +	GASGAUGE_INTR_OFFSET,	/* Bit 15	CC_AUTOCAL		*/
>> +
>> +	USBOTG_INTR_OFFSET,	/* Bit 16	ID_WKUP			*/
>> +	USBOTG_INTR_OFFSET,	/* Bit 17	VBUS_WKUP		*/
>> +	USBOTG_INTR_OFFSET,	/* Bit 18	ID			*/
>> +	USB_PRES_INTR_OFFSET,	/* Bit 19	VBUS			*/
>> +	CHARGER_INTR_OFFSET,	/* Bit 20	CHRG_CTRL		*/
>> +	CHARGERFAULT_INTR_OFFSET,	/* Bit 21	EXT_CHRG	*/
>> +	CHARGERFAULT_INTR_OFFSET,	/* Bit 22	INT_CHRG	*/
>
> OCD failure. ;)
>
> NB: Kidding, you don't have to do anything about this.
>
>> +	RSV_INTR_OFFSET,	/* Bit 23	Reserved		*/
>> +};
>> +
>>   /*----------------------------------------------------------------------*/
>>
>>   static unsigned twl6030_irq_base;
>> @@ -91,6 +122,7 @@ static int twl_irq;
>>   static bool twl_irq_wake_enabled;
>>
>>   static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
>> +static const int *irq_mapping_tbl;
>
> What I'd actually like to see is the creation of 'struct twl6030' to
> keep all your goodies in; irq_domain, irq_mapping_tbl etc and for you
> to pass that around instead of creating more global variables e.g. via
> request_threaded_irq(..., void *dev_id) to access the aforementioned
> information.

I can add this as the first patch in series - Is It ok?

>
>>   static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
>>   				   unsigned long pm_event, void *unused)
>> @@ -164,7 +196,7 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>   		if (sts.int_sts & 0x1) {
>>   			int module_irq =
>>   				irq_find_mapping(irq_domain,
>> -						 twl6030_interrupt_mapping[i]);
>> +						 irq_mapping_tbl[i]);
>>   			if (module_irq)
>>   				handle_nested_irq(module_irq);
>>   			else
>> @@ -339,6 +371,12 @@ static struct irq_domain_ops twl6030_irq_domain_ops = {
>>   	.xlate	= irq_domain_xlate_onetwocell,
>>   };
>>
>> +static const struct of_device_id twl6030_of_match[] = {
>> +	{.compatible = "ti,twl6030", &twl6030_interrupt_mapping},
>> +	{.compatible = "ti,twl6032", &twl6032_interrupt_mapping},
>> +	{ },
>> +};
>> +
>>   int twl6030_init_irq(struct device *dev, int irq_num)
>>   {
>>   	struct			device_node *node = dev->of_node;
>> @@ -346,6 +384,15 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>   	int			status;
>>   	u8			mask[3];
>>   	struct irq_domain	*irq_domain;
>> +	const struct of_device_id *of_id;
>> +
>> +	of_id = of_match_device(twl6030_of_match, dev);
>> +	if (!of_id || !of_id->data) {
>> +		dev_err(dev, "Unknown TWL device model\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	irq_mapping_tbl = of_id->data;
>>
>>   	nr_irqs = TWL6030_NR_IRQS;
>>
>

Regards,
-grygorii