On Saturday, March 01, 2014 3:28 AM, Chase Southwood wrote: > Subject: [PATCH 1/2] Staging: comedi: introduce outl_1564_* and inl_1564_* helper functions in hwdrv_apci1564.c > > This patch introduces a handful of outl and inl helper functions with the > ultimate goal of improving code readability and allowing several lines > which violate the character limit to be shortened in a sane way. > > Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > Signed-off-by: Chase Southwood <chase.southwood@xxxxxxxxx> > --- > This patchset serves as a replacement to my previous cleanup patchset for > hwdrv_apci1564.c > > Dan, > After spending a little bit more time with this and trying out different > ways of cleaning this up, I decided that making helper functions for all > of the most common register sets would look the best, but I haven't made > a helper for a few of the least common inl/outl calls because if I did, > the sheer number of helper functions would get quite ridiculous. > Let me know if you think my selections of what to make into helper > functions seems appropriate. Chase, Sorry for jumping in here late. I think if you just tidy up the register map defines it would help. Some of them are actually used incorrectly anyway. For instance, these two define the "base" offset for the digital input registers, which is also to digital input register, and the offset from the "base" to the interrupt mode1 register. #define APCI1564_DIGITAL_IP 0x04 #define APCI1564_DIGITAL_IP_INTERRUPT_MODE1 4 One use of the APCI1564_DIGITAL_IP_INTERRUPT_MODE1 define is in i_APCI1564_ConfigDigitalInput(): outl(data[2], devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); But in i_APCI1564_Reset () it is used as: outl(0x0, devpriv->i_IobaseAmcc + APCI1564_DIGITAL_IP_INTERRUPT_MODE1); So in the first example the driver is writing to a register at offset 0x08 (0x04 + 4) but the second is writing to a register at offset 0x04 (4), which is really the digitial input register. I suggest you just fix the register map defines so they are the "real" offsets to each register and not a mix of real offsets and adders to those offsets. I would also rename them to help with the > 80 char lines. Something like this: /* * devpriv->i_IobaseAmcc Register map */ #define APCI1564_DI_REG 0x04 #define APCI1564_DI_INT_MODE1_REG 0x08 #define APCI1564_DI_INT_MODE2_REG 0x0c #define APCI1564_DI_INT_STATUS_REG 0x10 #define APCI1564_DI_IRQ_REG 0x14 #define APCI1564_DO_REG 0x18 #define APCI1564_DO_INT_CTRL_REG 0x1c #define APCI1564_DO_INT_STATUS_REG 0x20 #define APCI1564_DO_IRQ_REG 0x24 #define APCI1564_WDOG_REG 0x28 #define APCI1564_WDOG_RELOAD_REG 0x2c #define APCI1564_WDOG_TIMEBASE_REG 0x30 #define APCI1564_WDOG_CTRL_REG 0x34 #define APCI1564_WDOG_STATUS_REG 0x38 #define APCI1564_WDOG_IRQ_REG 0x3c #define APCI1564_WDOG_WARN_TIMEVAL_REG 0x40 #define APCI1564_WDOG_WARN_TIMEBASE_REG 0x44 #define APCI1564_TIMER_REG 0x48 #define APCI1564_TIMER_RELOAD_REG 0x4c #define APCI1564_TIMER_TIMEBASE_REG 0x50 #define APCI1564_TIMER_CTRL_REG 0x54 #define APCI1564_TIMER_STATUS_REG 0x58 #define APCI1564_TIMER_IRQ_REG 0x5c #define APCI1564_TIMER_WARN_TIMEVAL_REG 0x60 #define APCI1564_TIMER_WARN_TIMEBASE_REG 0x64 /* * devpriv->iobase Register map */ #define APCI1564_TCW_REG(x) (0x00 + ((x) * 20)) #define APCI1564_TCW_RELOAD_REG(x) (0x04 + ((x) * 20)) #define APCI1564_TCW_TIMEBASE_REG(x) (0x08 + ((x) * 20)) #define APCI1564_TCW_CTRL_REG(x) (0x0c + ((x) * 20)) #define APCI1564_TCW_STATUS_REG(x) (0x10 + ((x) * 20)) #define APCI1564_TCW_IRQ_REG(x) (0x14 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEVAL_REG(x) (0x18 + ((x) * 20)) #define APCI1564_TCW_WARN_TIMEBASE_REG(x) (0x1c + ((x) * 20)) You will probably want to split this into a couple patches to make reviewing easier. Maybe do it by subdevice: digital input, digital output, watchdog, timer, then the counters. Regards, Hartley _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
