On Mon, 7 Mar 2011, adharmap at codeaurora.org wrote:

+static int __devinit pm8921_add_subdevices(const struct pm8921_platform_data
+					   *pdata,
+					   struct pm8921 *pmic,
+					   u32 rev)
+{
+	int ret = 0;
+	int irq_base = 0;
+	void *irq_data;
+
+	if (pdata->irq_pdata) {

So if pdata->irq_pdata == NULL you return success. Is that correct ?
Also please return early on (!pdata->irq_pdata) and avoid that extra
indent level for the real code path.

+		pdata->irq_pdata->irq_cdata.nirqs = PM8921_NR_IRQS;
+		pdata->irq_pdata->irq_cdata.rev = rev;
+		irq_base = pdata->irq_pdata->irq_base;
+		irq_data = pm8xxx_irq_init(pmic->dev, pdata->irq_pdata);
+
+		if (IS_ERR(irq_data)) {
+			pr_err("Failed to init interrupts ret=%ld\n",
+					PTR_ERR(irq_data));
+			ret = PTR_ERR(irq_data);
+			goto bail;

  			return PTR_ERR(irq_data);

And then you have
    	        }
		pmic->irq_data = irq_data;
		return 0;

+		} else
+			pmic->irq_data = irq_data;
+	}
+
+bail:
+	return ret;
+}

+struct pm_irq_chip {
+	struct list_head	link;
+	struct device		*dev;
+	spinlock_t		pm_irq_lock;
+	u8			*irqs_allowed;
+	u16			*irqs_to_handle;
+	u8			*config;
+	unsigned long		*wake_enable;
+	unsigned int		devirq;
+	unsigned int		count_wakeable;
+	unsigned int		irq_base;
+	unsigned int		num_irqs;
+	unsigned int		num_blocks;
+	unsigned int		num_masters;
+};
+
+static LIST_HEAD(pm_irq_chips);
+
+/* Helper Functions */
+static DEFINE_RATELIMIT_STATE(pm8xxx_irq_ratelimit, 60 * HZ, 10);
+
+static inline int pm8xxx_can_print(void)
+{
+	return __ratelimit(&pm8xxx_irq_ratelimit);
+}
+
+static int
+pm8xxx_read_root_irq(const struct pm_irq_chip *chip, u8 *rp)
+{
+	return pm8xxx_readb(chip->dev, SSBI_REG_ADDR_IRQ_ROOT, rp);
+}
+
+static int
+pm8xxx_read_master_irq(const struct pm_irq_chip *chip, u8 m, u8 *bp)
+{
+	return pm8xxx_readb(chip->dev,
+			SSBI_REG_ADDR_IRQ_M_STATUS1 + m, bp);
+}
+
+static int
+pm8xxx_read_block_irq(const struct pm_irq_chip *chip, u8 bp, u8 *ip)
+{
+	int	rc;
+
+	rc = pm8xxx_writeb(chip->dev,
+				SSBI_REG_ADDR_IRQ_BLK_SEL, bp);
+	if (rc) {
+		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
+		goto bail_out;

These goto's are silly. return rc; is fine here.

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

And how are the callers of this function serialized against the other
functions which access SSBI_REG_ADDR_IRQ_BLK_SEL ?

+	if (rc) {
+		pr_err("Failed Selecting Block %d rc=%d\n", bp, rc);
+		goto bail_out;
+	}
+
+	rc = pm8xxx_writeb(chip->dev,
+				SSBI_REG_ADDR_IRQ_CONFIG, cp);
+	if (rc)
+		pr_err("Failed Configuring IRQ rc=%d\n", rc);
+bail_out:
+	return rc;
+}
+
+static int pm8xxx_irq_block_handler(struct pm_irq_chip *chip,
+				int block, int *handled)
+{
+	int ret = 0;
+	u8 bits;
+	int pmirq, irq, k;

Can you please collapse all int variables into one line. Also why are
the iterators in your various functions randomly named i, j, k and
whatever? We usually use i for the first iterator and j when we have a
nested section.

+	spin_lock(&chip->pm_irq_lock);
+	ret = pm8xxx_read_block_irq(chip, block, &bits);
+	if (ret) {
+		if (pm8xxx_can_print())
+			pr_err("Failed reading %d block ret=%d",
+				block, ret);
+		goto out;
+	}

You can drop chip->pm_irq_lock here and return if (!bits)

+	if (!bits) {
+		if (pm8xxx_can_print())
+			pr_err("block bit set in master but no irqs: %d",
+				block);
+		goto out;
+	}
+
+	/* Check IRQ bits */
+	for (k = 0; k < 8; k++) {
+		if (bits & (1 << k)) {
+			pmirq = block * 8 + k;
+			irq = pmirq + chip->irq_base;
+			chip->irqs_to_handle[*handled] = irq;
+			(*handled)++;

Why all this horrible indirection? Why don't you call
generic_handle_irq() right away?

+		}
+	}
+out:
+	spin_unlock(&chip->pm_irq_lock);
+	return ret;
+}
+
+static int pm8xxx_irq_master_handler(struct pm_irq_chip *chip,
+					int master, int *handled)
+{
+	int ret = 0;
+	u8 blockbits;
+	int block_number, j;
+
+	ret = pm8xxx_read_master_irq(chip, master, &blockbits);
+	if (ret) {
+		pr_err("Failed to read master %d ret=%d\n", master, ret);
+		return ret;
+	}
+	if (!blockbits) {
+		if (pm8xxx_can_print())

What's the point of this ratelimit? This should not happen at all and
if it happens often enough that you need a rate limit then you better
figure out why and fix the real problem instead of papering over it.

+			pr_err("master bit set in root but no blocks: %d",
+				master);
+		return 0;
+	}
+
+	for (j = 0; j < 8; j++)
+		if (blockbits & (1 << j)) {
+			block_number = master * 8 + j;	/* block # */
+			ret |= pm8xxx_irq_block_handler(chip, block_number,
+								handled);
+		}
+	return ret;
+}
+
+static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct pm_irq_chip *chip = get_irq_data(irq);
+	int	i, ret;
+	u8	root;
+	int	masters = 0, handled = 0;
+
+	ret = pm8xxx_read_root_irq(chip, &root);
+	if (ret) {
+		pr_err("Can't read root status ret=%d\n", ret);
+		return;
+	}
+
+	/* on pm8xxx series masters start from bit 1 of the root */
+	masters = root >> 1;
+
+	/* Read allowed masters for blocks. */
+	for (i = 0; i < chip->num_masters; i++)
+		if (masters & (1 << i))
+			pm8xxx_irq_master_handler(chip, i, &handled);
+
+	for (i = 0; i < handled; i++)
+		generic_handle_irq(chip->irqs_to_handle[i]);
+
+	desc->chip->ack(irq);

  chip->irq_ack()

+}
+
+static void pm8xxx_irq_ack(struct irq_data *d)
+{
+	const struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = d->irq - chip->irq_base;
+	u8	block, config;
+
+	block = pmirq / 8;
+
+	config = PM_IRQF_WRITE | chip->config[pmirq] | PM_IRQF_CLR;
+	/* Keep the mask */
+	if (!(chip->irqs_allowed[block] & (1 << (pmirq % 8))))
+		config |= PM_IRQF_MASK_FE | PM_IRQF_MASK_RE;

What's the point of this exercise? ack is called before mask and it
should never be called when the interrupt is masked.

+	pm8xxx_config_irq(chip, block, config);
+}
+
+static void pm8xxx_irq_mask(struct irq_data *d)
+{
+	const struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = d->irq - chip->irq_base;
+	int	master, irq_bit;
+	u8	block, config;
+
+	block = pmirq / 8;
+	master = block / 8;
+	irq_bit = pmirq % 8;
+
+	chip->irqs_allowed[block] &= ~(1 << irq_bit);
+
+	config = PM_IRQF_WRITE | chip->config[pmirq] |
+		PM_IRQF_MASK_FE | PM_IRQF_MASK_RE;

Why don't you define PM_IRQF_MASK as PM_IRQF_MASK_FE | PM_IRQF_MASK_RE
and use this instead of having those line breaks.

Also every function which calls pm8xxx_config_irq() ORs
PM_IRQF_WRITE. Why can't you do that in pm8xxx_config_irq() and only
OR the real relevant bits in the various callers ?

+	pm8xxx_config_irq(chip, block, config);
+}
+
+static void pm8xxx_irq_unmask(struct irq_data *d)
+{
+	const struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = d->irq - chip->irq_base;
+	int	master, irq_bit;
+	u8	block, config, old_irqs_allowed;
+
+	block = pmirq / 8;
+	master = block / 8;
+	irq_bit = pmirq % 8;
+
+	old_irqs_allowed = chip->irqs_allowed[block];

  ???

+	chip->irqs_allowed[block] |= 1 << irq_bit;
+
+	config = PM_IRQF_WRITE | chip->config[pmirq];
+	pm8xxx_config_irq(chip, block, config);
+}
+
+static int pm8xxx_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+	const struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = d->irq - chip->irq_base;
+	int master, irq_bit;
+	u8 block, config;
+
+	block = pmirq / 8;
+	master = block / 8;
+	irq_bit  = pmirq % 8;
+
+	chip->config[pmirq] = (irq_bit << PM_IRQF_BITS_SHIFT) |
+			PM_IRQF_MASK_RE | PM_IRQF_MASK_FE;
+	if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
+		if (flow_type & IRQF_TRIGGER_RISING)
+			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
+		if (flow_type & IRQF_TRIGGER_FALLING)
+			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
+	} else {
+		chip->config[pmirq] |= PM_IRQF_LVL_SEL;
+
+		if (flow_type & IRQF_TRIGGER_HIGH)
+			chip->config[pmirq] &= ~PM_IRQF_MASK_RE;
+		else
+			chip->config[pmirq] &= ~PM_IRQF_MASK_FE;
+	}
+
+	config = PM_IRQF_WRITE
+		| chip->config[pmirq] | PM_IRQF_CLR;

Grrr. These random line breaks all over the place are horrible.

Also please make that:

     cfg = chip->config[pmirq] | PM_IRQF_WRITE | PM_IRQF_CLR;

So all the bits which you OR to the stored config are together.

+	return pm8xxx_config_irq(chip, block, config);
+}
+
+static int pm8xxx_irq_set_wake(struct irq_data *d, unsigned int on)
+{ 
+	struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
+	unsigned int pmirq = d->irq - chip->irq_base;
+
+	if (on) {
+		set_bit(pmirq, chip->wake_enable);
+		chip->count_wakeable++;
+	} else {
+		clear_bit(pmirq, chip->wake_enable);
+		chip->count_wakeable--;
+	}
+
+	return 0;
+}
+
+static struct irq_chip pm8xxx_irq_chip = {
+	.name		= "pm8xxx",
+	.irq_ack	= pm8xxx_irq_ack,
+	.irq_mask	= pm8xxx_irq_mask,
+	.irq_unmask	= pm8xxx_irq_unmask,
+	.irq_set_type	= pm8xxx_irq_set_type,
+	.irq_set_wake	= pm8xxx_irq_set_wake,
+};
+
+int pm8xxx_get_irq_stat(void *data, int irq)
+{
+	struct pm_irq_chip *chip = data;
+	int pmirq;
+	int     rc;
+	u8      block, bits, bit;
+	unsigned long flags;
+
+	if (chip == NULL || irq < chip->irq_base ||
+			irq >= chip->irq_base + chip->num_irqs)
+		return -EINVAL;
+
+	pmirq = irq - chip->irq_base;
+
+	block = pmirq / 8;
+	bit = pmirq % 8;
+
+	spin_lock_irqsave(&chip->pm_irq_lock, flags);
+
+	rc = pm8xxx_writeb(chip->dev,
+				SSBI_REG_ADDR_IRQ_BLK_SEL, block);
+	if (rc) {
+		pr_err("Failed Selecting block irq=%d pmirq=%d blk=%d rc=%d\n",
+			irq, pmirq, block, rc);
+		goto bail_out;
+	}
+
+	rc = pm8xxx_readb(chip->dev,
+				SSBI_REG_ADDR_IRQ_RT_STATUS, &bits);
+	if (rc) {
+		pr_err("Failed Configuring irq=%d pmirq=%d blk=%d rc=%d\n",
+			irq, pmirq, block, rc);
+		goto bail_out;
+	}
+
+	rc = (bits & (1 << bit)) ? 1 : 0;
+
+bail_out:
+	spin_unlock_irqrestore(&chip->pm_irq_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(pm8xxx_get_irq_stat);

EXPORT_SYMBOL_GPL if at all. Why needs this to be exported?

+
+#ifdef CONFIG_PM
+int pm8xxx_suspend_irq(const void *data)
+{
+	const struct pm_irq_chip *chip = data;
+	int pmirq;
+
+	for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) {
+		if (chip->config[i] && !test_bit(i, chip->wake_enable)) {
+			if (!((chip->config[i] & PM_IRQF_MASK_ALL)
+			      == PM_IRQF_MASK_ALL)) {
+				irq = i + chip->irq_base;
+				pm8xxx_irq_mask(get_irq_data(irq));
+			}
+		}
+	}
+
+	if (!chip->count_wakeable)
+		disable_irq(chip->dev->irq);
+
+	return 0;
+}
+
+void pm8xxx_show_resume_irq(void)
+{
+	struct pm_irq_chip *chip;
+	u8 block, bits;
+	int pmirq;
+
+	list_for_each_entry(chip, &pm_irq_chips, link) {
+		for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) {
+			if (test_bit(pmirq, chip->wake_enable)) {
+				block = pmirq / 8;
+				if (!pm8xxx_read_block_irq(chip,
+							&block, &bits)) {
+					if (bits & (1 << (pmirq & 0x7)))
+						pr_warning("%d triggered\n",
+						pmirq + chip->pdata.irq_base);
+				}
+			}
+		}
+	}
+}

What's the point of this function?

+int pm8xxx_resume_irq(const void *data)
+{
+	const struct pm_irq_chip *chip = data;
+	int pmirq;
+
+	for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) {
+		if (chip->config[i] && !test_bit(i, chip->wake_enable)) {
+			if (!((chip->config[i] & PM_IRQF_MASK_ALL)
+			      == PM_IRQF_MASK_ALL)) {
+				irq = i + chip->irq_base;
+				pm8xxx_irq_unmask(get_irq_data(irq));
+			}
+		}
+	}
+
+	if (!chip->count_wakeable)
+		enable_irq(chip->dev->irq);
+
+	return 0;
+}
+#else
+#define	pm8xxx_suspend		NULL
+#define	pm8xxx_resume		NULL

Where is pm8xxx_suspend/pm8xxx_resume defined for the !PM case and
where are those used at all ?

+#endif
+
+void * __devinit pm8xxx_irq_init(struct device *dev,
+				const struct pm8xxx_irq_platform_data *pdata)
+{
+	struct pm_irq_chip  *chip;
+	int devirq;
+	int rc;
+	unsigned int pmirq;
+
+	if (!pdata) {
+		pr_err("No platform data\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	devirq = pdata->devirq;
+	if (devirq < 0) {
+		pr_err("missing devirq\n");
+		rc = devirq;
+		goto out;
+	}
+
+	chip = kzalloc(sizeof(struct pm_irq_chip), GFP_KERNEL);
+	if (!chip) {
+		pr_err("Cannot alloc pm_irq_chip struct\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	chip->dev = dev;
+	chip->devirq = devirq;
+	chip->irq_base = pdata->irq_base;
+	chip->num_irqs = pdata->irq_cdata.nirqs;
+	chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
+	chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
+	spin_lock_init(&chip->pm_irq_lock);
+
+	chip->irqs_allowed = kzalloc(sizeof(u8) * chip->num_blocks, GFP_KERNEL);
+	if (!chip->irqs_allowed) {
+		pr_err("Cannot alloc irqs_allowed array\n");
+		rc = -ENOMEM;
+		goto free_pm_irq_chip;
+	}
+
+	chip->irqs_to_handle = kzalloc(sizeof(u16) * chip->num_irqs,
+								GFP_KERNEL);
+	if (!chip->irqs_to_handle) {
+		pr_err("Cannot alloc irqs_to_handle array\n");
+		rc = -ENOMEM;
+		goto free_irqs_allowed;
+	}
+	chip->config = kzalloc(sizeof(u8) * chip->num_irqs, GFP_KERNEL);
+	if (!chip->config) {
+		pr_err("Cannot alloc config array\n");
+		rc = -ENOMEM;
+		goto free_irqs_to_handle;
+	}
+	chip->wake_enable = kzalloc(sizeof(unsigned long)
+			* DIV_ROUND_UP(chip->num_irqs, BITS_PER_LONG),
+			GFP_KERNEL);
+	if (!chip->wake_enable) {
+		pr_err("Cannot alloc wake_enable array\n");
+		rc = -ENOMEM;
+		goto free_config;
+	}
+	list_add(&chip->link, &pm_irq_chips);

What's that list for and how is it protected ?

+	for (pmirq = 0; pmirq < chip->num_irqs; pmirq++) {
+		set_irq_chip(chip->irq_base + pmirq, &pm8xxx_irq_chip);
+		set_irq_chip_data(chip->irq_base + pmirq, chip);
+		set_irq_handler(chip->irq_base + pmirq, handle_level_irq);
+#ifdef CONFIG_ARM
+		set_irq_flags(chip->irq_base + pmirq, IRQF_VALID);
+#else
+		set_irq_noprobe(chip->irq_base + pmirq);
+#endif
+	}
+
+	set_irq_type(devirq, pdata->irq_trigger_flag);
+	set_irq_data(devirq, chip);
+	set_irq_chained_handler(devirq, pm8xxx_irq_handler);
+	set_irq_wake(devirq, 1);
+
+	return chip;
+
+free_config:
+	kfree(chip->config);
+free_irqs_to_handle:
+	kfree(chip->irqs_to_handle);
+free_irqs_allowed:
+	kfree(chip->irqs_allowed);

No need for 3 separate labels. You kzalloc() chip, so you can call
kfree() on chip->xxx unconditionally.

+free_pm_irq_chip:
+	kfree(chip);
+out:
+	return ERR_PTR(rc);
+}

+#ifdef CONFIG_MFD_PM8XXX_IRQ
+/**
+ * pm8xxx_get_irq_stat - get the status of the irq line
+ * @dev: the interrupt device
+ * @irq: the irq number
+ *
+ * The pm8xxx gpio and mpp rely on the interrupt block to read
+ * the values on their pins. This function is to facilitate reading
+ * the status of a gpio or an mpp line. The caller has to convert the
+ * gpio number to irq number.
+ *
+ * RETURNS:
+ * an int indicating the value read on that line
+ */

Please move that comment into the implementation.

+int pm8xxx_get_irq_stat(void *data, int irq);

Thanks,

	tglx