Re: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
On Wed, Nov 18, 2015 at 04:26:52PM +, Ian Abbott wrote: > >+#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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
On 18/11/15 16:26, Ian Abbott wrote: On 14/11/15 17:10, Ranjith Thangavel wrote: 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. -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
On 14/11/15 17:10, Ranjith Thangavel wrote: 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. Signed-off-by: Ranjith Thangavel --- drivers/staging/comedi/drivers/dmm32at.c | 104 +++--- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/drivers/staging/comedi/drivers/dmm32at.c b/drivers/staging/comedi/drivers/dmm32at.c index 958c0d4..0836d39 100644 --- a/drivers/staging/comedi/drivers/dmm32at.c +++ b/drivers/staging/comedi/drivers/dmm32at.c @@ -46,74 +46,74 @@ #define DMM32AT_AI_START_CONV_REG 0x00 #define DMM32AT_AI_LSB_REG0x00 #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_REG0x01 #define DMM32AT_AI_LO_CHAN_REG0x02 #define DMM32AT_AI_HI_CHAN_REG0x03 #define DMM32AT_AUX_DI_REG0x04 -#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_DI3BIT(3) /* J3.45 - ADCLK (CLKSEL) */ +#define DMM32AT_AUX_DI2BIT(2) /* J3.46 - GATE12 (GT12EN) */ +#define DMM32AT_AUX_DI1BIT(1) /* J3.47 - GATE0 (GT0EN) */ +#define DMM32AT_AUX_DI0BIT(0) /* J3.48 - CLK0 (SRC0) */ #define DMM32AT_AO_LSB_REG0x04 #define DMM32AT_AO_MSB_REG0x05 #define DMM32AT_AO_MSB_DACH(x)((x) << 6) #define DMM32AT_FIFO_DEPTH_REG0x06 #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_OVFBIT(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). #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_RESETABIT(5) +#define DMM32AT_CTRL_RESETDBIT(4) +#define DMM32AT_CTRL_INTRSTBIT(3) +#define DMM32AT_CTRL_PAGE_8254 0 +#define DMM32AT_CTRL_PAGE_8255 BIT(0) +#define DMM32AT_CTRL_PAGE_CALIB0 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.) #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_STAT
RE: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
On Monday, November 16, 2015 10:09 AM, Ranjith Thangavel wrote: > 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 > --- > drivers/staging/comedi/drivers/dmm32at.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/dmm32at.c > b/drivers/staging/comedi/drivers/dmm32at.c > index d312fda..9f8c9eb 100644 > --- a/drivers/staging/comedi/drivers/dmm32at.c > +++ b/drivers/staging/comedi/drivers/dmm32at.c > @@ -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); Nothing in this patch uses the BIT macro. Please make sure your commit messages actually match the patch. You also posted a different patch on Saturday with the exact same commit message. Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 --- drivers/staging/comedi/drivers/dmm32at.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/comedi/drivers/dmm32at.c b/drivers/staging/comedi/drivers/dmm32at.c index d312fda..9f8c9eb 100644 --- a/drivers/staging/comedi/drivers/dmm32at.c +++ b/drivers/staging/comedi/drivers/dmm32at.c @@ -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); -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
This one is buggy. For example: > -#define DMM32AT_FIFO_STATUS_PAGE_MASK(3 << 0) > +#define DMM32AT_FIFO_STATUS_PAGE_MASK0 There are other bugs as well. Please, be more careful. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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 --- drivers/staging/comedi/drivers/dmm32at.c | 104 +++--- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/drivers/staging/comedi/drivers/dmm32at.c b/drivers/staging/comedi/drivers/dmm32at.c index 958c0d4..0836d39 100644 --- a/drivers/staging/comedi/drivers/dmm32at.c +++ b/drivers/staging/comedi/drivers/dmm32at.c @@ -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_DI3BIT(3) /* J3.45 - ADCLK (CLKSEL) */ +#define DMM32AT_AUX_DI2BIT(2) /* J3.46 - GATE12 (GT12EN) */ +#define DMM32AT_AUX_DI1BIT(1) /* J3.47 - GATE0 (GT0EN) */ +#define DMM32AT_AUX_DI0BIT(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_REG0x07 -#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_OVFBIT(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_RESETABIT(5) +#define DMM32AT_CTRL_RESETDBIT(4) +#define DMM32AT_CTRL_INTRSTBIT(3) +#define DMM32AT_CTRL_PAGE_8254 0 +#define DMM32AT_CTRL_PAGE_8255 BIT(0) +#define DMM32AT_CTRL_PAGE_CALIB0 #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_DINTBIT(6) +#define DMM32AT_INTCLK_TINTBIT(5) +#define DMM32AT_INTCLK_CLKEN BIT(1) /* 1=see below 0=softwar
