Re: [PATCH 1/6] mfd: add lp8788 mfd driver

From: Mark Brown
Date: Wed Jul 18 2012 - 10:50:30 EST


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...

Attachment: signature.asc
Description: Digital signature