On Thursday, June 28, 2012 12:06 PM, Dan Carpenter wrote: > On Wed, Jun 27, 2012 at 02:56:43PM -0700, H Hartley Sweeten wrote: >> Add defines for the register map of the device. These will be >> used to clarify the code. >> >> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx> >> Cc: Ian Abbott <abbotti@xxxxxxxxx> >> Cc: Frank Mori Hess <fmhess@xxxxxxxxxxxxxxxxxxxxx> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >> --- >> drivers/staging/comedi/drivers/adl_pci6208.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/staging/comedi/drivers/adl_pci6208.c b/drivers/staging/comedi/drivers/adl_pci6208.c >> index f949d20..b6a8439 100644 >> --- a/drivers/staging/comedi/drivers/adl_pci6208.c >> +++ b/drivers/staging/comedi/drivers/adl_pci6208.c >> @@ -53,6 +53,18 @@ References: >> */ >> #include "../comedidev.h" >> >> +/* >> + * PCI-6208/6216-GL register map >> + */ >> +#define PCI6208_AO_CONTROL(x) (0x00 + (2 * (x))) >> +#define PCI6208_AO_STATUS 0x00 >> +#define PCI6208_AO_STATUS_DATA_SEND (1 << 0) >> +#define PCI6208_DIO 0x40 >> +#define PCI6208_DIO_DO_MASK (0x0f) >> +#define PCI6208_DIO_DO_SHIFT (0) >> +#define PCI6208_DIO_DI_MASK (0xf0) >> +#define PCI6208_DIO_DI_SHIFT (4) > > This series is nice and I'm not nacking anything, but really is it > that useful to say: > status = inw(dev->iobase + PCI6208_AO_STATUS); > instead of just?: > status = inw(dev->iobase); Either would work. But the '+ PCI6208_AO_STATUS' used in the function makes it easily apparent that the 'status' register is being read without having to go back and see what the assumed '+ 0' register is. > I'm not sure what the 0x00 in PCI6208_AO_CONTROL represents. Some > of these are not used like PCI6208_DIO_DI_SHIFT. Sorry about that. Maybe there should be a comment. The PCI-6208 has 8 separate "control" registers, one for each DAC output (the PCI-6216 has 16). They are at offsets 0x00, 0x02, 0x04, ... 0x0e (0x1e for The PCI-6216). The PCI6208_AO_CONTROL macro is used to calculate the necessary offset based on the DAC channel. The original code used the same open-coded logic. The unused ones can be removed. When I created the patch that added them I just added everything that might be useful from the manual for the PCI-6208. I was quite sure what ones would end up un used. > Does checkpatch.pl complain if you leave off these parenthesis? If > so I will complain again to the checkpatch.pl people. Extra > parenthesis are silly and there not used consistently. Only > PCI6208_AO_CONTROL() and PCI6208_AO_STATUS_DATA_SEND() need > paranthesis. No, checkpatch.pl does not complain about the parenthesis either way. I usually use the parenthesis in the 'bit' defines and not for the 'register' defines. It helps my brain keep them separate... ;-) But, they can be removed it needed. > Again, I'm fine with this patch and the whole series. These are > just comments. Thanks for the comments! Regards, Hartley _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
