On Wed, Mar 02, 2011 at 02:13:17PM -0800, adharmap at codeaurora.org wrote:

Change-Id: Ibb23878cd382af9a750d62ab49482f5dc72e3714
Signed-off-by: Abhijeet Dharmapurikar <redacted>

Remove the change IDs from upstream submissions.  The kernel doesn't use
gerritt.

 struct pm8921 {
-	struct device *dev;
+	struct device			*dev;
+	struct device			*irq_dev;

Is it really useful to register a struct device purely for the interrupt
controller?  I'd have expected this to be core functionality of the
device.  The fact that you need to store the device at all is a bit odd
too as you're using the MFD API.

 static struct pm8xxx_drvdata pm8921_drvdata = {
-	.pmic_readb	= pm8921_readb,
-	.pmic_writeb	= pm8921_writeb,
-	.pmic_read_buf	= pm8921_read_buf,
-	.pmic_write_buf = pm8921_write_buf,
+	.pmic_readb		= pm8921_readb,
+	.pmic_writeb		= pm8921_writeb,
+	.pmic_read_buf		= pm8921_read_buf,
+	.pmic_write_buf		= pm8921_write_buf,
+	.pmic_read_irq_stat	= pm8921_read_irq_stat,
+};

It'd seem better to indent things as per the final driver in the first
patch - this reindentation creates a lot of noise in the diff.

 		goto err_read_rev;
 	}
-	pr_info("PMIC revision:   %02X\n", val);
+	pr_info("PMIC revision 1: %02X\n", val);
+	rev = val;
 

Again, do this in the first patch.

+static int
+pm8xxx_read_block(const struct pm_irq_chip *chip, u8 bp, u8 *ip)
+{
+	int	rc;
+
+	rc = pm8xxx_writeb(chip->dev->parent,
+				SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
+	if (rc) {
+		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
+		goto bail_out;
+	}
+
+	rc = pm8xxx_readb(chip->dev->parent,
+			SSBI_REG_ADDR_IRQ_IT_STATUS, ip);
+	if (rc)
+		pr_err("Failed Reading Status rc=%d\n", rc);
+bail_out:
+	return rc;
+}

The namespacing here is odd, this looks like it should be a generic API
not a block specific one.

+	/* Check IRQ bits */
+	for (k = 0; k < 8; k++) {
+		if (bits & (1 << k)) {
+			pmirq = block * 8 + k;
+			irq = pmirq + chip->irq_base;
+			/* Check spurious interrupts */
+			if (((1 << k) & chip->irqs_allowed[block])) {
+				/* Found one */
+				chip->irqs_to_handle[*handled] = irq;
+				(*handled)++;
+			} else { /* Clear and mask wrong one */
+				config = PM_IRQF_W_C_M |
+					(k << PM_IRQF_BITS_SHIFT);
+
+				pm8xxx_config_irq(chip,
+						  block, config);
+
+				if (pm8xxx_can_print())
+					pr_err("Spurious IRQ: %d "
+					       "[block, bit]="
+					       "[%d, %d]\n",
+					       irq, block, k);
+			}

The generic IRQ code should be able to take care of spurious interrupts
for you?  It's a bit surprising that there's all this logic - I'd expect
an IRQ chip to just defer logic about which interrupts are valid and so
on to the generic IRQ code.

 #include <linux/device.h>
+#include <linux/mfd/pm8xxx/irq.h>
+
+#define NR_PM8921_IRQS 256

Traditionally this'd be namespaced like this:

+#define PM8921_NR_IRQS 256