i2c: iproc: Change driver to use 'BIT' macro

PATCH Applied Ray Jui 3 participants 8 messages Apr 3, 2019 – Apr 23, 2019

Subsystems (2)

I2C subsystem host drivers Broadcom iproc Arm architecture

lore

Share & tools

lore

Ray Jui

author

2019-04-03 21:05 UTC #0 lore raw

PATCH 1/1 i2c: iproc: Change driver to use 'BIT' macro

Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code

Signed-off-by Ray Jui <[email protected]>

---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-bcm-iproc.c +3 -3

index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}

@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

@@ -995,7 +995,7 @@ static int bcm_iproc_i2c_resume(struct device *dev)
 
 	/* configure to the desired bus speed */
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (iproc_i2c->bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

Signature · 1 line

2.17.1

Peter Rosin

2019-04-12 22:59 UTC #1 lore raw

Quoted 3 lines · Ray Jui

Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code

All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though? Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be

#define XXX BIT(<xxx>)

instead of

#define XXX_SHIFT <xxx>

but that triggers more churn, so is obviously more error prone. You might not dare it?

Cheers,
Peter

Quoted 37 lines

Signed-off-by Ray Jui <[email protected]>

---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-bcm-iproc.c +3 -3

index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}

@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

@@ -995,7 +995,7 @@ static int bcm_iproc_i2c_resume(struct device *dev)
 
 	/* configure to the desired bus speed */
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (iproc_i2c->bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

Peter Rosin

2019-04-15 06:56 UTC #2 lore raw

Quoted 6 lines · Peter Rosin

Quoted 3 lines · Ray Jui
Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?

I verified that, and yes indeed, I was behind. That said, see below...

Quoted 36 lines

Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be
#define XXX BIT(<xxx>)
instead of
#define XXX_SHIFT <xxx>
but that triggers more churn, so is obviously more error prone. You might not dare it?
Cheers,
Peter
Quoted 20 lines
Signed-off-by Ray Jui <[email protected]>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 

Quoted 5 lines

drivers/i2c/busses/i2c-bcm-iproc.c static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) · line 848
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;

These two statements now no longer "match". One uses BIT and the other open codes the shift. I think that's bad. Losing the _SHIFT suffix and including BIT in the macro expansion, as suggested above, yields:

	val &= ~TIM_CFG_MODE_400;
	if (bus_speed == 400000)
		val |= TIM_CFG_MODE_400;

which is perhaps one more line, but also more readable IMO.

But all this is of course in deep nit-pick-territory...

Cheers,
Peter

Quoted 13 lines

Quoted 12 lines

drivers/i2c/busses/i2c-bcm-iproc.c +1 -1

 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

@@ -995,7 +995,7 @@ static int bcm_iproc_i2c_resume(struct device *dev)
 
 	/* configure to the desired bus speed */
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (iproc_i2c->bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
 	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);

Ray Jui

author

2019-04-17 23:48 UTC #3 lore raw

Quoted 10 lines · Peter Rosin

Quoted 6 lines · Peter Rosin
Quoted 3 lines · Ray Jui
Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?
I verified that, and yes indeed, I was behind. That said, see below...

Right. Previous change that this change depends on is already queued in i2c/for-next.

Quoted 6 lines

Quoted 6 lines
Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be
#define XXX BIT(<xxx>)

Quoted 7 lines

Quoted 7 lines
instead of
#define XXX_SHIFT <xxx>
but that triggers more churn, so is obviously more error prone. You might not dare it?

With the current code, I don't see how that is cleaner. With XXX_SHIFT specified, it makes it very clear to the user that the define a for a bit location within a register. You can argue and say it makes the define longer, but not less clear.

Quoted 33 lines

Quoted 23 lines
Cheers,
Peter
Quoted 20 lines
Signed-off-by Ray Jui <[email protected]>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) · line 848
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
These two statements now no longer "match". One uses BIT and the other open codes the shift. I think that's bad. Losing the _SHIFT suffix and including BIT in the macro expansion, as suggested above, yields:

Quoted 6 lines

	val &= ~TIM_CFG_MODE_400;
	if (bus_speed == 400000)
		val |= TIM_CFG_MODE_400;
which is perhaps one more line, but also more readable IMO.

A single line with evaluation embedded is nice and clean to me. I guess this is subjective.

I'll leave the decision to Wolfram. If he also prefers the above change to be made, sure. Otherwise, I'll leave it as it is.

Quoted 5 lines

But all this is of course in deep nit-pick-territory...
Cheers,
Peter

Thanks,

Ray

Peter Rosin

2019-04-18 06:21 UTC #4 lore raw

Quoted 73 lines · Ray Jui

Quoted 10 lines · Peter Rosin
Quoted 6 lines · Peter Rosin
Quoted 3 lines · Ray Jui
Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?
I verified that, and yes indeed, I was behind. That said, see below...
Right. Previous change that this change depends on is already queued in i2c/for-next.
Quoted 6 lines
Quoted 6 lines
Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be
#define XXX BIT(<xxx>)
Quoted 7 lines
Quoted 7 lines
instead of
#define XXX_SHIFT <xxx>
but that triggers more churn, so is obviously more error prone. You might not dare it?
With the current code, I don't see how that is cleaner. With XXX_SHIFT specified, it makes it very clear to the user that the define a for a bit location within a register. You can argue and say it makes the define longer, but not less clear.
Quoted 37 lines
Quoted 23 lines
Cheers,
Peter
Quoted 20 lines
Signed-off-by Ray Jui <[email protected]>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) · line 848
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
These two statements now no longer "match". One uses BIT and the other open codes the shift. I think that's bad. Losing the _SHIFT suffix and including BIT in the macro expansion, as suggested above, yields:
	val &= ~TIM_CFG_MODE_400;
	if (bus_speed == 400000)
		val |= TIM_CFG_MODE_400;

Quoted 5 lines

Quoted 2 lines
which is perhaps one more line, but also more readable IMO.
A single line with evaluation embedded is nice and clean to me. I guess this is subjective.

The "problem" I had when I looked at the driver was not any one specific instance. It was just that, for my taste, the code had too many shifts etc inline with the real code. Replacing 1 << xyz_SHIFT with BIT(xyz_SHIFT) is not a real improvement, they are just about equal to me, it's just that there are still too many mechanical things happening that could easily be tucked away with the suggested approach.

Quoted 2 lines

I'll leave the decision to Wolfram. If he also prefers the above change to be made, sure. Otherwise, I'll leave it as it is.

But if you see no value in my suggestion and/or don't what to take the cleanup one step further, then just leave it as-is.

Quoted 10 lines

Quoted 5 lines
But all this is of course in deep nit-pick-territory...
Cheers,
Peter
Thanks,
Ray

Ray Jui

author

2019-04-18 17:25 UTC #5 lore raw

Quoted 80 lines · Peter Rosin

Quoted 73 lines · Ray Jui
Quoted 10 lines · Peter Rosin
Quoted 6 lines · Peter Rosin
Quoted 3 lines · Ray Jui
Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?
I verified that, and yes indeed, I was behind. That said, see below...
Right. Previous change that this change depends on is already queued in i2c/for-next.
Quoted 6 lines
Quoted 6 lines
Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be
#define XXX BIT(<xxx>)
Quoted 7 lines
Quoted 7 lines
instead of
#define XXX_SHIFT <xxx>
but that triggers more churn, so is obviously more error prone. You might not dare it?
With the current code, I don't see how that is cleaner. With XXX_SHIFT specified, it makes it very clear to the user that the define a for a bit location within a register. You can argue and say it makes the define longer, but not less clear.
Quoted 37 lines
Quoted 23 lines
Cheers,
Peter
Quoted 20 lines
Signed-off-by Ray Jui <[email protected]>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) · line 848
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
These two statements now no longer "match". One uses BIT and the other open codes the shift. I think that's bad. Losing the _SHIFT suffix and including BIT in the macro expansion, as suggested above, yields:
	val &= ~TIM_CFG_MODE_400;
	if (bus_speed == 400000)
		val |= TIM_CFG_MODE_400;
Quoted 5 lines
Quoted 2 lines
which is perhaps one more line, but also more readable IMO.
A single line with evaluation embedded is nice and clean to me. I guess this is subjective.

Quoted 7 lines

The "problem" I had when I looked at the driver was not any one specific instance. It was just that, for my taste, the code had too many shifts etc inline with the real code. Replacing 1 << xyz_SHIFT with BIT(xyz_SHIFT) is not a real improvement, they are just about equal to me, it's just that there are still too many mechanical things happening that could easily be tucked away with the suggested approach.

Right, for your taste. Like I said, I feel this is very subjective. To me, and many other I2C driver owners (I just checked how many other I2C drivers also appear to prefer to use XXX_SHIFT and there are a lot of them), using XXX_SHIFT makes it more clear that the define is intended to be used for bit shift operation.

Quoted 6 lines

Quoted 2 lines
I'll leave the decision to Wolfram. If he also prefers the above change to be made, sure. Otherwise, I'll leave it as it is.
But if you see no value in my suggestion and/or don't what to take the cleanup one step further, then just leave it as-is.

Again, this is subjective. Personally I do not feel this is "cleanup one step further". To me, this change will make the code less clear on the intended operation.

Quoted 11 lines

Quoted 10 lines
Quoted 5 lines
But all this is of course in deep nit-pick-territory...
Cheers,
Peter
Thanks,
Ray

Peter Rosin

2019-04-18 21:27 UTC #6 lore raw

Quoted 91 lines · Ray Jui

Quoted 80 lines · Peter Rosin
Quoted 73 lines · Ray Jui
Quoted 10 lines · Peter Rosin
Quoted 6 lines · Peter Rosin
Quoted 3 lines · Ray Jui
Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
All? I see lots more '1 << XXX_SHIFT' matches. I might be behind though?
I verified that, and yes indeed, I was behind. That said, see below...
Right. Previous change that this change depends on is already queued in i2c/for-next.
Quoted 6 lines
Quoted 6 lines
Anyway, if you are cleaning up, I'm just flagging that BIT(XXX_SHIFT) looks a bit clunky to me. You might consider renaming all those single-bit XXX_SHIFT macros to simple be
#define XXX BIT(<xxx>)
Quoted 7 lines
Quoted 7 lines
instead of
#define XXX_SHIFT <xxx>
but that triggers more churn, so is obviously more error prone. You might not dare it?
With the current code, I don't see how that is cleaner. With XXX_SHIFT specified, it makes it very clear to the user that the define a for a bit location within a register. You can argue and say it makes the define longer, but not less clear.
Quoted 37 lines
Quoted 23 lines
Cheers,
Peter
Quoted 20 lines
Signed-off-by Ray Jui <[email protected]>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
index 562942d0c05c..a845b8decac8 100644
@@ -717,7 +717,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 			/* mark the last byte */
 			if (i == msg->len - 1)
-				val |= 1 << M_TX_WR_STATUS_SHIFT;
+				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
@@ -844,7 +844,7 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c) · line 848
Quoted 5 lines
drivers/i2c/busses/i2c-bcm-iproc.c +1 -1
 	iproc_i2c->bus_speed = bus_speed;
 	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
These two statements now no longer "match". One uses BIT and the other open codes the shift. I think that's bad. Losing the _SHIFT suffix and including BIT in the macro expansion, as suggested above, yields:
	val &= ~TIM_CFG_MODE_400;
	if (bus_speed == 400000)
		val |= TIM_CFG_MODE_400;
Quoted 5 lines
Quoted 2 lines
which is perhaps one more line, but also more readable IMO.
A single line with evaluation embedded is nice and clean to me. I guess this is subjective.
Quoted 7 lines
The "problem" I had when I looked at the driver was not any one specific instance. It was just that, for my taste, the code had too many shifts etc inline with the real code. Replacing 1 << xyz_SHIFT with BIT(xyz_SHIFT) is not a real improvement, they are just about equal to me, it's just that there are still too many mechanical things happening that could easily be tucked away with the suggested approach.

Quoted 5 lines

Right, for your taste. Like I said, I feel this is very subjective. To me, and many other I2C driver owners (I just checked how many other I2C drivers also appear to prefer to use XXX_SHIFT and there are a lot of them), using XXX_SHIFT makes it more clear that the define is intended to be used for bit shift operation.

Which is a very strange thing to say about my suggestion. There is no need for a _SHIFT suffix for the macro names if they are not going to used in shifts! That's the whole friggin point.

Regarding other I2C drivers, I just had a brief look at about 10 or so picked at random, and NONE of them use the XXX_SHIFT paradigm that this driver is using. The ones I picked were:

i2c-acorn.c
i2c-ali563.c
i2c-altera.c
i2c-at91.c
i2c-cmp.c
i2c-davinci.c
i2c-digicolor.c
i2c-elektor.c
i2c-st.c

The only one I looked at not doing it the way I suggested is i2c-dln2.c which does not appear to need any register field accesses at all.

So, perhaps you should read the suggestion again with more care? Or not. Anyway, I'm not going to waste any more time here.

Cheers,
Peter

Quoted 23 lines

Quoted 7 lines
Quoted 3 lines
I'll leave the decision to Wolfram. If he also prefers the above change to be made, sure. Otherwise, I'll leave it as it is.
But if you see no value in my suggestion and/or don't what to take the cleanup one step further, then just leave it as-is.
Again, this is subjective. Personally I do not feel this is "cleanup one step further". To me, this change will make the code less clear on the intended operation.
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
But all this is of course in deep nit-pick-territory...
Cheers,
Peter
Thanks,
Ray

Wolfram Sang

Applied

2019-04-23 21:36 UTC #7 lore raw

In reply to Ray Jui

Quoted 5 lines · Ray Jui

Change the iProc I2C driver to use the 'BIT' macro from all '1 << XXX' bit operations to get rid of compiler warning and improve readability of the code
Signed-off-by Ray Jui <[email protected]>

Applied to for-next, thanks!