linux-kernel - Re: [PATCH 1/4] MFD: palmas PMIC device support

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]

Date:	Mon, 14 May 2012 09:28:33 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Graeme Gregory <gg@...mlogic.co.uk>
Cc:	linux-kernel@...r.kernel.org, sameo@...ux.intel.com, lrg@...com,
	b-cousson@...com, linux-omap@...r.kernel.org
Subject: Re: [PATCH 1/4] MFD: palmas PMIC device support

On Mon, May 14, 2012 at 10:58:29AM +0900, Graeme Gregory wrote:

>  drivers/mfd/palmas-irq.c   |  241 +++++

The IRQ support here seems to be following a pretty common pattern for
dense IRQ maps - could we factor it out into regmap-irq?  It'd also be
nice if it were using an irq_domain - while it's far from essential it
is something Grant has been pushing and I believe it'll be required when
you do device tree support.

> +	if (palmas->irq_mask != reg_mask) {
> +		addr = PALMAS_BASE_TO_REG(PALMAS_INTERRUPT_BASE,
> +				PALMAS_INT1_MASK);
> +		reg = palmas->irq_mask & 0xFF;
> +		regmap_write(palmas->regmap[slave], addr, reg);

This looks like you've open coded some regmap_update_bits() calls?

> +	if (!palmas->irq_base) {
> +		dev_warn(palmas->dev, "No interrupt support, no IRQ base\n");
> +		return -EINVAL;
> +	}

If you use an irqdomain you can automatically allocate the interrupt
range much more easily (even if you don't if you pass -1 as the base
irq_alloc_descs() it's supposed to figure things out to you - it looks
like you're not calling irq_alloc_decs()).

With the irqdomain you should also find that as the interrupts are
always registered (even if they can't fire) then you can just assume
they're there in function drivers which makes the code simpler.

> +	ret = request_threaded_irq(palmas->irq, NULL, palmas_irq, IRQF_ONESHOT,
> +				   "palmas-irq", palmas);
> +
> +	irq_set_irq_type(palmas->irq, IRQ_TYPE_LEVEL_LOW);

Why not just IRQF_TRIGGER_LOW?

> +static const struct mfd_cell palmas_children[] = {
> +};

I'd just go ahead and fill this in, it makes merging much easier as the
function drivers don't have a merge dependency on the core.

> +static const struct regmap_config palmas_regmap_config[PALMAS_NUM_CLIENTS] = {
> +	{
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.cache_type = REGCACHE_NONE,

Shouldn't need to explicitly set REGCACHE_NONE.  max_register might be
useful to get you register dumps in debugfs.

> +	palmas = kzalloc(sizeof(struct palmas), GFP_KERNEL);
> +	if (palmas == NULL)
> +		return -ENOMEM;

devm_kzalloc().

> +	ret = irq_alloc_descs(-1, 0, PALMAS_NUM_IRQ, 0);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "failed to allocate IRQ descs\n");
> +		goto err;
> +	}

Oh, here's the irq_alloc_descs() call - seems useful to put it in the
generic irq init?

> +		palmas->regmap[i] = regmap_init_i2c(palmas->i2c_clients[i],
> +				&palmas_regmap_config[i]);

devm_regmap_init_i2c().

> +static const struct i2c_device_id palmas_i2c_id[] = {
> +	{ "palmas", PALMAS_ID_TWL6035 },
> +};
> +MODULE_DEVICE_TABLE(i2c, palmas_i2c_id);

I'd suggest also including the part names as an option (so having an
entry for "twl6035") - makes life easier for users as they don't need to
think about if the device is compatible.

> +/* Bit definitions for MONTHS_REG */
> +#define MONTHS_REG_MONTH1					0x10
> +#define MONTHS_REG_MONTH1_SHIFT					4
> +#define MONTHS_REG_MONTH0_MASK					0x0f
> +#define MONTHS_REG_MONTH0_SHIFT					0

Some of these registers should be namespaced (many are already).

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