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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Mar 02, 2011 at 02:13:17PM -0800, adharmap@codeaurora.org wrote:
> 
> Change-Id: Ibb23878cd382af9a750d62ab49482f5dc72e3714
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

Remove the change IDs from upstream submissions.  The kernel doesn't use
gerritt.

>  struct pm8921 {
> -	struct device *dev;
> +	struct device			*dev;
> +	struct device			*irq_dev;

Is it really useful to register a struct device purely for the interrupt
controller?  I'd have expected this to be core functionality of the
device.  The fact that you need to store the device at all is a bit odd
too as you're using the MFD API.

>  static struct pm8xxx_drvdata pm8921_drvdata = {
> -	.pmic_readb	= pm8921_readb,
> -	.pmic_writeb	= pm8921_writeb,
> -	.pmic_read_buf	= pm8921_read_buf,
> -	.pmic_write_buf = pm8921_write_buf,
> +	.pmic_readb		= pm8921_readb,
> +	.pmic_writeb		= pm8921_writeb,
> +	.pmic_read_buf		= pm8921_read_buf,
> +	.pmic_write_buf		= pm8921_write_buf,
> +	.pmic_read_irq_stat	= pm8921_read_irq_stat,
> +};

It'd seem better to indent things as per the final driver in the first
patch - this reindentation creates a lot of noise in the diff.

>  		goto err_read_rev;
>  	}
> -	pr_info("PMIC revision:   %02X\n", val);
> +	pr_info("PMIC revision 1: %02X\n", val);
> +	rev = val;
>  

Again, do this in the first patch.

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

The namespacing here is odd, this looks like it should be a generic API
not a block specific one.

> +	/* Check IRQ bits */
> +	for (k = 0; k < 8; k++) {
> +		if (bits & (1 << k)) {
> +			pmirq = block * 8 + k;
> +			irq = pmirq + chip->irq_base;
> +			/* Check spurious interrupts */
> +			if (((1 << k) & chip->irqs_allowed[block])) {
> +				/* Found one */
> +				chip->irqs_to_handle[*handled] = irq;
> +				(*handled)++;
> +			} else { /* Clear and mask wrong one */
> +				config = PM_IRQF_W_C_M |
> +					(k << PM_IRQF_BITS_SHIFT);
> +
> +				pm8xxx_config_irq(chip,
> +						  block, config);
> +
> +				if (pm8xxx_can_print())
> +					pr_err("Spurious IRQ: %d "
> +					       "[block, bit]="
> +					       "[%d, %d]\n",
> +					       irq, block, k);
> +			}

The generic IRQ code should be able to take care of spurious interrupts
for you?  It's a bit surprising that there's all this logic - I'd expect
an IRQ chip to just defer logic about which interrupts are valid and so
on to the generic IRQ code.

>  #include <linux/device.h>
> +#include <linux/mfd/pm8xxx/irq.h>
> +
> +#define NR_PM8921_IRQS 256

Traditionally this'd be namespaced like this:

+#define PM8921_NR_IRQS 256