On Wed, 16 Mar 2011, adharmap at codeaurora.org wrote:

+static void pm8xxx_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct pm_irq_chip *chip = get_irq_data(irq);
+	struct irq_chip *irq_chip = get_irq_chip(irq);

Sigh. You have a reference to irq_desc, so why want you to get the
data with a sparse lookup ?

     chip = irq_desc_get_handler_data(desc);
     irq_chip = irq_desc_get_chip(desc);

+	u8	root;
+	int	i, ret, masters = 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);
+
+	irq_chip->irq_ack(&desc->irq_data);
+}
+
+static void pm8xxx_irq_ack(struct irq_data *d)
+{
+	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 = chip->config[pmirq] | PM_IRQF_CLR;
+	/* Keep the mask */
+	if (!(chip->irqs_allowed[block] & (1 << (pmirq % 8))))
+		config |= PM_IRQF_MASK_ALL;

Why do you insist to keep that state thing around? Implement a
mask_ack() callback and get rid of it.

+	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--;
+	}

This bitfield and the count are completely useless. Get rid of it.

+	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,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND,
+};
+
+/**
+ * pm8xxx_get_irq_stat - get the status of the irq line
+ * @dev: the interrupt device

Please run this through the doc generator. dev is not an argument to
this function.

+ * @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
+ */
+int pm8xxx_get_irq_stat(void *data, int irq)

Why is this void * ?

+void * __devinit pm8xxx_irq_init(struct device *dev,
+				const struct pm8xxx_irq_platform_data *pdata)

Please return struct pm_irq_chip *

You don't have to make the struct definition public. All you need in
the shared header file is a forward declaration.

struct pm_irq_chip;

The you can operate with type safe pointers at the callsites, but you
cannot dereference them there.

+{
+	struct pm_irq_chip  *chip;
+	int devirq, 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;
+		return ERR_PTR(-EINVAL);
+	}
+
+	chip = kzalloc(sizeof(struct pm_irq_chip), GFP_KERNEL);
+	if (!chip) {
+		pr_err("Cannot alloc pm_irq_chip struct\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	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_all;
+	}
+
+	chip->config = kzalloc(sizeof(u8) * chip->num_irqs, GFP_KERNEL);
+	if (!chip->config) {
+		pr_err("Cannot alloc config array\n");
+		rc = -ENOMEM;
+		goto free_all;
+	}
+	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_all;
+	}

If you got rid of all that state crap, then you can do:

struct pm_irq_chip {
       ...
       ...
       u8       config[0];
};

and allocate the whole thing in one go.

    chip = kzalloc(sizeof(struct pm_irq_chip) + sizeof(u8) * num_irqs, GFP_KERNEL);

 
+static inline int pm8xxx_read_irq_stat(const struct device *dev, int irq)
+{
+	struct pm8xxx_drvdata *dd = dev_get_drvdata(dev);
+
+	BUG_ON(!dd);

No need for BUG_ON(). You can handle that gracefully with a proper
return code.

+	return dd->pmic_read_irq_stat(dev, irq);
+}

Thanks,

	tglx