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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120718145032.GB4495@opensource.wolfsonmicro.com>
Date:	Wed, 18 Jul 2012 15:50:32 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	"Kim, Milo" <Milo.Kim@...com>
Cc:	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Girdwood, Liam" <lrg@...com>,
	Anton Vorontsov <cbouatmailru@...il.com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"a.zummo@...ertech.it" <a.zummo@...ertech.it>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Richard Purdie <rpurdie@...ys.net>,
	Bryan Wu <bryan.wu@...onical.com>
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...

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)