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

[Mark Brown]

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

Attachment: signature.asc
Description: Digital signature