[PATCH 0/4 v6] mmc: tmio, sdhi: provide multiple irq handlers

[PATCH 0/4 v6] mmc: tmio, sdhi: provide multiple irq handlers

[Simon Horman]

The SDHI driver already supports making use of up to three interrupt
sources.

This series breaks up the existing interrupt handler into three handlers,
one for card access, one for card detect interrupts, and one for SDIO
interrupts.  A cover-all handler, which makes use of these new broken-out
handlers is provided for for the case where there is only one interrupt
source.

This series also wires up the broken-out irq handlers in the SDHI driver

[PATCH 0/4 v6] mmc: tmio, sdhi: provide multiple irq handlers

[Simon Horman]

The SDHI driver already supports making use of up to three interrupt
sources.

This series breaks up the existing interrupt handler into three handlers,
one for card access, one for card detect interrupts, and one for SDIO
interrupts.  A cover-all handler, which makes use of these new broken-out
handlers is provided for for the case where there is only one interrupt
source.

This series also wires up the broken-out irq handlers in the SDHI driver

[PATCH 1/4] mmc: tmio: Cache interrupt masks

[Simon Horman]

This avoids the need to look up the masks each time an interrupt
is handled.

As suggested by Guennadi.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested

v3
* As suggested by Guennadi Liakhovetski
  - Only read sdcard_irq_mask once and never read sdio_irq_mask,
    instead use the cached values as much as possible

v2
* Initial release
---
 drivers/mmc/host/tmio_mmc.h     |    4 ++++
 drivers/mmc/host/tmio_mmc_pio.c |   34 ++++++++++++++++++----------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index eeaf643..1cf8db5 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -79,6 +79,10 @@ struct tmio_mmc_host {
 	struct delayed_work	delayed_reset_work;
 	struct work_struct	done;
 
+	/* Cache IRQ mask */
+	u32			sdcard_irq_mask;
+	u32			sdio_irq_mask;
+
 	spinlock_t		lock;		/* protect host private data */
 	unsigned long		last_req_ts;
 	struct mutex		ios_lock;	/* protect set_ios() context */
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 1f16357..f0c7830 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -48,14 +48,14 @@
 
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) | (i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask |= (i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 static void tmio_mmc_ack_mmc_irqs(struct tmio_mmc_host *host, u32 i)
@@ -127,11 +127,13 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 
 	if (enable) {
 		host->sdio_irq_enabled = 1;
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
+					~TMIO_SDIO_STAT_IOIRQ;
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK,
-			(TMIO_SDIO_MASK_ALL & ~TMIO_SDIO_STAT_IOIRQ));
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 	} else {
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, TMIO_SDIO_MASK_ALL);
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000);
 		host->sdio_irq_enabled = 0;
 	}
@@ -548,26 +550,25 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	struct tmio_mmc_host *host = devid;
 	struct mmc_host *mmc = host->mmc;
 	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, irq_mask, status;
-	unsigned int sdio_ireg, sdio_irq_mask, sdio_status;
+	unsigned int ireg, status;
+	unsigned int sdio_ireg, sdio_status;
 
 	pr_debug("MMC IRQ begin\n");
 
 	status = sd_ctrl_read32(host, CTL_STATUS);
-	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
-	ireg = status & TMIO_MASK_IRQ & ~irq_mask;
+	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
 	sdio_ireg = 0;
 	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
 		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL & ~sdio_irq_mask;
+		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
+				~host->sdio_irq_mask;
 
 		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
 
 		if (sdio_ireg && !host->sdio_irq_enabled) {
 			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, sdio_irq_mask, sdio_ireg);
+				   sdio_status, host->sdio_irq_mask, sdio_ireg);
 			tmio_mmc_enable_sdio_irq(mmc, 0);
 			goto out;
 		}
@@ -623,9 +624,9 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	}
 
 	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
+		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
 	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
+	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
 
 out:
 	return IRQ_HANDLED;
@@ -882,6 +883,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	tmio_mmc_clk_stop(_host);
 	tmio_mmc_reset(_host);
 
+	_host->sdcard_irq_mask = sd_ctrl_read32(_host, CTL_IRQ_MASK);
 	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
 	if (pdata->flags & TMIO_MMC_SDIO_IRQ)
 		tmio_mmc_enable_sdio_irq(mmc, 0);
-- 
1.7.5.4

[PATCH 1/4] mmc: tmio: Cache interrupt masks

[Simon Horman]

This avoids the need to look up the masks each time an interrupt
is handled.

As suggested by Guennadi.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested

v3
* As suggested by Guennadi Liakhovetski
  - Only read sdcard_irq_mask once and never read sdio_irq_mask,
    instead use the cached values as much as possible

v2
* Initial release
---
 drivers/mmc/host/tmio_mmc.h     |    4 ++++
 drivers/mmc/host/tmio_mmc_pio.c |   34 ++++++++++++++++++----------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index eeaf643..1cf8db5 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -79,6 +79,10 @@ struct tmio_mmc_host {
 	struct delayed_work	delayed_reset_work;
 	struct work_struct	done;
 
+	/* Cache IRQ mask */
+	u32			sdcard_irq_mask;
+	u32			sdio_irq_mask;
+
 	spinlock_t		lock;		/* protect host private data */
 	unsigned long		last_req_ts;
 	struct mutex		ios_lock;	/* protect set_ios() context */
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 1f16357..f0c7830 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -48,14 +48,14 @@
 
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) & ~(i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask &= ~(i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i)
 {
-	u32 mask = sd_ctrl_read32(host, CTL_IRQ_MASK) | (i & TMIO_MASK_IRQ);
-	sd_ctrl_write32(host, CTL_IRQ_MASK, mask);
+	host->sdcard_irq_mask |= (i & TMIO_MASK_IRQ);
+	sd_ctrl_write32(host, CTL_IRQ_MASK, host->sdcard_irq_mask);
 }
 
 static void tmio_mmc_ack_mmc_irqs(struct tmio_mmc_host *host, u32 i)
@@ -127,11 +127,13 @@ static void tmio_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable)
 
 	if (enable) {
 		host->sdio_irq_enabled = 1;
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL &
+					~TMIO_SDIO_STAT_IOIRQ;
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0001);
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK,
-			(TMIO_SDIO_MASK_ALL & ~TMIO_SDIO_STAT_IOIRQ));
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 	} else {
-		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, TMIO_SDIO_MASK_ALL);
+		host->sdio_irq_mask = TMIO_SDIO_MASK_ALL;
+		sd_ctrl_write16(host, CTL_SDIO_IRQ_MASK, host->sdio_irq_mask);
 		sd_ctrl_write16(host, CTL_TRANSACTION_CTL, 0x0000);
 		host->sdio_irq_enabled = 0;
 	}
@@ -548,26 +550,25 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	struct tmio_mmc_host *host = devid;
 	struct mmc_host *mmc = host->mmc;
 	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, irq_mask, status;
-	unsigned int sdio_ireg, sdio_irq_mask, sdio_status;
+	unsigned int ireg, status;
+	unsigned int sdio_ireg, sdio_status;
 
 	pr_debug("MMC IRQ begin\n");
 
 	status = sd_ctrl_read32(host, CTL_STATUS);
-	irq_mask = sd_ctrl_read32(host, CTL_IRQ_MASK);
-	ireg = status & TMIO_MASK_IRQ & ~irq_mask;
+	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
 	sdio_ireg = 0;
 	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
 		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		sdio_irq_mask = sd_ctrl_read16(host, CTL_SDIO_IRQ_MASK);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL & ~sdio_irq_mask;
+		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
+				~host->sdio_irq_mask;
 
 		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
 
 		if (sdio_ireg && !host->sdio_irq_enabled) {
 			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, sdio_irq_mask, sdio_ireg);
+				   sdio_status, host->sdio_irq_mask, sdio_ireg);
 			tmio_mmc_enable_sdio_irq(mmc, 0);
 			goto out;
 		}
@@ -623,9 +624,9 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 	}
 
 	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, irq_mask, ireg);
+		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
 	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~irq_mask);
+	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
 
 out:
 	return IRQ_HANDLED;
@@ -882,6 +883,7 @@ int __devinit tmio_mmc_host_probe(struct tmio_mmc_host **host,
 	tmio_mmc_clk_stop(_host);
 	tmio_mmc_reset(_host);
 
+	_host->sdcard_irq_mask = sd_ctrl_read32(_host, CTL_IRQ_MASK);
 	tmio_mmc_disable_mmc_irqs(_host, TMIO_MASK_ALL);
 	if (pdata->flags & TMIO_MMC_SDIO_IRQ)
 		tmio_mmc_enable_sdio_irq(mmc, 0);
-- 
1.7.5.4

[PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

Provide separate interrupt handlers which may be used by platforms where
SDHI has three interrupt sources.

This patch also removes the commented-out handling of CRC and other errors.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested

v4
* As suggested by Guennadi Liakhovetski
  - Use bool as return type for __tmio_mmc_sdcard_irq() and
    __tmio_mmc_card_detect_irq()

v3
* Rebase for updated "mmc: tmio: Cache interrupt masks"
* As suggested by Guennadi Liakhovetski
  - Do not alter logic to handle more than one interupt at once
  - Add missing "static" to declartion of __tmio_mmc_sdcard_irq()

v2
* As suggested by Guennadi Liakhovetski
  - Combine 3 patches into one
  - Reduce the number of __tmio_..._irq() functions
  - Rename "...card_access..." functions as "...sdcard..."
---
 drivers/mmc/host/tmio_mmc.h     |    3 +
 drivers/mmc/host/tmio_mmc_pio.c |  131 ++++++++++++++++++++++++--------------
 2 files changed, 86 insertions(+), 48 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 1cf8db5..3020f98 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -97,6 +97,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host);
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 irqreturn_t tmio_mmc_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid);
 
 static inline char *tmio_mmc_kmap_atomic(struct scatterlist *sg,
 					 unsigned long *flags)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index f0c7830..6275e3d 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -545,44 +545,20 @@ out:
 	spin_unlock(&host->lock);
 }
 
-irqreturn_t tmio_mmc_irq(int irq, void *devid)
+static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
+				       int *ireg, int *status)
 {
-	struct tmio_mmc_host *host = devid;
-	struct mmc_host *mmc = host->mmc;
-	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, status;
-	unsigned int sdio_ireg, sdio_status;
-
-	pr_debug("MMC IRQ begin\n");
-
-	status = sd_ctrl_read32(host, CTL_STATUS);
-	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
+	*status = sd_ctrl_read32(host, CTL_STATUS);
+	*ireg = *status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
-	sdio_ireg = 0;
-	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
-		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
-				~host->sdio_irq_mask;
-
-		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
-
-		if (sdio_ireg && !host->sdio_irq_enabled) {
-			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, host->sdio_irq_mask, sdio_ireg);
-			tmio_mmc_enable_sdio_irq(mmc, 0);
-			goto out;
-		}
-
-		if (mmc->caps & MMC_CAP_SDIO_IRQ &&
-			sdio_ireg & TMIO_SDIO_STAT_IOIRQ)
-			mmc_signal_sdio_irq(mmc);
-
-		if (sdio_ireg)
-			goto out;
-	}
+	pr_debug_status(*status);
+	pr_debug_status(*ireg);
+}
 
-	pr_debug_status(status);
-	pr_debug_status(ireg);
+static bool __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
+				      int ireg, int status)
+{
+	struct mmc_host *mmc = host->mmc;
 
 	/* Card insert / remove attempts */
 	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
@@ -592,43 +568,102 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
 		    !work_pending(&mmc->detect.work))
 			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
-		goto out;
+		return true;
 	}
 
-	/* CRC and other errors */
-/*	if (ireg & TMIO_STAT_ERR_IRQ)
- *		handled |= tmio_error_irq(host, irq, stat);
- */
+	return false;
+}
+
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
 
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, status);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_card_detect_irq);
+
+static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host,
+				 int ireg, int status)
+{
 	/* Command completion */
 	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
 		tmio_mmc_ack_mmc_irqs(host,
 			     TMIO_STAT_CMDRESPEND |
 			     TMIO_STAT_CMDTIMEOUT);
 		tmio_mmc_cmd_irq(host, status);
-		goto out;
+		return true;
 	}
 
 	/* Data transfer */
 	if (ireg & (TMIO_STAT_RXRDY | TMIO_STAT_TXRQ)) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_RXRDY | TMIO_STAT_TXRQ);
 		tmio_mmc_pio_irq(host);
-		goto out;
+		return true;
 	}
 
 	/* Data transfer completion */
 	if (ireg & TMIO_STAT_DATAEND) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
 		tmio_mmc_data_irq(host);
-		goto out;
+		return true;
 	}
 
-	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
-	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
+	return false;
+}
+
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_sdcard_irq(host, ireg, status);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdcard_irq);
+
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	struct mmc_host *mmc = host->mmc;
+	struct tmio_mmc_data *pdata = host->pdata;
+	unsigned int ireg, status;
+
+	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
+		return IRQ_HANDLED;
+
+	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
+	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
+
+	sd_ctrl_write16(host, CTL_SDIO_STATUS, status & ~TMIO_SDIO_MASK_ALL);
+
+	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
+		mmc_signal_sdio_irq(mmc);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdio_irq);
+
+irqreturn_t tmio_mmc_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	unsigned int ireg, status;
+
+	pr_debug("MMC IRQ begin\n");
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	if (__tmio_mmc_card_detect_irq(host, ireg, status))
+		return IRQ_HANDLED;
+	if (__tmio_mmc_sdcard_irq(host, ireg, status))
+		return IRQ_HANDLED;
+
+	tmio_mmc_sdio_irq(irq, devid);
 
-out:
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL(tmio_mmc_irq);
-- 
1.7.5.4

[PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

Provide separate interrupt handlers which may be used by platforms where
SDHI has three interrupt sources.

This patch also removes the commented-out handling of CRC and other errors.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

* SDCARD portion tested on AP4/Mackerel
* SDIO portion untested

v4
* As suggested by Guennadi Liakhovetski
  - Use bool as return type for __tmio_mmc_sdcard_irq() and
    __tmio_mmc_card_detect_irq()

v3
* Rebase for updated "mmc: tmio: Cache interrupt masks"
* As suggested by Guennadi Liakhovetski
  - Do not alter logic to handle more than one interupt at once
  - Add missing "static" to declartion of __tmio_mmc_sdcard_irq()

v2
* As suggested by Guennadi Liakhovetski
  - Combine 3 patches into one
  - Reduce the number of __tmio_..._irq() functions
  - Rename "...card_access..." functions as "...sdcard..."
---
 drivers/mmc/host/tmio_mmc.h     |    3 +
 drivers/mmc/host/tmio_mmc_pio.c |  131 ++++++++++++++++++++++++--------------
 2 files changed, 86 insertions(+), 48 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 1cf8db5..3020f98 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -97,6 +97,9 @@ void tmio_mmc_do_data_irq(struct tmio_mmc_host *host);
 void tmio_mmc_enable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 void tmio_mmc_disable_mmc_irqs(struct tmio_mmc_host *host, u32 i);
 irqreturn_t tmio_mmc_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid);
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid);
 
 static inline char *tmio_mmc_kmap_atomic(struct scatterlist *sg,
 					 unsigned long *flags)
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index f0c7830..6275e3d 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -545,44 +545,20 @@ out:
 	spin_unlock(&host->lock);
 }
 
-irqreturn_t tmio_mmc_irq(int irq, void *devid)
+static void tmio_mmc_card_irq_status(struct tmio_mmc_host *host,
+				       int *ireg, int *status)
 {
-	struct tmio_mmc_host *host = devid;
-	struct mmc_host *mmc = host->mmc;
-	struct tmio_mmc_data *pdata = host->pdata;
-	unsigned int ireg, status;
-	unsigned int sdio_ireg, sdio_status;
-
-	pr_debug("MMC IRQ begin\n");
-
-	status = sd_ctrl_read32(host, CTL_STATUS);
-	ireg = status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
+	*status = sd_ctrl_read32(host, CTL_STATUS);
+	*ireg = *status & TMIO_MASK_IRQ & ~host->sdcard_irq_mask;
 
-	sdio_ireg = 0;
-	if (!ireg && pdata->flags & TMIO_MMC_SDIO_IRQ) {
-		sdio_status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
-		sdio_ireg = sdio_status & TMIO_SDIO_MASK_ALL &
-				~host->sdio_irq_mask;
-
-		sd_ctrl_write16(host, CTL_SDIO_STATUS, sdio_status & ~TMIO_SDIO_MASK_ALL);
-
-		if (sdio_ireg && !host->sdio_irq_enabled) {
-			pr_warning("tmio_mmc: Spurious SDIO IRQ, disabling! 0x%04x 0x%04x 0x%04x\n",
-				   sdio_status, host->sdio_irq_mask, sdio_ireg);
-			tmio_mmc_enable_sdio_irq(mmc, 0);
-			goto out;
-		}
-
-		if (mmc->caps & MMC_CAP_SDIO_IRQ &&
-			sdio_ireg & TMIO_SDIO_STAT_IOIRQ)
-			mmc_signal_sdio_irq(mmc);
-
-		if (sdio_ireg)
-			goto out;
-	}
+	pr_debug_status(*status);
+	pr_debug_status(*ireg);
+}
 
-	pr_debug_status(status);
-	pr_debug_status(ireg);
+static bool __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
+				      int ireg, int status)
+{
+	struct mmc_host *mmc = host->mmc;
 
 	/* Card insert / remove attempts */
 	if (ireg & (TMIO_STAT_CARD_INSERT | TMIO_STAT_CARD_REMOVE)) {
@@ -592,43 +568,102 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
 		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
 		    !work_pending(&mmc->detect.work))
 			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
-		goto out;
+		return true;
 	}
 
-	/* CRC and other errors */
-/*	if (ireg & TMIO_STAT_ERR_IRQ)
- *		handled |= tmio_error_irq(host, irq, stat);
- */
+	return false;
+}
+
+irqreturn_t tmio_mmc_card_detect_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
 
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_card_detect_irq(host, ireg, status);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_card_detect_irq);
+
+static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host,
+				 int ireg, int status)
+{
 	/* Command completion */
 	if (ireg & (TMIO_STAT_CMDRESPEND | TMIO_STAT_CMDTIMEOUT)) {
 		tmio_mmc_ack_mmc_irqs(host,
 			     TMIO_STAT_CMDRESPEND |
 			     TMIO_STAT_CMDTIMEOUT);
 		tmio_mmc_cmd_irq(host, status);
-		goto out;
+		return true;
 	}
 
 	/* Data transfer */
 	if (ireg & (TMIO_STAT_RXRDY | TMIO_STAT_TXRQ)) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_RXRDY | TMIO_STAT_TXRQ);
 		tmio_mmc_pio_irq(host);
-		goto out;
+		return true;
 	}
 
 	/* Data transfer completion */
 	if (ireg & TMIO_STAT_DATAEND) {
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_DATAEND);
 		tmio_mmc_data_irq(host);
-		goto out;
+		return true;
 	}
 
-	pr_warning("tmio_mmc: Spurious irq, disabling! "
-		"0x%08x 0x%08x 0x%08x\n", status, host->sdcard_irq_mask, ireg);
-	pr_debug_status(status);
-	tmio_mmc_disable_mmc_irqs(host, status & ~host->sdcard_irq_mask);
+	return false;
+}
+
+irqreturn_t tmio_mmc_sdcard_irq(int irq, void *devid)
+{
+	unsigned int ireg, status;
+	struct tmio_mmc_host *host = devid;
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	__tmio_mmc_sdcard_irq(host, ireg, status);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdcard_irq);
+
+irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	struct mmc_host *mmc = host->mmc;
+	struct tmio_mmc_data *pdata = host->pdata;
+	unsigned int ireg, status;
+
+	if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
+		return IRQ_HANDLED;
+
+	status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
+	ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
+
+	sd_ctrl_write16(host, CTL_SDIO_STATUS, status & ~TMIO_SDIO_MASK_ALL);
+
+	if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg & TMIO_SDIO_STAT_IOIRQ)
+		mmc_signal_sdio_irq(mmc);
+
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL(tmio_mmc_sdio_irq);
+
+irqreturn_t tmio_mmc_irq(int irq, void *devid)
+{
+	struct tmio_mmc_host *host = devid;
+	unsigned int ireg, status;
+
+	pr_debug("MMC IRQ begin\n");
+
+	tmio_mmc_card_irq_status(host, &ireg, &status);
+	if (__tmio_mmc_card_detect_irq(host, ireg, status))
+		return IRQ_HANDLED;
+	if (__tmio_mmc_sdcard_irq(host, ireg, status))
+		return IRQ_HANDLED;
+
+	tmio_mmc_sdio_irq(irq, devid);
 
-out:
 	return IRQ_HANDLED;
 }
 EXPORT_SYMBOL(tmio_mmc_irq);
-- 
1.7.5.4

[PATCH 3/4] mmc: sdhi: Make use of per-source irq handlers

[Simon Horman]

Make use of per-source irq handles if the
platform (data) has multiple irq sources.

Also, as suggested by Guennadi Liakhovetski,
add and use defines the index or irqs in platform data.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

v6
* As discussed with Guennadi Liakhovetski:
  - The SDCARD/SDIO change in v5 was not implemented as described.
    And the logic wasn't fully described in any case. This
    version (hope to!) implement the following logic:
    1) 1 IRQ: only resource #0 (CARD_DETECT, use tmio_mmc_irq())
    2) 2 or 3 IRQs: compulsory SDCARD and any further IRQs: use respective
       specialised ISRs.

v5
* As suggested by Guennadi Liakhovetski:
  - Allow only SH_MOBILE_SDHI_IRQ_SDCARD and
    SH_MOBILE_SDHI_IRQ_SDIO to be specified in platform data

v4
* As suggested by Guennadi Liakhovetski:
  - Correct inverted values of SH_MOBILE_SDHI_IRQ_SDCARD and
    SH_MOBILE_SDHI_IRQ_CARD_DETECT

v3
* Update for changes to "mmc: tmio: Provide separate interrupt handlers"
* As suggested by Guennadi Liakhovetski:
  - Merge in patch "mmc: sdhi: Add defines for platform irq indexes"
  - Use an enum instead of defines for irq indexes

v2
* Update for changes to "mmc: tmio: Provide separate interrupt handlers"
* Make use of defines provided by
  "mmc: sdhi: Add defines for platform irq indexes"
* As suggested by Guennadi Liakhovetski:
  - Don't use a loop to initialise irq handlers, the unrolled version
    is easier on the eyes (and exactly the same number of lines of code!)
---
 drivers/mmc/host/sh_mobile_sdhi.c  |   65 +++++++++++++++++++++++------------
 include/linux/mmc/sh_mobile_sdhi.h |    7 ++++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 774f643..2fa7bbc 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -96,7 +96,8 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
 	struct tmio_mmc_host *host;
 	char clk_name[8];
-	int i, irq, ret;
+	int irq, ret, irq_count = 0;
+	irqreturn_t (*f)(int irq, void *devid);
 
 	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
 	if (priv = NULL) {
@@ -153,27 +154,39 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto eprobe;
 
-	for (i = 0; i < 3; i++) {
-		irq = platform_get_irq(pdev, i);
-		if (irq < 0) {
-			if (i) {
-				continue;
-			} else {
-				ret = irq;
-				goto eirq;
-			}
-		}
-		ret = request_irq(irq, tmio_mmc_irq, 0,
+	/* Allow a single IRQ resource #0 (CARD_DETECT) which will
+	 * use tmio_mmc_irq() or;
+	 * Allow 2 or 3 IRQ resources in which case SDCARD is required
+	 * and specialised ISRs are used.
+	 */
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
+	if (irq >= 0) {
+		irq_count++;
+		ret = request_irq(irq, tmio_mmc_sdcard_irq, 0,
 				  dev_name(&pdev->dev), host);
-		if (ret) {
-			while (i--) {
-				irq = platform_get_irq(pdev, i);
-				if (irq >= 0)
-					free_irq(irq, host);
-			}
-			goto eirq;
-		}
+		if (ret)
+			goto eirq_sdcard;
 	}
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq_count && irq >= 0) {
+		irq_count++;
+		ret = request_irq(irq, tmio_mmc_sdio_irq, 0,
+				  dev_name(&pdev->dev), host);
+		if (ret)
+			goto eirq_sdio;
+	} else if (!irq_count)
+		goto eirq_sdio;
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
+	if (irq >= 0) {
+		f = irq_count ? tmio_mmc_card_detect_irq : tmio_mmc_irq;
+		ret = request_irq(irq, f, 0, dev_name(&pdev->dev), host);
+		if (ret)
+			goto eirq_card_detect;
+	} else if (irq_count < 2)
+		goto eirq_card_detect;
+
 	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
 		 mmc_hostname(host->mmc), (unsigned long)
 		 (platform_get_resource(pdev,IORESOURCE_MEM, 0)->start),
@@ -181,7 +194,15 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 
 	return ret;
 
-eirq:
+eirq_card_detect:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_sdio:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_sdcard:
 	tmio_mmc_host_remove(host);
 eprobe:
 	clk_disable(priv->clk);
@@ -203,7 +224,7 @@ static int sh_mobile_sdhi_remove(struct platform_device *pdev)
 
 	tmio_mmc_host_remove(host);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < SH_MOBILE_SDHI_IRQ_MAX; i++) {
 		irq = platform_get_irq(pdev, i);
 		if (irq >= 0)
 			free_irq(irq, host);
diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index bd50b36..80d3629 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -3,6 +3,13 @@
 
 #include <linux/types.h>
 
+enum {
+	SH_MOBILE_SDHI_IRQ_CARD_DETECT = 0,
+	SH_MOBILE_SDHI_IRQ_SDCARD,
+	SH_MOBILE_SDHI_IRQ_SDIO,
+	SH_MOBILE_SDHI_IRQ_MAX
+};
+
 struct platform_device;
 struct tmio_mmc_data;
 
-- 
1.7.5.4

[PATCH 3/4] mmc: sdhi: Make use of per-source irq handlers

[Simon Horman]

Make use of per-source irq handles if the
platform (data) has multiple irq sources.

Also, as suggested by Guennadi Liakhovetski,
add and use defines the index or irqs in platform data.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

v6
* As discussed with Guennadi Liakhovetski:
  - The SDCARD/SDIO change in v5 was not implemented as described.
    And the logic wasn't fully described in any case. This
    version (hope to!) implement the following logic:
    1) 1 IRQ: only resource #0 (CARD_DETECT, use tmio_mmc_irq())
    2) 2 or 3 IRQs: compulsory SDCARD and any further IRQs: use respective
       specialised ISRs.

v5
* As suggested by Guennadi Liakhovetski:
  - Allow only SH_MOBILE_SDHI_IRQ_SDCARD and
    SH_MOBILE_SDHI_IRQ_SDIO to be specified in platform data

v4
* As suggested by Guennadi Liakhovetski:
  - Correct inverted values of SH_MOBILE_SDHI_IRQ_SDCARD and
    SH_MOBILE_SDHI_IRQ_CARD_DETECT

v3
* Update for changes to "mmc: tmio: Provide separate interrupt handlers"
* As suggested by Guennadi Liakhovetski:
  - Merge in patch "mmc: sdhi: Add defines for platform irq indexes"
  - Use an enum instead of defines for irq indexes

v2
* Update for changes to "mmc: tmio: Provide separate interrupt handlers"
* Make use of defines provided by
  "mmc: sdhi: Add defines for platform irq indexes"
* As suggested by Guennadi Liakhovetski:
  - Don't use a loop to initialise irq handlers, the unrolled version
    is easier on the eyes (and exactly the same number of lines of code!)
---
 drivers/mmc/host/sh_mobile_sdhi.c  |   65 +++++++++++++++++++++++------------
 include/linux/mmc/sh_mobile_sdhi.h |    7 ++++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
index 774f643..2fa7bbc 100644
--- a/drivers/mmc/host/sh_mobile_sdhi.c
+++ b/drivers/mmc/host/sh_mobile_sdhi.c
@@ -96,7 +96,8 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	struct sh_mobile_sdhi_info *p = pdev->dev.platform_data;
 	struct tmio_mmc_host *host;
 	char clk_name[8];
-	int i, irq, ret;
+	int irq, ret, irq_count = 0;
+	irqreturn_t (*f)(int irq, void *devid);
 
 	priv = kzalloc(sizeof(struct sh_mobile_sdhi), GFP_KERNEL);
 	if (priv == NULL) {
@@ -153,27 +154,39 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto eprobe;
 
-	for (i = 0; i < 3; i++) {
-		irq = platform_get_irq(pdev, i);
-		if (irq < 0) {
-			if (i) {
-				continue;
-			} else {
-				ret = irq;
-				goto eirq;
-			}
-		}
-		ret = request_irq(irq, tmio_mmc_irq, 0,
+	/* Allow a single IRQ resource #0 (CARD_DETECT) which will
+	 * use tmio_mmc_irq() or;
+	 * Allow 2 or 3 IRQ resources in which case SDCARD is required
+	 * and specialised ISRs are used.
+	 */
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
+	if (irq >= 0) {
+		irq_count++;
+		ret = request_irq(irq, tmio_mmc_sdcard_irq, 0,
 				  dev_name(&pdev->dev), host);
-		if (ret) {
-			while (i--) {
-				irq = platform_get_irq(pdev, i);
-				if (irq >= 0)
-					free_irq(irq, host);
-			}
-			goto eirq;
-		}
+		if (ret)
+			goto eirq_sdcard;
 	}
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq_count && irq >= 0) {
+		irq_count++;
+		ret = request_irq(irq, tmio_mmc_sdio_irq, 0,
+				  dev_name(&pdev->dev), host);
+		if (ret)
+			goto eirq_sdio;
+	} else if (!irq_count)
+		goto eirq_sdio;
+
+	ret = irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_CARD_DETECT);
+	if (irq >= 0) {
+		f = irq_count ? tmio_mmc_card_detect_irq : tmio_mmc_irq;
+		ret = request_irq(irq, f, 0, dev_name(&pdev->dev), host);
+		if (ret)
+			goto eirq_card_detect;
+	} else if (irq_count < 2)
+		goto eirq_card_detect;
+
 	dev_info(&pdev->dev, "%s base at 0x%08lx clock rate %u MHz\n",
 		 mmc_hostname(host->mmc), (unsigned long)
 		 (platform_get_resource(pdev,IORESOURCE_MEM, 0)->start),
@@ -181,7 +194,15 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev)
 
 	return ret;
 
-eirq:
+eirq_card_detect:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDIO);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_sdio:
+	irq = platform_get_irq(pdev, SH_MOBILE_SDHI_IRQ_SDCARD);
+	if (irq >= 0)
+		free_irq(irq, host);
+eirq_sdcard:
 	tmio_mmc_host_remove(host);
 eprobe:
 	clk_disable(priv->clk);
@@ -203,7 +224,7 @@ static int sh_mobile_sdhi_remove(struct platform_device *pdev)
 
 	tmio_mmc_host_remove(host);
 
-	for (i = 0; i < 3; i++) {
+	for (i = 0; i < SH_MOBILE_SDHI_IRQ_MAX; i++) {
 		irq = platform_get_irq(pdev, i);
 		if (irq >= 0)
 			free_irq(irq, host);
diff --git a/include/linux/mmc/sh_mobile_sdhi.h b/include/linux/mmc/sh_mobile_sdhi.h
index bd50b36..80d3629 100644
--- a/include/linux/mmc/sh_mobile_sdhi.h
+++ b/include/linux/mmc/sh_mobile_sdhi.h
@@ -3,6 +3,13 @@
 
 #include <linux/types.h>
 
+enum {
+	SH_MOBILE_SDHI_IRQ_CARD_DETECT = 0,
+	SH_MOBILE_SDHI_IRQ_SDCARD,
+	SH_MOBILE_SDHI_IRQ_SDIO,
+	SH_MOBILE_SDHI_IRQ_MAX
+};
+
 struct platform_device;
 struct tmio_mmc_data;
 
-- 
1.7.5.4

[PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

This is intended to make it easier to correctly order IRQs.

As suggested by Guennadi Liakhovetski.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Depends on "mmc: sdhi: Make use of per-source irq handlers"

v4
* Update for corrected ordering of SH_MOBILE_SDHI_IRQ_SDCARD and
  SH_MOBILE_SDHI_IRQ_CARD_DETECT

v2
* Initial release
---
 arch/arm/mach-shmobile/board-ag5evm.c   |   12 ++++++------
 arch/arm/mach-shmobile/board-mackerel.c |   18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index ce5c251..0d543bb 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xee1000ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(83),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(84),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(85),
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -395,15 +395,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xee1200ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(87),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(88),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(89),
 		.flags	= IORESOURCE_IRQ,
 	},
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index d41c01f..f9d3a93 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1022,15 +1022,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xe68500ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0e00) /* SDHI0_SDHI0I0 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0e20) /* SDHI0_SDHI0I1 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0e40) /* SDHI0_SDHI0I2 */,
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1065,15 +1065,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xe68600ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0e80), /* SDHI1_SDHI1I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0ea0), /* SDHI1_SDHI1I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0ec0), /* SDHI1_SDHI1I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1116,15 +1116,15 @@ static struct resource sdhi2_resources[] = {
 		.end	= 0xe68700ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x1200), /* SDHI2_SDHI2I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x1220), /* SDHI2_SDHI2I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x1240), /* SDHI2_SDHI2I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
-- 
1.7.5.4

[PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

This is intended to make it easier to correctly order IRQs.

As suggested by Guennadi Liakhovetski.

Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

Depends on "mmc: sdhi: Make use of per-source irq handlers"

v4
* Update for corrected ordering of SH_MOBILE_SDHI_IRQ_SDCARD and
  SH_MOBILE_SDHI_IRQ_CARD_DETECT

v2
* Initial release
---
 arch/arm/mach-shmobile/board-ag5evm.c   |   12 ++++++------
 arch/arm/mach-shmobile/board-mackerel.c |   18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-ag5evm.c b/arch/arm/mach-shmobile/board-ag5evm.c
index ce5c251..0d543bb 100644
--- a/arch/arm/mach-shmobile/board-ag5evm.c
+++ b/arch/arm/mach-shmobile/board-ag5evm.c
@@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xee1000ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(83),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(84),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(85),
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -395,15 +395,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xee1200ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= gic_spi(87),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= gic_spi(88),
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= gic_spi(89),
 		.flags	= IORESOURCE_IRQ,
 	},
diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
index d41c01f..f9d3a93 100644
--- a/arch/arm/mach-shmobile/board-mackerel.c
+++ b/arch/arm/mach-shmobile/board-mackerel.c
@@ -1022,15 +1022,15 @@ static struct resource sdhi0_resources[] = {
 		.end	= 0xe68500ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0e00) /* SDHI0_SDHI0I0 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0e20) /* SDHI0_SDHI0I1 */,
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0e40) /* SDHI0_SDHI0I2 */,
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1065,15 +1065,15 @@ static struct resource sdhi1_resources[] = {
 		.end	= 0xe68600ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x0e80), /* SDHI1_SDHI1I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x0ea0), /* SDHI1_SDHI1I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x0ec0), /* SDHI1_SDHI1I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
@@ -1116,15 +1116,15 @@ static struct resource sdhi2_resources[] = {
 		.end	= 0xe68700ff,
 		.flags	= IORESOURCE_MEM,
 	},
-	[1] = {
+	[1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
 		.start	= evt2irq(0x1200), /* SDHI2_SDHI2I0 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[2] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
 		.start	= evt2irq(0x1220), /* SDHI2_SDHI2I1 */
 		.flags	= IORESOURCE_IRQ,
 	},
-	[3] = {
+	[1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
 		.start	= evt2irq(0x1240), /* SDHI2_SDHI2I2 */
 		.flags	= IORESOURCE_IRQ,
 	},
-- 
1.7.5.4

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> Provide separate interrupt handlers which may be used by platforms where
> SDHI has three interrupt sources.
>
> This patch also removes the commented-out handling of CRC and other errors.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---

> +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> +{
> +       struct tmio_mmc_host *host = devid;
> +       unsigned int ireg, status;
> +
> +       pr_debug("MMC IRQ begin\n");
> +
> +       tmio_mmc_card_irq_status(host, &ireg, &status);
> +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> +               return IRQ_HANDLED;
> +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> +               return IRQ_HANDLED;
> +
> +       tmio_mmc_sdio_irq(irq, devid);
>
> -out:
>        return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL(tmio_mmc_irq);

Is there any particular reason for returning early in this interrupt
handler? By returning early I mean the "if ... return IRQ_HANDLED"
cases above.

I realize the old ISR code in the driver does just this, so if the
goal is to stay compatible then I guess we should keep this behavior.
From my point of view it usually makes more sense to try to handle all
events that may be associated with the IRQ.

Thanks,

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> Provide separate interrupt handlers which may be used by platforms where
> SDHI has three interrupt sources.
>
> This patch also removes the commented-out handling of CRC and other errors.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---

> +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> +{
> +       struct tmio_mmc_host *host = devid;
> +       unsigned int ireg, status;
> +
> +       pr_debug("MMC IRQ begin\n");
> +
> +       tmio_mmc_card_irq_status(host, &ireg, &status);
> +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> +               return IRQ_HANDLED;
> +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> +               return IRQ_HANDLED;
> +
> +       tmio_mmc_sdio_irq(irq, devid);
>
> -out:
>        return IRQ_HANDLED;
>  }
>  EXPORT_SYMBOL(tmio_mmc_irq);

Is there any particular reason for returning early in this interrupt
handler? By returning early I mean the "if ... return IRQ_HANDLED"
cases above.

I realize the old ISR code in the driver does just this, so if the
goal is to stay compatible then I guess we should keep this behavior.
From my point of view it usually makes more sense to try to handle all
events that may be associated with the IRQ.

Thanks,

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > Provide separate interrupt handlers which may be used by platforms where
> > SDHI has three interrupt sources.
> >
> > This patch also removes the commented-out handling of CRC and other errors.
> >
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> 
> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > +{
> > +       struct tmio_mmc_host *host = devid;
> > +       unsigned int ireg, status;
> > +
> > +       pr_debug("MMC IRQ begin\n");
> > +
> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> > +               return IRQ_HANDLED;
> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> > +               return IRQ_HANDLED;
> > +
> > +       tmio_mmc_sdio_irq(irq, devid);
> >
> > -out:
> >        return IRQ_HANDLED;
> >  }
> >  EXPORT_SYMBOL(tmio_mmc_irq);
> 
> Is there any particular reason for returning early in this interrupt
> handler? By returning early I mean the "if ... return IRQ_HANDLED"
> cases above.
> 
> I realize the old ISR code in the driver does just this, so if the
> goal is to stay compatible then I guess we should keep this behavior.
> >From my point of view it usually makes more sense to try to handle all
> events that may be associated with the IRQ.

My original post had the behaviour that you suggest but Guennadi
indicted that he would be much more comfortable with keeping the original
behaviour as it is know to work on a wide range of hardware.

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > Provide separate interrupt handlers which may be used by platforms where
> > SDHI has three interrupt sources.
> >
> > This patch also removes the commented-out handling of CRC and other errors.
> >
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> 
> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> > +{
> > +       struct tmio_mmc_host *host = devid;
> > +       unsigned int ireg, status;
> > +
> > +       pr_debug("MMC IRQ begin\n");
> > +
> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> > +               return IRQ_HANDLED;
> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> > +               return IRQ_HANDLED;
> > +
> > +       tmio_mmc_sdio_irq(irq, devid);
> >
> > -out:
> >        return IRQ_HANDLED;
> >  }
> >  EXPORT_SYMBOL(tmio_mmc_irq);
> 
> Is there any particular reason for returning early in this interrupt
> handler? By returning early I mean the "if ... return IRQ_HANDLED"
> cases above.
> 
> I realize the old ISR code in the driver does just this, so if the
> goal is to stay compatible then I guess we should keep this behavior.
> >From my point of view it usually makes more sense to try to handle all
> events that may be associated with the IRQ.

My original post had the behaviour that you suggest but Guennadi
indicted that he would be much more comfortable with keeping the original
behaviour as it is know to work on a wide range of hardware.

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> This is intended to make it easier to correctly order IRQs.
>
> As suggested by Guennadi Liakhovetski.

> --- a/arch/arm/mach-shmobile/board-ag5evm.c
> +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
>                .end    = 0xee1000ff,
>                .flags  = IORESOURCE_MEM,
>        },
> -       [1] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
>                .start  = gic_spi(83),
>                .flags  = IORESOURCE_IRQ,
>        },
> -       [2] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>                .start  = gic_spi(84),
>                .flags  = IORESOURCE_IRQ,
>        },
> -       [3] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
>                .start  = gic_spi(85),
>                .flags  = IORESOURCE_IRQ,
>        },

Hm...

So I know you guys have been discussing this back and forth, so I'm
not sure if jumping in to this thread makes things any better. But
anyhow, here are my opinions. Feel free to ignore them. =)

First of all, having some kind of association with each IRQ is a good
thing. I am however not convinced that using the index number of the
platform device resource irq is the best option. Consider the case
when someone modifies the SDHI resource in the code above to only
include this interrupt:

        [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
                .start  = gic_spi(84),
                .flags  = IORESOURCE_IRQ,
        },

From my point of view, the common sense for this would be that only
the SDCARD interrupt would be enabled and the rest would be disabled
since they are unspecified. However, with the current code the
behavior would be something else, and since the index number of SDCARD
is not matching it will be detected as CARD_DETECT.

So isn't it really ugly to depend on the number of IRQs when they are
supposed to be used as an index? I've been toying around with this
driver for a few years now, and when I have a hard time creating
correct platform data then it's _probably_ a sign that there must be
better ways to implement this.

I would propose just adding interrupts in struct resource [] as usual,
and then have thee separate flags in the platform data for each
interrupt type. If the number of IRQ bits set in the platform data
flags doesn't match the number of interrupt resources then return
error. If they match then simply go through each flag set in the
platform data flags and assign next available interrupt resource. And
if no flags are set then go for the combined interrupt handler for all
available interrupt resources.

That's what I would do at least. Any other ideas? Perhaps just keep an
array of interrupt numbers in the platform data as the sh-sci driver
does and use the fixed indexes there if non-zero?

Thanks,

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> This is intended to make it easier to correctly order IRQs.
>
> As suggested by Guennadi Liakhovetski.

> --- a/arch/arm/mach-shmobile/board-ag5evm.c
> +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
>                .end    = 0xee1000ff,
>                .flags  = IORESOURCE_MEM,
>        },
> -       [1] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
>                .start  = gic_spi(83),
>                .flags  = IORESOURCE_IRQ,
>        },
> -       [2] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>                .start  = gic_spi(84),
>                .flags  = IORESOURCE_IRQ,
>        },
> -       [3] = {
> +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
>                .start  = gic_spi(85),
>                .flags  = IORESOURCE_IRQ,
>        },

Hm...

So I know you guys have been discussing this back and forth, so I'm
not sure if jumping in to this thread makes things any better. But
anyhow, here are my opinions. Feel free to ignore them. =)

First of all, having some kind of association with each IRQ is a good
thing. I am however not convinced that using the index number of the
platform device resource irq is the best option. Consider the case
when someone modifies the SDHI resource in the code above to only
include this interrupt:

        [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
                .start  = gic_spi(84),
                .flags  = IORESOURCE_IRQ,
        },

From my point of view, the common sense for this would be that only
the SDCARD interrupt would be enabled and the rest would be disabled
since they are unspecified. However, with the current code the
behavior would be something else, and since the index number of SDCARD
is not matching it will be detected as CARD_DETECT.

So isn't it really ugly to depend on the number of IRQs when they are
supposed to be used as an index? I've been toying around with this
driver for a few years now, and when I have a hard time creating
correct platform data then it's _probably_ a sign that there must be
better ways to implement this.

I would propose just adding interrupts in struct resource [] as usual,
and then have thee separate flags in the platform data for each
interrupt type. If the number of IRQ bits set in the platform data
flags doesn't match the number of interrupt resources then return
error. If they match then simply go through each flag set in the
platform data flags and assign next available interrupt resource. And
if no flags are set then go for the combined interrupt handler for all
available interrupt resources.

That's what I would do at least. Any other ideas? Perhaps just keep an
array of interrupt numbers in the platform data as the sh-sci driver
does and use the fixed indexes there if non-zero?

Thanks,

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
>> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> > Provide separate interrupt handlers which may be used by platforms where
>> > SDHI has three interrupt sources.
>> >
>> > This patch also removes the commented-out handling of CRC and other errors.
>> >
>> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > Cc: Magnus Damm <magnus.damm@gmail.com>
>> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >
>> > ---
>>
>> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
>> > +{
>> > +       struct tmio_mmc_host *host = devid;
>> > +       unsigned int ireg, status;
>> > +
>> > +       pr_debug("MMC IRQ begin\n");
>> > +
>> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
>> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
>> > +               return IRQ_HANDLED;
>> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
>> > +               return IRQ_HANDLED;
>> > +
>> > +       tmio_mmc_sdio_irq(irq, devid);
>> >
>> > -out:
>> >        return IRQ_HANDLED;
>> >  }
>> >  EXPORT_SYMBOL(tmio_mmc_irq);
>>
>> Is there any particular reason for returning early in this interrupt
>> handler? By returning early I mean the "if ... return IRQ_HANDLED"
>> cases above.
>>
>> I realize the old ISR code in the driver does just this, so if the
>> goal is to stay compatible then I guess we should keep this behavior.
>> >From my point of view it usually makes more sense to try to handle all
>> events that may be associated with the IRQ.
>
> My original post had the behaviour that you suggest but Guennadi
> indicted that he would be much more comfortable with keeping the original
> behaviour as it is know to work on a wide range of hardware.

I see, thanks for your patience...

So I may remember this wrong, but for the 3 different interrupt
sources I believe that the SDIO IRQ code was added by Arnd for one of
the Renesas SDHI platforms. Back then Ian disliked supporting more
than a single interrupt source, so for that reason the SDIO IRQ code
was added on top of the common interrupt handler. We had no
documentation either, so it was added in a rather random way. I recall
the SDIO IRQ being handled before the other interrupt types in the
common handler not last, but that's not very important. Anyway, with
the fact that SDIO IRQ support was added for SDHI platforms in mind
then we can assume that other platforms won't need it. Not sure if
this fact will improve our situation or not.

As for the two remaining interrupts, I believe they share hardware
registers somehow. I guess I'm OK keeping the original behavior
somehow, but I still believe it's incorrect. It may not matter very
much though since it's rather unlikely that hotplug insertion or eject
coincides with the data IRQs.

Thanks for your help!

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
>> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> > Provide separate interrupt handlers which may be used by platforms where
>> > SDHI has three interrupt sources.
>> >
>> > This patch also removes the commented-out handling of CRC and other errors.
>> >
>> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > Cc: Magnus Damm <magnus.damm@gmail.com>
>> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >
>> > ---
>>
>> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
>> > +{
>> > +       struct tmio_mmc_host *host = devid;
>> > +       unsigned int ireg, status;
>> > +
>> > +       pr_debug("MMC IRQ begin\n");
>> > +
>> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
>> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
>> > +               return IRQ_HANDLED;
>> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
>> > +               return IRQ_HANDLED;
>> > +
>> > +       tmio_mmc_sdio_irq(irq, devid);
>> >
>> > -out:
>> >        return IRQ_HANDLED;
>> >  }
>> >  EXPORT_SYMBOL(tmio_mmc_irq);
>>
>> Is there any particular reason for returning early in this interrupt
>> handler? By returning early I mean the "if ... return IRQ_HANDLED"
>> cases above.
>>
>> I realize the old ISR code in the driver does just this, so if the
>> goal is to stay compatible then I guess we should keep this behavior.
>> >From my point of view it usually makes more sense to try to handle all
>> events that may be associated with the IRQ.
>
> My original post had the behaviour that you suggest but Guennadi
> indicted that he would be much more comfortable with keeping the original
> behaviour as it is know to work on a wide range of hardware.

I see, thanks for your patience...

So I may remember this wrong, but for the 3 different interrupt
sources I believe that the SDIO IRQ code was added by Arnd for one of
the Renesas SDHI platforms. Back then Ian disliked supporting more
than a single interrupt source, so for that reason the SDIO IRQ code
was added on top of the common interrupt handler. We had no
documentation either, so it was added in a rather random way. I recall
the SDIO IRQ being handled before the other interrupt types in the
common handler not last, but that's not very important. Anyway, with
the fact that SDIO IRQ support was added for SDHI platforms in mind
then we can assume that other platforms won't need it. Not sure if
this fact will improve our situation or not.

As for the two remaining interrupts, I believe they share hardware
registers somehow. I guess I'm OK keeping the original behavior
somehow, but I still believe it's incorrect. It may not matter very
much though since it's rather unlikely that hotplug insertion or eject
coincides with the data IRQs.

Thanks for your help!

/ magnus

Re: [PATCH 1/4] mmc: tmio: Cache interrupt masks

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> This avoids the need to look up the masks each time an interrupt
> is handled.
>
> As suggested by Guennadi.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>

Good idea, thanks for this!

/ magnus

Re: [PATCH 1/4] mmc: tmio: Cache interrupt masks

[Magnus Damm]

On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> This avoids the need to look up the masks each time an interrupt
> is handled.
>
> As suggested by Guennadi.
>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>

Good idea, thanks for this!

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Fri, Aug 19, 2011 at 01:27:41PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
> >> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > Provide separate interrupt handlers which may be used by platforms where
> >> > SDHI has three interrupt sources.
> >> >
> >> > This patch also removes the commented-out handling of CRC and other errors.
> >> >
> >> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> > Cc: Magnus Damm <magnus.damm@gmail.com>
> >> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >> >
> >> > ---
> >>
> >> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> >> > +{
> >> > +       struct tmio_mmc_host *host = devid;
> >> > +       unsigned int ireg, status;
> >> > +
> >> > +       pr_debug("MMC IRQ begin\n");
> >> > +
> >> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
> >> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> >> > +               return IRQ_HANDLED;
> >> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> >> > +               return IRQ_HANDLED;
> >> > +
> >> > +       tmio_mmc_sdio_irq(irq, devid);
> >> >
> >> > -out:
> >> >        return IRQ_HANDLED;
> >> >  }
> >> >  EXPORT_SYMBOL(tmio_mmc_irq);
> >>
> >> Is there any particular reason for returning early in this interrupt
> >> handler? By returning early I mean the "if ... return IRQ_HANDLED"
> >> cases above.
> >>
> >> I realize the old ISR code in the driver does just this, so if the
> >> goal is to stay compatible then I guess we should keep this behavior.
> >> >From my point of view it usually makes more sense to try to handle all
> >> events that may be associated with the IRQ.
> >
> > My original post had the behaviour that you suggest but Guennadi
> > indicted that he would be much more comfortable with keeping the original
> > behaviour as it is know to work on a wide range of hardware.
> 
> I see, thanks for your patience...
> 
> So I may remember this wrong, but for the 3 different interrupt
> sources I believe that the SDIO IRQ code was added by Arnd for one of
> the Renesas SDHI platforms. Back then Ian disliked supporting more
> than a single interrupt source, so for that reason the SDIO IRQ code
> was added on top of the common interrupt handler. We had no
> documentation either, so it was added in a rather random way. I recall
> the SDIO IRQ being handled before the other interrupt types in the
> common handler not last, but that's not very important. Anyway, with
> the fact that SDIO IRQ support was added for SDHI platforms in mind
> then we can assume that other platforms won't need it. Not sure if
> this fact will improve our situation or not.
> 
> As for the two remaining interrupts, I believe they share hardware
> registers somehow. I guess I'm OK keeping the original behavior
> somehow, but I still believe it's incorrect. It may not matter very
> much though since it's rather unlikely that hotplug insertion or eject
> coincides with the data IRQs.

Hi Magnus,

to be honest I'm unsure if the current behaviour is correct or not.
But it does appear to work on a wide range of hardware. And as
tmio_mmc_irq() is an implementation of the single-source ISR intended
for legacy purposes it does seem reasonable to maintain the existing
behaviour.

If there is a need to change the behaviour of tmio_mmc_irq() in some way
then I think that is best dealt with in a separate patch (series)
subsequent to the current series that is intended to introduce broken-out
handlers.

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Simon Horman]

On Fri, Aug 19, 2011 at 01:27:41PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
> >> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > Provide separate interrupt handlers which may be used by platforms where
> >> > SDHI has three interrupt sources.
> >> >
> >> > This patch also removes the commented-out handling of CRC and other errors.
> >> >
> >> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> > Cc: Magnus Damm <magnus.damm@gmail.com>
> >> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >> >
> >> > ---
> >>
> >> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
> >> > +{
> >> > +       struct tmio_mmc_host *host = devid;
> >> > +       unsigned int ireg, status;
> >> > +
> >> > +       pr_debug("MMC IRQ begin\n");
> >> > +
> >> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
> >> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
> >> > +               return IRQ_HANDLED;
> >> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
> >> > +               return IRQ_HANDLED;
> >> > +
> >> > +       tmio_mmc_sdio_irq(irq, devid);
> >> >
> >> > -out:
> >> >        return IRQ_HANDLED;
> >> >  }
> >> >  EXPORT_SYMBOL(tmio_mmc_irq);
> >>
> >> Is there any particular reason for returning early in this interrupt
> >> handler? By returning early I mean the "if ... return IRQ_HANDLED"
> >> cases above.
> >>
> >> I realize the old ISR code in the driver does just this, so if the
> >> goal is to stay compatible then I guess we should keep this behavior.
> >> >From my point of view it usually makes more sense to try to handle all
> >> events that may be associated with the IRQ.
> >
> > My original post had the behaviour that you suggest but Guennadi
> > indicted that he would be much more comfortable with keeping the original
> > behaviour as it is know to work on a wide range of hardware.
> 
> I see, thanks for your patience...
> 
> So I may remember this wrong, but for the 3 different interrupt
> sources I believe that the SDIO IRQ code was added by Arnd for one of
> the Renesas SDHI platforms. Back then Ian disliked supporting more
> than a single interrupt source, so for that reason the SDIO IRQ code
> was added on top of the common interrupt handler. We had no
> documentation either, so it was added in a rather random way. I recall
> the SDIO IRQ being handled before the other interrupt types in the
> common handler not last, but that's not very important. Anyway, with
> the fact that SDIO IRQ support was added for SDHI platforms in mind
> then we can assume that other platforms won't need it. Not sure if
> this fact will improve our situation or not.
> 
> As for the two remaining interrupts, I believe they share hardware
> registers somehow. I guess I'm OK keeping the original behavior
> somehow, but I still believe it's incorrect. It may not matter very
> much though since it's rather unlikely that hotplug insertion or eject
> coincides with the data IRQs.

Hi Magnus,

to be honest I'm unsure if the current behaviour is correct or not.
But it does appear to work on a wide range of hardware. And as
tmio_mmc_irq() is an implementation of the single-source ISR intended
for legacy purposes it does seem reasonable to maintain the existing
behaviour.

If there is a need to change the behaviour of tmio_mmc_irq() in some way
then I think that is best dealt with in a separate patch (series)
subsequent to the current series that is intended to introduce broken-out
handlers.

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Simon Horman]

On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > This is intended to make it easier to correctly order IRQs.
> >
> > As suggested by Guennadi Liakhovetski.
> 
> > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> >                .end    = 0xee1000ff,
> >                .flags  = IORESOURCE_MEM,
> >        },
> > -       [1] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> >                .start  = gic_spi(83),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> > -       [2] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >                .start  = gic_spi(84),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> > -       [3] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> >                .start  = gic_spi(85),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> 
> Hm...
> 
> So I know you guys have been discussing this back and forth, so I'm
> not sure if jumping in to this thread makes things any better. But
> anyhow, here are my opinions. Feel free to ignore them. =)
> 
> First of all, having some kind of association with each IRQ is a good
> thing. I am however not convinced that using the index number of the
> platform device resource irq is the best option. Consider the case
> when someone modifies the SDHI resource in the code above to only
> include this interrupt:
> 
>         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>                 .start  = gic_spi(84),
>                 .flags  = IORESOURCE_IRQ,
>         },
> 
> >From my point of view, the common sense for this would be that only
> the SDCARD interrupt would be enabled and the rest would be disabled
> since they are unspecified. However, with the current code the
> behavior would be something else, and since the index number of SDCARD
> is not matching it will be detected as CARD_DETECT.
> 
> So isn't it really ugly to depend on the number of IRQs when they are
> supposed to be used as an index? I've been toying around with this
> driver for a few years now, and when I have a hard time creating
> correct platform data then it's _probably_ a sign that there must be
> better ways to implement this.
> 
> I would propose just adding interrupts in struct resource [] as usual,
> and then have thee separate flags in the platform data for each
> interrupt type. If the number of IRQ bits set in the platform data
> flags doesn't match the number of interrupt resources then return
> error. If they match then simply go through each flag set in the
> platform data flags and assign next available interrupt resource. And
> if no flags are set then go for the combined interrupt handler for all
> available interrupt resources.
> 
> That's what I would do at least. Any other ideas? Perhaps just keep an
> array of interrupt numbers in the platform data as the sh-sci driver
> does and use the fixed indexes there if non-zero?

Hi Magnus,

unfortunately during the course of the review of this series a number
of changes have crept in which I regard as being tangential to the
goal of the series - which is to introduce broken-out handlers (I have
already introduce support for broken-out IRQ sources).

One area where such changes have occurred is in the subtle altering of the
list of IRQ sources that it is valid to supply (again, support for
broken-out IRQ sources is not introduced by this series).

With regards to your comment and example above. I believe that your
understanding of my code is incorrect. The configuration you suggest will
be rejected because CARD_DETECT is not supplied, not because of an index
miss-match. It could be made acceptable within the current framework by
simply loosening up the logic a little (specifically to allow CARD_DETECT
to be omitted even if only one other IRQ source is supplied).  Incidently,
I think that would make sense but Guennadi specifically asked for that
combination to be regarded as invalid.

You suggestion to not rely on the array offset is an interesting one
and perhaps it would make things easier (to understand). But I don't
believe it would entirely eliminate the possibility of creating bogus
platform data. And in any case I believe it is tangential to the aim
of this series - to introduce broken-out handlers.

Moreover, I think that there needs to be some agreement between Guennadi
and yourself with regards to what IRQ combinations are valid before I (or
anyone else) can implement something that is acceptable to you both. And
I think that in the mean time the current implementation is reasonable
given that it works with all current use-cases (and several non-existent
ones too).

In short, can we have this discussion in the context of further enhancements?

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > This is intended to make it easier to correctly order IRQs.
> >
> > As suggested by Guennadi Liakhovetski.
> 
> > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> >                .end    = 0xee1000ff,
> >                .flags  = IORESOURCE_MEM,
> >        },
> > -       [1] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> >                .start  = gic_spi(83),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> > -       [2] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >                .start  = gic_spi(84),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> > -       [3] = {
> > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> >                .start  = gic_spi(85),
> >                .flags  = IORESOURCE_IRQ,
> >        },
> 
> Hm...
> 
> So I know you guys have been discussing this back and forth, so I'm
> not sure if jumping in to this thread makes things any better. But
> anyhow, here are my opinions. Feel free to ignore them. =)
> 
> First of all, having some kind of association with each IRQ is a good
> thing. I am however not convinced that using the index number of the
> platform device resource irq is the best option. Consider the case
> when someone modifies the SDHI resource in the code above to only
> include this interrupt:
> 
>         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>                 .start  = gic_spi(84),
>                 .flags  = IORESOURCE_IRQ,
>         },
> 
> >From my point of view, the common sense for this would be that only
> the SDCARD interrupt would be enabled and the rest would be disabled
> since they are unspecified. However, with the current code the
> behavior would be something else, and since the index number of SDCARD
> is not matching it will be detected as CARD_DETECT.
> 
> So isn't it really ugly to depend on the number of IRQs when they are
> supposed to be used as an index? I've been toying around with this
> driver for a few years now, and when I have a hard time creating
> correct platform data then it's _probably_ a sign that there must be
> better ways to implement this.
> 
> I would propose just adding interrupts in struct resource [] as usual,
> and then have thee separate flags in the platform data for each
> interrupt type. If the number of IRQ bits set in the platform data
> flags doesn't match the number of interrupt resources then return
> error. If they match then simply go through each flag set in the
> platform data flags and assign next available interrupt resource. And
> if no flags are set then go for the combined interrupt handler for all
> available interrupt resources.
> 
> That's what I would do at least. Any other ideas? Perhaps just keep an
> array of interrupt numbers in the platform data as the sh-sci driver
> does and use the fixed indexes there if non-zero?

Hi Magnus,

unfortunately during the course of the review of this series a number
of changes have crept in which I regard as being tangential to the
goal of the series - which is to introduce broken-out handlers (I have
already introduce support for broken-out IRQ sources).

One area where such changes have occurred is in the subtle altering of the
list of IRQ sources that it is valid to supply (again, support for
broken-out IRQ sources is not introduced by this series).

With regards to your comment and example above. I believe that your
understanding of my code is incorrect. The configuration you suggest will
be rejected because CARD_DETECT is not supplied, not because of an index
miss-match. It could be made acceptable within the current framework by
simply loosening up the logic a little (specifically to allow CARD_DETECT
to be omitted even if only one other IRQ source is supplied).  Incidently,
I think that would make sense but Guennadi specifically asked for that
combination to be regarded as invalid.

You suggestion to not rely on the array offset is an interesting one
and perhaps it would make things easier (to understand). But I don't
believe it would entirely eliminate the possibility of creating bogus
platform data. And in any case I believe it is tangential to the aim
of this series - to introduce broken-out handlers.

Moreover, I think that there needs to be some agreement between Guennadi
and yourself with regards to what IRQ combinations are valid before I (or
anyone else) can implement something that is acceptable to you both. And
I think that in the mean time the current implementation is reasonable
given that it works with all current use-cases (and several non-existent
ones too).

In short, can we have this discussion in the context of further enhancements?

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 1:59 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 01:27:41PM +0900, Magnus Damm wrote:
>> On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
>> > On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
>> >> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > Provide separate interrupt handlers which may be used by platforms where
>> >> > SDHI has three interrupt sources.
>> >> >
>> >> > This patch also removes the commented-out handling of CRC and other errors.
>> >> >
>> >> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> > Cc: Magnus Damm <magnus.damm@gmail.com>
>> >> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >> >
>> >> > ---
>> >>
>> >> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
>> >> > +{
>> >> > +       struct tmio_mmc_host *host = devid;
>> >> > +       unsigned int ireg, status;
>> >> > +
>> >> > +       pr_debug("MMC IRQ begin\n");
>> >> > +
>> >> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
>> >> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
>> >> > +               return IRQ_HANDLED;
>> >> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
>> >> > +               return IRQ_HANDLED;
>> >> > +
>> >> > +       tmio_mmc_sdio_irq(irq, devid);
>> >> >
>> >> > -out:
>> >> >        return IRQ_HANDLED;
>> >> >  }
>> >> >  EXPORT_SYMBOL(tmio_mmc_irq);
>> >>
>> >> Is there any particular reason for returning early in this interrupt
>> >> handler? By returning early I mean the "if ... return IRQ_HANDLED"
>> >> cases above.
>> >>
>> >> I realize the old ISR code in the driver does just this, so if the
>> >> goal is to stay compatible then I guess we should keep this behavior.
>> >> >From my point of view it usually makes more sense to try to handle all
>> >> events that may be associated with the IRQ.
>> >
>> > My original post had the behaviour that you suggest but Guennadi
>> > indicted that he would be much more comfortable with keeping the original
>> > behaviour as it is know to work on a wide range of hardware.
>>
>> I see, thanks for your patience...
>>
>> So I may remember this wrong, but for the 3 different interrupt
>> sources I believe that the SDIO IRQ code was added by Arnd for one of
>> the Renesas SDHI platforms. Back then Ian disliked supporting more
>> than a single interrupt source, so for that reason the SDIO IRQ code
>> was added on top of the common interrupt handler. We had no
>> documentation either, so it was added in a rather random way. I recall
>> the SDIO IRQ being handled before the other interrupt types in the
>> common handler not last, but that's not very important. Anyway, with
>> the fact that SDIO IRQ support was added for SDHI platforms in mind
>> then we can assume that other platforms won't need it. Not sure if
>> this fact will improve our situation or not.
>>
>> As for the two remaining interrupts, I believe they share hardware
>> registers somehow. I guess I'm OK keeping the original behavior
>> somehow, but I still believe it's incorrect. It may not matter very
>> much though since it's rather unlikely that hotplug insertion or eject
>> coincides with the data IRQs.
>
> Hi Magnus,
>
> to be honest I'm unsure if the current behaviour is correct or not.
> But it does appear to work on a wide range of hardware. And as
> tmio_mmc_irq() is an implementation of the single-source ISR intended
> for legacy purposes it does seem reasonable to maintain the existing
> behaviour.

Sure, I agree.

> If there is a need to change the behaviour of tmio_mmc_irq() in some way
> then I think that is best dealt with in a separate patch (series)
> subsequent to the current series that is intended to introduce broken-out
> handlers.

Sounds good, I agree that this should be handled separately if needed.

Thanks,

/ magnus

Re: [PATCH 2/4] mmc: tmio: Provide separate interrupt handlers

[Magnus Damm]

On Fri, Aug 19, 2011 at 1:59 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 01:27:41PM +0900, Magnus Damm wrote:
>> On Fri, Aug 19, 2011 at 12:30 PM, Simon Horman <horms@verge.net.au> wrote:
>> > On Fri, Aug 19, 2011 at 12:09:50PM +0900, Magnus Damm wrote:
>> >> On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> >> > Provide separate interrupt handlers which may be used by platforms where
>> >> > SDHI has three interrupt sources.
>> >> >
>> >> > This patch also removes the commented-out handling of CRC and other errors.
>> >> >
>> >> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> > Cc: Magnus Damm <magnus.damm@gmail.com>
>> >> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >> >
>> >> > ---
>> >>
>> >> > +irqreturn_t tmio_mmc_irq(int irq, void *devid)
>> >> > +{
>> >> > +       struct tmio_mmc_host *host = devid;
>> >> > +       unsigned int ireg, status;
>> >> > +
>> >> > +       pr_debug("MMC IRQ begin\n");
>> >> > +
>> >> > +       tmio_mmc_card_irq_status(host, &ireg, &status);
>> >> > +       if (__tmio_mmc_card_detect_irq(host, ireg, status))
>> >> > +               return IRQ_HANDLED;
>> >> > +       if (__tmio_mmc_sdcard_irq(host, ireg, status))
>> >> > +               return IRQ_HANDLED;
>> >> > +
>> >> > +       tmio_mmc_sdio_irq(irq, devid);
>> >> >
>> >> > -out:
>> >> >        return IRQ_HANDLED;
>> >> >  }
>> >> >  EXPORT_SYMBOL(tmio_mmc_irq);
>> >>
>> >> Is there any particular reason for returning early in this interrupt
>> >> handler? By returning early I mean the "if ... return IRQ_HANDLED"
>> >> cases above.
>> >>
>> >> I realize the old ISR code in the driver does just this, so if the
>> >> goal is to stay compatible then I guess we should keep this behavior.
>> >> >From my point of view it usually makes more sense to try to handle all
>> >> events that may be associated with the IRQ.
>> >
>> > My original post had the behaviour that you suggest but Guennadi
>> > indicted that he would be much more comfortable with keeping the original
>> > behaviour as it is know to work on a wide range of hardware.
>>
>> I see, thanks for your patience...
>>
>> So I may remember this wrong, but for the 3 different interrupt
>> sources I believe that the SDIO IRQ code was added by Arnd for one of
>> the Renesas SDHI platforms. Back then Ian disliked supporting more
>> than a single interrupt source, so for that reason the SDIO IRQ code
>> was added on top of the common interrupt handler. We had no
>> documentation either, so it was added in a rather random way. I recall
>> the SDIO IRQ being handled before the other interrupt types in the
>> common handler not last, but that's not very important. Anyway, with
>> the fact that SDIO IRQ support was added for SDHI platforms in mind
>> then we can assume that other platforms won't need it. Not sure if
>> this fact will improve our situation or not.
>>
>> As for the two remaining interrupts, I believe they share hardware
>> registers somehow. I guess I'm OK keeping the original behavior
>> somehow, but I still believe it's incorrect. It may not matter very
>> much though since it's rather unlikely that hotplug insertion or eject
>> coincides with the data IRQs.
>
> Hi Magnus,
>
> to be honest I'm unsure if the current behaviour is correct or not.
> But it does appear to work on a wide range of hardware. And as
> tmio_mmc_irq() is an implementation of the single-source ISR intended
> for legacy purposes it does seem reasonable to maintain the existing
> behaviour.

Sure, I agree.

> If there is a need to change the behaviour of tmio_mmc_irq() in some way
> then I think that is best dealt with in a separate patch (series)
> subsequent to the current series that is intended to introduce broken-out
> handlers.

Sounds good, I agree that this should be handled separately if needed.

Thanks,

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Simon Horman]

On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > > This is intended to make it easier to correctly order IRQs.
> > >
> > > As suggested by Guennadi Liakhovetski.
> > 
> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> > >                .end    = 0xee1000ff,
> > >                .flags  = IORESOURCE_MEM,
> > >        },
> > > -       [1] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> > >                .start  = gic_spi(83),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > > -       [2] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> > >                .start  = gic_spi(84),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > > -       [3] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> > >                .start  = gic_spi(85),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > 
> > Hm...
> > 
> > So I know you guys have been discussing this back and forth, so I'm
> > not sure if jumping in to this thread makes things any better. But
> > anyhow, here are my opinions. Feel free to ignore them. =)
> > 
> > First of all, having some kind of association with each IRQ is a good
> > thing. I am however not convinced that using the index number of the
> > platform device resource irq is the best option. Consider the case
> > when someone modifies the SDHI resource in the code above to only
> > include this interrupt:
> > 
> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >                 .start  = gic_spi(84),
> >                 .flags  = IORESOURCE_IRQ,
> >         },
> > 
> > >From my point of view, the common sense for this would be that only
> > the SDCARD interrupt would be enabled and the rest would be disabled
> > since they are unspecified. However, with the current code the
> > behavior would be something else, and since the index number of SDCARD
> > is not matching it will be detected as CARD_DETECT.
> > 
> > So isn't it really ugly to depend on the number of IRQs when they are
> > supposed to be used as an index? I've been toying around with this
> > driver for a few years now, and when I have a hard time creating
> > correct platform data then it's _probably_ a sign that there must be
> > better ways to implement this.
> > 
> > I would propose just adding interrupts in struct resource [] as usual,
> > and then have thee separate flags in the platform data for each
> > interrupt type. If the number of IRQ bits set in the platform data
> > flags doesn't match the number of interrupt resources then return
> > error. If they match then simply go through each flag set in the
> > platform data flags and assign next available interrupt resource. And
> > if no flags are set then go for the combined interrupt handler for all
> > available interrupt resources.
> > 
> > That's what I would do at least. Any other ideas? Perhaps just keep an
> > array of interrupt numbers in the platform data as the sh-sci driver
> > does and use the fixed indexes there if non-zero?
> 
> Hi Magnus,
> 
> unfortunately during the course of the review of this series a number
> of changes have crept in which I regard as being tangential to the
> goal of the series - which is to introduce broken-out handlers (I have
> already introduce support for broken-out IRQ sources).
> 
> One area where such changes have occurred is in the subtle altering of the
> list of IRQ sources that it is valid to supply (again, support for
> broken-out IRQ sources is not introduced by this series).
> 
> With regards to your comment and example above. I believe that your
> understanding of my code is incorrect. The configuration you suggest will
> be rejected because CARD_DETECT is not supplied, not because of an index
> miss-match. It could be made acceptable within the current framework by
> simply loosening up the logic a little (specifically to allow CARD_DETECT
> to be omitted even if only one other IRQ source is supplied).  Incidently,
> I think that would make sense but Guennadi specifically asked for that
> combination to be regarded as invalid.

[snip]

After some discussion off-line I now realise that there is a problem
with my implementation. Specifically that it assumes that
platform_get_irq() takes into account the index of resource entries -
it does not.

As we are already on the slippery slope of allowing combinations
other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
a variant of your flag idea. The variation being to use names instead
because a) that allows the use of platform_get_irq_byname() and b)
the flags bits seem to be full and not driver-specific.

And as are already on the slippery slope of allowing combinations
if IRQ sources beyond 1 and 3 I intend to implement logic to allow
any number of unique specific IRQ sources to be handled. In practice
hardware for some of these combinations is unlikely to exist. But that
doesn't seem to be a particularly good reason to add extra logic
to disallow them in the driver - its up to platform data to specify
something valid.

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> > > This is intended to make it easier to correctly order IRQs.
> > >
> > > As suggested by Guennadi Liakhovetski.
> > 
> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> > >                .end    = 0xee1000ff,
> > >                .flags  = IORESOURCE_MEM,
> > >        },
> > > -       [1] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> > >                .start  = gic_spi(83),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > > -       [2] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> > >                .start  = gic_spi(84),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > > -       [3] = {
> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> > >                .start  = gic_spi(85),
> > >                .flags  = IORESOURCE_IRQ,
> > >        },
> > 
> > Hm...
> > 
> > So I know you guys have been discussing this back and forth, so I'm
> > not sure if jumping in to this thread makes things any better. But
> > anyhow, here are my opinions. Feel free to ignore them. =)
> > 
> > First of all, having some kind of association with each IRQ is a good
> > thing. I am however not convinced that using the index number of the
> > platform device resource irq is the best option. Consider the case
> > when someone modifies the SDHI resource in the code above to only
> > include this interrupt:
> > 
> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >                 .start  = gic_spi(84),
> >                 .flags  = IORESOURCE_IRQ,
> >         },
> > 
> > >From my point of view, the common sense for this would be that only
> > the SDCARD interrupt would be enabled and the rest would be disabled
> > since they are unspecified. However, with the current code the
> > behavior would be something else, and since the index number of SDCARD
> > is not matching it will be detected as CARD_DETECT.
> > 
> > So isn't it really ugly to depend on the number of IRQs when they are
> > supposed to be used as an index? I've been toying around with this
> > driver for a few years now, and when I have a hard time creating
> > correct platform data then it's _probably_ a sign that there must be
> > better ways to implement this.
> > 
> > I would propose just adding interrupts in struct resource [] as usual,
> > and then have thee separate flags in the platform data for each
> > interrupt type. If the number of IRQ bits set in the platform data
> > flags doesn't match the number of interrupt resources then return
> > error. If they match then simply go through each flag set in the
> > platform data flags and assign next available interrupt resource. And
> > if no flags are set then go for the combined interrupt handler for all
> > available interrupt resources.
> > 
> > That's what I would do at least. Any other ideas? Perhaps just keep an
> > array of interrupt numbers in the platform data as the sh-sci driver
> > does and use the fixed indexes there if non-zero?
> 
> Hi Magnus,
> 
> unfortunately during the course of the review of this series a number
> of changes have crept in which I regard as being tangential to the
> goal of the series - which is to introduce broken-out handlers (I have
> already introduce support for broken-out IRQ sources).
> 
> One area where such changes have occurred is in the subtle altering of the
> list of IRQ sources that it is valid to supply (again, support for
> broken-out IRQ sources is not introduced by this series).
> 
> With regards to your comment and example above. I believe that your
> understanding of my code is incorrect. The configuration you suggest will
> be rejected because CARD_DETECT is not supplied, not because of an index
> miss-match. It could be made acceptable within the current framework by
> simply loosening up the logic a little (specifically to allow CARD_DETECT
> to be omitted even if only one other IRQ source is supplied).  Incidently,
> I think that would make sense but Guennadi specifically asked for that
> combination to be regarded as invalid.

[snip]

After some discussion off-line I now realise that there is a problem
with my implementation. Specifically that it assumes that
platform_get_irq() takes into account the index of resource entries -
it does not.

As we are already on the slippery slope of allowing combinations
other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
a variant of your flag idea. The variation being to use names instead
because a) that allows the use of platform_get_irq_byname() and b)
the flags bits seem to be full and not driver-specific.

And as are already on the slippery slope of allowing combinations
if IRQ sources beyond 1 and 3 I intend to implement logic to allow
any number of unique specific IRQ sources to be handled. In practice
hardware for some of these combinations is unlikely to exist. But that
doesn't seem to be a particularly good reason to add extra logic
to disallow them in the driver - its up to platform data to specify
something valid.

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 2:20 PM, Simon Horman <horms@verge.net.au> wrote:
...
> Moreover, I think that there needs to be some agreement between Guennadi
> and yourself with regards to what IRQ combinations are valid before I (or
> anyone else) can implement something that is acceptable to you both. And
> I think that in the mean time the current implementation is reasonable
> given that it works with all current use-cases (and several non-existent
> ones too).

The most common use cases for SDHI are:

Soldered-on SD/MMC hardware:
1) SDIO module (for wifi for instance)
Data IRQ + SDIO IRQ

2) eMMC chip
Data IRQ

External SD/MMC slot:
3) On hardware platforms with GPIO hotplug:
Data IRQ + SDIO IRQ

4) On hardware platforms without GPIO hotplug:
Hotplug IRQ + Data IRQ + SDIO IRQ

Legacy support:
5) Common ISR
All present IRQs

To keep things simple and flexible I would prefer if the platform
device could freely specify which interrupts to make use of. And still
have the legacy behavior.

Thanks,

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 2:20 PM, Simon Horman <horms@verge.net.au> wrote:
...
> Moreover, I think that there needs to be some agreement between Guennadi
> and yourself with regards to what IRQ combinations are valid before I (or
> anyone else) can implement something that is acceptable to you both. And
> I think that in the mean time the current implementation is reasonable
> given that it works with all current use-cases (and several non-existent
> ones too).

The most common use cases for SDHI are:

Soldered-on SD/MMC hardware:
1) SDIO module (for wifi for instance)
Data IRQ + SDIO IRQ

2) eMMC chip
Data IRQ

External SD/MMC slot:
3) On hardware platforms with GPIO hotplug:
Data IRQ + SDIO IRQ

4) On hardware platforms without GPIO hotplug:
Hotplug IRQ + Data IRQ + SDIO IRQ

Legacy support:
5) Common ISR
All present IRQs

To keep things simple and flexible I would prefer if the platform
device could freely specify which interrupts to make use of. And still
have the legacy behavior.

Thanks,

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
>> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
>> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> > > This is intended to make it easier to correctly order IRQs.
>> > >
>> > > As suggested by Guennadi Liakhovetski.
>> >
>> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
>> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
>> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
>> > >                .end    = 0xee1000ff,
>> > >                .flags  = IORESOURCE_MEM,
>> > >        },
>> > > -       [1] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
>> > >                .start  = gic_spi(83),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> > > -       [2] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>> > >                .start  = gic_spi(84),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> > > -       [3] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
>> > >                .start  = gic_spi(85),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> >
>> > Hm...
>> >
>> > So I know you guys have been discussing this back and forth, so I'm
>> > not sure if jumping in to this thread makes things any better. But
>> > anyhow, here are my opinions. Feel free to ignore them. =)
>> >
>> > First of all, having some kind of association with each IRQ is a good
>> > thing. I am however not convinced that using the index number of the
>> > platform device resource irq is the best option. Consider the case
>> > when someone modifies the SDHI resource in the code above to only
>> > include this interrupt:
>> >
>> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>> >                 .start  = gic_spi(84),
>> >                 .flags  = IORESOURCE_IRQ,
>> >         },
>> >
>> > >From my point of view, the common sense for this would be that only
>> > the SDCARD interrupt would be enabled and the rest would be disabled
>> > since they are unspecified. However, with the current code the
>> > behavior would be something else, and since the index number of SDCARD
>> > is not matching it will be detected as CARD_DETECT.
>> >
>> > So isn't it really ugly to depend on the number of IRQs when they are
>> > supposed to be used as an index? I've been toying around with this
>> > driver for a few years now, and when I have a hard time creating
>> > correct platform data then it's _probably_ a sign that there must be
>> > better ways to implement this.
>> >
>> > I would propose just adding interrupts in struct resource [] as usual,
>> > and then have thee separate flags in the platform data for each
>> > interrupt type. If the number of IRQ bits set in the platform data
>> > flags doesn't match the number of interrupt resources then return
>> > error. If they match then simply go through each flag set in the
>> > platform data flags and assign next available interrupt resource. And
>> > if no flags are set then go for the combined interrupt handler for all
>> > available interrupt resources.
>> >
>> > That's what I would do at least. Any other ideas? Perhaps just keep an
>> > array of interrupt numbers in the platform data as the sh-sci driver
>> > does and use the fixed indexes there if non-zero?
>>
>> Hi Magnus,
>>
>> unfortunately during the course of the review of this series a number
>> of changes have crept in which I regard as being tangential to the
>> goal of the series - which is to introduce broken-out handlers (I have
>> already introduce support for broken-out IRQ sources).
>>
>> One area where such changes have occurred is in the subtle altering of the
>> list of IRQ sources that it is valid to supply (again, support for
>> broken-out IRQ sources is not introduced by this series).
>>
>> With regards to your comment and example above. I believe that your
>> understanding of my code is incorrect. The configuration you suggest will
>> be rejected because CARD_DETECT is not supplied, not because of an index
>> miss-match. It could be made acceptable within the current framework by
>> simply loosening up the logic a little (specifically to allow CARD_DETECT
>> to be omitted even if only one other IRQ source is supplied).  Incidently,
>> I think that would make sense but Guennadi specifically asked for that
>> combination to be regarded as invalid.
>
> [snip]
>
> After some discussion off-line I now realise that there is a problem
> with my implementation. Specifically that it assumes that
> platform_get_irq() takes into account the index of resource entries -
> it does not.

Right, this is a thing that only bites you once. =)

> As we are already on the slippery slope of allowing combinations
> other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> a variant of your flag idea. The variation being to use names instead
> because a) that allows the use of platform_get_irq_byname() and b)
> the flags bits seem to be full and not driver-specific.

Great, platform_get_irq_byname() seems like a perfect match.

May I suggest "hotplug", "data" and "sdio" as names? I don't care very
much about names except keeping them short and precise to prevent
errors that can only be caught during runtime.

> And as are already on the slippery slope of allowing combinations
> if IRQ sources beyond 1 and 3 I intend to implement logic to allow
> any number of unique specific IRQ sources to be handled. In practice
> hardware for some of these combinations is unlikely to exist. But that
> doesn't seem to be a particularly good reason to add extra logic
> to disallow them in the driver - its up to platform data to specify
> something valid.

Yes, I totally agree. Thanks a lot for your help!

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Magnus Damm]

On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
>> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
>> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
>> > > This is intended to make it easier to correctly order IRQs.
>> > >
>> > > As suggested by Guennadi Liakhovetski.
>> >
>> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
>> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
>> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
>> > >                .end    = 0xee1000ff,
>> > >                .flags  = IORESOURCE_MEM,
>> > >        },
>> > > -       [1] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
>> > >                .start  = gic_spi(83),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> > > -       [2] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>> > >                .start  = gic_spi(84),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> > > -       [3] = {
>> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
>> > >                .start  = gic_spi(85),
>> > >                .flags  = IORESOURCE_IRQ,
>> > >        },
>> >
>> > Hm...
>> >
>> > So I know you guys have been discussing this back and forth, so I'm
>> > not sure if jumping in to this thread makes things any better. But
>> > anyhow, here are my opinions. Feel free to ignore them. =)
>> >
>> > First of all, having some kind of association with each IRQ is a good
>> > thing. I am however not convinced that using the index number of the
>> > platform device resource irq is the best option. Consider the case
>> > when someone modifies the SDHI resource in the code above to only
>> > include this interrupt:
>> >
>> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
>> >                 .start  = gic_spi(84),
>> >                 .flags  = IORESOURCE_IRQ,
>> >         },
>> >
>> > >From my point of view, the common sense for this would be that only
>> > the SDCARD interrupt would be enabled and the rest would be disabled
>> > since they are unspecified. However, with the current code the
>> > behavior would be something else, and since the index number of SDCARD
>> > is not matching it will be detected as CARD_DETECT.
>> >
>> > So isn't it really ugly to depend on the number of IRQs when they are
>> > supposed to be used as an index? I've been toying around with this
>> > driver for a few years now, and when I have a hard time creating
>> > correct platform data then it's _probably_ a sign that there must be
>> > better ways to implement this.
>> >
>> > I would propose just adding interrupts in struct resource [] as usual,
>> > and then have thee separate flags in the platform data for each
>> > interrupt type. If the number of IRQ bits set in the platform data
>> > flags doesn't match the number of interrupt resources then return
>> > error. If they match then simply go through each flag set in the
>> > platform data flags and assign next available interrupt resource. And
>> > if no flags are set then go for the combined interrupt handler for all
>> > available interrupt resources.
>> >
>> > That's what I would do at least. Any other ideas? Perhaps just keep an
>> > array of interrupt numbers in the platform data as the sh-sci driver
>> > does and use the fixed indexes there if non-zero?
>>
>> Hi Magnus,
>>
>> unfortunately during the course of the review of this series a number
>> of changes have crept in which I regard as being tangential to the
>> goal of the series - which is to introduce broken-out handlers (I have
>> already introduce support for broken-out IRQ sources).
>>
>> One area where such changes have occurred is in the subtle altering of the
>> list of IRQ sources that it is valid to supply (again, support for
>> broken-out IRQ sources is not introduced by this series).
>>
>> With regards to your comment and example above. I believe that your
>> understanding of my code is incorrect. The configuration you suggest will
>> be rejected because CARD_DETECT is not supplied, not because of an index
>> miss-match. It could be made acceptable within the current framework by
>> simply loosening up the logic a little (specifically to allow CARD_DETECT
>> to be omitted even if only one other IRQ source is supplied).  Incidently,
>> I think that would make sense but Guennadi specifically asked for that
>> combination to be regarded as invalid.
>
> [snip]
>
> After some discussion off-line I now realise that there is a problem
> with my implementation. Specifically that it assumes that
> platform_get_irq() takes into account the index of resource entries -
> it does not.

Right, this is a thing that only bites you once. =)

> As we are already on the slippery slope of allowing combinations
> other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> a variant of your flag idea. The variation being to use names instead
> because a) that allows the use of platform_get_irq_byname() and b)
> the flags bits seem to be full and not driver-specific.

Great, platform_get_irq_byname() seems like a perfect match.

May I suggest "hotplug", "data" and "sdio" as names? I don't care very
much about names except keeping them short and precise to prevent
errors that can only be caught during runtime.

> And as are already on the slippery slope of allowing combinations
> if IRQ sources beyond 1 and 3 I intend to implement logic to allow
> any number of unique specific IRQ sources to be handled. In practice
> hardware for some of these combinations is unlikely to exist. But that
> doesn't seem to be a particularly good reason to add extra logic
> to disallow them in the driver - its up to platform data to specify
> something valid.

Yes, I totally agree. Thanks a lot for your help!

/ magnus

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Simon Horman]

On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
> >> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> >> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > > This is intended to make it easier to correctly order IRQs.
> >> > >
> >> > > As suggested by Guennadi Liakhovetski.
> >> >
> >> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> >> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> >> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> >> > >                .end    = 0xee1000ff,
> >> > >                .flags  = IORESOURCE_MEM,
> >> > >        },
> >> > > -       [1] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> >> > >                .start  = gic_spi(83),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> > > -       [2] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >> > >                .start  = gic_spi(84),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> > > -       [3] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> >> > >                .start  = gic_spi(85),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> >
> >> > Hm...
> >> >
> >> > So I know you guys have been discussing this back and forth, so I'm
> >> > not sure if jumping in to this thread makes things any better. But
> >> > anyhow, here are my opinions. Feel free to ignore them. =)
> >> >
> >> > First of all, having some kind of association with each IRQ is a good
> >> > thing. I am however not convinced that using the index number of the
> >> > platform device resource irq is the best option. Consider the case
> >> > when someone modifies the SDHI resource in the code above to only
> >> > include this interrupt:
> >> >
> >> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >> >                 .start  = gic_spi(84),
> >> >                 .flags  = IORESOURCE_IRQ,
> >> >         },
> >> >
> >> > >From my point of view, the common sense for this would be that only
> >> > the SDCARD interrupt would be enabled and the rest would be disabled
> >> > since they are unspecified. However, with the current code the
> >> > behavior would be something else, and since the index number of SDCARD
> >> > is not matching it will be detected as CARD_DETECT.
> >> >
> >> > So isn't it really ugly to depend on the number of IRQs when they are
> >> > supposed to be used as an index? I've been toying around with this
> >> > driver for a few years now, and when I have a hard time creating
> >> > correct platform data then it's _probably_ a sign that there must be
> >> > better ways to implement this.
> >> >
> >> > I would propose just adding interrupts in struct resource [] as usual,
> >> > and then have thee separate flags in the platform data for each
> >> > interrupt type. If the number of IRQ bits set in the platform data
> >> > flags doesn't match the number of interrupt resources then return
> >> > error. If they match then simply go through each flag set in the
> >> > platform data flags and assign next available interrupt resource. And
> >> > if no flags are set then go for the combined interrupt handler for all
> >> > available interrupt resources.
> >> >
> >> > That's what I would do at least. Any other ideas? Perhaps just keep an
> >> > array of interrupt numbers in the platform data as the sh-sci driver
> >> > does and use the fixed indexes there if non-zero?
> >>
> >> Hi Magnus,
> >>
> >> unfortunately during the course of the review of this series a number
> >> of changes have crept in which I regard as being tangential to the
> >> goal of the series - which is to introduce broken-out handlers (I have
> >> already introduce support for broken-out IRQ sources).
> >>
> >> One area where such changes have occurred is in the subtle altering of the
> >> list of IRQ sources that it is valid to supply (again, support for
> >> broken-out IRQ sources is not introduced by this series).
> >>
> >> With regards to your comment and example above. I believe that your
> >> understanding of my code is incorrect. The configuration you suggest will
> >> be rejected because CARD_DETECT is not supplied, not because of an index
> >> miss-match. It could be made acceptable within the current framework by
> >> simply loosening up the logic a little (specifically to allow CARD_DETECT
> >> to be omitted even if only one other IRQ source is supplied).  Incidently,
> >> I think that would make sense but Guennadi specifically asked for that
> >> combination to be regarded as invalid.
> >
> > [snip]
> >
> > After some discussion off-line I now realise that there is a problem
> > with my implementation. Specifically that it assumes that
> > platform_get_irq() takes into account the index of resource entries -
> > it does not.
> 
> Right, this is a thing that only bites you once. =)
> 
> > As we are already on the slippery slope of allowing combinations
> > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > a variant of your flag idea. The variation being to use names instead
> > because a) that allows the use of platform_get_irq_byname() and b)
> > the flags bits seem to be full and not driver-specific.
> 
> Great, platform_get_irq_byname() seems like a perfect match.
> 
> May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> much about names except keeping them short and precise to prevent
> errors that can only be caught during runtime.

Earlier on in the life of this series Guennadi suggested
the names "card_detect", "sdcard" and "sdio". While I am
not particularly attached to those names the do seem
reasonable and are already used consistently by this series.
So I would prefer to use those names.

> > And as are already on the slippery slope of allowing combinations
> > if IRQ sources beyond 1 and 3 I intend to implement logic to allow
> > any number of unique specific IRQ sources to be handled. In practice
> > hardware for some of these combinations is unlikely to exist. But that
> > doesn't seem to be a particularly good reason to add extra logic
> > to disallow them in the driver - its up to platform data to specify
> > something valid.
> 
> Yes, I totally agree. Thanks a lot for your help!
> 
> / magnus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Fri, Aug 19, 2011 at 02:20:11PM +0900, Simon Horman wrote:
> >> On Fri, Aug 19, 2011 at 01:16:09PM +0900, Magnus Damm wrote:
> >> > On Fri, Aug 19, 2011 at 10:10 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > > This is intended to make it easier to correctly order IRQs.
> >> > >
> >> > > As suggested by Guennadi Liakhovetski.
> >> >
> >> > > --- a/arch/arm/mach-shmobile/board-ag5evm.c
> >> > > +++ b/arch/arm/mach-shmobile/board-ag5evm.c
> >> > > @@ -352,15 +352,15 @@ static struct resource sdhi0_resources[] = {
> >> > >                .end    = 0xee1000ff,
> >> > >                .flags  = IORESOURCE_MEM,
> >> > >        },
> >> > > -       [1] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_CARD_DETECT] = {
> >> > >                .start  = gic_spi(83),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> > > -       [2] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >> > >                .start  = gic_spi(84),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> > > -       [3] = {
> >> > > +       [1 + SH_MOBILE_SDHI_IRQ_SDIO] = {
> >> > >                .start  = gic_spi(85),
> >> > >                .flags  = IORESOURCE_IRQ,
> >> > >        },
> >> >
> >> > Hm...
> >> >
> >> > So I know you guys have been discussing this back and forth, so I'm
> >> > not sure if jumping in to this thread makes things any better. But
> >> > anyhow, here are my opinions. Feel free to ignore them. =)
> >> >
> >> > First of all, having some kind of association with each IRQ is a good
> >> > thing. I am however not convinced that using the index number of the
> >> > platform device resource irq is the best option. Consider the case
> >> > when someone modifies the SDHI resource in the code above to only
> >> > include this interrupt:
> >> >
> >> >         [1 + SH_MOBILE_SDHI_IRQ_SDCARD] = {
> >> >                 .start  = gic_spi(84),
> >> >                 .flags  = IORESOURCE_IRQ,
> >> >         },
> >> >
> >> > >From my point of view, the common sense for this would be that only
> >> > the SDCARD interrupt would be enabled and the rest would be disabled
> >> > since they are unspecified. However, with the current code the
> >> > behavior would be something else, and since the index number of SDCARD
> >> > is not matching it will be detected as CARD_DETECT.
> >> >
> >> > So isn't it really ugly to depend on the number of IRQs when they are
> >> > supposed to be used as an index? I've been toying around with this
> >> > driver for a few years now, and when I have a hard time creating
> >> > correct platform data then it's _probably_ a sign that there must be
> >> > better ways to implement this.
> >> >
> >> > I would propose just adding interrupts in struct resource [] as usual,
> >> > and then have thee separate flags in the platform data for each
> >> > interrupt type. If the number of IRQ bits set in the platform data
> >> > flags doesn't match the number of interrupt resources then return
> >> > error. If they match then simply go through each flag set in the
> >> > platform data flags and assign next available interrupt resource. And
> >> > if no flags are set then go for the combined interrupt handler for all
> >> > available interrupt resources.
> >> >
> >> > That's what I would do at least. Any other ideas? Perhaps just keep an
> >> > array of interrupt numbers in the platform data as the sh-sci driver
> >> > does and use the fixed indexes there if non-zero?
> >>
> >> Hi Magnus,
> >>
> >> unfortunately during the course of the review of this series a number
> >> of changes have crept in which I regard as being tangential to the
> >> goal of the series - which is to introduce broken-out handlers (I have
> >> already introduce support for broken-out IRQ sources).
> >>
> >> One area where such changes have occurred is in the subtle altering of the
> >> list of IRQ sources that it is valid to supply (again, support for
> >> broken-out IRQ sources is not introduced by this series).
> >>
> >> With regards to your comment and example above. I believe that your
> >> understanding of my code is incorrect. The configuration you suggest will
> >> be rejected because CARD_DETECT is not supplied, not because of an index
> >> miss-match. It could be made acceptable within the current framework by
> >> simply loosening up the logic a little (specifically to allow CARD_DETECT
> >> to be omitted even if only one other IRQ source is supplied).  Incidently,
> >> I think that would make sense but Guennadi specifically asked for that
> >> combination to be regarded as invalid.
> >
> > [snip]
> >
> > After some discussion off-line I now realise that there is a problem
> > with my implementation. Specifically that it assumes that
> > platform_get_irq() takes into account the index of resource entries -
> > it does not.
> 
> Right, this is a thing that only bites you once. =)
> 
> > As we are already on the slippery slope of allowing combinations
> > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > a variant of your flag idea. The variation being to use names instead
> > because a) that allows the use of platform_get_irq_byname() and b)
> > the flags bits seem to be full and not driver-specific.
> 
> Great, platform_get_irq_byname() seems like a perfect match.
> 
> May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> much about names except keeping them short and precise to prevent
> errors that can only be caught during runtime.

Earlier on in the life of this series Guennadi suggested
the names "card_detect", "sdcard" and "sdio". While I am
not particularly attached to those names the do seem
reasonable and are already used consistently by this series.
So I would prefer to use those names.

> > And as are already on the slippery slope of allowing combinations
> > if IRQ sources beyond 1 and 3 I intend to implement logic to allow
> > any number of unique specific IRQ sources to be handled. In practice
> > hardware for some of these combinations is unlikely to exist. But that
> > doesn't seem to be a particularly good reason to add extra logic
> > to disallow them in the driver - its up to platform data to specify
> > something valid.
> 
> Yes, I totally agree. Thanks a lot for your help!
> 
> / magnus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Guennadi Liakhovetski]

On Fri, 19 Aug 2011, Magnus Damm wrote:

> On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> > As we are already on the slippery slope of allowing combinations
> > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > a variant of your flag idea. The variation being to use names instead
> > because a) that allows the use of platform_get_irq_byname() and b)
> > the flags bits seem to be full and not driver-specific.
> 
> Great, platform_get_irq_byname() seems like a perfect match.

Indeed, good idea! This API is (unfortunately) so rarely used, that it was  
pretty much completely absent from my "active dictionary."

> May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> much about names except keeping them short and precise to prevent
> errors that can only be caught during runtime.

I don't care about names all that much either, but as Simon suggested in 
his other mail, consistency is good. Besides, yes, I thought about "data"
as a name for the main SDHI IRQ, but I decided it didn't reflect its 
function very well. Firstly, it is not only data, secondly, SDIO IRQs can 
also announce data availability, I think. The even more precise 
description of these two IRQs would be, perhaps, "synchronous" and 
"asynchronous," but that's probably not very intuitivy.

So, I'll accept any names, that you two agree upon:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Guennadi Liakhovetski]

On Fri, 19 Aug 2011, Magnus Damm wrote:

> On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> > As we are already on the slippery slope of allowing combinations
> > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > a variant of your flag idea. The variation being to use names instead
> > because a) that allows the use of platform_get_irq_byname() and b)
> > the flags bits seem to be full and not driver-specific.
> 
> Great, platform_get_irq_byname() seems like a perfect match.

Indeed, good idea! This API is (unfortunately) so rarely used, that it was  
pretty much completely absent from my "active dictionary."

> May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> much about names except keeping them short and precise to prevent
> errors that can only be caught during runtime.

I don't care about names all that much either, but as Simon suggested in 
his other mail, consistency is good. Besides, yes, I thought about "data"
as a name for the main SDHI IRQ, but I decided it didn't reflect its 
function very well. Firstly, it is not only data, secondly, SDIO IRQs can 
also announce data availability, I think. The even more precise 
description of these two IRQs would be, perhaps, "synchronous" and 
"asynchronous," but that's probably not very intuitivy.

So, I'll accept any names, that you two agree upon:-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Guennadi Liakhovetski]

On Fri, 19 Aug 2011, Simon Horman wrote:

> On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> > > As we are already on the slippery slope of allowing combinations
> > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > > a variant of your flag idea. The variation being to use names instead
> > > because a) that allows the use of platform_get_irq_byname() and b)
> > > the flags bits seem to be full and not driver-specific.
> > 
> > Great, platform_get_irq_byname() seems like a perfect match.
> > 
> > May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> > much about names except keeping them short and precise to prevent
> > errors that can only be caught during runtime.
> 
> Earlier on in the life of this series Guennadi suggested
> the names "card_detect", "sdcard" and "sdio". While I am
> not particularly attached to those names the do seem
> reasonable and are already used consistently by this series.
> So I would prefer to use those names.

Just one more thing I forgot to mention in the previous mail: using names 
also makes the transition simple: first patch all platforms with names (at 
least those with multiple IRQs, if your legacy fallback implementation 
will accept unnamed IRQs), and then patch sh_mobile_sdhi.c

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Guennadi Liakhovetski]

On Fri, 19 Aug 2011, Simon Horman wrote:

> On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:

[snip]

> > > As we are already on the slippery slope of allowing combinations
> > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > > a variant of your flag idea. The variation being to use names instead
> > > because a) that allows the use of platform_get_irq_byname() and b)
> > > the flags bits seem to be full and not driver-specific.
> > 
> > Great, platform_get_irq_byname() seems like a perfect match.
> > 
> > May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> > much about names except keeping them short and precise to prevent
> > errors that can only be caught during runtime.
> 
> Earlier on in the life of this series Guennadi suggested
> the names "card_detect", "sdcard" and "sdio". While I am
> not particularly attached to those names the do seem
> reasonable and are already used consistently by this series.
> So I would prefer to use those names.

Just one more thing I forgot to mention in the previous mail: using names 
also makes the transition simple: first patch all platforms with names (at 
least those with multiple IRQs, if your legacy fallback implementation 
will accept unnamed IRQs), and then patch sh_mobile_sdhi.c

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index

[Simon Horman]

On Fri, Aug 19, 2011 at 09:52:17AM +0200, Guennadi Liakhovetski wrote:
> On Fri, 19 Aug 2011, Simon Horman wrote:
> 
> > On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> > > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> 
> [snip]
> 
> > > > As we are already on the slippery slope of allowing combinations
> > > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > > > a variant of your flag idea. The variation being to use names instead
> > > > because a) that allows the use of platform_get_irq_byname() and b)
> > > > the flags bits seem to be full and not driver-specific.
> > > 
> > > Great, platform_get_irq_byname() seems like a perfect match.
> > > 
> > > May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> > > much about names except keeping them short and precise to prevent
> > > errors that can only be caught during runtime.
> > 
> > Earlier on in the life of this series Guennadi suggested
> > the names "card_detect", "sdcard" and "sdio". While I am
> > not particularly attached to those names the do seem
> > reasonable and are already used consistently by this series.
> > So I would prefer to use those names.
> 
> Just one more thing I forgot to mention in the previous mail: using names 
> also makes the transition simple: first patch all platforms with names (at 
> least those with multiple IRQs, if your legacy fallback implementation 
> will accept unnamed IRQs), and then patch sh_mobile_sdhi.c

Yes, I agree that is a nice feature of using names.

Re: [PATCH 4/4] ARM: shmobile: ag5evm, ap4: Make use of irq index enum

[Simon Horman]

On Fri, Aug 19, 2011 at 09:52:17AM +0200, Guennadi Liakhovetski wrote:
> On Fri, 19 Aug 2011, Simon Horman wrote:
> 
> > On Fri, Aug 19, 2011 at 03:51:49PM +0900, Magnus Damm wrote:
> > > On Fri, Aug 19, 2011 at 3:39 PM, Simon Horman <horms@verge.net.au> wrote:
> 
> [snip]
> 
> > > > As we are already on the slippery slope of allowing combinations
> > > > other than 1 (legacy) or 3 (specific) IRQ sources I plan to implement
> > > > a variant of your flag idea. The variation being to use names instead
> > > > because a) that allows the use of platform_get_irq_byname() and b)
> > > > the flags bits seem to be full and not driver-specific.
> > > 
> > > Great, platform_get_irq_byname() seems like a perfect match.
> > > 
> > > May I suggest "hotplug", "data" and "sdio" as names? I don't care very
> > > much about names except keeping them short and precise to prevent
> > > errors that can only be caught during runtime.
> > 
> > Earlier on in the life of this series Guennadi suggested
> > the names "card_detect", "sdcard" and "sdio". While I am
> > not particularly attached to those names the do seem
> > reasonable and are already used consistently by this series.
> > So I would prefer to use those names.
> 
> Just one more thing I forgot to mention in the previous mail: using names 
> also makes the transition simple: first patch all platforms with names (at 
> least those with multiple IRQs, if your legacy fallback implementation 
> will accept unnamed IRQs), and then patch sh_mobile_sdhi.c

Yes, I agree that is a nice feature of using names.