Skip to contentSkip to conversation

comedi: dmm32at: Fix coding style - use BIT macro

PATCH In review Ranjith Thangavel 3 participants 5 messages Nov 14, 2015 – Nov 19, 2015 Staging

lore
Share & tools
lore

Ranjith Thangavel

author
#0 lore raw

PATCH 1/1 comedi: dmm32at: Fix coding style - use BIT macro

BIT macro is used for defining BIT location instead of shifting operator, usleep_range is preferred over udelay - coding style issue

Signed-off-by Ranjith Thangavel <[email protected]>
---
 drivers/staging/comedi/drivers/dmm32at.c |  104 +++++++++++++++---------------
 1 file changed, 52 insertions(+), 52 deletions(-)
drivers/staging/comedi/drivers/dmm32at.c +52 -52
index 958c0d4..0836d39 100644
@@ -46,74 +46,74 @@
 #define DMM32AT_AI_START_CONV_REG	0x00
 #define DMM32AT_AI_LSB_REG		0x00
 #define DMM32AT_AUX_DOUT_REG		0x01
-#define DMM32AT_AUX_DOUT2		(1 << 2)  /* J3.42 - OUT2 (OUT2EN) */
-#define DMM32AT_AUX_DOUT1		(1 << 1)  /* J3.43 */
-#define DMM32AT_AUX_DOUT0		(1 << 0)  /* J3.44 - OUT0 (OUT0EN) */
+#define DMM32AT_AUX_DOUT2		BIT(2)  /* J3.42 - OUT2 (OUT2EN) */
+#define DMM32AT_AUX_DOUT1		BIT(1)  /* J3.43 */
+#define DMM32AT_AUX_DOUT0		BIT(0)  /* J3.44 - OUT0 (OUT0EN) */
 #define DMM32AT_AI_MSB_REG		0x01
 #define DMM32AT_AI_LO_CHAN_REG		0x02
 #define DMM32AT_AI_HI_CHAN_REG		0x03
 #define DMM32AT_AUX_DI_REG		0x04
-#define DMM32AT_AUX_DI_DACBUSY		(1 << 7)
-#define DMM32AT_AUX_DI_CALBUSY		(1 << 6)
-#define DMM32AT_AUX_DI3			(1 << 3)  /* J3.45 - ADCLK (CLKSEL) */
-#define DMM32AT_AUX_DI2			(1 << 2)  /* J3.46 - GATE12 (GT12EN) */
-#define DMM32AT_AUX_DI1			(1 << 1)  /* J3.47 - GATE0 (GT0EN) */
-#define DMM32AT_AUX_DI0			(1 << 0)  /* J3.48 - CLK0 (SRC0) */
+#define DMM32AT_AUX_DI_DACBUSY		BIT(7)
+#define DMM32AT_AUX_DI_CALBUSY		BIT(6)
+#define DMM32AT_AUX_DI3			BIT(3)  /* J3.45 - ADCLK (CLKSEL) */
+#define DMM32AT_AUX_DI2			BIT(2)  /* J3.46 - GATE12 (GT12EN) */
+#define DMM32AT_AUX_DI1			BIT(1)  /* J3.47 - GATE0 (GT0EN) */
+#define DMM32AT_AUX_DI0			BIT(0)  /* J3.48 - CLK0 (SRC0) */
 #define DMM32AT_AO_LSB_REG		0x04
 #define DMM32AT_AO_MSB_REG		0x05
 #define DMM32AT_AO_MSB_DACH(x)		((x) << 6)
 #define DMM32AT_FIFO_DEPTH_REG		0x06
 #define DMM32AT_FIFO_CTRL_REG		0x07
-#define DMM32AT_FIFO_CTRL_FIFOEN	(1 << 3)
-#define DMM32AT_FIFO_CTRL_SCANEN	(1 << 2)
-#define DMM32AT_FIFO_CTRL_FIFORST	(1 << 1)
+#define DMM32AT_FIFO_CTRL_FIFOEN	BIT(3)
+#define DMM32AT_FIFO_CTRL_SCANEN	BIT(2)
+#define DMM32AT_FIFO_CTRL_FIFORST	BIT(1)
 #define DMM32AT_FIFO_STATUS_REG		0x07
-#define DMM32AT_FIFO_STATUS_EF		(1 << 7)
-#define DMM32AT_FIFO_STATUS_HF		(1 << 6)
-#define DMM32AT_FIFO_STATUS_FF		(1 << 5)
-#define DMM32AT_FIFO_STATUS_OVF		(1 << 4)
-#define DMM32AT_FIFO_STATUS_FIFOEN	(1 << 3)
-#define DMM32AT_FIFO_STATUS_SCANEN	(1 << 2)
-#define DMM32AT_FIFO_STATUS_PAGE_MASK	(3 << 0)
+#define DMM32AT_FIFO_STATUS_EF		BIT(7)
+#define DMM32AT_FIFO_STATUS_HF		BIT(6)
+#define DMM32AT_FIFO_STATUS_FF		BIT(5)
+#define DMM32AT_FIFO_STATUS_OVF		BIT(4)
+#define DMM32AT_FIFO_STATUS_FIFOEN	BIT(3)
+#define DMM32AT_FIFO_STATUS_SCANEN	BIT(2)
+#define DMM32AT_FIFO_STATUS_PAGE_MASK	0
 #define DMM32AT_CTRL_REG		0x08
-#define DMM32AT_CTRL_RESETA		(1 << 5)
-#define DMM32AT_CTRL_RESETD		(1 << 4)
-#define DMM32AT_CTRL_INTRST		(1 << 3)
-#define DMM32AT_CTRL_PAGE_8254		(0 << 0)
-#define DMM32AT_CTRL_PAGE_8255		(1 << 0)
-#define DMM32AT_CTRL_PAGE_CALIB		(3 << 0)
+#define DMM32AT_CTRL_RESETA		BIT(5)
+#define DMM32AT_CTRL_RESETD		BIT(4)
+#define DMM32AT_CTRL_INTRST		BIT(3)
+#define DMM32AT_CTRL_PAGE_8254		0
+#define DMM32AT_CTRL_PAGE_8255		BIT(0)
+#define DMM32AT_CTRL_PAGE_CALIB		0
 #define DMM32AT_AI_STATUS_REG		0x08
-#define DMM32AT_AI_STATUS_STS		(1 << 7)
-#define DMM32AT_AI_STATUS_SD1		(1 << 6)
-#define DMM32AT_AI_STATUS_SD0		(1 << 5)
+#define DMM32AT_AI_STATUS_STS		BIT(7)
+#define DMM32AT_AI_STATUS_SD1		BIT(6)
+#define DMM32AT_AI_STATUS_SD0		BIT(5)
 #define DMM32AT_AI_STATUS_ADCH_MASK	(0x1f << 0)
 #define DMM32AT_INTCLK_REG		0x09
-#define DMM32AT_INTCLK_ADINT		(1 << 7)
-#define DMM32AT_INTCLK_DINT		(1 << 6)
-#define DMM32AT_INTCLK_TINT		(1 << 5)
-#define DMM32AT_INTCLK_CLKEN		(1 << 1)  /* 1=see below  0=software */
-#define DMM32AT_INTCLK_CLKSEL		(1 << 0)  /* 1=OUT2  0=EXTCLK */
+#define DMM32AT_INTCLK_ADINT		BIT(7)
+#define DMM32AT_INTCLK_DINT		BIT(6)
+#define DMM32AT_INTCLK_TINT		BIT(5)
+#define DMM32AT_INTCLK_CLKEN		BIT(1)  /* 1=see below  0=software */
+#define DMM32AT_INTCLK_CLKSEL		BIT(0)  /* 1=OUT2  0=EXTCLK */
 #define DMM32AT_CTRDIO_CFG_REG		0x0a
-#define DMM32AT_CTRDIO_CFG_FREQ12	(1 << 7)  /* CLK12 1=100KHz 0=10MHz */
-#define DMM32AT_CTRDIO_CFG_FREQ0	(1 << 6)  /* CLK0  1=10KHz  0=10MHz */
-#define DMM32AT_CTRDIO_CFG_OUT2EN	(1 << 5)  /* J3.42 1=OUT2 is DOUT2 */
-#define DMM32AT_CTRDIO_CFG_OUT0EN	(1 << 4)  /* J3,44 1=OUT0 is DOUT0 */
-#define DMM32AT_CTRDIO_CFG_GT0EN	(1 << 2)  /* J3.47 1=DIN1 is GATE0 */
-#define DMM32AT_CTRDIO_CFG_SRC0		(1 << 1)  /* CLK0 is 0=FREQ0 1=J3.48 */
-#define DMM32AT_CTRDIO_CFG_GT12EN	(1 << 0)  /* J3.46 1=DIN2 is GATE12 */
+#define DMM32AT_CTRDIO_CFG_FREQ12	BIT(7)  /* CLK12 1=100KHz 0=10MHz */
+#define DMM32AT_CTRDIO_CFG_FREQ0	BIT(6)  /* CLK0  1=10KHz  0=10MHz */
+#define DMM32AT_CTRDIO_CFG_OUT2EN	BIT(5)  /* J3.42 1=OUT2 is DOUT2 */
+#define DMM32AT_CTRDIO_CFG_OUT0EN	BIT(4)  /* J3,44 1=OUT0 is DOUT0 */
+#define DMM32AT_CTRDIO_CFG_GT0EN	BIT(2)  /* J3.47 1=DIN1 is GATE0 */
+#define DMM32AT_CTRDIO_CFG_SRC0		BIT(1)  /* CLK0 is 0=FREQ0 1=J3.48 */
+#define DMM32AT_CTRDIO_CFG_GT12EN	BIT(0)  /* J3.46 1=DIN2 is GATE12 */
 #define DMM32AT_AI_CFG_REG		0x0b
-#define DMM32AT_AI_CFG_SCINT_20US	(0 << 4)
-#define DMM32AT_AI_CFG_SCINT_15US	(1 << 4)
-#define DMM32AT_AI_CFG_SCINT_10US	(2 << 4)
-#define DMM32AT_AI_CFG_SCINT_5US	(3 << 4)
-#define DMM32AT_AI_CFG_RANGE		(1 << 3)  /* 0=5V  1=10V */
-#define DMM32AT_AI_CFG_ADBU		(1 << 2)  /* 0=bipolar  1=unipolar */
+#define DMM32AT_AI_CFG_SCINT_20US	0
+#define DMM32AT_AI_CFG_SCINT_15US	BIT(4)
+#define DMM32AT_AI_CFG_SCINT_10US	(BIT(5) & ~BIT(4))
+#define DMM32AT_AI_CFG_SCINT_5US	(BIT(5) | BIT(4))
+#define DMM32AT_AI_CFG_RANGE		BIT(3)  /* 0=5V  1=10V */
+#define DMM32AT_AI_CFG_ADBU		BIT(2)  /* 0=bipolar  1=unipolar */
 #define DMM32AT_AI_CFG_GAIN(x)		((x) << 0)
 #define DMM32AT_AI_READBACK_REG		0x0b
-#define DMM32AT_AI_READBACK_WAIT	(1 << 7)  /* DMM32AT_AI_STATUS_STS */
-#define DMM32AT_AI_READBACK_RANGE	(1 << 3)
-#define DMM32AT_AI_READBACK_ADBU	(1 << 2)
-#define DMM32AT_AI_READBACK_GAIN_MASK	(3 << 0)
+#define DMM32AT_AI_READBACK_WAIT	BIT(7)  /* DMM32AT_AI_STATUS_STS */
+#define DMM32AT_AI_READBACK_RANGE	BIT(3)
+#define DMM32AT_AI_READBACK_ADBU	BIT(2)
+#define DMM32AT_AI_READBACK_GAIN_MASK	0
 
 #define DMM32AT_CLK1 0x0d
 #define DMM32AT_CLK2 0x0e

@@ -508,7 +508,7 @@ static int dmm32at_reset(struct comedi_device *dev)
 	outb(DMM32AT_CTRL_RESETA, dev->iobase + DMM32AT_CTRL_REG);
 
 	/* allow a millisecond to reset */
-	udelay(1000);
+	usleep_range(1000, 1050);
 
 	/* zero scan and fifo control */
 	outb(0x0, dev->iobase + DMM32AT_FIFO_CTRL_REG);

@@ -524,7 +524,7 @@ static int dmm32at_reset(struct comedi_device *dev)
 	outb(DMM32AT_RANGE_U10, dev->iobase + DMM32AT_AI_CFG_REG);
 
 	/* should take 10 us to settle, here's a hundred */
-	udelay(100);
+	usleep_range(100, 150);
 
 	/* read back the values */
 	ailo = inb(dev->iobase + DMM32AT_AI_LO_CHAN_REG);
Signature · 1 line
1.7.10.4
#1 lore raw

This one is buggy. For example:

drivers/staging/comedi/drivers/dmm32at.c line 77

Quoted 2 lines
drivers/staging/comedi/drivers/dmm32at.c +1 -1
-#define DMM32AT_FIFO_STATUS_PAGE_MASK	(3 << 0)
+#define DMM32AT_FIFO_STATUS_PAGE_MASK	0

There are other bugs as well. Please, be more careful.

regards,
dan carpenter

#2 lore raw
In reply to Ranjith Thangavel
Quoted 3 lines · Ranjith Thangavel

BIT macro is used for defining BIT location instead of shifting operator, usleep_range is preferred over udelay - coding style issue

Those two things should be done in separate patches.

Quoted 51 lines
Signed-off-by Ranjith Thangavel <[email protected]>
---
  drivers/staging/comedi/drivers/dmm32at.c |  104 +++++++++++++++---------------
  1 file changed, 52 insertions(+), 52 deletions(-)
drivers/staging/comedi/drivers/dmm32at.c +12 -16
index 958c0d4..0836d39 100644
@@ -46,74 +46,74 @@
  #define DMM32AT_AI_START_CONV_REG	0x00
  #define DMM32AT_AI_LSB_REG		0x00
  #define DMM32AT_AUX_DOUT_REG		0x01
-#define DMM32AT_AUX_DOUT2		(1 << 2)  /* J3.42 - OUT2 (OUT2EN) */
-#define DMM32AT_AUX_DOUT1		(1 << 1)  /* J3.43 */
-#define DMM32AT_AUX_DOUT0		(1 << 0)  /* J3.44 - OUT0 (OUT0EN) */
+#define DMM32AT_AUX_DOUT2		BIT(2)  /* J3.42 - OUT2 (OUT2EN) */
+#define DMM32AT_AUX_DOUT1		BIT(1)  /* J3.43 */
+#define DMM32AT_AUX_DOUT0		BIT(0)  /* J3.44 - OUT0 (OUT0EN) */
  #define DMM32AT_AI_MSB_REG		0x01
  #define DMM32AT_AI_LO_CHAN_REG		0x02
  #define DMM32AT_AI_HI_CHAN_REG		0x03
  #define DMM32AT_AUX_DI_REG		0x04
-#define DMM32AT_AUX_DI_DACBUSY		(1 << 7)
-#define DMM32AT_AUX_DI_CALBUSY		(1 << 6)
-#define DMM32AT_AUX_DI3			(1 << 3)  /* J3.45 - ADCLK (CLKSEL) */
-#define DMM32AT_AUX_DI2			(1 << 2)  /* J3.46 - GATE12 (GT12EN) */
-#define DMM32AT_AUX_DI1			(1 << 1)  /* J3.47 - GATE0 (GT0EN) */
-#define DMM32AT_AUX_DI0			(1 << 0)  /* J3.48 - CLK0 (SRC0) */
+#define DMM32AT_AUX_DI_DACBUSY		BIT(7)
+#define DMM32AT_AUX_DI_CALBUSY		BIT(6)
+#define DMM32AT_AUX_DI3			BIT(3)  /* J3.45 - ADCLK (CLKSEL) */
+#define DMM32AT_AUX_DI2			BIT(2)  /* J3.46 - GATE12 (GT12EN) */
+#define DMM32AT_AUX_DI1			BIT(1)  /* J3.47 - GATE0 (GT0EN) */
+#define DMM32AT_AUX_DI0			BIT(0)  /* J3.48 - CLK0 (SRC0) */
  #define DMM32AT_AO_LSB_REG		0x04
  #define DMM32AT_AO_MSB_REG		0x05
  #define DMM32AT_AO_MSB_DACH(x)		((x) << 6)
  #define DMM32AT_FIFO_DEPTH_REG		0x06
  #define DMM32AT_FIFO_CTRL_REG		0x07
-#define DMM32AT_FIFO_CTRL_FIFOEN	(1 << 3)
-#define DMM32AT_FIFO_CTRL_SCANEN	(1 << 2)
-#define DMM32AT_FIFO_CTRL_FIFORST	(1 << 1)
+#define DMM32AT_FIFO_CTRL_FIFOEN	BIT(3)
+#define DMM32AT_FIFO_CTRL_SCANEN	BIT(2)
+#define DMM32AT_FIFO_CTRL_FIFORST	BIT(1)
  #define DMM32AT_FIFO_STATUS_REG		0x07
-#define DMM32AT_FIFO_STATUS_EF		(1 << 7)
-#define DMM32AT_FIFO_STATUS_HF		(1 << 6)
-#define DMM32AT_FIFO_STATUS_FF		(1 << 5)
-#define DMM32AT_FIFO_STATUS_OVF		(1 << 4)

drivers/staging/comedi/drivers/dmm32at.c line 77

Quoted 10 lines
drivers/staging/comedi/drivers/dmm32at.c +7 -3
-#define DMM32AT_FIFO_STATUS_FIFOEN	(1 << 3)
-#define DMM32AT_FIFO_STATUS_SCANEN	(1 << 2)
-#define DMM32AT_FIFO_STATUS_PAGE_MASK	(3 << 0)
+#define DMM32AT_FIFO_STATUS_EF		BIT(7)
+#define DMM32AT_FIFO_STATUS_HF		BIT(6)
+#define DMM32AT_FIFO_STATUS_FF		BIT(5)
+#define DMM32AT_FIFO_STATUS_OVF		BIT(4)
+#define DMM32AT_FIFO_STATUS_FIFOEN	BIT(3)
+#define DMM32AT_FIFO_STATUS_SCANEN	BIT(2)
+#define DMM32AT_FIFO_STATUS_PAGE_MASK	0

As Dan Carpenter pointer out the new value of DMM32AT_FIFO_STATUS_PAGE_MASK is wrong (the old value was 3).

drivers/staging/comedi/drivers/dmm32at.c line 84

Quoted 13 lines
drivers/staging/comedi/drivers/dmm32at.c +6 -6
  #define DMM32AT_CTRL_REG		0x08
-#define DMM32AT_CTRL_RESETA		(1 << 5)
-#define DMM32AT_CTRL_RESETD		(1 << 4)
-#define DMM32AT_CTRL_INTRST		(1 << 3)
-#define DMM32AT_CTRL_PAGE_8254		(0 << 0)
-#define DMM32AT_CTRL_PAGE_8255		(1 << 0)
-#define DMM32AT_CTRL_PAGE_CALIB		(3 << 0)
+#define DMM32AT_CTRL_RESETA		BIT(5)
+#define DMM32AT_CTRL_RESETD		BIT(4)
+#define DMM32AT_CTRL_INTRST		BIT(3)
+#define DMM32AT_CTRL_PAGE_8254		0
+#define DMM32AT_CTRL_PAGE_8255		BIT(0)
+#define DMM32AT_CTRL_PAGE_CALIB		0

The new value of DMM32AT_CTRL_PAGE_CALIB is wrong - the old value is 3.
DMM32AT_CTRL_PAGE_8254, DMM32AT_CTRL_PAGE_8255 and
DMM32AT_CTRL_PAGE_CALIB are 2-bit values, so the BIT() macro isn't really the best representation.

(To be honest, I don't really know what the preferred representation of shifted multi-bit values is.)

Quoted 35 lines
drivers/staging/comedi/drivers/dmm32at.c +15 -15
  #define DMM32AT_AI_STATUS_REG		0x08
-#define DMM32AT_AI_STATUS_STS		(1 << 7)
-#define DMM32AT_AI_STATUS_SD1		(1 << 6)
-#define DMM32AT_AI_STATUS_SD0		(1 << 5)
+#define DMM32AT_AI_STATUS_STS		BIT(7)
+#define DMM32AT_AI_STATUS_SD1		BIT(6)
+#define DMM32AT_AI_STATUS_SD0		BIT(5)
  #define DMM32AT_AI_STATUS_ADCH_MASK	(0x1f << 0)
  #define DMM32AT_INTCLK_REG		0x09
-#define DMM32AT_INTCLK_ADINT		(1 << 7)
-#define DMM32AT_INTCLK_DINT		(1 << 6)
-#define DMM32AT_INTCLK_TINT		(1 << 5)
-#define DMM32AT_INTCLK_CLKEN		(1 << 1)  /* 1=see below  0=software */
-#define DMM32AT_INTCLK_CLKSEL		(1 << 0)  /* 1=OUT2  0=EXTCLK */
+#define DMM32AT_INTCLK_ADINT		BIT(7)
+#define DMM32AT_INTCLK_DINT		BIT(6)
+#define DMM32AT_INTCLK_TINT		BIT(5)
+#define DMM32AT_INTCLK_CLKEN		BIT(1)  /* 1=see below  0=software */
+#define DMM32AT_INTCLK_CLKSEL		BIT(0)  /* 1=OUT2  0=EXTCLK */
  #define DMM32AT_CTRDIO_CFG_REG		0x0a
-#define DMM32AT_CTRDIO_CFG_FREQ12	(1 << 7)  /* CLK12 1=100KHz 0=10MHz */
-#define DMM32AT_CTRDIO_CFG_FREQ0	(1 << 6)  /* CLK0  1=10KHz  0=10MHz */
-#define DMM32AT_CTRDIO_CFG_OUT2EN	(1 << 5)  /* J3.42 1=OUT2 is DOUT2 */
-#define DMM32AT_CTRDIO_CFG_OUT0EN	(1 << 4)  /* J3,44 1=OUT0 is DOUT0 */
-#define DMM32AT_CTRDIO_CFG_GT0EN	(1 << 2)  /* J3.47 1=DIN1 is GATE0 */
-#define DMM32AT_CTRDIO_CFG_SRC0		(1 << 1)  /* CLK0 is 0=FREQ0 1=J3.48 */
-#define DMM32AT_CTRDIO_CFG_GT12EN	(1 << 0)  /* J3.46 1=DIN2 is GATE12 */
+#define DMM32AT_CTRDIO_CFG_FREQ12	BIT(7)  /* CLK12 1=100KHz 0=10MHz */
+#define DMM32AT_CTRDIO_CFG_FREQ0	BIT(6)  /* CLK0  1=10KHz  0=10MHz */
+#define DMM32AT_CTRDIO_CFG_OUT2EN	BIT(5)  /* J3.42 1=OUT2 is DOUT2 */
+#define DMM32AT_CTRDIO_CFG_OUT0EN	BIT(4)  /* J3,44 1=OUT0 is DOUT0 */
+#define DMM32AT_CTRDIO_CFG_GT0EN	BIT(2)  /* J3.47 1=DIN1 is GATE0 */
+#define DMM32AT_CTRDIO_CFG_SRC0		BIT(1)  /* CLK0 is 0=FREQ0 1=J3.48 */
+#define DMM32AT_CTRDIO_CFG_GT12EN	BIT(0)  /* J3.46 1=DIN2 is GATE12 */
  #define DMM32AT_AI_CFG_REG		0x0b

drivers/staging/comedi/drivers/dmm32at.c line 108

Quoted 10 lines
drivers/staging/comedi/drivers/dmm32at.c +4 -6
-#define DMM32AT_AI_CFG_SCINT_20US	(0 << 4)
-#define DMM32AT_AI_CFG_SCINT_15US	(1 << 4)
-#define DMM32AT_AI_CFG_SCINT_10US	(2 << 4)
-#define DMM32AT_AI_CFG_SCINT_5US	(3 << 4)
-#define DMM32AT_AI_CFG_RANGE		(1 << 3)  /* 0=5V  1=10V */
-#define DMM32AT_AI_CFG_ADBU		(1 << 2)  /* 0=bipolar  1=unipolar */
+#define DMM32AT_AI_CFG_SCINT_20US	0
+#define DMM32AT_AI_CFG_SCINT_15US	BIT(4)
+#define DMM32AT_AI_CFG_SCINT_10US	(BIT(5) & ~BIT(4))
+#define DMM32AT_AI_CFG_SCINT_5US	(BIT(5) | BIT(4))

The values of DMM32AT_AI_CFG_SCINT_20US etc. are numerically correct, but look a bit strange. The (BIT(5) & ~BIT(4)) looks especially strange and could be changed to BIT(5). These are all really shifted 2-bit values, so perhaps the BIT() macro isn't the best representation.

drivers/staging/comedi/drivers/dmm32at.c line 116

Quoted 12 lines
drivers/staging/comedi/drivers/dmm32at.c +6 -4
+#define DMM32AT_AI_CFG_RANGE		BIT(3)  /* 0=5V  1=10V */
+#define DMM32AT_AI_CFG_ADBU		BIT(2)  /* 0=bipolar  1=unipolar */
  #define DMM32AT_AI_CFG_GAIN(x)		((x) << 0)
  #define DMM32AT_AI_READBACK_REG		0x0b
-#define DMM32AT_AI_READBACK_WAIT	(1 << 7)  /* DMM32AT_AI_STATUS_STS */
-#define DMM32AT_AI_READBACK_RANGE	(1 << 3)
-#define DMM32AT_AI_READBACK_ADBU	(1 << 2)
-#define DMM32AT_AI_READBACK_GAIN_MASK	(3 << 0)
+#define DMM32AT_AI_READBACK_WAIT	BIT(7)  /* DMM32AT_AI_STATUS_STS */
+#define DMM32AT_AI_READBACK_RANGE	BIT(3)
+#define DMM32AT_AI_READBACK_ADBU	BIT(2)
+#define DMM32AT_AI_READBACK_GAIN_MASK	0

The value of DMM32AT_AI_READBACK_GAIN_MASK is wrong - the old value was 3.

Quoted 14 lines
drivers/staging/comedi/drivers/dmm32at.c +1 -1
  #define DMM32AT_CLK1 0x0d
  #define DMM32AT_CLK2 0x0e

@@ -508,7 +508,7 @@ static int dmm32at_reset(struct comedi_device *dev)
  	outb(DMM32AT_CTRL_RESETA, dev->iobase + DMM32AT_CTRL_REG);
 
  	/* allow a millisecond to reset */
-	udelay(1000);
+	usleep_range(1000, 1050);
 
  	/* zero scan and fifo control */
  	outb(0x0, dev->iobase + DMM32AT_FIFO_CTRL_REG);

drivers/staging/comedi/drivers/dmm32at.c static int dmm32at_reset(struct comedi_device *dev) · line 530

Quoted 8 lines
drivers/staging/comedi/drivers/dmm32at.c +1 -1
@@ -524,7 +524,7 @@ static int dmm32at_reset(struct comedi_device *dev)
  	outb(DMM32AT_RANGE_U10, dev->iobase + DMM32AT_AI_CFG_REG);
 
  	/* should take 10 us to settle, here's a hundred */
-	udelay(100);
+	usleep_range(100, 150);
 
  	/* read back the values */
  	ailo = inb(dev->iobase + DMM32AT_AI_LO_CHAN_REG);

The usleep_range() stuff is unrelated to the use of the BIT() macro and should be in a separate patch.

Signature · 2 lines
-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
#3 lore raw
Quoted 6 lines · Ian Abbott
Quoted 3 lines · Ranjith Thangavel

BIT macro is used for defining BIT location instead of shifting operator, usleep_range is preferred over udelay - coding style issue

Those two things should be done in separate patches.

Sorry, I didn't see you've reposted this as two separate patches, so ignore my previous reply.

Signature · 2 lines
-=( Ian Abbott @ MEV Ltd.    E-mail: <[email protected]> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
#4 lore raw
In reply to Ian Abbott
Quoted 9 lines · Ian Abbott

drivers/staging/comedi/drivers/dmm32at.c line 108

Quoted 2 lines
drivers/staging/comedi/drivers/dmm32at.c +2 -0
+#define DMM32AT_AI_CFG_SCINT_10US	(BIT(5) & ~BIT(4))
+#define DMM32AT_AI_CFG_SCINT_5US	(BIT(5) | BIT(4))

The values of DMM32AT_AI_CFG_SCINT_20US etc. are numerically correct, but look a bit strange. The (BIT(5) & ~BIT(4)) looks especially strange and could be changed to BIT(5). These are all really shifted 2-bit values, so perhaps the BIT() macro isn't the best representation.

BIT(5) & ~BIT(4) is silly. Don't do that.

The original code was fine.

regards,
dan carpenter