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

[Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>]

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