Re: [PM8921 MFD V3 2/6] mfd: pm8xxx: Add irq support

[Thomas Gleixner]

On Wed, 16 Mar 2011, adharmap@xxxxxxxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/