by Mark Brown

[permalink] [raw]

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

Attachments:

(No filename) (2.97 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-05-14 08:52:28

On Mon, May 14, 2012 at 07:26:31PM +0900, Graeme Gregory wrote:
> On 14/05/12 17:28, Mark Brown wrote:
> > 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.

> The IRQ map is not dense. It is split into 4 registers which are not
> contiguous. I think the overhead of translating to 4 reqmap irqs would
> negate the point of using regmap irq. I can add to TODO to add this
> handling to regmap_irq.

If the register blocks are nicely spaced we can probably find a
framework way to do this.

> I am confused on the whole irq_domain business, is it replacement for
> sparse irq? I don't see many users in drivers/mfd and not much
> Documentation.

regmap-irq in -next as of today has an example of using them for this
type of device (and of course half the point of regmap-irq is that it
avoids individual drivers needing to worry so much about things like
this, they can just use data!). I also posted a patch for wm831x though
that's got a bunch of noise in the function drivers due to passing the
interrupt numbers through as resources, the core should be fairly clear
though and is a bit more direct.

Attachments:

(No filename) (1.47 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2012-05-15 06:13:05

by Graeme Gregory

[permalink] [raw]

Subject: Re: [PATCH 3/4] REGULATOR: regulator driver for Palmas series chips

On 14/05/12 17:52, Mark Brown wrote:
> On Mon, May 14, 2012 at 10:58:31AM +0900, Graeme Gregory wrote:
>
> Looks good, quite a few of the things below are updates for very
> recently introduced APIs.
>
>> +static int palmas_smps_read(struct palmas *palmas, unsigned int reg,
>> + unsigned int *dest)
>> +{
>> + int slave;
>> + unsigned int addr;
>> +
>> + slave = PALMAS_BASE_TO_SLAVE(PALMAS_SMPS_BASE);
>> + addr = PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, reg);
>> +
>> + return regmap_read(palmas->regmap[slave], addr, dest);
>> +}
> Looks like the LDO and SPMS regulators both use the same regmap?
Yes have tidied this up a little in latest version.
>> + .get_voltage = palmas_get_voltage_smps,
> Why not get_voltage_sel?
Mild amnesia is my excuse, is fixed now!

>> + .set_voltage_sel = palmas_set_voltage_smps_sel,
>> + .list_voltage = palmas_list_voltage_smps,
> Implementing map_voltage() too would be nice.
I have implemented it for smps.
>> +static int palmas_is_enabled_smps10(struct regulator_dev *dev)
>> +{
>> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
>> + unsigned int reg;
>> +
>> + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_STATUS, &reg);
>> +
>> + reg &= SMPS10_BOOST_EN;
>> +
> Should be able to use regulator_is_enabled_regmap() and friends.
Has been converted thanks!

>> +static int palmas_list_voltage_smps10(struct regulator_dev *dev,
>> + unsigned selector)
>> +{
>> + if (selector)
>> + return 5000000;
>> +
>> + return 3750000;
> This could be written a little more transparently!
Have re-written it hopefully clearer.

>> +static int palmas_get_voltage_smps10(struct regulator_dev *dev)
>> +{
>> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
>> + int selector;
>> + unsigned int reg;
>> +
>> + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_CTRL, &reg);
>> +
>> + selector = (reg & SMPS10_VSEL) >> 3;
>> +
>> + return palmas_list_voltage_smps10(dev, selector);
> Should be get_voltage_sel() really, or beter still should be using
> regulator_set_voltage_sel_regmap(). Similarly for the set (which is
> already using _sel()).
I have implemented it using the regmap functions thanks for tip!

>> +static int palmas_enable_ldo(struct regulator_dev *dev)
>> +{
>> + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
>> + int id = rdev_get_id(dev);
>> + unsigned int reg;
>> +
>> + palmas_ldo_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, &reg);
>> +
>> + reg |= LDO1_CTRL_MODE_ACTIVE;
>> +
>> + palmas_ldo_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg);
> Could use the core regmap stuff for the LDOs too.
Enable/Disable now use regmap but is_enabled cant as it reads from a
different bitmask.

>> + /* Adjust selector to match list_voltage ranges */
>> + if (selector > 49)
>> + selector = 49;
>> +
>> + return palmas_list_voltage_ldo(dev, selector);
> get_voltage_sel().
Has been fixed!

>> + if (!pdata)
>> + return -EINVAL;
>> + if (!pdata->reg_data)
>> + return -EINVAL;
> I'd expect the driver to be able to register the regulators with no
> platform data at all.
Ok, I misunderstood how the pdata was used, hopefully I have got it
correct in the new version.

>> + pmic = kzalloc(sizeof(*pmic), GFP_KERNEL);
>> + if (!pmic)
>> + return -ENOMEM;
> devm_kzalloc()
Fixed!

>> + pmic->desc = kcalloc(PALMAS_NUM_REGS,
>> + sizeof(struct regulator_desc), GFP_KERNEL);
>> + if (!pmic->desc) {
>> + ret = -ENOMEM;
>> + goto err_free_pmic;
>> + }
> Just embed this in the pmic struct? It's always the same size.
Have embedded, this was a left over from when I was originally planning
the muxing handling.

>> + pmic->rdev = kcalloc(PALMAS_NUM_REGS,
>> + sizeof(struct regulator_dev *), GFP_KERNEL);
> Similarly heere.
Same as above.

Thanks for taking time to review. Am sending v2 patches soon.

Graeme

2012-05-15 18:11:53

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 3/4] REGULATOR: regulator driver for Palmas series chips

On Tue, May 15, 2012 at 03:12:45PM +0900, Graeme Gregory wrote:
> On 14/05/12 17:52, Mark Brown wrote:
> > On Mon, May 14, 2012 at 10:58:31AM +0900, Graeme Gregory wrote:

> >> +
> >> + palmas_ldo_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg);

> > Could use the core regmap stuff for the LDOs too.

> Enable/Disable now use regmap but is_enabled cant as it reads from a
> different bitmask.

Yeah, I did notice that and actually meant to go back and say that
really your current is_enabled() ought to be get_status(). The former
is supposed to return what the driver request to the hardware is, the
latter is supposed to return what the physical status is. Not super
critical though.

Attachments:

(No filename) (698.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments