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

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 11 June 2011 13:37:06 Mark Brown wrote:
> On Sat, Jun 11, 2011 at 12:49:04PM +0200, Arnd Bergmann wrote:

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

Ok, thanks for the explanation.

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

Right, except that in this case when you expand the macros, all you get is

{
	[ 0] = { 0x00000001 },
	[ 1] = { 0x00000002 },
	[ 2] = { 0x00000004 },
	[ 3] = { 0x00000008 },
	...
	[ n] = { 0x1 << n },
	...
	[30] = { 0x40000000 },
	[31] = { 0x80000000 },
}

This is entirely pointless for this particular driver. While I can
see good reasons to share idioms across similar drivers, this one
just doesn't need it. The only two functions where the data is used
AFAICT are da9052_irq_sync_unlock and da9052_irq_unmask, and both
could replace the table lookup with a trivial computation.

> > > +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 only looked at the first function in the list (da9052_adc_manual_read)
and noticed that it doesn't have any users at all. It's certainly
ok to export a complete API set when some functions belong together,
but I had the impression that in this case it wasn't actually clear
what the API is or should be.

Maybe an explanation about what da9052_adc_manual_read does or why
it's exported would be useful, I'm objecting the other exports.

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

Ah, very nice.

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

Right, they could go into drivers/mfd/da905x.h header.

	Arnd