[PATCH] i2c: iproc: Change driver to use 'BIT' macro

[PATCH] i2c: iproc: Change driver to use 'BIT' macro

[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

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Peter Rosin]

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);
>  
> 

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Peter Rosin]

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);
>>  
>>
> 

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Ray Jui]

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

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Peter Rosin]

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
> 

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Ray Jui]

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
>>
> 

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Peter Rosin]

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
>>>
>>

Re: [PATCH] i2c: iproc: Change driver to use 'BIT' macro

[Wolfram Sang]

[-- 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 --]