Message-ID: <alpine.LFD.2.00.1103081201520.2787@localhost6.localdomain6>
Date:	Tue, 8 Mar 2011 13:04:04 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Abhijeet Dharmapurikar <adharmap@...eaurora.org>
cc:	davidb@...eaurora.org, "David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	Daniel Walker <dwalker@...o99.com>,
	David Collins <collinsd@...eaurora.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Stepan Moskovchenko <stepanm@...eaurora.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Linus Walleij <linux.walleij@...rricsson.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Qualcomm PM8921 MFD v2 2/6] mfd: pm8xxx: Add irq support

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

Powered by blists - more mailing lists