* [PATCH] i2c: iproc: Change driver to use 'BIT' macro @ 2019-04-03 21:05 Ray Jui 2019-04-12 22:59 ` Peter Rosin 2019-04-23 21:36 ` Wolfram Sang 0 siblings, 2 replies; 8+ messages in thread From: Ray Jui @ 2019-04-03 21:05 UTC (permalink / raw) To: Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur, 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 <ray.jui@broadcom.com> --- drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c index 562942d0c05c..a845b8decac8 100644 --- a/drivers/i2c/busses/i2c-bcm-iproc.c +++ b/drivers/i2c/busses/i2c-bcm-iproc.c @@ -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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-03 21:05 [PATCH] i2c: iproc: Change driver to use 'BIT' macro Ray Jui @ 2019-04-12 22:59 ` Peter Rosin 2019-04-15 6:56 ` Peter Rosin 2019-04-23 21:36 ` Wolfram Sang 1 sibling, 1 reply; 8+ messages in thread From: Peter Rosin @ 2019-04-12 22:59 UTC (permalink / raw) To: Ray Jui, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 2019-04-03 23:05, Ray Jui wrote: > 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 > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > --- > drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c > index 562942d0c05c..a845b8decac8 100644 > --- a/drivers/i2c/busses/i2c-bcm-iproc.c > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c > @@ -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); > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-12 22:59 ` Peter Rosin @ 2019-04-15 6:56 ` Peter Rosin 2019-04-17 23:48 ` Ray Jui 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2019-04-15 6:56 UTC (permalink / raw) To: Ray Jui, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 2019-04-13 00:59, Peter Rosin wrote: > On 2019-04-03 23:05, Ray Jui wrote: >> 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... > 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 > >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> --- >> drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c >> index 562942d0c05c..a845b8decac8 100644 >> --- a/drivers/i2c/busses/i2c-bcm-iproc.c >> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >> @@ -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; 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 >> 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); >> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-15 6:56 ` Peter Rosin @ 2019-04-17 23:48 ` Ray Jui 2019-04-18 6:21 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Ray Jui @ 2019-04-17 23:48 UTC (permalink / raw) To: Peter Rosin, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 4/14/2019 11:56 PM, Peter Rosin wrote: > On 2019-04-13 00:59, Peter Rosin wrote: >> On 2019-04-03 23:05, Ray Jui wrote: >>> 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. >> 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? >> 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. >> Cheers, >> Peter >> >>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>> --- >>> drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c >>> index 562942d0c05c..a845b8decac8 100644 >>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c >>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >>> @@ -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; > > 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. > 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. > But all this is of course in deep nit-pick-territory... > > Cheers, > Peter > Thanks, Ray ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-17 23:48 ` Ray Jui @ 2019-04-18 6:21 ` Peter Rosin 2019-04-18 17:25 ` Ray Jui 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2019-04-18 6:21 UTC (permalink / raw) To: Ray Jui, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 2019-04-18 01:48, Ray Jui wrote: > > > On 4/14/2019 11:56 PM, Peter Rosin wrote: >> On 2019-04-13 00:59, Peter Rosin wrote: >>> On 2019-04-03 23:05, Ray Jui wrote: >>>> 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. > >>> 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? >>> > > 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. > >>> Cheers, >>> Peter >>> >>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>> --- >>>> drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c >>>> index 562942d0c05c..a845b8decac8 100644 >>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c >>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >>>> @@ -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; >> >> 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. >> > > 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. > 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. >> But all this is of course in deep nit-pick-territory... >> >> Cheers, >> Peter >> > > Thanks, > > Ray > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-18 6:21 ` Peter Rosin @ 2019-04-18 17:25 ` Ray Jui 2019-04-18 21:27 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Ray Jui @ 2019-04-18 17:25 UTC (permalink / raw) To: Peter Rosin, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 4/17/2019 11:21 PM, Peter Rosin wrote: > On 2019-04-18 01:48, Ray Jui wrote: >> >> >> On 4/14/2019 11:56 PM, Peter Rosin wrote: >>> On 2019-04-13 00:59, Peter Rosin wrote: >>>> On 2019-04-03 23:05, Ray Jui wrote: >>>>> 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. >> >>>> 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? >>>> >> >> 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. >> >>>> Cheers, >>>> Peter >>>> >>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>> --- >>>>> drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c >>>>> index 562942d0c05c..a845b8decac8 100644 >>>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c >>>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >>>>> @@ -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; >>> >>> 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. >>> >> >> 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. > 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. >> 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. >>> But all this is of course in deep nit-pick-territory... >>> >>> Cheers, >>> Peter >>> >> >> Thanks, >> >> Ray >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-18 17:25 ` Ray Jui @ 2019-04-18 21:27 ` Peter Rosin 0 siblings, 0 replies; 8+ messages in thread From: Peter Rosin @ 2019-04-18 21:27 UTC (permalink / raw) To: Ray Jui, Wolfram Sang Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur On 2019-04-18 19:25, Ray Jui wrote: > > > On 4/17/2019 11:21 PM, Peter Rosin wrote: >> On 2019-04-18 01:48, Ray Jui wrote: >>> >>> >>> On 4/14/2019 11:56 PM, Peter Rosin wrote: >>>> On 2019-04-13 00:59, Peter Rosin wrote: >>>>> On 2019-04-03 23:05, Ray Jui wrote: >>>>>> 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. >>> >>>>> 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? >>>>> >>> >>> 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. >>> >>>>> Cheers, >>>>> Peter >>>>> >>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>> --- >>>>>> drivers/i2c/busses/i2c-bcm-iproc.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c >>>>>> index 562942d0c05c..a845b8decac8 100644 >>>>>> --- a/drivers/i2c/busses/i2c-bcm-iproc.c >>>>>> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c >>>>>> @@ -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; >>>> >>>> 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. >>>> >>> >>> 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. >> > > 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 > >>> 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. > >>>> But all this is of course in deep nit-pick-territory... >>>> >>>> Cheers, >>>> Peter >>>> >>> >>> Thanks, >>> >>> Ray >>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro 2019-04-03 21:05 [PATCH] i2c: iproc: Change driver to use 'BIT' macro Ray Jui 2019-04-12 22:59 ` Peter Rosin @ 2019-04-23 21:36 ` Wolfram Sang 1 sibling, 0 replies; 8+ messages in thread From: Wolfram Sang @ 2019-04-23 21:36 UTC (permalink / raw) To: Ray Jui Cc: linux-i2c, linux-kernel, bcm-kernel-feedback-list, Rayagonda Kokatanur [-- Attachment #1: Type: text/plain, Size: 307 bytes --] On Wed, Apr 03, 2019 at 02:05:35PM -0700, Ray Jui wrote: > 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 <ray.jui@broadcom.com> Applied to for-next, thanks! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-23 21:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-03 21:05 [PATCH] i2c: iproc: Change driver to use 'BIT' macro Ray Jui 2019-04-12 22:59 ` Peter Rosin 2019-04-15 6:56 ` Peter Rosin 2019-04-17 23:48 ` Ray Jui 2019-04-18 6:21 ` Peter Rosin 2019-04-18 17:25 ` Ray Jui 2019-04-18 21:27 ` Peter Rosin 2019-04-23 21:36 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).