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

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, 8 Mar 2011, Abhijeet Dharmapurikar wrote:
> Thomas Gleixner wrote:
> > On Mon, 7 Mar 2011, adharmap@codeaurora.org wrote:
> > > +	if (pdata->irq_pdata) {
> > 
> > So if pdata->irq_pdata == NULL you return success. Is that correct ?
> 
> Yes. The board configuration may choose not to use pmic interrupts.

Ok.
 
> > Also please return early on (!pdata->irq_pdata) and avoid that extra
> > indent level for the real code path.
> 
> I did not do that because there are other subdevices that I will be adding in
> the later patches. I cannot return early. well I will change it for this
> patch.

Maybe splitting out the various init subsections into different
functions which are called from here might be a good thing.
 
> > > +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
> 
> The register design is such that we cannot only clear the interrupt. One has
> to write to the trigger bits while clearing it. Now trigger bits define
> whether the interrupt is masked or unmasked. If unmasked they define whether
> the interrupt rising/falling/level high/level low triggered.
> So the code remembers which interrupts are masked and for them it clears and
> rewrite the masked status in trigger bits. For unmasked ones it clears and
> writes to the trigger bits essentially configuring them same way as it was
> before. That is why the if satement to check interrupt was masked earlier,
> chip->irqs_allowed[] maintains which interrupt are unmasked.
> 
> > ack is called before mask and it
> > should never be called when the interrupt is masked.
> 
> I didnt quite understand this comment. handle_level_irq calls mask_ack which
> masks the interrupt and then acks it. In this case the ack is called after the

Indeed, sorry. So the right way to deal with that is to provide a
mask_ack() callback which does it in the correct order for your
HW. That way you avoid all the local state storage.

Thanks,

	tglx