Re: [PATCHv3 -next] MFD: MFD module of DA9052 PMIC driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Sat, Jun 11, 2011 at 12:49:04PM +0200, Arnd Bergmann wrote:

> > +static struct resource da9052_rtc_resource = {
> > +       .name = "ALM",
> > +       .start = DA9052_IRQ_ALARM,
> > +       .end   = DA9052_IRQ_ALARM,
> > +       .flags = IORESOURCE_IRQ,
> > +};
> > +
> > +static struct resource da9052_onkey_resource = {
> > +       .name = "ONKEY",
> > +       .start = DA9052_IRQ_NONKEY,
> > +       .end   = DA9052_IRQ_NONKEY,
> > +       .flags = IORESOURCE_IRQ,
> > +};
> > +
> > +static struct resource da9052_power_resources[] = {
> > +       {
> > +               .name = "CHGEND",
> > +               .start = DA9052_IRQ_CHGEND,
> > +               .end   = DA9052_IRQ_CHGEND,
> > +               .flags = IORESOURCE_IRQ,
> > +       },
> > +       {
> > +               .name = "TBAT",
> > +               .start = DA9052_IRQ_TBAT,
> > +               .end   = DA9052_IRQ_TBAT,
> > +               .flags = IORESOURCE_IRQ,
> > +       },
> > +};
> 
> I may have missed some major development here, but it seems to me that
> hardcoding interrupt numbers from a device driver does not work when
> those numbers conflict with other interrupt numbers. Can anyone explain
> how this works?

This is fine - it's all handled by the MFD core.  When a MFD registers
its subdevices it passes in a base interrupt and all the resources are
adjusted to be relative to that before the devices are instantiated.

> > +static struct da9052_irq_data da9052_irqs[] = {
> > +       [DA9052_IRQ_DCIN] = {
> > +               .mask = DA9052_IRQMASK_A_M_DCIN_VLD,
> > +               .offset = 0,
> > +       },
> > +       [DA9052_IRQ_VBUS] = {
> > +               .mask = DA9052_IRQMASK_A_M_VBUS_VLD,
> > +               .offset = 0,
> > +       },

> This long array would probably be more readable without the member names in it,
> especially since the struct only has two members:

This bit is reasonably idiomatic for the subsystem, mostly because
that's how I wrote the wm835x IRQ controller code (which had some
optionally used members originally though it doesn't any more) and lots
of people have drawn inspiration from it.

> static struct da9052_irq_data da9052_irqs[] = {
>         [DA9052_IRQ_DCIN]	= { DA9052_IRQMASK_A_M_DCIN_VLD, 0 },
>         [DA9052_IRQ_VBUS]	= { DA9052_IRQMASK_A_M_VBUS_VLD, 0 },
>         [DA9052_IRQ_DCINREM]	= { DA9052_IRQMASK_A_M_DCIN_REM, 0 },
>         [DA9052_IRQ_VBUSREM]	= { DA9052_IRQMASK_A_M_VBUS_REM, 0 },
> 	...
> };

> Since the DA9052_IRQMASK_... macros are only used in this one place,
> it would be even better to just get rid of the macros and open-code
> the contents here, to avoid having the reader look it up in another
> file:

Likewise here.  I did this for wm831x because the constants are
automatically generated for me and it allows one to map the code directly
onto the datasheet without having to work through numbers.  It doesn't
seem unreasonable for other people to take the same decision.

> > +int da9052_clear_bits(struct da9052 *da9052, unsigned char reg,
> > +                   unsigned char bit_mask);
> > +
> > +int da9052_device_init(struct da9052 *da9052);
> > +void da9052_device_exit(struct da9052 *da9052);

> > +int da9052_irq_init(struct da9052 *da9052, struct da9052_pdata *pdata);
> > +void da9052_irq_exit(struct da9052 *da9052);

> Not all of these functions are actually used by any of the client drivers,
> so please make them static if you don't need them.

This is fairly idiomatic for MFD drivers.  It makes life easier if we
can get the register I/O functions exported from the MFD when we need
them so we don't have to faff about doing cross tree stuff to export a
new function when you need it.  I'm not a big fan of having *all* the
I/O functions (many of them are redundant, you just need the whole
register and a single update bitmask operation) but it's reasonable for
drivers to do this.

I've got a regmap API I'm intending to post shortly which factors out
the register I/O code for most I2C and SPI devices and should mean that
drivers don't need to implement any of this stuff at all.  I just need
to bash ASoC into using it (it's a factoring out of the shared I/O code
ASoC has) but there's some infelicities in the ASoC code structure here
due to multiple past refactorings which make that more annoying than it
should be.

The device init/exit functions get shared between mulitple source files
in the mfd directory so they can't actually be static, though they don't
need to be fully exported.