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