Messages in this thread | | | Date | Wed, 18 Jul 2012 15:50:32 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/6] mfd: add lp8788 mfd driver |
| |
On Wed, Jul 18, 2012 at 02:32:40PM +0000, Kim, Milo wrote:
> +struct lp8788_irq_data { > + struct lp8788 *lp; > + struct irq_domain *irqdm; > + struct mutex irq_lock; > + struct delayed_work work; > + struct workqueue_struct *thread; > + int enabled[LP8788_INT_MAX]; > + int irq; > + int irq_base; > +};
Can you use regmap-irq? If not can we fix things so that's possible?
> +static irqreturn_t lp8788_irq_handler(int irq, void *ptr) > +{ > + struct lp8788_irq_data *irqd = ptr; > + unsigned long delay = msecs_to_jiffies(DEBOUNCE_MSEC); > + > + queue_delayed_work(irqd->thread, &irqd->work, delay); > + > + return IRQ_HANDLED; > +}
Why a delayed work? That's *very* unusual.
> + if (!lp->pdata) { > + dev_warn(lp->dev, "no platform data for irq\n"); > + goto no_err; > + }
Given that you're using irq domains why does the device need platform data?
> + if (irq_base) { > + irq_base = irq_alloc_descs(irq_base, 0, LP8788_INT_MAX, 0); > + if (irq_base < 0) { > + dev_warn(lp->dev, "no allocated irq: %d\n", irq_base); > + goto no_err; > + } > + }
This shouldn't be needed with irq domains.
> +EXPORT_SYMBOL(lp8788_read_byte);
You're reexporting regmap functionality with looser licensing requirements...
> +unsigned int lp8788_get_adc(struct lp8788 *lp, enum lp8788_adc_id id, > + enum lp8788_adc_resolution res)
For new drivers the ADC should probably be integrated into IIO.
> +static int lp8788_add_devices(struct lp8788 *lp) > +{ > + int ret; > + int irq_base = lp->pdata ? lp->pdata->irq_base : 0; > + > + ret = mfd_add_devices(lp->dev, -1, lp8788_devs, ARRAY_SIZE(lp8788_devs), > + NULL, irq_base);
Since you're using an irq domains you shouldn't need an irq base specifying in order to use interrupts.
> + if (!i2c_check_functionality(cl->adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) > + return -EIO;
I'd expect that's going to give some false negatives... [unhandled content-type:application/pgp-signature] | |