Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCHv3 -next] MFD: MFD module of DA9052 PMIC driver | Date | Sat, 11 Jun 2011 16:35:42 +0200 |
| |
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
| |