lkml.org 
[lkml]   [2011]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Qualcomm PM8921 MFD 2/6] mfd: pm8xxx: Add irq support
    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


    \
     
     \ /
      Last update: 2011-03-02 23:49    [W:0.026 / U:30.528 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site