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


    \
     
     \ /
      Last update: 2011-03-08 13:09    [W:0.060 / U:0.760 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site